D issues are now tracked on GitHub. This Bugzilla instance remains as a read-only archive.
Issue 13586 - Destructors not run when argument list evaluation throws
Summary: Destructors not run when argument list evaluation throws
Status: RESOLVED FIXED
Alias: None
Product: D
Classification: Unclassified
Component: dmd (show other issues)
Version: D2
Hardware: All All
: P1 blocker
Assignee: No Owner
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-10-08 00:50 UTC by Walter Bright
Modified: 2015-02-18 03:38 UTC (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this issue.
Description Walter Bright 2014-10-08 00:50:12 UTC
The following test case illustrates it - only s should be constructed, and when throwit() throws, it should be destructed. It isn't.

------------------------------------------

import core.stdc.stdio;

struct S {
    static int count;
    ~this() { ++count; }
}

int throwit() { throw new Exception("whatevah"); }

void neverland(S s, int x, S t) { }

void main() {
    try {
        neverland(S(), throwit(), S());
    }
    catch (Exception e) {
        printf("%d\n", S.count);
        assert(S.count == 1);
    }
}

C:\cbx\mars>foo
0
core.exception.AssertError@foo.d(19): Assertion failure

-----------------------------------------------
The fix is similar to what Kenji did to ensure left to right order of evaluation of arguments. If any arguments have destructors, then the argument list has to be constructed before attempting to put the argument list on the stack, because destructing them on a partially built stack is problematic. The arguments are then blitted to the stack, because the blit operation cannot throw.

Note that this solution relies on D's requirement that a struct instance cannot have internal pointers to itself.

This bug blocks a reliable ref counting implementation.
Comment 1 anonymous4 2014-10-08 18:14:10 UTC
Shouldn't the argument be destructed by the caller when the callee returns? So it couldn't be constructed in place anyway.
Comment 2 monarchdodra 2014-10-09 08:15:21 UTC
(In reply to Walter Bright from comment #0)
> The fix is similar to what Kenji did to ensure left to right order of
> evaluation of arguments. If any arguments have destructors, then the
> argument list has to be constructed before attempting to put the argument
> list on the stack, because destructing them on a partially built stack is
> problematic. The arguments are then blitted to the stack, because the blit
> operation cannot throw.

It's also similar to what "opAssign implemented in terms of postblit" does. Speaking of which, in both cases, if all the arguments can be evaluated in nothrow context, then it should be OK to build the arguments in place directly.

Aren't you worried that we'll take a general performance hit building the arguments and then moving them every time we make a call?

Hows does C++ deal with this?
Comment 3 Walter Bright 2014-10-09 09:11:29 UTC
(In reply to Sobirari Muhomori from comment #1)
> Shouldn't the argument be destructed by the caller when the callee returns?

Yes. Once the function gets called, it gets marked by the compiler as "don't destroy this".

> So it couldn't be constructed in place anyway.
Comment 4 Walter Bright 2014-10-09 09:14:08 UTC
(In reply to monarchdodra from comment #2)
> Aren't you worried that we'll take a general performance hit building the
> arguments and then moving them every time we make a call?

1. first make it work, then optimize

2. it'll only happen with structs with destructors, so (hopefully) it will be less common


> Hows does C++ deal with this?

I imagine it's very compiler dependent - there are many ways to do it. I haven't checked. Why not give it a try? :-)
Comment 5 Andrei Alexandrescu 2014-10-09 14:42:19 UTC
(In reply to Walter Bright from comment #3)
> (In reply to Sobirari Muhomori from comment #1)
> > Shouldn't the argument be destructed by the caller when the callee returns?
> 
> Yes. Once the function gets called, it gets marked by the compiler as "don't
> destroy this".

Wait, I'm confused. On the normal path (no exceptions) isn't the callee destroying its by-value arguments?
Comment 6 monarchdodra 2014-10-09 20:00:10 UTC
(In reply to Andrei Alexandrescu from comment #5)
> (In reply to Walter Bright from comment #3)
> > (In reply to Sobirari Muhomori from comment #1)
> > > Shouldn't the argument be destructed by the caller when the callee returns?
> > 
> > Yes. Once the function gets called, it gets marked by the compiler as "don't
> > destroy this".
> 
> Wait, I'm confused. On the normal path (no exceptions) isn't the callee
> destroying its by-value arguments?

I think Walter missunderstood the question, as his answer seems to mean what you just said (the opposite of the original question).

Walter?
Comment 7 Walter Bright 2014-10-14 04:33:31 UTC
(In reply to Andrei Alexandrescu from comment #5)
> (In reply to Walter Bright from comment #3)
> > (In reply to Sobirari Muhomori from comment #1)
> > > Shouldn't the argument be destructed by the caller when the callee returns?
> > 
> > Yes. Once the function gets called, it gets marked by the compiler as "don't
> > destroy this".
> 
> Wait, I'm confused. On the normal path (no exceptions) isn't the callee
> destroying its by-value arguments?

Yes. If the function doesn't get called, then the caller has to call the destructors on the partially constructed argument list. If the function does get called, then the function destroys the arguments.

Hence having the compiler mark it to be careful where the destructor code gets placed.
Comment 8 Walter Bright 2014-10-14 04:44:18 UTC
(This is one of the complications that make ref counting not so performant.)
Comment 9 Andrei Alexandrescu 2014-10-14 06:11:55 UTC
Thanks!
Comment 10 anonymous4 2014-10-17 07:14:28 UTC
(In reply to Andrei Alexandrescu from comment #5)
> Wait, I'm confused. On the normal path (no exceptions) isn't the callee
> destroying its by-value arguments?

Huh? Why not caller?
Comment 11 Walter Bright 2014-10-18 09:41:19 UTC
(In reply to Sobirari Muhomori from comment #10)
> (In reply to Andrei Alexandrescu from comment #5)
> > Wait, I'm confused. On the normal path (no exceptions) isn't the callee
> > destroying its by-value arguments?
> 
> Huh? Why not caller?

Because the caller can transfer ownership to the callee. If the callee never destructed its parameters, this could not be done.
Comment 13 github-bugzilla 2014-10-26 10:06:36 UTC
Commits pushed to master at https://github.com/D-Programming-Language/dmd

https://github.com/D-Programming-Language/dmd/commit/aea25d6b6ea8383058f4987fe11ae16bbda18852
fix Issue 13586 - Destructors not run when argument list evaluation throws

https://github.com/D-Programming-Language/dmd/commit/11276b83f93a301a7d258e72bf8cea6a504d28b7
Merge pull request #4078 from WalterBright/fix13586

fix Issue 13586 - Destructors not run when argument list evaluation throws
Comment 14 anonymous4 2014-10-26 10:45:44 UTC
(In reply to Walter Bright from comment #11)
> Because the caller can transfer ownership to the callee. If the callee never
> destructed its parameters, this could not be done.

If transfer of ownership makes things messy, it's better to not do that. Is there a reason, why the caller can't keep ownership to itself?
Comment 15 monarchdodra 2014-10-26 11:35:19 UTC
(In reply to Sobirari Muhomori from comment #14)
> (In reply to Walter Bright from comment #11)
> > Because the caller can transfer ownership to the callee. If the callee never
> > destructed its parameters, this could not be done.
> 
> If transfer of ownership makes things messy, it's better to not do that. Is
> there a reason, why the caller can't keep ownership to itself?

I could be talking out of my ass, but I'd *assume* ownership transfer is required for proper move semantics. The easiest way to move an object from scope A to scope B would be for A to "give ownership" to B.

EG:

foo(getLValue());

Here the outerscope calls neither postblit nor destructor.

Speaking of which: Walter, is there anything blocking https://issues.dlang.org/show_bug.cgi?id=12684 ?

Or has it simply not been brought to DMD team's attention?
Comment 16 anonymous4 2014-10-27 08:01:50 UTC
(In reply to monarchdodra from comment #15)
> I could be talking out of my ass, but I'd *assume* ownership transfer is
> required for proper move semantics.

AFAIK, move semantics is need only for return values, not for arguments.

> foo(getLValue());
> 
> Here the outerscope calls neither postblit nor destructor.

A proposed solution to this issue is to place arguments on the caller's stack, so that's where they're moved, not to the callee's stack.
Comment 17 Andrei Alexandrescu 2014-10-27 15:38:59 UTC
(In reply to Sobirari Muhomori from comment #16)
> (In reply to monarchdodra from comment #15)
> > I could be talking out of my ass, but I'd *assume* ownership transfer is
> > required for proper move semantics.
> 
> AFAIK, move semantics is need only for return values, not for arguments.

Passing ownership into the callee is needed everywhere the callee gets an rvalue and then e.g. forwards it along. It's a great optimization that simplifies D code compared to C++ code, which is always burdened with cleaning up objects at the caller level.
Comment 18 anonymous4 2014-10-28 07:58:46 UTC
What optimization does it provide? Most parameters are scoped, i.e. not owned by the callee, so since they are owned by the caller, they are destroyed by the caller.
Comment 19 anonymous4 2014-11-14 07:50:49 UTC
If the caller owns the arguments, it can also help with issue 12684 and similar.
Comment 20 Andrei Alexandrescu 2014-11-14 15:42:05 UTC
(In reply to Sobirari Muhomori from comment #19)
> If the caller owns the arguments, it can also help with issue 12684 and
> similar.

Once the callee gets called, it must own its by-value arguments. That way we essentially avoid C++'s issue with unnecessary copying (callee copies, call, caller makes a copy inside, callee destroys the copy) without a costly feature (C++'s rvalue references).
Comment 21 anonymous4 2014-11-17 10:31:32 UTC
(In reply to Andrei Alexandrescu from comment #20)
> C++'s issue with unnecessary copying
I see it as a choice between two suboptimal implementations. The copying happens because the callee is forced to own its arguments. D already does some things differently from C++ to be efficient, I think something similar can be done in this case too.
Comment 22 github-bugzilla 2015-02-18 03:38:55 UTC
Commits pushed to 2.067 at https://github.com/D-Programming-Language/dmd

https://github.com/D-Programming-Language/dmd/commit/aea25d6b6ea8383058f4987fe11ae16bbda18852
fix Issue 13586 - Destructors not run when argument list evaluation throws

https://github.com/D-Programming-Language/dmd/commit/11276b83f93a301a7d258e72bf8cea6a504d28b7
Merge pull request #4078 from WalterBright/fix13586