Bug 13586 – Destructors not run when argument list evaluation throws

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 [email protected](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.

Shouldn't the argument be destructed by the caller when the callee returns? So it couldn't be constructed in place anyway.

(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?

(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.

(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? :-)

(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?

(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?

(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.

(This is one of the complications that make ref counting not so performant.)

Thanks!

(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?

(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.

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

(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?

(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?

(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.

(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.

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.

If the caller owns the arguments, it can also help with issue 12684 and similar.

(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).

(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.

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