Fix 14831 - Each function local symbols should have unique mangled name #12235

MoonlightSentinel · 2021-02-26T22:19:20Z

PR #12119 already fixed collisions for several declarations but missed
local variables.

The check for isDataSeg was a remnant of the old error raised for
static/__gshared variables. Removing it enables the mangling fixup
for local variables.

Note that this doesn't fix https://issues.dlang.org/show_bug.cgi?id=10619
which is not directly related to the mangling. See #12236.

Changes to other TEST_OUTPUT sections are caused by different sizes of localsymtab which defines the sequence number for anonymous symbols.

dlang-bot · 2021-02-27T00:03:16Z

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

Auto-close	Bugzilla	Severity	Description
✓	14831	major	Each function local symbols should have unique mangled name

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#12235"

ibuclaw · 2021-02-27T04:37:28Z

src/dmd/expressionsem.d

                if ((s.isFuncDeclaration() ||
                     s.isAggregateDeclaration() ||
                     s.isEnumDeclaration() ||
                     s.isTemplateDeclaration() ||
-                     v && v.isDataseg()) && !sc.func.localsymtab.insert(s))
+                     v


Maybe put this first in the order of conditions, to keep it in order of least to most expensive.

WalterBright · 2021-02-27T06:39:55Z

test/compilable/test14831.d

+void main()
+{
+    {
+        int x;


What would ever be the use of mangling a stack variable?

Hmm, I missed that earlier in the review. You're right, local vars should not have a mangle.

Passing local variables as template alias parameters, see the test case in #12236:

void main() { { int x = 1; print!x(); pragma(msg, print!x.mangleof); } { int x = 2; print!x(); pragma(msg, print!x.mangleof); } }

Both instances of print receive the same mangling (_D9test10619__T5printS_DQw4mainFZ1xiZQwMFNbNiZv) without this patch.

With this patch:

_D9test10619__T5printS_DQw4mainFZ1xiZQwMFNbNiZv _D9test10619__T5printS_DQw4mainFZ5__S11xiZQBbMFNbNiZv

That's correct, though, isn't it? The resultant code could use localized frames/closures if we know that the current scope isn't the top-level function scope.

void main() { __closure_main __closure; { __scoped_closure_main1 __scoped_closure; __scoped_closure.x = 1; print(&__scoped_closure); // print!x() } { __scoped_closure_main2 __scoped_closure; __scoped_closure.x = 2; print(&__scoped_closure); // print!x(); } __closure.y =1; print(&__closure); // print!y(); }

That the current implementation of closures puts both local x variables in the same closure record (at different offsets) is an issue of the closure implementation.

To suggest another left field idea, you could also detect local variable name collisions and put them in a union within the closure record.

So you end up with:

void main() { struct __closure_main { union { int x; long x; real x; int x; } int y; } __closure_main __closure; { __closure.x = 1; // int x = 1; print(&__closure); // print!x(); } { __closure.x = 2; // long x = 2; print(&__closure); // print!x(); } { __closure.x = 3.4; // real x = 3.4; print(&__closure); // print!x(); } { __closure.x = 5; // int x = 5; print(&__closure); // print!x(); } __closure.y = 6; // int y = 6; print(&__closure); // print!y(); }

Your suggestions assume that it's valid to merge both instances IIUC.
This might not be the case for such templates because one can inspect the alias parameter, e.g.

void print(alias symbol)() { import core.stdc.stdio : printf; printf("%d at line %d\n", symbol, __traits(getLocation, symbol)[1]); } void main() { { int x = 1; print!x(); } { int x = 2; print!x(); } }

Hmm, but your suggestion of giving the local variable a different name would mean that you could not use x to look-up the value in a debugger, no?

What about shadowing detection? This would also bypass that as well?

Hmm, but your suggestion of giving the local variable a different name would mean that you could not use x to look-up the value in a debugger, no?

Debug information for local informations is not affected by this change (but is not correct atm)

Small test case:

/* https://issues.dlang.org/show_bug.cgi?id=10619 REQUIRED_ARGS: -g PERMUTE_ARGS: GDB_SCRIPT: --- b gdb10619.d:26 b gdb10619.d:30 run info locals print x continue info locals print x continue --- */ void main() { { int x = 1; print!x(); // first breakpoint } { int x = 2; print!x(); // second breakpoint } } void print(alias symbol)() {}

Stable:

Breakpoint 1 at 0x80b: file runnable/gdb10619.d, line 26. Breakpoint 2 at 0x819: file runnable/gdb10619.d, line 30. [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1". Breakpoint 1, D main () at runnable/gdb10619.d:26 26 print!x(); x = 1 x = 0 $1 = 1 Breakpoint 2, D main () at runnable/gdb10619.d:30 30 print!x(); x = 1 x = 2 $2 = 1 [Inferior 1 (process 466) exited normally]

This PR:

Breakpoint 1 at 0x80b: file runnable/gdb10619.d, line 26. Breakpoint 2 at 0x819: file runnable/gdb10619.d, line 30. [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1". Breakpoint 1, D main () at runnable/gdb10619.d:26 26 print!x(); x = 1 x = 0 $1 = 1 Breakpoint 2, D main () at runnable/gdb10619.d:30 30 print!x(); x = 1 x = 2 $2 = 1 [Inferior 1 (process 553) exited normally]

EDIT: DMD apparently doesn't emit the actual blocks using DW_TAG_lexical_block.

What about shadowing detection? This would also bypass that as well?

Shadowing detection is not affected by this PR, e.g.

void main() { int x; { int x = 1; // Error: variable `x` is shadowing variable `gdb10619.main.x` } }

PR dlang#12119 already fixed collisions for several declarations but missed local variables. The check for `isDataSeg` was a remnant of the old error raised for `static`/`__gshared` variables. Removing it enables the mangling fixup for local variables. Note that this doesn't fix https://issues.dlang.org/show_bug.cgi?id=10619 which is unrelated to the mangling.

MoonlightSentinel · 2021-02-28T12:23:16Z

test/runnable/test10619.d

 RUN_OUTPUT:
 ---
 1
-1
+2
 3
 4
 ---
-
-print => 2 will be fixed by https://github.com/dlang/dmd/pull/12235


Rebased to stable to expand on #12236 (as both fix wrong code)

RazvanN7

Looks good to me

MoonlightSentinel force-pushed the unique-local-mangling branch 2 times, most recently from e5472f9 to b4619da Compare February 26, 2021 23:27

MoonlightSentinel marked this pull request as ready for review February 26, 2021 23:54

dlang-bot added the Bug Fix label Feb 27, 2021

MoonlightSentinel force-pushed the unique-local-mangling branch from b4619da to d0aae10 Compare February 27, 2021 00:08

ibuclaw reviewed Feb 27, 2021

View reviewed changes

WalterBright reviewed Feb 27, 2021

View reviewed changes

MoonlightSentinel force-pushed the unique-local-mangling branch from d0aae10 to 1d2b5ec Compare February 28, 2021 12:20

MoonlightSentinel changed the base branch from master to stable February 28, 2021 12:21

MoonlightSentinel commented Feb 28, 2021

View reviewed changes

RazvanN7 approved these changes Mar 1, 2021

View reviewed changes

thewilsonator added the auto-merge label Mar 1, 2021

dlang-bot merged commit 8b653c5 into dlang:stable Mar 1, 2021

MoonlightSentinel deleted the unique-local-mangling branch June 20, 2021 19:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix 14831 - Each function local symbols should have unique mangled name #12235

Fix 14831 - Each function local symbols should have unique mangled name #12235

MoonlightSentinel commented Feb 26, 2021 •

edited

dlang-bot commented Feb 27, 2021 •

edited

ibuclaw Feb 27, 2021

WalterBright Feb 27, 2021

ibuclaw Feb 27, 2021

MoonlightSentinel Feb 27, 2021 •

edited

ibuclaw Feb 27, 2021 •

edited

ibuclaw Feb 27, 2021 •

edited

MoonlightSentinel Feb 27, 2021

ibuclaw Feb 28, 2021

ibuclaw Feb 28, 2021

MoonlightSentinel Feb 28, 2021 •

edited

MoonlightSentinel Feb 28, 2021

MoonlightSentinel Feb 28, 2021 •

edited

RazvanN7 left a comment

Fix 14831 - Each function local symbols should have unique mangled name #12235

Fix 14831 - Each function local symbols should have unique mangled name #12235

Conversation

MoonlightSentinel commented Feb 26, 2021 • edited

dlang-bot commented Feb 27, 2021 • edited

Bugzilla references

Testing this PR locally

ibuclaw Feb 27, 2021

Choose a reason for hiding this comment

WalterBright Feb 27, 2021

Choose a reason for hiding this comment

ibuclaw Feb 27, 2021

Choose a reason for hiding this comment

MoonlightSentinel Feb 27, 2021 • edited

Choose a reason for hiding this comment

ibuclaw Feb 27, 2021 • edited

Choose a reason for hiding this comment

ibuclaw Feb 27, 2021 • edited

Choose a reason for hiding this comment

MoonlightSentinel Feb 27, 2021

Choose a reason for hiding this comment

ibuclaw Feb 28, 2021

Choose a reason for hiding this comment

ibuclaw Feb 28, 2021

Choose a reason for hiding this comment

MoonlightSentinel Feb 28, 2021 • edited

Choose a reason for hiding this comment

MoonlightSentinel Feb 28, 2021

Choose a reason for hiding this comment

MoonlightSentinel Feb 28, 2021 • edited

Choose a reason for hiding this comment

RazvanN7 left a comment

Choose a reason for hiding this comment

MoonlightSentinel commented Feb 26, 2021 •

edited

dlang-bot commented Feb 27, 2021 •

edited

MoonlightSentinel Feb 27, 2021 •

edited

ibuclaw Feb 27, 2021 •

edited

ibuclaw Feb 27, 2021 •

edited

MoonlightSentinel Feb 28, 2021 •

edited

MoonlightSentinel Feb 28, 2021 •

edited