Fix invalid characters in mangling by jacob-carlborg · Pull Request #7531 · dlang/dmd

jacob-carlborg · 2017-12-27T00:15:46Z

Running the DMD test suite with a compiler compiled in debug mode fails due to invalid characters, -, exist in the mangled names of some unittest blocks.

This fix centralizes the validation of mangled characters and properly replaces - in the mangled names of unittest blocks with _.

The issue was introduced in #6727.

dlang-bot · 2017-12-27T00:15:47Z

Thanks for your pull request and interest in making D better, @jacob-carlborg! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
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
✓	19050	regression	Running the DMD test suite with a compiler compiled in debug mode fails due to invalid characters, -

Testing this PR locally

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

dub fetch digger
dub run digger -- build "master + dmd#7531"

wilzbach · 2017-12-27T00:52:34Z

CIs are failing with:

 ... fail_compilation/ice14424.d    -o- -unittest ()
Test failed.  The logged output:
../generated/linux/release/64/dmd -conf= -m64 -Ifail_compilation -verrors=0 -o- -unittest  -odtest_results/fail_compilation -oftest_results/fail_compilation/ice14424_0.o -c fail_compilation/ice14424.d
fail_compilation/ice14424.d(12): Error: `tuple(__unittest_fail_compilation_imports_a14424.d_3_0)` has no effect


==============================
Test failed: 
expected:
----
fail_compilation/ice14424.d(12): Error: `tuple(__unittest_fail_compilation_imports_a14424_d_3_0)` has no effect
----
actual:
----
fail_compilation/ice14424.d(12): Error: `tuple(__unittest_fail_compilation_imports_a14424.d_3_0)` has no effect
----

Makefile:204: recipe for target 'test_results/fail_compilation/ice14424.d.out' failed

Also mind the d-unit failure:

dunit 1.0.14: building configuration "dunit-test-library"...

source/dunit/mockable.d-mixin-156(156,78): Error: semicolon expected, not `.`

source/dunit/mockable.d-mixin-156(156,104): Error: no identifier for declarator `.d_431_0FZ1T7method1MxFiZi`

source/dunit/mockable.d-mixin-156(157,79): Error: semicolon expected, not `.`

source/dunit/mockable.d-mixin-156(157,105): Error: no identifier for declarator `.d_431_0FZ1T7method2MxFiZv`

source/dunit/mockable.d-mixin-156(158,97): Error: semicolon expected, not `.`

source/dunit/mockable.d-mixin-156(158,129): Error: no identifier for declarator `.d_431_0FZ1T7method3MxFNaNbNfiZi`

source/dunit/mockable.d-mixin-156(159,98): Error: semicolon expected, not `.`

source/dunit/mockable.d-mixin-156(159,130): Error: no identifier for declarator `.d_431_0FZ1T7method4MxFNaNbNfiZv`

source/dunit/mockable.d-mixin-156(160,100): Error: semicolon expected, not `.`

source/dunit/mockable.d-mixin-156(160,132): Error: no identifier for declarator `.d_431_0FZ1T7method5MxFNaNbNeiZi`

source/dunit/mockable.d-mixin-156(161,101): Error: semicolon expected, not `.`

source/dunit/mockable.d-mixin-156(161,133): Error: no identifier for declarator `.d_431_0FZ1T7method6MxFNaNbNeiZv`

source/dunit/mockable.d(111,14): Error: template instance dunit.mockable.__unittest_source_dunit_mockable.d_431_0.T.Mockable!(T).Mock!(T) error instantiating

source/dunit/mockable.d(447,23):        instantiated from here: getMock!()

source/dunit/mockable.d-mixin-156(156,78): Error: semicolon expected, not `.`

source/dunit/mockable.d-mixin-156(156,104): Error: no identifier for declarator `.d_464_1FZ1T7method1MxFiZi`

source/dunit/mockable.d-mixin-156(157,79): Error: semicolon expected, not `.`

source/dunit/mockable.d-mixin-156(157,105): Error: no identifier for declarator `.d_464_1FZ1T7method2MxFiZv`

source/dunit/mockable.d-mixin-156(158,97): Error: semicolon expected, not `.`

source/dunit/mockable.d-mixin-156(158,129): Error: no identifier for declarator `.d_464_1FZ1T7method3MxFNaNbNfiZi`

source/dunit/mockable.d-mixin-156(159,98): Error: semicolon expected, not `.`

WalterBright · 2017-12-27T08:06:25Z

This is a regression fix, and so should have a bugzilla entry for it.

jacob-carlborg · 2017-12-27T11:44:57Z

I looked at one of the failing tests in Phobos. The test inspects the demangle of a symbol. Before this PR the mangled name was _D3std6traits32__unittest_std_traits_d_7443_193FZ3fooi, with this PR the mangled name is _D3std6traits32__unittest_std_traits.d_7443_193FZ3fooi. Note the dot between traits and d. The demangler fails to demangle this name with the dot.

I did what @ibuclaw suggested on Slack and took the validation that is performed on pragma(mangle) and reused that, which allows a dot, while the code before replaced any dots with an underscore.

rainers · 2017-12-27T13:06:20Z

I would restrain from putting any special character into the mangling as this will byte you back later. For example @ has some special meaning on linux (version information IIRC).

So, . in the name should be avoided, it also doesn't fit the current mangling grammar.

The validation in dmangle is not strict enough for most D symbols and too strict for symbols in general, as any characters can creep into the mangled string as part of a template alias parameter.

jacob-carlborg · 2017-12-27T13:35:30Z

@rainers so perhaps we shouldn't use the exact same validation everywhere. Because with pragma(mangle) one should be able to use all the special symbols, in my opinion.

rainers · 2017-12-27T14:24:30Z

Yes, I support the comment in dymbolsem.d: "Ideally pragma(mangle) can accept a string of any content."
That would mean we cannot make any simple reliable validation, though. Running core.demangle on the mangled name might work, but will cause a bad performance penalty.

jacob-carlborg · 2017-12-27T20:13:39Z

I would expect core.demangle to only work on the D mangling, not arbitrary mangled name put in pragma(mangle).

rainers · 2017-12-27T20:22:17Z

Yes, core.demangle doesn't work for the plain symbol set by pragma(mangle), but should be able to handle "embedded" symbols, i.e. as template alias parameters. That means it skips them correctly while decoding the rest.

jacob-carlborg · 2017-12-27T21:51:04Z

So what kind of validation would we like to have in the case of unittest blocks?

rainers · 2017-12-28T08:50:43Z

So what kind of validation would we like to have in the case of unittest blocks?

Not sure it needs any validation, because I don't think the filename is a good identifier to begin with. It might contain relative or absolute path components which makes it rather unpredictable.

I'd recommend using the module name instead. If you want to replace the dots with an underscore and disambiguate with a module name already containing the underscore, use the mangling scheme for the module name, e.g. 3pkg7modname.

Long term I think all those internal identifiers should follow the normal naming scheme, e.g. 3pkg7modname13__unittest_L2 or even the line number somehow not part of the identifier, but for eexample as an (template) argument. Typeinfo is another example that decodes badly.

wilzbach · 2018-01-25T16:52:12Z

Rebased and interestingly modifying the test was no longer required.

jacob-carlborg · 2018-01-25T21:47:31Z

Rebased and interestingly modifying the test was no longer required.

The tests are still not passing. I think Atila's PR #7761 will fix this as well. But this might be good to have anyway.

wilzbach

Rebased again. As #7761 was merged, the modification in of UnitTestDeclaration::createIdentifier was no longer needed and maybe this finally passes the testsuite.

Geod24 · 2018-07-02T05:05:11Z

src/dmd/func.d

 import dmd.declaration;
 import dmd.delegatize;
 import dmd.dinterpret;
+import dmd.dmangle;


Doesn't seem used anywhere ?

Indeed. Most likely the original patch had the semantic pass in this module. Now it's been moved to dsymbolsem.

ibuclaw · 2018-07-03T05:57:54Z

Why is this still open? Valid code doesn't assemble because of this.

Running the DMD test suite with a compiler compiled in debug mode fails due to invalid characters, `-`, exist in the mangled names of some unittest blocks. This fix centralizes the validation of mangled characters and properly replaces `-` in the mangled names of unittest blocks with `_`.

wilzbach · 2018-07-03T06:07:35Z

Why is this still open? Valid code doesn't assemble because of this.

Because it was failing on auto-tester for a long time and then was having merge conflicts.
I don't see any other reason.

jacob-carlborg requested a review from WalterBright as a code owner December 27, 2017 00:15

jacob-carlborg requested a review from ibuclaw December 27, 2017 00:16

wilzbach added Review:Blocking Other Work review and pulling should be a priority Severity:Regression PRs that fix regressions labels Dec 29, 2017

dlang-bot added Review:Needs Rebase Review:Needs Work labels Jan 1, 2018

wilzbach force-pushed the fix_invalid_mangling branch from 31b08eb to a81e139 Compare January 25, 2018 16:47

dlang-bot removed Review:Needs Work Review:Needs Rebase labels Jan 25, 2018

dlang-bot added Review:Needs Rebase Review:Needs Work labels Feb 4, 2018

wilzbach force-pushed the fix_invalid_mangling branch from a81e139 to d08ed57 Compare July 1, 2018 20:19

dlang-bot removed Review:Needs Work Review:Needs Rebase labels Jul 1, 2018

wilzbach approved these changes Jul 1, 2018

View reviewed changes

Geod24 reviewed Jul 2, 2018

View reviewed changes

wilzbach force-pushed the fix_invalid_mangling branch from d08ed57 to ad25e05 Compare July 3, 2018 06:03

wilzbach force-pushed the fix_invalid_mangling branch from ad25e05 to 9d08623 Compare July 3, 2018 06:05

dlang-bot added the Severity:Bug Fix label Jul 3, 2018

wilzbach added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Jul 3, 2018

Geod24 approved these changes Jul 3, 2018

View reviewed changes

JinShil approved these changes Jul 8, 2018

View reviewed changes

JinShil added the Merge:auto-merge label Jul 8, 2018

wilzbach closed this Jul 9, 2018

wilzbach reopened this Jul 9, 2018

wilzbach merged commit 81082d5 into dlang:master Jul 9, 2018

jacob-carlborg deleted the fix_invalid_mangling branch July 9, 2018 06:33

Uh oh!

Comments

Conversation

jacob-carlborg commented Dec 27, 2017

Uh oh!

dlang-bot commented Dec 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

Testing this PR locally

Uh oh!

wilzbach commented Dec 27, 2017

Uh oh!

WalterBright commented Dec 27, 2017

Uh oh!

jacob-carlborg commented Dec 27, 2017

Uh oh!

rainers commented Dec 27, 2017

Uh oh!

jacob-carlborg commented Dec 27, 2017

Uh oh!

rainers commented Dec 27, 2017

Uh oh!

jacob-carlborg commented Dec 27, 2017

Uh oh!

rainers commented Dec 27, 2017

Uh oh!

jacob-carlborg commented Dec 27, 2017

Uh oh!

rainers commented Dec 28, 2017

Uh oh!

wilzbach commented Jan 25, 2018

Uh oh!

jacob-carlborg commented Jan 25, 2018

Uh oh!

wilzbach left a comment

Choose a reason for hiding this comment

Uh oh!

Geod24 Jul 2, 2018

Choose a reason for hiding this comment

Uh oh!

ibuclaw Jul 3, 2018

Choose a reason for hiding this comment

Uh oh!

ibuclaw commented Jul 3, 2018

Uh oh!

wilzbach commented Jul 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

dlang-bot commented Dec 27, 2017 •

edited

Loading