Make lambda symbols stable post a3abf1187ec019fac1ae98ffe78d68d7523b5665 by yanok · Pull Request #17102 · dlang/dmd

yanok · 2024-12-02T16:20:27Z

Commit a3abf11 fixes some cases of lambdas having unstable symbol names between compilation units by using generateIdWithLoc to generate stable lambda names, however since LOC doesn't uniquely identify a lambda instance (because templates, mixins, static foreach and foreach unrolling), generateIdWithLoc adds a counter, so there is still some instability going on.

generateIdWithLoc makes the name uniq per file+loc, by adding adding a numeric suffix. But the order of instantiations might be different across compilation units, so with this counting scheme we are back to unstable names, so one module might have

t!0.__lambda_LOC and
t!1.__lambda_LOC_1

while another one has

t!1.__lambda_LOC

This is not a critical problem, but at very least the code gets duplicated for no reason. I also have an example where it leads to linking error, but since it's not a small one and fails to minimize further, I suspect it's a result of interaction with some other bug.

The thing is we don't even need uniqueness for those lambdas inside templates/mixins: their final names will have the instantiation prefix anyway. But we can't also just disable this uniqueness check completely: static foreach as well as unrollings of the normal foreach with lambdas in the loop body will have several copies of a single lambda with the same file+loc. So here we do want to keep making them unique. Fortunately, I don't think a foreach could be iterated in different order in different compilation units, so hopefully if we limit the counting to this case only, it won't make symbols unstable.

To implement this idea, I've added an extra parent argument to generateIdWithLoc: it works like using parent ~ prefix prefix, but without adding parent to the final output.

dlang-bot · 2024-12-02T16:20:31Z

Thanks for your pull request and interest in making D better, @yanok! 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.

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 "master + dmd#17102"

yanok · 2024-12-02T16:22:57Z

/cc @dkorpel

dkorpel · 2024-12-02T16:49:18Z

Looks good so far, are you going to make use of it or is that for a next Pull Request?

yanok · 2024-12-03T09:15:51Z

Looks good so far, are you going to make use of it or is that for a next Pull Request?

I definitely want this for our local branch:

Without it there is even more code duplication, and we have a lot of duplicated code already :)
I have one case there linking breaks without this fix: module A gets an unresolved symbol for a lambda inside a template and module B has a definition of that symbol, but with a different suffix. This might be LDC specific or another bug, I still plan to debug it more, but didn't have time recently.

But I also think it's the right thing to merge into the main branch as well, since it finally stabilize lambda symbols across compilation unit.

dkorpel · 2024-12-03T09:35:47Z

But in this Pull Request, the new parent parameter is yet unused, or am I missing something?

yanok · 2024-12-03T10:33:03Z

But in this Pull Request, the new parent parameter is yet unused, or am I missing something?

Ouch, you are right, sorry. Must have been lost with the merge. Let me add it back and update the PR.

compiler/src/dmd/expressionsem.d

dkorpel

Do you have a test case for this?

yanok · 2024-12-03T15:58:07Z

Do you have a test case for this?

I have a nice small test case for lambda getting another name, but that example works in a sense: the lambda just gets emitted twice, but the binary links and runs. So I can only detect it with nm and not by compiling and running. Is it possible to use nm from a shell test?

dkorpel · 2024-12-03T16:05:44Z

Is it possible to use nm from a shell test?

Probably, but I'm not a fan of shell tests for specific implementation details, because they break easily and are hard to debug. I was hoping maybe there's an easy .mangleof test or something, but if not, I'm fine with merging as is.

yanok · 2024-12-03T17:20:27Z

I was hoping maybe there's an easy .mangleof test or something, but if not, I'm fine with merging as is.

It's a cool idea! Unfortunately I've tried and failed :( We need a mangle of a lambda/delegate literal, but it looks like mangleof doesn't work for rvalues: the mangle I get doesn't even contain lambda or template name... And if I put it into a lvalue, I get a mangle of this lvalue, of course.

yanok · 2024-12-03T23:50:51Z

UPD: I've managed to make an example with mangleof, will add a test tomorrow.

Commit a3abf11 fixes some cases of lambdas having unstable symbol names between compilation units by using `generateIdWithLoc` to generate stable lambda names, however since LOC doesn't uniquely identify a lambda instance (because templates, mixins, static foreach and foreach unrolling), `generateIdWithLoc` adds a counter, so there is still some instability going on. `generateIdWithLoc` makes the name uniq per file+loc, by adding adding a numeric suffix. But the order of instantiations might be different across compilation units, so with this counting scheme we are back to unstable names, so one module might have `t!0.__lambda_LOC` and `t!1.__lambda_LOC_1` while another one has `t!1.__lambda_LOC` This is not a critical problem, but at very least the code gets duplicated for no reason. I also have an example where it leads to linking error, but since it's not a small one and fails to minimize further, I suspect it's a result of interaction with some other bug. The thing is we don't even need uniqueness for those lambdas inside templates/mixins: their final names will have the instantiation prefix anyway. But we can't also just disable this uniqueness check completely: `static foreach` as well as unrollings of the normal `foreach` with lambdas in the loop body will have several copies of a single lambda with the same file+loc. So here we do want to keep making them unique. Fortunately, I don't think a `foreach` could be iterated in different order in different compilation units, so hopefully if we limit the counting to this case only, it won't make symbols unstable. To implement this idea, I've added an extra `parent` argument to `generateIdWithLoc`: it works like using `parent ~ prefix` prefix, but without adding `parent` to the final output. Fixes since last review: 1. Changed `fromStringz` to `toDString` 2. Added a test to showcase the problem

yanok · 2024-12-04T10:07:17Z

I was hoping maybe there's an easy .mangleof test or something, but if not, I'm fine with merging as is.

Added a test, PTAL.

dkorpel · 2024-12-04T10:46:44Z

LGTM

kinke · 2025-05-05T14:05:38Z

This approach apparently suffers from unstable toPrettyChars() results for a specific symbol, depending on semantic analysis: #21331 (comment). Would the sc.parent identity work for Weka too, i.e., just using the sc.parent as void* for the Key.parent field? Edit: #21343.

yanok · 2025-05-05T14:42:17Z

This approach apparently suffers from unstable toPrettyChars() results for a specific symbol, depending on semantic analysis: #21331 (comment). Would the sc.parent identity work for Weka too, i.e., just using the sc.parent as void* for the Key.parent field?

@kinke Uh.. sorry for the mess. I think comparing by reference should generally work too... my only concern are template instances, maybe we have to pass sc.parent.inst if it's non-null, so we catch "different but the same" template instances case.

Can't we get the mangle of sc.parent? That would be the safest thing to do, since the ultimate goal of this third key is to avoid adding suffixes to things that would already have different mangles because of different parents. So what really matters is the parent's mangle.

kinke · 2025-05-05T15:15:29Z

Thx, that's a good idea I think - pushed as 2nd commit in #21343.

…al), to prevent linker errors for separate compilation case where the lambda is not emitted (because supposedly already emitted in linked library). Internal linkage breaks that assumption (because not linkable by other module). Lambda mangles should be stable now, because of dlang/dmd#17102 (and related earlier fixes).

yanok force-pushed the lambda-mangle2 branch 2 times, most recently from 5436517 to 44429c2 Compare December 3, 2024 10:41

dkorpel reviewed Dec 3, 2024

View reviewed changes

compiler/src/dmd/expressionsem.d Outdated Show resolved Hide resolved

dkorpel approved these changes Dec 3, 2024

View reviewed changes

yanok force-pushed the lambda-mangle2 branch from d73a4e1 to 451c159 Compare December 4, 2024 10:05

dkorpel added the Merge:auto-merge label Dec 4, 2024

dkorpel merged commit b076573 into dlang:master Dec 4, 2024

This was referenced Dec 7, 2024

CI: Add extra Phobos tests (publictests and betterc make targets) #17108

Closed

[stable] Cherry-pick #17102 to fix regression #17109

Merged

kinke mentioned this pull request May 5, 2025

[REG2.111] Colliding lambda symbol mangles #21331

Closed

kinke mentioned this pull request May 5, 2025

[stable] Try to fix #21331 #21343

Merged

Uh oh!

Conversation

yanok commented Dec 2, 2024

Uh oh!

dlang-bot commented Dec 2, 2024

Bugzilla references

Testing this PR locally

Uh oh!

yanok commented Dec 2, 2024

Uh oh!

dkorpel commented Dec 2, 2024

Uh oh!

yanok commented Dec 3, 2024

Uh oh!

dkorpel commented Dec 3, 2024

Uh oh!

yanok commented Dec 3, 2024

Uh oh!

Uh oh!

dkorpel left a comment

Choose a reason for hiding this comment

Uh oh!

yanok commented Dec 3, 2024

Uh oh!

dkorpel commented Dec 3, 2024

Uh oh!

yanok commented Dec 3, 2024

Uh oh!

yanok commented Dec 3, 2024

Uh oh!

yanok commented Dec 4, 2024

Uh oh!

dkorpel commented Dec 4, 2024

Uh oh!

kinke commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yanok commented May 5, 2025

Uh oh!

kinke commented May 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kinke commented May 5, 2025 •

edited

Loading