Fix Issue 19731 - auto struct methods whose address is taken don't test invariants #9467

RazvanN7 · 2019-03-19T13:09:22Z

Alternative fix to: [1].

When a struct is semantically analyzed, if any member functions (for some reason) have had semantic3 ran on them and the struct has invariants, then the function is reduced to having just semantic ran on it. As things stand, I don't see any other way, as semantic3 for function declarations is a monster function that does tons of things besides solving the return type of a function.

CC @FeepingCreature

[1] #9448

dlang-bot · 2019-03-19T13:09:25Z

Thanks for your pull request and interest in making D better, @RazvanN7! 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
✓	19731	normal	auto struct methods whose address is taken don't test invariants

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

jacob-carlborg · 2019-03-19T13:37:02Z

Does this only apply to structs?

RazvanN7 · 2019-03-19T13:40:16Z

@jacob-carlborg No, it also applies to classes, but I haven't figured a solution to that.

RazvanN7 · 2019-03-19T13:45:43Z

Any ideas why the autotester is failing my test with SegFault while circleCi passes? I cannot reproduce the failure locally.

thewilsonator

It would be nice to convert the loop to use all early returns to reduce nesting.

RazvanN7 · 2019-03-19T14:26:36Z

Any ideas why the autotester is failing my test with SegFault while circleCi passes? I cannot reproduce the failure locally.

Got it; it's because of -inline -O; it seems that with these 2 switches the compiler catches null dereferences and outputs an error. Is there a way to tell the test runner to not use -inline and -O ?

RazvanN7 · 2019-03-20T09:15:36Z

I added the fix for classes. I think this is ready to be merged

…st invariants

jacob-carlborg · 2019-03-20T10:11:24Z

src/dmd/dsymbolsem.d

+    private void reinforceInvariant(AggregateDeclaration ad, Scope* sc)
+    {
+        // for each member
+        for(int i = 0; i < ad.members.dim; i++)


foreachDsymbol.

kinke · 2019-04-27T13:05:26Z

test/runnable/test19731.d

+
+    auto obj7()
+    {
+        return this.obj_;


For LDC, the this VarDeclaration's parent2 isn't the codegen'd Foo.obj7 anymore (so we wrongly assume it's a nested variable). It's another obj7, so I guess the one from the first semantic pass...

dnadlinger · 2019-05-05T15:11:06Z

@RazvanN7: See @kinke's comment – it seems like this broke the resulting AST in quite a nasty way (assuming this is not LDC-specific, which it doesn't appear to be). Any pointers as to the best fix would be appreciated.

kinke · 2019-05-05T15:26:04Z

I gained more insight (analysis performed with LDC and its slightly modified front-end): expressionSemantic() for a ThisExp doesn't just set its type (and do nothing if it's already set), but may also set the ThisExp.var VarDeclaration. This crucial step is skipped after syntaxCopying an already analyzed ThisExp, as a later expressionSemantic() for that copy was a no-op and var still pointed to the original one.

The syntaxCopied testcase's ThisExp.var still points to the variable in the original function, so LDC assumes it is a nested variable and fails to find a matching parent function.

With this (edit: updated) override in ThisExp:

override Expression syntaxCopy()
{
    auto r = cast(ThisExp) super.syntaxCopy();
    // require new semantic (possibly new `var` etc.)
    r.type = null;
    r.var = null;
    return r;
}

the testcase works fine.

expressionSemantic() for a ThisExp doesn't just set its type (and do nothing if it's already set), but may also set the ThisExp.var VarDeclaration. This crucial step was previously skipped after syntaxCopying an already analyzed ThisExp, as a later expressionSemantic() for that copy was a no-op and `var` still pointed to the original one. This fixes runnable/test19731.d, as dlang/dmd#9467 may lead to a syntaxCopy of a sema3'd FuncDeclaration, and that copy replacing the original function. A ThisExp's `var` still pointed to the variable in the original function, so LDC assumed it was a nested variable and failed to find a matching parent function.

kinke · 2019-05-05T16:25:57Z

So the questions are

Is this just the tip of the ice berg?
Why does DMD not suffer from/bail out on this?

ibuclaw · 2019-05-06T04:11:48Z

Why does DMD not suffer from/bail out on this?

DMD has never been too strict on its own codegen AST.

ibuclaw · 2019-05-06T04:46:30Z

Is this just the tip of the ice berg?

It would appear at a naive glance that TraitsExp is doing something nasty to force complete a decl whose context is only partially done.

One litmus test to see how deep the rabbit hole goes would be to replace semanticRun with a getter/setter, and enforce in the setter that a decl cannot be marked PASSsemanticX/PASSsemanticXdone unless its parent is also PASSsemanticX (in the middle of running that semantic).

expressionSemantic() for a ThisExp doesn't just set its type (and do nothing if it's already set), but may also set the ThisExp.var VarDeclaration. This crucial step was previously skipped after syntaxCopying an already analyzed ThisExp, as a later expressionSemantic() for that copy was a no-op and `var` still pointed to the original one, while that variable may have been syntaxCopied too. See the comments in dlang#9467; this is covered by an existing test which happens to pass with DMD (but failed with LDC).

See dlang#9467. Be less optimistic, as IMO this approach is likely to cause issues.

ibuclaw · 2019-08-22T20:22:38Z

Another regression this has caused, is that now functions are sent to the backend twice. First without the invariant code, and second with invariant code.

ibuclaw · 2019-08-22T20:39:39Z

And infact, you can reproduce this in dmd by compiling the test with -inline.

ibuclaw · 2019-08-22T20:41:59Z

@RazvanN7 - I've reopened the issue.

dlang-bot added the Bug Fix label Mar 19, 2019

thewilsonator approved these changes Mar 19, 2019

View reviewed changes

RazvanN7 force-pushed the Issue_19731 branch from f35426d to 26a60cc Compare March 19, 2019 13:52

thewilsonator reviewed Mar 19, 2019

View reviewed changes

RazvanN7 force-pushed the Issue_19731 branch 2 times, most recently from 330207e to 9005e32 Compare March 20, 2019 09:15

Fix Issue 19731 - auto struct methods whose address is taken don't te…

3f52528

…st invariants

RazvanN7 force-pushed the Issue_19731 branch from 9005e32 to 3f52528 Compare March 20, 2019 09:17

thewilsonator approved these changes Mar 20, 2019

View reviewed changes

thewilsonator added the auto-merge label Mar 20, 2019

jacob-carlborg reviewed Mar 20, 2019

View reviewed changes

dlang-bot merged commit 3a298e5 into dlang:master Mar 20, 2019

kinke reviewed Apr 27, 2019

View reviewed changes

kinke mentioned this pull request Apr 27, 2019

Upgrade front-end & libs to v2.086.0 ldc-developers/ldc#3062

Merged

kinke added a commit to kinke/dmd that referenced this pull request May 18, 2019

Don't gag errors when replacing function by re-analyzed clone

67931be

See dlang#9467. Be less optimistic, as IMO this approach is likely to cause issues.

kinke mentioned this pull request May 19, 2019

[stable] Enforce re-semantic for ThisExp.syntaxCopy() result #9819

Merged

ibuclaw mentioned this pull request Jul 16, 2021

Revert "Fix Issue 22118 - Const union causes false multiple-initialization error in constructor" #12881

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Issue 19731 - auto struct methods whose address is taken don't test invariants #9467

Fix Issue 19731 - auto struct methods whose address is taken don't test invariants #9467

RazvanN7 commented Mar 19, 2019 •

edited

dlang-bot commented Mar 19, 2019

jacob-carlborg commented Mar 19, 2019

RazvanN7 commented Mar 19, 2019

RazvanN7 commented Mar 19, 2019 •

edited

thewilsonator left a comment

RazvanN7 commented Mar 19, 2019 •

edited

RazvanN7 commented Mar 20, 2019

jacob-carlborg Mar 20, 2019

kinke Apr 27, 2019

dnadlinger commented May 5, 2019

kinke commented May 5, 2019 •

edited

kinke commented May 5, 2019

ibuclaw commented May 6, 2019

ibuclaw commented May 6, 2019

ibuclaw commented Aug 22, 2019

ibuclaw commented Aug 22, 2019

ibuclaw commented Aug 22, 2019

Fix Issue 19731 - auto struct methods whose address is taken don't test invariants #9467

Fix Issue 19731 - auto struct methods whose address is taken don't test invariants #9467

Conversation

RazvanN7 commented Mar 19, 2019 • edited

dlang-bot commented Mar 19, 2019

Bugzilla references

Testing this PR locally

jacob-carlborg commented Mar 19, 2019

RazvanN7 commented Mar 19, 2019

RazvanN7 commented Mar 19, 2019 • edited

thewilsonator left a comment

Choose a reason for hiding this comment

RazvanN7 commented Mar 19, 2019 • edited

RazvanN7 commented Mar 20, 2019

jacob-carlborg Mar 20, 2019

Choose a reason for hiding this comment

kinke Apr 27, 2019

Choose a reason for hiding this comment

dnadlinger commented May 5, 2019

kinke commented May 5, 2019 • edited

kinke commented May 5, 2019

ibuclaw commented May 6, 2019

ibuclaw commented May 6, 2019

ibuclaw commented Aug 22, 2019

ibuclaw commented Aug 22, 2019

ibuclaw commented Aug 22, 2019

RazvanN7 commented Mar 19, 2019 •

edited

RazvanN7 commented Mar 19, 2019 •

edited

RazvanN7 commented Mar 19, 2019 •

edited

kinke commented May 5, 2019 •

edited