fix Issue 13586 - Destructors not run when argument list evaluation throws #4078

WalterBright · 2014-10-20T20:27:43Z

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

WalterBright · 2014-10-22T04:33:04Z

This is necessary for reference counting to work reliably.

yebblies · 2014-10-22T04:35:54Z

src/backend/gflow.c

@@ -774,7 +774,7 @@ STATIC void defstarkill()

 #if 1
 /* The following program fails for this:
-import core.stdc.stdio;
+import std.c.stdio;


A merge error I'd overlooked. Fixed.

yebblies · 2014-10-22T04:44:40Z

Why can't this be done entirely in the glue layer? Have you tested that the modifications to the ast don't break ctfe?

Why is the new volatile stc necessary? Would it be necessary if this was all done in the glue layer instead of half in the frontend?

WalterBright · 2014-10-22T05:30:37Z

Why can't this be done entirely in the glue layer?

Note the changes to how struct value arguments are handled in functionParameters(). Doing it in the glue layer would require awkward detection and undoing of those changes. The new code does it all in the front end. I thought about this for several days before deciding this was the most practical approach.

Also, note that the glue layer still handles the actual insertion of destructor code.

Also, the logic in the glue layer should be minimal. There's already too much in there. The idea with lowering in the front end is to reduce code and bugs by exploiting commonality through rewrite rules.

Have you tested that the modifications to the ast don't break ctfe?

I don't know why it would. Why should it? Note that what this logic does is equivalent to:

   (S(), foo(), T())

which had better work in CTFE.

Why is the new volatile stc necessary?

In order to prevent data flow optimizations to it, since the destructor code can be called out of line. This follows the optimizer logic that basically disables optimizations in finally blocks.

Would it be necessary if this was all done in the glue layer instead of half in the frontend?

Yes.

yebblies · 2014-10-22T07:31:58Z

I don't know why it would. Why should it? Note that what this logic does is equivalent to:
   (S(), foo(), T())
which had better work in CTFE.

I can see for CallExp that argprefix is getting Expression::combined, but for NewExp it's getting stored in the ast... How is the interpreter going to know about these ast nodes and call them correctly?

WalterBright · 2014-10-22T09:01:09Z

Ah, good point. Will check it out. Note that this is yet another reason for lowering rather than putting the logic in the glue layer!

yebblies · 2014-10-22T09:07:48Z

Yeah, or at least an argument for putting the entire thing in one layer. Sometimes a higher-level construct is easier to interpret than a lower one.

WalterBright · 2014-10-23T07:15:22Z

Added CTFE support.

9rnsr · 2014-10-23T12:04:07Z

Also, note that the glue layer still handles the actual insertion of destructor code.

All destructor calls must be handled in AST for correct CTFE interpretation and temporary struct objects.
For example: Issue 13095 - Someteims struct destructor is called if constructor throws

WalterBright · 2014-10-23T20:48:02Z

@9rnsr those are separate issues and should not be handled by this PR. If every destructor issue was handled by one PR, then it would not be very reviewable.

WalterBright · 2014-10-24T20:22:03Z

Sigh, another heisenbug in std.concurrency

9rnsr · 2014-10-26T07:48:56Z

test/runnable/sdtor.d

@@ -3351,6 +3351,90 @@ void test13303()

 /**********************************/

+void test13586()


Do not use hard tab.

9rnsr · 2014-10-26T07:54:39Z

I'm not sure how the backend change is related with the front-end fix. I'd suggest to separate backend changes in its own commit with explanation commit log.

9rnsr · 2014-10-26T08:01:46Z

Note that, the co-working of front-end and glue-layer will be harmful for CTFE. Actually, this PR does not fix issue 13586 during CTFE. @WalterBright, is that intended?

…hrows

WalterBright · 2014-10-26T08:44:36Z

This PR produces actual expressions in the dtor trees, and so also needed a bit of upgrading the back end to handle it. The front and back end changes need each other. Also, I don't know how to test for those other back end changes, as this PR caused a slightly different path to be taken through it which exposed a couple problems. They're simple enough fixes.

WalterBright · 2014-10-26T08:47:37Z

Note that, the co-working of front-end and glue-layer will be harmful for CTFE.

Do you mean the insertion of dtors by e2ir.c?

Actually, this PR does not fix issue 13586 during CTFE. @WalterBright, is that intended?

As far as I can tell, CTFE has never handled destructors in expressions. This PR does not fix that, that is true, but it is a separate bug and should be handled separately.

9rnsr · 2014-10-26T09:13:51Z

Current CTFE engine does not specially handle destructor calls those are inserted in glue layer.
But OK, at least fixing runtime behavior would be better than nothing.

9rnsr · 2014-10-26T09:13:54Z

Auto-merge toggled on

fix Issue 13586 - Destructors not run when argument list evaluation throws

sinkuu · 2014-11-03T02:17:20Z

This PR introduced a regression(ICE).
https://issues.dlang.org/show_bug.cgi?id=13673

MartinNowak · 2015-04-17T16:23:37Z

This seems to have introduced another regression.
Issue 14443 – [Reg 2.067.0] Incorrect double freeing of reference counted struct

CyberShadow · 2015-06-13T21:58:03Z

This pull request introduced a regression:
https://issues.dlang.org/show_bug.cgi?id=13720

9rnsr · 2016-02-09T14:05:14Z

This pull request introduced a regression:
https://issues.dlang.org/show_bug.cgi?id=15661

By the PR dlang#4078, some preparation code has been inserted before a constructor call expression. Then, AssignExp has failed to recognize the ctor call form.

By the PR dlang#4078, some preparation code has been inserted before a constructor call expression. Then, `AssignExp` had failed to recognize the ctor call form.

WalterBright force-pushed the fix13586 branch 6 times, most recently from e255f68 to 48dac1f Compare October 21, 2014 20:41

yebblies reviewed Oct 22, 2014
View reviewed changes

WalterBright force-pushed the fix13586 branch from 48dac1f to a36c5c9 Compare October 22, 2014 04:41

WalterBright force-pushed the fix13586 branch from a36c5c9 to 8e2d927 Compare October 23, 2014 06:59

9rnsr reviewed Oct 26, 2014
View reviewed changes

fix Issue 13586 - Destructors not run when argument list evaluation t…

aea25d6

…hrows

WalterBright force-pushed the fix13586 branch from 8e2d927 to aea25d6 Compare October 26, 2014 08:40

9rnsr added a commit that referenced this pull request Oct 26, 2014

Merge pull request #4078 from WalterBright/fix13586

11276b8

fix Issue 13586 - Destructors not run when argument list evaluation throws

9rnsr merged commit 11276b8 into dlang:master Oct 26, 2014

WalterBright deleted the fix13586 branch October 26, 2014 19:00

9rnsr mentioned this pull request Apr 18, 2015

[REG2.067] Issue 14443 - Incorrect double freeing of reference counted struct #4595

Merged

kinke mentioned this pull request Apr 26, 2015

WIP: Codegen for NewExp::argprefix. ldc-developers/ldc#914

Merged

9rnsr mentioned this pull request Feb 9, 2016

[REG2.067.0] Issue 15661 - Destructor called while object still alive #5431

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix Issue 13586 - Destructors not run when argument list evaluation throws #4078

fix Issue 13586 - Destructors not run when argument list evaluation throws #4078

WalterBright commented Oct 20, 2014

WalterBright commented Oct 22, 2014

yebblies Oct 22, 2014

WalterBright Oct 22, 2014

yebblies commented Oct 22, 2014

WalterBright commented Oct 22, 2014

yebblies commented Oct 22, 2014

WalterBright commented Oct 22, 2014

yebblies commented Oct 22, 2014

WalterBright commented Oct 23, 2014

9rnsr commented Oct 23, 2014

WalterBright commented Oct 23, 2014

WalterBright commented Oct 24, 2014

9rnsr Oct 26, 2014

WalterBright Oct 26, 2014

9rnsr commented Oct 26, 2014

9rnsr commented Oct 26, 2014

WalterBright commented Oct 26, 2014

WalterBright commented Oct 26, 2014

9rnsr commented Oct 26, 2014

9rnsr commented Oct 26, 2014

sinkuu commented Nov 3, 2014

MartinNowak commented Apr 17, 2015

CyberShadow commented Jun 13, 2015

9rnsr commented Feb 9, 2016

		@@ -3351,6 +3351,90 @@ void test13303()

		/**********************************/

		void test13586()

fix Issue 13586 - Destructors not run when argument list evaluation throws #4078

fix Issue 13586 - Destructors not run when argument list evaluation throws #4078

Conversation

WalterBright commented Oct 20, 2014

WalterBright commented Oct 22, 2014

yebblies Oct 22, 2014

Choose a reason for hiding this comment

WalterBright Oct 22, 2014

Choose a reason for hiding this comment

yebblies commented Oct 22, 2014

WalterBright commented Oct 22, 2014

yebblies commented Oct 22, 2014

WalterBright commented Oct 22, 2014

yebblies commented Oct 22, 2014

WalterBright commented Oct 23, 2014

9rnsr commented Oct 23, 2014

WalterBright commented Oct 23, 2014

WalterBright commented Oct 24, 2014

9rnsr Oct 26, 2014

Choose a reason for hiding this comment

WalterBright Oct 26, 2014

Choose a reason for hiding this comment

9rnsr commented Oct 26, 2014

9rnsr commented Oct 26, 2014

WalterBright commented Oct 26, 2014

WalterBright commented Oct 26, 2014

9rnsr commented Oct 26, 2014

9rnsr commented Oct 26, 2014

sinkuu commented Nov 3, 2014

MartinNowak commented Apr 17, 2015

CyberShadow commented Jun 13, 2015

9rnsr commented Feb 9, 2016