clang fails to destroy return value when a destructor throws

[rjmccall]

Bugzilla Link	12286
Version	trunk
OS	All
CC	@DougGregor,@seanbaxter,@mizvekov,@tavianator,@yuanfang-chen

Extended Description

If an exception is thrown after a non-POD return value is successfully constructed in the return slot but before the returning function terminates, the value in the return slot is not destroyed. This can be clearly seen in the following test case. The really easy solution would be to push an inactive destructor cleanup in the prologue and activate it whenever evaluation completes for a return value, but that wouldn't be the correct ordering of destruction as specified in [except.ctor]p1. Needs more thought.

#include <stdio.h>

struct A {
  A() { printf("creating A at %p\n", this); }
  A(const A &a) { printf("copying A at %p into %p\n", &a, this); }
  ~A() { printf("destroying A at %p\n", this); }
};

struct B {
  B() { printf("entering B\n"); }
  ~B() { printf("exiting B, throwing!\n"); throw 0; }
};

A test() {
  B b;
  return A();
}

int main() {
  try {
    printf("running\n");
    A a = test();
  } catch (int i) {
    printf("caught\n");
  }
}

[llvmbot]

This issue is giving me memory leaks, and it seems difficult to work around without undesirable interface changes.

[llvmbot]

It seems that this is still the case for all clang++ version (up until the latest 9.0.0). The behavior seems wrong as there is not strict requirement (is it?) about non-throwing destructors.

[rjmccall]

It's definitely wrong, but nobody has had time to work on it, and the fact that it's specific to throwing destructors does make it a little easier to just keep putting off. If you'd like to work on it, I have some ideas about the infrastructure work that will be necessary.

[seanbaxter]

This bug was just fixed in gcc last month, after sitting in their bug system for 13 years.

Here's a clean example:
https://godbolt.org/z/NJLU9C

I ran into it while trying to implement mandatory NRVO. That requires a closely-related EH behavior. Anyone have a clue on how to implement this? I mean, the semantics are a bit tricky.

[rjmccall]

I think it's basically the same fix for return values as for local-variable initialization. Basically, we have an object we just constructed, and we need to destroy it if one of such-and-such cleanups throws. Conveniently, the object we want to destroy is always the most recent still-living object that was constructed in this function, which means we never need to do this as anything except an EH cleanup, and so we just need the cleanup-emission code to push that EH cleanup to destroy it during the emission of all the normal cleanups that we're going to run after the return object or local variable is fully constructed.

Now, unfortunately there's some extra complexity which comes from the fact that Clang tries to emit every normal cleanup uniquely. For returns, we might have other, non-return branches threaded through those cleanups, e.g. in code like:

  {
    HasThrowingDestructor var;
    if (condition1) {
      // On this branch through var's cleanup, we should not run
      // the return-value destructor as an EH cleanup.
      break;
    } else if (condition2) {
      // On this branch through var's cleanup, we should run it.
      return std::string("help");
    }
    // On the fall-through branch when leaving the scope, we should not run it.
  }

I think the same thing can happen with local variables because of statement-expressions, unfortunately:

  std::string var = ({ if (condition) break; "help" });

And, unfortunately, there's a very silly situation where a cleanup can have to worry about both, although I think dynamically they can't coincide.

  // The normal cleanup for the `HasThrowingDestructor` temporary is
  // active during both the return branch in the statement-expression
  // and the end-of-initialization branch for `var`.
  std::string var = HasThrowingDestructor().buildString(
                      ({ if (condition) return std::string("returned"); 
                         "value" }));

So to handle this right, I think you need three things.

1. We need to track three bits on every cleanup:

• whether it was active during a return branch
• whether it was active during an end-of-initialization branch
• whether it was active during a branch that wasn't either of the above

2. We need to be able to tell the cleanup manager that a branch through cleanups is happening for one of those two special reasons.

3. We need to make the cleanup manager capable of both:

• pushing a destruction EH cleanup for the return value during the emission of a cleanup if it was active during a return branch and

• pushing a destruction EH cleanup for the current initialized address (which can be maintained via some SaveAndRestore gadget during the emission of an initializer full-expression) during the emission of a cleanup if it was active during an end-of-initialization.

And those EH cleanups need to be conditional if the cleanup being emitted wasn't solely active during the appropriate kind of branch, and setting up that condition flag might be really difficult.

[rjmccall]

Honestly, it's things like this that make me wonder if we should just emit cleanups non-uniquely. That has a serious code-size cost but is much simpler.

[seanbaxter]

Honestly, it's things like this that make me wonder if we should just emit
cleanups non-uniquely. That has a serious code-size cost but is much
simpler.

You only need to add a non-unique cleanup for the return object (for both NRVO and URVO cases) from the landing pad for each potentially-throwing dtor call. After executing that return object's dtor, you'd branch back into the chain of unique dtor calls for the remaining initialized objects.

Conveniently, the object we want to destroy is always the most recent still-living object that was constructed in this function.

This is true only for the URVO case. For NRVO, which is the return object may be initialized before the object with the throwing dtor:

a_t a("result object");  // initialize a into the sret
b_t b("throwing dtor");
if(cond)
  return a;  // an nrvo scenario.

In this case, we can (I think) get by with the unique cleanup code for a, and can call it unconditionally from b's EH cleanup code.

b_t b("throwing dtor");
a_t a("result object");  // initialize a into the sret
if(cond)
  return a;  // an nrvo scenario.

In this case, the result object's declaration is in the potential scope of the potentially throwing dtor's object, so b's landing pad would have to conditionally cleanup the result object before branching to the remaining unique cleanup calls.

To implement normal destruction of NRVO I alloca a single bit to hold the returned state of the return object. It's cleared when the function is entered. After the named return object is initialized, it's set to 1. When the return statement that returns it is encountered, all that codegen does is clear that flag and do return (void) control flow. When the normal dtor chain is executed, it sees that the return object has a cleared flag, and therefore skips over its cleanup and continues to the prior objects.

I believe we can handle EH cleanup with an additional flag: one that is set after the return object's ctor finishes, and is not cleared by the return statement. Then we amend the behavior for the normal path to only cleanup the return object if the first flag is cleared and the second is set. The EH code cleans up if the first flag (is initialized) is set, and ignores the second flag (the one cleared by return).

a_t a("return object"); // initialized into sret
try {
  b_t b("throwing object"); // ~b throws
  return a;

  // throw!

} catch(...) {
  // Catch's ~b's exception.
  // a must still be initialized, but we'll need to 
  // return it YET AGAIN, which should be another 
  // no-op.
  return a;
}

There are weird things like this, where catching a dtor exception effectively un-returns a return object. That object was constructed in the sret, and since it was declared before b, b's EH doesn't clean it up, but the catch clause takes us out of the EH world and into the normal, so I guess we have to be smart enough to clear the flag indicating that a was returned, then set it again at the second return statement.

I'm not eager to implement this stuff. I can't even hold it in my mind. But I suspect the committee people are going to be giving it greater scrutiny now that mandatory NRVO is being reviewed.

[rjmccall]

Honestly, it's things like this that make me wonder if we should just emit
cleanups non-uniquely. That has a serious code-size cost but is much
simpler.

You only need to add a non-unique cleanup for the return object (for both
NRVO and URVO cases) from the landing pad for each potentially-throwing dtor
call. After executing that return object's dtor, you'd branch back into the
chain of unique dtor calls for the remaining initialized objects.

Yes, but that's not the part of uniqueness that's an issue. It's the uniqueness of the potentially-throwing dtor call that's a problem.

Conveniently, the object we want to destroy is always the most recent still-living object that was constructed in this function.

This is true only for the URVO case. For NRVO, which is the return object
may be initialized before the object with the throwing dtor:

Ah, right, of course. This needs almost totally different handling.

I believe we can handle EH cleanup with an additional flag: one that is set
after the return object's ctor finishes, and is not cleared by the return
statement. Then we amend the behavior for the normal path to only cleanup
the return object if the first flag is cleared and the second is set. The EH
code cleans up if the first flag (is initialized) is set, and ignores the
second flag (the one cleared by return).

We could push a second cleanup for NRVO variables just for this purpose and track it on the cleanup manager. But the trick is that cleanups outside that cleanup still need the floating-return-value-cleanup treatment.

[llvmbot]

@llvm/issue-subscribers-clang-codegen

| | | | --- | --- | | Bugzilla Link | [12286](https://llvm.org/bz12286) | | Version | trunk | | OS | All | | CC | @DougGregor,@seanbaxter,@mizvekov,@tavianator,@yuanfang-chen |

Extended Description

If an exception is thrown after a non-POD return value is successfully constructed in the return slot but before the returning function terminates, the value in the return slot is not destroyed. This can be clearly seen in the following test case. The really easy solution would be to push an inactive destructor cleanup in the prologue and activate it whenever evaluation completes for a return value, but that wouldn't be the correct ordering of destruction as specified in [except.ctor]p1. Needs more thought.

#include <stdio.h>

struct A {
  A() { printf("creating A at %p\n", this); }
  A(const A &a) { printf("copying A at %p into %p\n", &a, this); }
  ~A() { printf("destroying A at %p\n", this); }
};

struct B {
  B() { printf("entering B\n"); }
  ~B() { printf("exiting B, throwing!\n"); throw 0; }
};

A test() {
  B b;
  return A();
}

int main() {
  try {
    printf("running\n");
    A a = test();
  } catch (int i) {
    printf("caught\n");
  }
}

[DimitryAndric]

It's also interesting what gcc says about this case:

$ g++12 pr12658.cpp -o pr12658-gcc12
pr12658.cpp: In destructor 'B::~B()':
pr12658.cpp:11:44: warning: 'throw' will always call 'terminate' [-Wterminate]
   11 |   ~B() { printf("exiting B, throwing!\n"); throw 0; }
      |                                            ^~~~~~~
pr12658.cpp:11:44: note: in C++11 destructors default to 'noexcept'

Which is because it defaults to gnu++17 now, and indeed it calls terminate at runtime, as it should.

With an explicit standard before C++11, it does not warn, but still manages to destruct the A instance:

$ g++12 -std=gnu++98 pr12658.cpp -o pr12658-gcc12

$ ./pr12658-gcc12
running
entering B
creating A at 0x820a23d1b
exiting B, throwing!
destroying A at 0x820a23d1b
caught

[seanbaxter]

I'm going to wake this old thread up. Here's another example of a throwing destructor that causes a leak, and it's not related to return value optimizations:
https://godbolt.org/z/Gsq55fb1s

gcc gets it right going all the way back to gcc-6. I don't know if the faulty code Clang in clang causing this bug is the same as the code that causes the OP bug, but the root problem is the same, which is we don't know how to reason about throwing destructors in the presence of initialization.

I'm rewriting Circle's codegen to lower to MIR, instead of directly to LLVM, and this issue is challenging my design on how to nest destructors. I want to fix it this time around. I think Clang has roughly the same design in this part and that's why we both have this old bug. You have a stack of initialized objects with basic block pointers for running the dtor in both the normal paths and the cleanup paths. When you want to emit an InvokeInst, you'd create or recycle a landing pad block the exits to the cleanup path for the object at the top of the stack. When lexical scopes are exited, you unwind the objects by filling out the dtor blocks for both the normal and cleanup paths, and chain them together, so you just get one dtor emitted in each path. That's nice and tidy.

The problem with this example is that object b is "activated" after the full expression has ended, and all the temporaries used to create it are destroyed. But one of those has a throwing destructor, so it requests a landing pad, and the top of the active object stack does not include the freshly-initialized object b, so its dtor is not emitted.

What is the right way to look at this? We don't want to push b to the active stack before its initializer is run, because then if A throws in its ctor, the request for a landing pad would generate a call to b's dtor, but b isn't initialized yet.

What if you push b as an active object when its initializer has been lowered, but before the temporary scope set up for initialization has been unwound? Now the scopes are not stack-like: b's dtor would be called before the dtor for the temporary of type A.

Going back to what @rjmccall said 4 years ago:

Honestly, it's things like this that make me wonder if we should just emit cleanups non-uniquely. That has a serious code-size cost but is much simpler.

I think this is the way out, at least in the presence of initializers that have throwing destructors.

If you're in the context of initializing an object b, and some temporary in the active object stack above the point where the b will be pushed have throwing dtors, then push an action to destroy b from the cleanup path only and then join the innermost cleanup path of the temporaries. This kills the outer object before any of the temporaries, but it lets us continue with stack-like nesting.

I haven't implemented it yet, but it feels like it should solve both this new bug and the OP bug. In the context of initialization, after the initializer expression has been lowered but before temporaries have been unwound, push to the stack an action that does nothing on the normal path but runs the new object's dtor on the cleanup path, before continuing to the innermost of the temporary object cleanup blocks.

Edit: tried implementing this and nope, this ain't the fix.

When emitting the drop chain for the temporary objects, you pop an active object from the stack and emit its normal path dtor and cleanup path dtor. If the normal path dtor throws, you generate a landing pad block that enters the cleanup path for the new top of the active object stack. You can't put the outer object's dtor at the top of the stack, because it'll get unwound and emit nothing, since its cleanup block has no users. Then you're back at the temporary objects, and the outer declaration won't get destroyed. I don't know a good solution. Inserting this dtor into the middle of the stack would work for simple cases but I could see it breaking in complex cases where there are statement-expressions or some other thing where you have nested temporary scopes.

[shafik]

Here is the original example: https://godbolt.org/z/v4Gs6K4xo

It required noexcept(false) on B's destructor to obtain the expected output.

[seanbaxter]

Here is the original example: https://godbolt.org/z/v4Gs6K4xo

It required noexcept(false) on B's destructor to obtain the expected output.

It doesn't have the expected output with that noexcept(false) though. It's not destroying a.

[shafik]

Here is the original example: https://godbolt.org/z/v4Gs6K4xo
It required noexcept(false) on B's destructor to obtain the expected output.

It doesn't have the expected output with that noexcept(false) though. It's not destroying a.

Apologies, that is what I meant, we expect a to not be destroyed, which I believe is what the original example was trying to demonstrate.

[seanbaxter]

Apologies, that is what I meant, we expect a to not be destroyed, which I believe is what the original example was trying to demonstrate.

Ahh I see. When I woke this thread up in 2020 I think I posted a different problem, which remains unfixed:
#12658 (comment)

But that one is equivalent to what I just posted. Separating it from RVOs should help in understanding it.