[CodeGen] Check the cleanup flag before destructing lifetime-extended temporaries created in conditional expressions

[ahatanak]

Summary

The lifetime-extended temporary of the true operand shouldn't be destructed when the false operand is evaluated and throws. Call pushFullExprCleanup instead of EHScopeStack::pushCleanup in pushLifetimeExtendedDestroy so that the destructor is called conditionally when we are in a conditional expression.

rdar://problem/64829372

[rjmccall]

Please adjust the commit message to be clear that this is about lifetime-extended temporaries; it's not like we got this wrong for all temporaries.

Do we need to do anything to ensure that the after-full-expression cleanup is configured conditionally?

[ahatanak]

After-full-expression cleanup looks fine to me. pushCleanupAfterFullExpr sets the flags and saves the values when it's in a conditional branch.

I think ideally we shouldn't have to clean up anything in the true expression when the false expression throws, but I wasn't able to come up with an easy way to make IRGen avoid that.

[rjmccall]

In D82999#2129031, @ahatanak wrote:

After-full-expression cleanup looks fine to me. pushCleanupAfterFullExpr sets the flags and saves the values when it's in a conditional branch.

I think ideally we shouldn't have to clean up anything in the true expression when the false expression throws, but I wasn't able to come up with an easy way to make IRGen avoid that.

It's unavoidable; you can have code after the merge point that can throw, and you can only know dynamically which side you came from at that point.

[ahatanak]

In test case test13 in clang/test/CodeGenCXX/exceptions.cpp, I think you can turn invoke void @_ZN6test131AC1Ev into call void @_ZN6test131AC1Ev, no? If the false expression throws, there is nothing to clean up in the false expression and also nothing in the true expression has to be cleaned up.

[ahatanak]

And if the constructor call for A in the true expression throws, it's not necessary to call the destructor either since the object hasn't been constructed.

[rjmccall]

In D82999#2129417, @ahatanak wrote:

In test case test13 in clang/test/CodeGenCXX/exceptions.cpp, I think you can turn invoke void @_ZN6test131AC1Ev into call void @_ZN6test131AC1Ev, no? If the false expression throws, there is nothing to clean up in the false expression and also nothing in the true expression has to be cleaned up.

Yes, this is true. It would be possible to enhance Clang's cleanup stack to support this sort of thing — we'd want to be able to mark a cleanup as "currently known inactive" without potentially popping it off the cleanup stack, and then we could have conditional scopes remember the cleanups that were added, deactivate them this way, and then reactivate them after the merge. Swift's cleanup manager supports something similar.

[ahatanak]

In D82999#2129855, @rjmccall wrote:

In D82999#2129417, @ahatanak wrote:

In test case test13 in clang/test/CodeGenCXX/exceptions.cpp, I think you can turn invoke void @_ZN6test131AC1Ev into call void @_ZN6test131AC1Ev, no? If the false expression throws, there is nothing to clean up in the false expression and also nothing in the true expression has to be cleaned up.

Yes, this is true. It would be possible to enhance Clang's cleanup stack to support this sort of thing — we'd want to be able to mark a cleanup as "currently known inactive" without potentially popping it off the cleanup stack, and then we could have conditional scopes remember the cleanups that were added, deactivate them this way, and then reactivate them after the merge. Swift's cleanup manager supports something similar.

OK, I see. Since the enhancement isn't trivial to implement, I think we should do it in a separate patch if it has a large impact on code size. WDYT?

[rjmccall]

I agree, that can be done separately.

[ahatanak]

This was fixed by https://reviews.llvm.org/D86854.

clang/lib/CodeGen/CGDecl.cpp

 
 void CodeGenFunction::pushLifetimeExtendedDestroy(
     CleanupKind cleanupKind, Address addr, QualType type,
     Destroyer *destroyer, bool useEHCleanupForArray) {
   // Push an EH-only cleanup for the object now.
   // FIXME: When popping normal cleanups, we need to keep this EH cleanup
   // around in case a temporary's destructor throws an exception.
   if (cleanupKind & EHCleanup)
-    EHStack.pushCleanup<DestroyObject>(
+    pushFullExprCleanup<DestroyObject>(
         static_cast<CleanupKind>(cleanupKind & ~NormalCleanup), addr, type,
         destroyer, useEHCleanupForArray);
 
   // Remember that we need to push a full cleanup for the object at the
   // end of the full-expression.
   pushCleanupAfterFullExpr<DestroyObject>(
       cleanupKind, addr, type, destroyer, useEHCleanupForArray);
 }

clang/test/CodeGenCXX/exceptions.cpp

 // RUN: %clang_cc1 %s -triple=x86_64-linux-gnu -emit-llvm -std=c++98 -o - -fcxx-exceptions -fexceptions | FileCheck -check-prefix=CHECK -check-prefix=CHECK98 %s
 // RUN: %clang_cc1 %s -triple=x86_64-linux-gnu -emit-llvm -std=c++11 -o - -fcxx-exceptions -fexceptions | FileCheck -check-prefix=CHECK -check-prefix=CHECK11 %s
 
+// CHECK: %[[STRUCT_TEST13_A:.*]] = type { i32, i32 }
+
 typedef __typeof(sizeof(0)) size_t;
 
 // Declare the reserved global placement new.
 void *operator new(size_t, void*);
 
 // This just shouldn't crash.
 namespace test0 {
   struct allocator {
namespace test12 {
   // CHECK-NEXT:  [[CAST:%.*]] = bitcast i8* [[PTR]] to [[A:%.*]]*
   // CHECK-NEXT:  invoke void @_ZN6test121AC1Ev([[A]]* [[CAST]])
   // CHECK:       ret [[A]]* [[CAST]]
 
   // CHECK98:       invoke void @_ZN6test121AdlEPvS1_(i8* [[PTR]], i8* [[PTR]])
   // CHECK11:       call void @_ZN6test121AdlEPvS1_(i8* [[PTR]], i8* [[PTR]])
 }
 
+namespace test13 {
+
+struct A {
+  A();
+  ~A();
+  int a, b;
+};
+
+// CHECK: define void @_ZN6test134testEi(
+// CHECK: %[[REF_TMP:.*]] = alloca %[[STRUCT_TEST13_A]], align 4
+// CHECK: %[[CLEANUP_COND:.*]] = alloca i1, align 1
+// CHECK: %[[CLEANUP_COND1:.*]] = alloca i1, align 1
+// CHECK: %[[REF_TMP2:.*]] = alloca %[[STRUCT_TEST13_A]], align 4
+// CHECK: %[[CLEANUP_COND3:.*]] = alloca i1, align 1
+// CHECK: %[[CLEANUP_COND4:.*]] = alloca i1, align 1
+
+// CHECK: call void @_ZN6test131AC1Ev(%[[STRUCT_TEST13_A]]* %[[REF_TMP]])
+// CHECK: store i1 true, i1* %[[CLEANUP_COND]], align 1
+// CHECK: store i1 true, i1* %[[CLEANUP_COND1]], align 1
+// CHECK: br
+
+// CHECK: invoke void @_ZN6test131AC1Ev(%[[STRUCT_TEST13_A]]* %[[REF_TMP2]])
+
+// CHECK: store i1 true, i1* %[[CLEANUP_COND3]], align 1
+// CHECK: store i1 true, i1* %[[CLEANUP_COND4]], align 1
+// CHECK: br
+
+//   Check the flag before destructing the temporary.
+
+// CHECK: landingpad { i8*, i32 }
+// CHECK: %[[CLEANUP_IS_ACTIVE:.*]] = load i1, i1* %[[CLEANUP_COND]], align 1
+// CHECK: br i1 %[[CLEANUP_IS_ACTIVE]],
+
+// CHECK: void @_ZN6test131AD1Ev(%[[STRUCT_TEST13_A]]* %[[REF_TMP]])
+
+void test(int c) {
+  const A &s = c ? static_cast<const A &>(A()) : static_cast<const A &>(A());
+}
+
+}
+
 // CHECK98: attributes [[NI_NR_NUW]] = { noinline noreturn nounwind }

clang/test/CodeGenCXX/temporaries.cpp

namespace Conditional {
   struct A {};
   struct B : A { B(); ~B(); };
   struct C : A { C(); ~C(); };
 
   void g();
 
   // CHECK-LABEL: define {{.*}} @_ZN11Conditional1fEb(
   void f(bool b) {
+    // CHECK: store i1 false, i1* %{{.*}},
     // CHECK: store i1 false, i1* %[[CLEANUP_B:.*]],
+    // CHECK: store i1 false, i1* %{{.*}},
     // CHECK: store i1 false, i1* %[[CLEANUP_C:.*]],
     // CHECK: br i1
     //
     // CHECK: call {{.*}} @_ZN11Conditional1BC1Ev(
+    // CHECK: store i1 true, i1* %{{.*}},
     // CHECK: store i1 true, i1* %[[CLEANUP_B]],
     // CHECK: br label
     //
     // CHECK: call {{.*}} @_ZN11Conditional1CC1Ev(
+    // CHECK: store i1 true, i1* %{{.*}},
     // CHECK: store i1 true, i1* %[[CLEANUP_C]],
     // CHECK: br label
     A &&r = b ? static_cast<A&&>(B()) : static_cast<A&&>(C());
 
     // CHECK: call {{.*}} @_ZN11Conditional1gEv(
     g();
 
     // CHECK: load {{.*}} %[[CLEANUP_C]]
     // CHECK: br i1
     // CHECK: call {{.*}} @_ZN11Conditional1CD1Ev(
     // CHECK: br label
 
     // CHECK: load {{.*}} %[[CLEANUP_B]]
     // CHECK: br i1
     // CHECK: call {{.*}} @_ZN11Conditional1BD1Ev(
     // CHECK: br label
   }
 
   struct D { A &&a; };
   // CHECK-LABEL: define {{.*}} @_ZN11Conditional10f_indirectEb(
   void f_indirect(bool b) {
+    // CHECK-CXX17: store i1 false, i1* %{{.*}},
     // CHECK: store i1 false, i1* %[[CLEANUP_B:.*]],
+    // CHECK-CXX17: store i1 false, i1* %{{.*}},
     // CHECK: store i1 false, i1* %[[CLEANUP_C:.*]],
     // CHECK: br i1
     //
     // CHECK: call {{.*}} @_ZN11Conditional1BC1Ev(
+    // CHECK-CXX17: store i1 true, i1* %{{.*}},
     // CHECK: store i1 true, i1* %[[CLEANUP_B]],
     // CHECK: br label
     //
     // CHECK: call {{.*}} @_ZN11Conditional1CC1Ev(
+    // CHECK-CXX17: store i1 true, i1* %{{.*}},
     // CHECK: store i1 true, i1* %[[CLEANUP_C]],
     // CHECK: br label
     D d = b ? D{B()} : D{C()};
 
     // In C++17, the expression D{...} directly initializes the 'd' object, so
     // lifetime-extending the temporaries to the lifetime of the D object
     // extends them past the call to g().
     //