Fix 23722 - Lambdas are mangled incorrectly when using multiple compi…

[dkorpel]

…lation units, resulting in incorrect code

The problem now is that FuncExp in an alias is emitted in neither compilation unit, resulting in an undefined reference error.

[dlang-bot]

Thanks for your pull request and interest in making D better, @dkorpel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

• In preparation for migrating from Bugzilla to GitHub Issues, the issue reference syntax has changed. Please add the word "Bugzilla" to issue references. For example, Fix Bugzilla Issue 12345 or Fix Bugzilla 12345.(Reminder: the edit needs to be done in the Git commit message, not the GitHub pull request.)

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "stable + dmd#15343"

[maxhaton]

necessary?

[dkorpel]

No

[PetarKirov]

@dkorpel I've rebased your PR to see how it fares on the CI nowadays.

[PetarKirov]

Not ready (yet):

... runnable/test23722.d           -betterC  -fPIC (-inline -release -g -O)==============================
Test 'runnable/test23722.d' failed. The logged output:
/home/runner/work/dmd/dmd/generated/linux/release/64/dmd -conf= -m64 -Irunnable -betterC  -fPIC  -odtest_results/runnable -c  runnable/test23722.d
/home/runner/work/dmd/dmd/generated/linux/release/64/dmd -conf= -m64 -Irunnable -betterC  -fPIC  -odtest_results/runnable -c  runnable/imports/test23722b.d
/home/runner/work/dmd/dmd/generated/linux/release/64/dmd -conf= -m64  -fPIC  -odtest_results/runnable -oftest_results/runnable/test23722_0 test_results/runnable/test23722.o test_results/runnable/test23722b.o
test_results/runnable/test23722.o:runnable/test23722.d:function _D9test237224do_yFZv: error: undefined reference to '_D10test23722b1A14__lambda_L6_C5FNbNiZv'
collect2: error: ld returned 1 exit status
Error: linker exited with status 1
==============================
Test 'runnable/test23722.d' failed: Expected rc == 0, but exited with rc == 1

[yanok]

Is this abandoned? Should I create a new PR? This is a quite nasty issue, can cause calling incorrect function at runtime.

[yanok]

@dkorpel Do you plan to update this PR?

[thewilsonator]

Should I create a new PR?

Yes, rebases of old PRs are always welcome. Note that this one failed CI so will need some changes.

[dkorpel]

I'll rebase this and take another look at the undefined reference problem

[yanok]

I'll rebase this and take another look at the undefined reference problem

Thanks a lot! I've also updated the bug. tl;dr: I believe this fix is a correct one, but the example in the bug triggers two separate issues, and the second one gets uncovered by this fix.

[dkorpel]

Yes, perhaps the test case should have -allinst for now.

Edit: not sure that affects the test case actually

[yanok]

Well, it's not an instantiation, so I think -allinst is not supposed to help. I've posted a smaller example that fails to compile due to this issue, you could use it as a test for the lambda naming issue.

[thewilsonator]

Looks good. Please resolve as appropriate 3706959#r1239935088

[yanok]

Uh, unfortunately I don't think that's a full fix... It's a bit more complicated than that.

generateIdWithLoc has additional logic to make the ids unique by adding a suffix if the same (prefix, loc) is requested more than once. But that's precisely what we want to avoid with lambdas and separate compilation: we could end up referencing both __lambda_LOC and __lambda_LOC_1, while the module with the definition only has __lambda_LOC...

[dkorpel]

You're right. I recall unittests used to simply use a counter, which wouldn't be stable with separate compilation.
Then the idea was to use source location, which is deterministic, but not unique when using templates / mixins / static foreach etc.
So now we have source location + counter, which works either with separate compilation or templates, but not both at the same time.

Hashing the function body wouldn't work, because you can have multiple identical anonymous functions.

The only thing I can think of is keeping track of a stack of instantation locations and combining all those until you get something unique.

[yanok]

You're right. I recall unittests used to simply use a counter, which wouldn't be stable with separate compilation. Then the idea was to use source location, which is deterministic, but not unique when using templates / mixins / static foreach etc. So now we have source location + counter, which works either with separate compilation or templates, but not both at the same time.

I think templates and mixins are covered, since they will have all the instantiations/mixins in the final symbol name, the only place there we really need to make lambda names based on Loc unique is static foreach and loop unrolling. I've added a commit yanok@19f6680 on top of your changes to implement this. PTAL. That combination makes it work for me (we are using LDC).

I failed to make a really failing example to showcase the problem with making lambdas inside templates even more unique, probably that problem with missing symbols I saw is LDC specific... What I can get with DMD so far is only code duplication: mod1.o contains f!1.__lambda_LOC_1 and mod2.o has f!1.__lambda_LOC while they are exactly the same (and had the same name before your changes).

The only thing I can think of is keeping track of a stack of instantation locations and combining all those until you get something unique.

Yes, that's the idea! We don't need to track them, I think, since the parent scope name is already what we want. Also, just adding that directly to the lambda ident will work, but with a lot of duplication. D's symbols are already long enough ;)

[dkorpel]

Meanwhile I'll merge this as a partial fix, making a note in the issue.