fix Issue 12486 - Function returning struct isn't called if `enum` of its result is accessed by FeepingCreature · Pull Request #8013 · dlang/dmd

FeepingCreature · 2018-03-12T10:03:17Z

Fix issue 12486

When accessing an enum member of a struct returned by an expression with side effects, don't discard the expression but evaluate it anyways. (No, this is not safe even if the expression is pure: it might loop infinitely.)

dlang-bot · 2018-03-12T10:03:19Z

Thanks for your pull request and interest in making D better, @FeepingCreature! 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
✗	12486	major	Function returning struct isn't called if `enum` of its result is accessed

FeepingCreature · 2018-03-12T11:09:20Z

Ah, I see, it has to be an expression but it is a struct.
Wait, huh?

FeepingCreature · 2018-03-12T11:52:32Z

Is there a way to say "the expression is trivial"? I don't like enumerating all the different kinds of trivial expressions manually. Maybe optimize just the comma expression, somehow?

wilzbach · 2018-03-12T12:42:22Z

Weird, auto-tester seems to be running the wrong version of the code. Gonna wait for everything to finish, then kick it off again.

No need to wait. auto-tester automatically refreshes itself though it might take a bit until it reruns your PR as there's usually a lot of traffic.

FeepingCreature · 2018-03-12T13:01:17Z

Okay. e.optimize is definitely the wrong way to go about this. Is there any way to decide whether an Expression is relatively trivial?

FeepingCreature · 2018-03-12T13:16:44Z

:stares at ci Jenkins:

Do you ever get that feeling that, while all the tests are green, they really shouldn't be?

wilzbach · 2018-03-12T13:25:29Z

Do you ever get that feeling that, while all the tests are green, they really shouldn't be?

Jenkins doesn't run the testsuite. It just compiles dmd and then builds ~40 projects and runs their testsuite.

FeepingCreature · 2018-03-12T14:32:21Z

:stares at AppVeyor:

Do you ever get that feeling that, while all the tests are green, they really shouldn't be?

I feel like the only reason this works is that nobody, me included, has come up with a sufficiently clever piece of code to break it yet.

And yet, it does seem to work everywhere, and the error it fixes was really bad, so...

FeepingCreature · 2018-03-12T16:15:42Z

Is it stuck? auto-tester has been at pending: 8 for hours now.

wilzbach · 2018-03-12T16:29:04Z

Is it stuck? auto-tester has been at pending: 8 for hours now.

No, auto-tester deletes all builds whenever a new PR is merged at DMD, druntime or phobos. Click on the link to see the history.
Don't worry too much about it. Once a PR is approved, it gets priority and lands in a special auto-merge queue.

FeepingCreature · 2018-03-13T13:27:00Z

How do I find a reviewer?

FeepingCreature · 2018-03-14T09:05:56Z

src/dmd/typesem.d

-                ve = ve.expressionSemantic(sc);
+                if (e.op == TOK.type || e.op == TOK.this_ || e.op == TOK.super_ || e.op == TOK.variable)
+                {
+                    ve = ve.expressionSemantic(sc);


The point here is to avoid being forced to optimize the expression if we know the lhs of the comma expression would be trivial, since optimization may change the meaning but is sometimes necessary so we don't have a pointless comma expression lying around. I wish there was a better way to do this.

Please add this explanation as a comment in the code. It will be useful in the future when people will try to understand the code.

Documentation added.

ibuclaw · 2018-03-14T09:51:28Z

How do I find a reviewer?

Ping or assign me and I'll get round to it. 😉

FeepingCreature · 2018-03-14T11:40:58Z

Um ... @ibuclaw ping? I don't know how to assign people. I don't think I can do that.

FeepingCreature · 2018-03-15T15:32:16Z

I debugged what's happening with the Phobos tests.

The Cartesian products being computed in std.setops are ridiculously too large. They look small, but due to the fact that 3D nested Cartesian product is strongly (quadratically) biased towards spread along the first dimension, actually take billions to trillions of cycles to run.

This was not noticed before because due to the bug, they essentially passed by coincidence. (canFind calls any, which is broken for infinite ranges.)

I'll make a pr to reduce the sizes of the last dimensions massively.

edit: To clarify what's happening here: DMD is right, the Phobos tests are wrong.

edit: Fix merged in Phobos. Tests should start passing now.

WalterBright · 2018-03-16T19:11:31Z

src/dmd/typesem.d

+                // variable.enum or type.enum, where we know the lhs does not have side effects,
+                // just use the enum directly.
+                // It would be preferable if we could just directly check if the left hand side of the
+                // expression is obviously side-effect free, but there does not seem to be a way to do that.


See https://github.com/dlang/dmd/blob/master/src/dmd/sideeffect.d

As I understand it, a function that never returns but is pure will still be considered as not having side effects by this code? But we must not optimize it away: we may optimize >1 calls to 1 call, but we mustn't optimize 1 call to 0 calls.

WalterBright · 2018-03-16T19:17:15Z

src/dmd/typesem.d

+                    ve = ve.expressionSemantic(sc);
+                }
+                else
+                {


This fix looks like it's in the wrong place. expressionSemantic() should not be removing expressions with side effects. Putting the fix here is papering over the problem for this case, and it'll just cause other problems. Need to find the root problem.

Right, the problem is if I keep the side effects via comma expression, as I have to in order to get correct code in the case call().ENUM, then I end up with unusable comma expressions downstream in the cases where the lhs genuinely is trivial and can be discarded safely. I can't fix it earlier, because this is where the access resolves.

edit: To clarify, what DMD does right now is removing side effects from expressions in that code: call().ENUM becomes ENUM, with call() being discarded. The problem is fixing that without breaking enum access for trivial lhs, say alias foo = this.bar.

what DMD does right now is removing side effects from expressions in that code: call().ENUM becomes ENUM, with call() being discarded.

That's a bug, as call() has side effects. That's where it needs to be fixed.

The problem is fixing that without breaking enum access for trivial lhs, say alias foo = this.bar.

Maybe that's there the special case should go.

I don't understand what you mean. This is where that happens, isn't it?

WalterBright

Fix is in the wrong place.

FeepingCreature · 2018-03-19T16:02:50Z

Ping? See review comments. The code is the place where the side effects are currently discarded.

quickfur · 2018-03-20T20:05:52Z

@FeepingCreature I think what Walter meant is that the proper fix should probably be somewhere in sideeffect.d so that the code here, unchanged, will do the right thing. In other words, the fact that the code here is discarding side effects when it shouldn't, means that sideeffect.d (or whatever else is the root cause of this problem) isn't doing its job properly. If sideeffect.d were doing its job, the code here should already do the right thing as-is.

quickfur · 2018-03-20T20:11:23Z

More specifically, there may be a defect somewhere in hasSideEffect or callSideEffect that isn't detecting the side-effect when it should be. Or perhaps hasSideEffect or callSideEffect isn't getting called when it should be, resulting in the compiler throwing away code thinking that there are no side-effects, when in fact there are.

FeepingCreature · 2018-03-20T23:30:29Z

Well, are infinite loops supposed to count as side effects? If not, istm there is a lack of an idiom here. isDisposable or such.

ibuclaw · 2018-03-21T06:13:15Z

Well, are infinite loops supposed to count as side effects? If not, istm there is a lack of an idiom here. isDisposable or such.

All calls should be considered as a side effect until inlined. There is isTrivialExp in the sideeffects module that likely fits your needs.

FeepingCreature · 2018-03-21T07:05:08Z

Thanks, that seems sensible. I'll fix it in a while.

I still don't get why Walter thinks the fix is in the wrong place. This is the place that throws away the side effect right now.

edit: Trying!

edit: Okay, how about this now.

edit: This is not working at all. It treats it like a property access, despite being an enum. Going back to the version that worked, for now.

…ember on its returned type

FeepingCreature · 2018-03-21T11:04:53Z

@WalterBright I tried to keep it as a DotExp instead of forcing it into a CommaExp, but if I do so all the way through codegen, it produces invalid code because (I guess) it seems to try and treat the enum access as a struct read. ISTM that CommaExp is just how enum dot accesses to expressions with side effects have to look right now. If it's possible to make a better fix for this, I don't know how. I don't think I can get the code materially better than this.

FeepingCreature · 2018-03-26T07:29:50Z

ping

ibuclaw

Seems reasonable to me.

@FeepingCreature - please rebase to restart failing tests.
@WalterBright - check this again.

wilzbach · 2018-03-26T07:45:55Z

please rebase to restart failing tests.

No need. auto-tester will automatically restart itself and all other CIs are passing.
Regarding the failures on auto-tester - they are spurious and I just submitted:

dlang/phobos#6347

which hopefully helps to weed them out.

FeepingCreature · 2018-04-03T08:17:10Z

ping

ibuclaw · 2018-04-04T20:47:26Z

I'll compile this patch in locally and inspect the change done, then will try to break it. 😉

If all is OK I see no problem with committing this.

ibuclaw · 2018-04-07T08:43:55Z

src/dmd/typesem.d

                }
                checkAccess(e.loc, sc, null, v);
                Expression ve = new VarExp(e.loc, v);
+                if (!isTrivialExp(e))


@FeepingCreature - you could also use hasSideEffects(e) (or e.hasSideEffects() if you prefer UFCS). When having an initial glance at the implementation, it didn't seem like it was a strong enough guarantee, but having a closer look, only strongly pure functions would have their calls omitted here. Which is harmless.

As far as I can tell, pure void foo() { while (true) { } } is strongly pure, but must under no circumstances be optimized away. That exact case was what triggered this bug in the first place.

Pure expressions can be reduced from two to one, but only trivial expressions can be reduced from one to zero.

Ah, bugger those looping pure functions. :-)

Yep, makes sense, and LGTM.

@FeepingCreature - For brownie points, you can add a comment with a link to the bugzilla reference.

ibuclaw · 2018-04-08T17:07:26Z

@WalterBright - Any last comments before I dismiss your stale review?

FeepingCreature force-pushed the fix/12486-dont-discard-expression-on-enum-access branch from 04df52e to b31b110 Compare March 12, 2018 10:08

FeepingCreature changed the title ~~Fix/12486 dont discard expression on enum access~~ Fix 12486: Function returning struct isn't called if enum of its result is accessed Mar 12, 2018

FeepingCreature changed the title ~~Fix 12486: Function returning struct isn't called if enum of its result is accessed~~ fix Issue 12486 - Function returning struct isn't called if enum of its result is accessed Mar 12, 2018

FeepingCreature force-pushed the fix/12486-dont-discard-expression-on-enum-access branch 4 times, most recently from 221f387 to 6f1efcc Compare March 12, 2018 11:51

FeepingCreature force-pushed the fix/12486-dont-discard-expression-on-enum-access branch 3 times, most recently from 469af15 to 809989c Compare March 12, 2018 12:12

FeepingCreature force-pushed the fix/12486-dont-discard-expression-on-enum-access branch from 809989c to fbb2755 Compare March 12, 2018 13:04

FeepingCreature force-pushed the fix/12486-dont-discard-expression-on-enum-access branch from fbb2755 to 1fd4769 Compare March 12, 2018 19:58

Add test from bug dlang#12486

0a6d7c4

FeepingCreature force-pushed the fix/12486-dont-discard-expression-on-enum-access branch from 1fd4769 to be22c7d Compare March 14, 2018 08:55

FeepingCreature commented Mar 14, 2018

View reviewed changes

FeepingCreature mentioned this pull request Mar 15, 2018

Reduce size of slow testcase in std.algorithm.setops dlang/phobos#6280

Merged

FeepingCreature force-pushed the fix/12486-dont-discard-expression-on-enum-access branch from be22c7d to 3be7e80 Compare March 16, 2018 15:03

WalterBright reviewed Mar 16, 2018

View reviewed changes

WalterBright requested changes Mar 16, 2018

View reviewed changes

FeepingCreature force-pushed the fix/12486-dont-discard-expression-on-enum-access branch 4 times, most recently from 003dadf to 416d231 Compare March 21, 2018 10:04

Fix dlang#12486: avoid discarding expression when accessing an enum m…

775ee59

…ember on its returned type

FeepingCreature force-pushed the fix/12486-dont-discard-expression-on-enum-access branch from 416d231 to 775ee59 Compare March 21, 2018 10:17

ibuclaw approved these changes Mar 26, 2018

View reviewed changes

ibuclaw self-assigned this Apr 4, 2018

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

ibuclaw reviewed Apr 7, 2018

View reviewed changes

ibuclaw added Merge:auto-merge and removed Merge:72h no objection -> merge The PR will be merged if there are no objections raised. labels Apr 8, 2018

dlang-bot merged commit 3cf5d36 into dlang:master Apr 8, 2018

FeepingCreature mentioned this pull request Jun 5, 2018

Add pureFree to core.memory dlang/druntime#1836

Merged

Uh oh!

Conversation

FeepingCreature commented Mar 12, 2018

Uh oh!

dlang-bot commented Mar 12, 2018

Bugzilla references

Uh oh!

FeepingCreature commented Mar 12, 2018

Uh oh!

FeepingCreature commented Mar 12, 2018

Uh oh!

wilzbach commented Mar 12, 2018

Uh oh!

FeepingCreature commented Mar 12, 2018

Uh oh!

FeepingCreature commented Mar 12, 2018

Uh oh!

wilzbach commented Mar 12, 2018

Uh oh!

FeepingCreature commented Mar 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FeepingCreature commented Mar 12, 2018

Uh oh!

wilzbach commented Mar 12, 2018

Uh oh!

FeepingCreature commented Mar 13, 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!

ibuclaw commented Mar 14, 2018

Uh oh!

FeepingCreature commented Mar 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FeepingCreature commented Mar 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

FeepingCreature Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WalterBright left a comment

Choose a reason for hiding this comment

Uh oh!

FeepingCreature commented Mar 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

quickfur commented Mar 20, 2018

Uh oh!

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

Uh oh!

Uh oh!

ibuclaw commented Mar 21, 2018

Uh oh!

FeepingCreature commented Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FeepingCreature commented Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FeepingCreature commented Mar 12, 2018 •

edited

Loading

FeepingCreature commented Mar 14, 2018 •

edited

Loading

FeepingCreature commented Mar 15, 2018 •

edited

Loading

FeepingCreature Mar 16, 2018 •

edited

Loading

FeepingCreature Mar 17, 2018 •

edited

Loading

FeepingCreature commented Mar 19, 2018 •

edited

Loading

quickfur commented Mar 20, 2018 •

edited

Loading

FeepingCreature commented Mar 20, 2018 •

edited

Loading

FeepingCreature commented Mar 21, 2018 •

edited

Loading

FeepingCreature commented Mar 21, 2018 •

edited

Loading

FeepingCreature Apr 7, 2018 •

edited

Loading