fix Issue 18868 - nondeterministic static ctor/dtor by JohanEngelen · Pull Request #8255 · dlang/dmd

JohanEngelen · 2018-05-16T22:49:56Z

Instead of using a counter to create unique static ctor and dtor function identifiers, use the deterministic line+column location that is also used for generating unittest function identifiers.

dlang-bot · 2018-05-16T22:49:58Z

Thanks for your pull request and interest in making D better, @JohanEngelen! 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
✓	18868	critical	Separate compilation generates two static this functions, runs it twice

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

Regression or critical bug fixes should always target the stable branch. Learn more about rebasing to stable or the D release process.

To target stable perform these two steps:

Rebase your branch to upstream/stable:

git rebase --onto upstream/stable upstream/master

Change the base branch of your PR to stable

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

Shachar · 2018-05-17T05:40:28Z

If the same static this is instantiated several times due to different template extraction, using the line number would create false-positive on the duplicate detection.

jacob-carlborg · 2018-05-17T08:14:36Z

src/dmd/identifier.d

+     * Generate deterministic named Id based on the `loc` such that
+     * the naming is consistent across multiple compilations.
+     * `prefix` must be a null-terminated string.
+     */


New functions should have standard Ddoc sections like Returns: and Params.

jacob-carlborg · 2018-05-17T08:14:58Z

test/runnable/imports/test18868_fls.d

@@ -0,0 +1,28 @@
+module imports.test18868_fls;
+
+template FLS(T) {


Curly braces on their own lines.

JohanEngelen · 2018-05-17T14:42:58Z

If the same static this is instantiated several times due to different template extraction, using the line number would create false-positive on the duplicate detection.

If it is exactly the same instantiation, yes the names will be the same which is exactly what we want, and there is no duplicate detection error because that doesn't happen with templated functions/variables (ODR rules apply and template symbols are marked to be merged by the linker if identical symbols are found). Edit: this is exactly what is happening in the test case.
If it is not exactly the same instantiation, the other parts of the symbol name will be different and there is no name clash.

JohanEngelen · 2018-05-17T19:23:20Z

I don't see how the jenkins failure is related to this PR.

wilzbach · 2018-05-17T20:01:38Z

I restarted Jenkins, let's hope it was just a spurious failure in Ocean.

JohanEngelen · 2018-05-17T20:45:22Z

@wilzbach Thanks, but it still fails :(

/var/lib/jenkins/dlang_projects@4/sociomantic-tsunami/ocean/integrationtest/unixlistener/main.d(281):
Error: incompatible types for `(handleEcho) : (handleShutdown)`:
`void function(const(char)[] args, void delegate(const(char)[] response) send_response) @system` and 
`void delegate(const(char)[] args, void delegate(const(char)[] response) send_response) @system`

In https://github.com/sociomantic-tsunami/ocean/blob/v4.1.x/integrationtest/unixlistener/main.d#L281

JohanEngelen · 2018-05-17T20:48:17Z

Confirmed not related to this PR: #8257 has the same problem.

WalterBright · 2018-05-17T20:59:32Z

src/dmd/identifier.d

+     *      Identifier (inside Identifier.idPool) with deterministic name based
+     *      on the source location.
+     */
+    static Identifier generateIdWithLoc(const(char)* prefix, const ref Loc loc)


Any reason this can't be string prefix ?

No, this should be string. Except when I do that:
dmd/identifier.d(151): Error: string is used as a type

Turns out Identifier has a const char* member called string! >_<

@WalterBright I've changed it to string, by prefixing with .. Ugly, but this PR shouldn't become a refactoring of Identifier. I'll do that now in another PR.

The refactoring PR has been merged now, the leading dot can now be removed?

@jacob-carlborg Yes, done.

wilzbach · 2018-05-18T01:24:23Z

Regarding the Jenkins failure - yes, the Ocean failure unrelated.
I disabled it temporarily -> dlang/ci#208 (after the next push, Jenkins should be green again).

jacob-carlborg · 2018-05-18T11:18:54Z

src/dmd/identifier.d

+     * such that the name is consistent across multiple compilations.
+     * Params:
+     *      prefix      = first part of the identifier name.
+     *      loc         = source location to use is the identifier name.


is doesn't sound correct. Should it be in?

Actually, I think think it should be "as".

should be in.

jacob-carlborg · 2018-05-18T11:22:26Z

Please squash commits when done.

JohanEngelen · 2018-05-18T11:40:54Z

all done

jacob-carlborg · 2018-05-18T11:54:06Z

Should this go to the stable branch?

JohanEngelen · 2018-05-18T11:58:36Z

Should this go to the stable branch?

It's a fairly critical bug, I find.
But because it depends on the refactor in identifier.d ....................................

JohanEngelen · 2018-05-18T12:07:10Z

Rebased and retargeted to stable

JohanEngelen · 2018-05-19T17:35:04Z

I don't understand why it is failing after rebasing onto stable. It's failing on a "__unittest*" declaration, but that name has not changed with this PR (the code is almost identical, replacing a printf with a number of print statements...). Don't know what's happening here...

JohanEngelen · 2018-05-19T21:45:06Z

https://issues.dlang.org/show_bug.cgi?id=18880

JohanEngelen · 2018-05-19T22:22:42Z

The source location is not unique enough, we need another unique-ifying deterministic term:

static foreach(s; ["666", "777"]) {
    mixin(genCtor(s));
}

int i;

string genCtor(string a)
{
    return "static this() { i += " ~ a ~ "; }";
}

void main()
{
    assert(i == 0 + 666 + 777);
}

JinShil · 2018-05-20T02:43:32Z

Maybe add a counter to compilation unit that can be incremented each time an id is generated and appended to the location-based identifier.

JohanEngelen · 2018-05-20T09:30:09Z

Maybe add a counter to compilation unit that can be incremented each time an id is generated and appended to the location-based identifier.

That won't be deterministic because it will depend on the order in which things are compiled.

Shachar · 2018-05-20T14:35:42Z

The only thing that comes to mind is a "static foreach iteration counter".

They will have to be recursivable (i.e.: if there is a static foreach inside another static foreach).

dnadlinger · 2018-05-20T18:34:29Z

Last time I thought about this (it's been a year or so since), my conclusion was that the way to go for these issues was to stick with a counter, but to make it local within each scope.

Scope in this sense means everything that is possibly an "entry point" for semantic analysis, i.e. modules, template instances, etc. The scope name would be part of the mangled symbol name, and within each such scope care would have to be taken that semantic analysis always proceeds linearly to make sure the symbol indices are stable.

There might be a few tricky cases to take care of regarding on-demand semantic analysis of forward references (i.e. we might need to disallow some more cyclic dependencies), but something like this is the only universal scheme I managed to come up with.

jacob-carlborg · 2018-05-20T18:37:57Z

What about using both? Both file name plus line number and a counter. Try to have a local counter so it will be 0 in most cases.

dnadlinger · 2018-05-20T18:48:05Z

What about using both? Both file name plus line number and a counter.

By the time you have a scheme that is stable for separate compilation, file name and line number don't serve to disambiguate any further. You can still include them to aid humans in tracing back symbol names to source code, of course.

JohanEngelen · 2018-05-22T17:33:47Z

Scope in this sense means everything that is possibly an "entry point" for semantic analysis, i.e. modules, template instances, etc. The scope name would be part of the mangled symbol name, and within each such scope care would have to be taken that semantic analysis always proceeds linearly to make sure the symbol indices are stable.

I think this won't work unless you define a new scope for each different particular template instantiation of the same source template. Because the order in which those are SemA'd is not deterministic.

An easy approach for this is (using nameclash as poor man's "scope detection" ;-)) : use linenumber+columnnumber. If a new symbol is generated and that name already exists: suffix with ever increasing counter. I'll implement this scheme and see if that does work well:

name = "_L_C", if (!name.alreadyInIdPool) return name;
tryname = name ~ "_1"; if (!tryname.alreadyInIdPool) return tryname;
tryname = name ~ "_2; if (!tryname.alreadyInIdPool) return tryname;
...

Extra problematic issue is that the current mixin linenumber scheme is already flawed:

two mixins on same line will have same line number and same columnnumber
two mixins on different lines can be crafted to result in same linenumber...
Resolution of these more advanced issues are for another PR/fix.

JohanEngelen · 2018-05-22T21:22:43Z

This PR also fixes https://issues.dlang.org/show_bug.cgi?id=18880 which is the same issue that is plaguing the static ctor/dtors.

JohanEngelen · 2018-05-22T21:24:49Z

I've implemented a basic version of the proposed unique-ify scheme.
It is not waterproof: a user can add the mangled name into the stringtable (a user variable?) to muck with it. But I find it highly unlikely that someone would do something like that unknowingly, so perhaps that caveat is acceptable.

jacob-carlborg

The code changes look good to me, not sure if it will be enough to fix the problems.

jacob-carlborg · 2018-05-23T05:59:25Z

src/dmd/identifier.d

+        buf.print(loc.charnum);
+        auto basesize = buf.peekSlice().length;
+        uint counter = 1;
+        while (stringtable.lookup(buf.peekSlice().ptr, buf.peekSlice().length) !is null) {


Curly braces on their own lines.

JohanEngelen · 2018-05-27T10:02:08Z

All good now, ready for merging.

Instead of using a counter to create unique static ctor and dtor function identifiers, use the deterministic line+column location that is also used for unittests. The further disambiguate mixin instantiations on the exact same location, use a local counter.

JohanEngelen · 2018-05-27T14:02:33Z

Squashed and rebased into one commit onto current master.

JohanEngelen · 2018-05-27T14:33:40Z

The semaphoreci failure is unrelated (the semaphore testing is very flaky, do more people experience that?).

JohanEngelen · 2018-05-27T14:50:01Z

Would appreciate if someone could restart the semaphoreci tester.

wilzbach · 2018-05-27T15:49:39Z

Restarted SemaphoreCi (and added you to the SemaphoreCI collaborators)
For the record the failure was:

../generated/linux/release/64/dmd -conf= -m64 -Irunnable  -fPIC -L-lstdc++  -odtest_results/runnable -oftest_results/runnable/externmangle_0  runnable/externmangle.d test_results/runnable/externmangle.cpp.o 
runnable/externmangle.d(128): Error: undefined identifier `cpp_ulong`, did you mean alias `c_ulong`?
runnable/externmangle.d(128): Error: undefined identifier `cpp_long`, did you mean alias `c_long`?
runnable/externmangle.d(128): Error: undefined identifier `cpp_ulong`, did you mean alias `c_ulong`?
runnable/externmangle.d(129): Error: undefined identifier `cpp_ulonglong`
runnable/externmangle.d(129): Error: undefined identifier `cpp_longlong`
runnable/externmangle.d(129): Error: undefined identifier `cpp_ulonglong`
runnable/externmangle.d(130): Error: undefined identifier `cpp_size_t`
runnable/externmangle.d(130): Error: undefined identifier `cpp_ptrdiff_t`
runnable/externmangle.d(130): Error: undefined identifier `cpp_size_t`


==============================
Test runnable/externmangle.d failed: expected rc == 0, exited with rc == 1

Seems like https://github.com/dlang/dmd/blob/master/test/runnable/externmangle.d was recently modified...

JohanEngelen requested a review from RazvanN7 as a code owner May 16, 2018 22:49

dlang-bot added the Severity:Bug Fix label May 16, 2018

jacob-carlborg reviewed May 17, 2018

View reviewed changes

jacob-carlborg suggested changes May 17, 2018

View reviewed changes

jacob-carlborg approved these changes May 17, 2018

View reviewed changes

WalterBright reviewed May 17, 2018

View reviewed changes

JohanEngelen force-pushed the fix18868 branch from a784312 to 4d49b89 Compare May 17, 2018 21:23

JohanEngelen force-pushed the fix18868 branch from 4d49b89 to 0583d54 Compare May 18, 2018 09:53

JohanEngelen added a commit to weka/ldc that referenced this pull request May 18, 2018

Backport dlang/dmd#8255 to fix WEKAPP-65659

5b10e5f

jacob-carlborg reviewed May 18, 2018

View reviewed changes

JohanEngelen force-pushed the fix18868 branch from 0583d54 to a76b6ad Compare May 18, 2018 11:39

JohanEngelen added a commit to weka/ldc that referenced this pull request May 18, 2018

Backport dlang/dmd#8255 to fix WEKAPP-65659

612c130

jacob-carlborg approved these changes May 18, 2018

View reviewed changes

JohanEngelen force-pushed the fix18868 branch from a76b6ad to 748eba1 Compare May 18, 2018 12:06

JohanEngelen requested a review from Geod24 as a code owner May 18, 2018 12:06

JohanEngelen changed the base branch from master to stable May 18, 2018 12:06

JohanEngelen force-pushed the fix18868 branch from 748eba1 to 8e84a96 Compare May 19, 2018 21:16

jacob-carlborg reviewed May 23, 2018

View reviewed changes

JohanEngelen changed the title ~~[WIP] fix Issue 18868 - nondeterministic static ctor/dtor~~ fix Issue 18868 - nondeterministic static ctor/dtor May 27, 2018

JohanEngelen added a commit to weka/ldc that referenced this pull request May 27, 2018

Backport dlang/dmd#8255 to fix WEKAPP-65659

80a36b5

JohanEngelen added a commit to weka/ldc that referenced this pull request May 27, 2018

Backport dlang/dmd#8255 to fix WEKAPP-65659

16ef71f

RazvanN7 removed the Review:WIP Work In Progress - not ready for review or pulling label May 27, 2018

RazvanN7 approved these changes May 27, 2018

View reviewed changes

JohanEngelen added a commit to weka/ldc that referenced this pull request May 27, 2018

Backport dlang/dmd#8255 to fix WEKAPP-65659

4fd8c54

JohanEngelen force-pushed the fix18868 branch from e50467e to 8491556 Compare May 27, 2018 14:02

jacob-carlborg approved these changes May 28, 2018

View reviewed changes

jacob-carlborg added the Merge:auto-merge label May 28, 2018

dlang-bot merged commit cf19b78 into dlang:master May 28, 2018

JohanEngelen deleted the fix18868 branch May 28, 2018 18:39

		@@ -0,0 +1,28 @@
		module imports.test18868_fls;

		template FLS(T) {

Uh oh!

Comments

Conversation

JohanEngelen commented May 16, 2018

Uh oh!

dlang-bot commented May 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

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

Testing this PR locally

Uh oh!

Shachar commented May 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JohanEngelen commented May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JohanEngelen commented May 17, 2018

Uh oh!

wilzbach commented May 17, 2018

Uh oh!

JohanEngelen commented May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JohanEngelen commented May 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented May 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacob-carlborg commented May 18, 2018

Uh oh!

JohanEngelen commented May 18, 2018

Uh oh!

jacob-carlborg commented May 18, 2018

Uh oh!

JohanEngelen commented May 18, 2018

Uh oh!

JohanEngelen commented May 18, 2018

Uh oh!

JohanEngelen commented May 19, 2018

Uh oh!

JohanEngelen commented May 19, 2018

Uh oh!

JohanEngelen commented May 19, 2018

Uh oh!

JinShil commented May 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JohanEngelen commented May 20, 2018

Uh oh!

Shachar commented May 20, 2018

Uh oh!

dnadlinger commented May 20, 2018

Uh oh!

jacob-carlborg commented May 20, 2018

Uh oh!

dnadlinger commented May 20, 2018

Uh oh!

dlang-bot commented May 16, 2018 •

edited

Loading

JohanEngelen commented May 17, 2018 •

edited

Loading

JohanEngelen commented May 17, 2018 •

edited

Loading

JinShil commented May 20, 2018 •

edited

Loading