[stable] Try to fix #21331#21343
Conversation
|
Thanks for your pull request and interest in making D better, @kinke! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
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 referencesYour 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 locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "stable + dmd#21343" |
| nm ${objfile} | grep -E '16__lambda_L13_C17.+15__lambda_L5_C19' | ||
| nm ${objfile} | grep -E '16__lambda_L13_C17.+17__lambda_L5_C19_1' | ||
| nm ${objfile} | grep -E '18__lambda_L13_C17_1.+15__lambda_L5_C19' | ||
| nm ${objfile} | grep -E '18__lambda_L13_C17_1.+17__lambda_L5_C19_1' |
There was a problem hiding this comment.
I gave up on a testcase for the actual issue, wrt. empty tuples in parameter UDAs being 'optimized' later and affecting toPrettyChars().
Instead, I went with this - v2.111 produces these symbols:
_D9test21331__T1SSQq3barFZ16__lambda_L13_C17ZQBe3fooMFZ__T15__lambda_L5_C19TiZQuFNaNbNiNfiZi
_D9test21331__T1SSQq3barFZ16__lambda_L13_C17ZQBe3fooMFZ__T17__lambda_L5_C19_1TiZQwFNaNbNiNfiZi
_D9test21331__T1SSQq3barFZ18__lambda_L13_C17_1ZQBg3fooMFZ__T17__lambda_L5_C19_2TiZQwFNaNbNiNfiZi
_D9test21331__T1SSQq3barFZ18__lambda_L13_C17_1ZQBg3fooMFZ__T17__lambda_L5_C19_3TiZQwFNaNbNiNfiZi
I.e., the inner lambda got renamed to _2 and _3 when the outer one was renamed to _1. Using the parent mangle avoids this and makes the inner one start without suffix again.
compiler/src/dmd/expressionsem.d
Outdated
| func.localsymtab = new DsymbolTable(); | ||
| } | ||
| symtab = func.localsymtab; | ||
| parentMangle = cast(string) mangleExact(func).toDString(); |
There was a problem hiding this comment.
This one is cached (in FuncDeclaration.mangleString).
|
Also passes LDC CI, and the LDC with this patch doesn't hit #21331 for that Symmetry code base anymore. |
|
Is merging blocked on failed CI tasks? They seem to be irrelevant. Can we rebase and retry? |
|
Yeah those failures are obviously unrelated and fixed in the meantime. The only thing I'm unsure about is whether getting the mangle of the parent might have unexpected side effects, like extra/earlier semantic analysis for the parent. |
This cherry-picks one commit from dlang/dmd#21343
|
I guess what could happen is that parent function mangles are now generated earlier, potentially before semantic analysis of the parent is completed. And as they are cached in The first variant, using the class-ref identity of the parent, seems safer in that regard. So I rather tend towards the original variant. |
|
This good to go? |
|
No; I was hoping @yanok would give some feedback wrt. reverting to the original variant. |
Yes, that might be the case. But did we see anything like this in the wild? With a proper dependency tracking I believe asking for a mangle should trigger all the inference and complain if it has to be changed later... but that's not the case with the current implementation...
I had some concerns about multiple |
|
I'm going to test building Weka code with the first version of the fix today, will post an update here. |
To answer my own question: yes, I do indeed see mangles of symbols loosing attributes wrt to unpatched ldc version, so the version with parent's mangle has undesired side-effects indeed. Still verifying the version with the ref comparison, the first results are good. |
|
I did testing with the ref comparison (the version in the first commit), so far so good. So I guess that's the version we want to proceed with. In the long run, we probably want to shift the uniqueness logic from |
This reverts commit cfaa3c4.
|
Perfect Ilya, thx a lot for testing! So I guess this is ready now; the commits are to be squashed (I left the 2nd variant in, just so that it's in the history of this PR).
Yeah I guess it's only statically unrolled loops which require the suffices for all nested lambdas. |
4 participants
See #17102 (comment) and following.