[CodeGen] Make sure the EH cleanup for block captures is conditional when the block literal is in a conditional context

[erik.pilkington]

Summary

Previously, clang was crashing on the attached test because the EH cleanup for the block capture was incorrectly emitted under the assumption that the expression wasn't conditionally evaluated. This was because before D81624, pushLifetimeExtendedDestroy was mainly used with C++ automatic lifetime extension (i.e. const T &x = tmp();), where a conditionally evaluated expression wasn't possible. Now that we're using this path for block captures, we need to handle this case.

rdar://66250047

[rjmccall]

LGTM modulo the minor fix

clang/lib/CodeGen/CGDecl.cpp

2107

Oh, yuck, that's unfortunate.

2117

"it's"

clang/lib/CodeGen/CGDecl.cpp

void CodeGenFunction::pushDestroy(CleanupKind cleanupKind, Address addr,
   pushFullExprCleanup<DestroyObject>(cleanupKind, addr, type,
                                      destroyer, useEHCleanupForArray);
 }
 
 void CodeGenFunction::pushStackRestore(CleanupKind Kind, Address SPMem) {
   EHStack.pushCleanup<CallStackRestore>(Kind, SPMem);
 }
 
-void CodeGenFunction::pushLifetimeExtendedDestroy(
-    CleanupKind cleanupKind, Address addr, QualType type,
-    Destroyer *destroyer, bool useEHCleanupForArray) {
+void CodeGenFunction::pushLifetimeExtendedDestroy(CleanupKind cleanupKind,
+                                                  Address addr, QualType type,
+                                                  Destroyer *destroyer,
+                                                  bool useEHCleanupForArray) {
+  // If we're not in a conditional branch, we don't need to bother generating a
+  // conditional cleanup.
+  if (!isInConditionalBranch()) {
     // 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>(
           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);
+    return pushCleanupAfterFullExprWithActiveFlag<DestroyObject>(
+        cleanupKind, Address::invalid(), addr, type, destroyer, useEHCleanupForArray);
+  }
+
+  // Otherwise, we should only destroy the object if it's been initialized.
+  // Re-use the active flag and saved address across both the EH and end of
+  // scope cleanups.
+
+  using SavedType = typename DominatingValue<Address>::saved_type;
+  using ConditionalCleanupType =
+      EHScopeStack::ConditionalCleanup<DestroyObject, Address, QualType,
+                                       Destroyer *, bool>;
+
+  Address ActiveFlag = createCleanupActiveFlag();
+  SavedType SavedAddr = saveValueInCond(addr);
+
+  if (cleanupKind & EHCleanup) {
+    EHStack.pushCleanup<ConditionalCleanupType>(
+        static_cast<CleanupKind>(cleanupKind & ~NormalCleanup), SavedAddr, type,
+        destroyer, useEHCleanupForArray);
+    initFullExprCleanupWithFlag(ActiveFlag);
+  }
+
+  pushCleanupAfterFullExprWithActiveFlag<ConditionalCleanupType>(
+      cleanupKind, ActiveFlag, SavedAddr, type, destroyer,
+      useEHCleanupForArray);
 }
 
 /// emitDestroy - Immediately perform the destruction of the given
 /// object.
 ///
 /// \param addr - the address of the object; a type*
 /// \param type - the type of the object; if an array type, all
 ///   objects are destroyed in reverse order

clang/lib/CodeGen/CodeGenFunction.h

void pushFullExprCleanup(CleanupKind kind, As... A) {
     typedef std::tuple<typename DominatingValue<As>::saved_type...> SavedTuple;
     SavedTuple Saved{saveValueInCond(A)...};
 
     typedef EHScopeStack::ConditionalCleanup<T, As...> CleanupType;
     EHStack.pushCleanupTuple<CleanupType>(kind, Saved);
     initFullExprCleanup();
   }
 
-  /// Queue a cleanup to be pushed after finishing the current
-  /// full-expression.
+  /// Queue a cleanup to be pushed after finishing the current full-expression,
+  /// potentially with an active flag.
   template <class T, class... As>
   void pushCleanupAfterFullExpr(CleanupKind Kind, As... A) {
     if (!isInConditionalBranch())
-      return pushCleanupAfterFullExprImpl<T>(Kind, Address::invalid(), A...);
+      return pushCleanupAfterFullExprWithActiveFlag<T>(Kind, Address::invalid(),
+                                                       A...);
 
     Address ActiveFlag = createCleanupActiveFlag();
     assert(!DominatingValue<Address>::needsSaving(ActiveFlag) &&
            "cleanup active flag should never need saving");
 
     typedef std::tuple<typename DominatingValue<As>::saved_type...> SavedTuple;
     SavedTuple Saved{saveValueInCond(A)...};
 
     typedef EHScopeStack::ConditionalCleanup<T, As...> CleanupType;
-    pushCleanupAfterFullExprImpl<CleanupType>(Kind, ActiveFlag, Saved);
+    pushCleanupAfterFullExprWithActiveFlag<CleanupType>(Kind, ActiveFlag, Saved);
   }
 
   template <class T, class... As>
-  void pushCleanupAfterFullExprImpl(CleanupKind Kind, Address ActiveFlag,
-                                    As... A) {
+  void pushCleanupAfterFullExprWithActiveFlag(CleanupKind Kind,
+                                              Address ActiveFlag, As... A) {
     LifetimeExtendedCleanupHeader Header = {sizeof(T), Kind,
                                             ActiveFlag.isValid()};
 
     size_t OldSize = LifetimeExtendedCleanupStack.size();
     LifetimeExtendedCleanupStack.resize(
         LifetimeExtendedCleanupStack.size() + sizeof(Header) + Header.Size +
         (Header.IsConditional ? sizeof(ActiveFlag) : 0));
 

clang/test/CodeGenObjC/arc-blocks-exceptions.m

+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -emit-llvm -fblocks -fobjc-arc -fobjc-runtime-has-weak -fexceptions -disable-llvm-passes -o - %s | FileCheck %s
+
+void test1(_Bool c) {
+  void test1_fn(void (^blk)());
+  __weak id weakId = 0;
+  test1_fn(c ? ^{ (void)weakId; } : 0);
+
+  // CHECK: [[CLEANUP_COND:%.*]] = alloca i1
+  // CHECK-NEXT: [[CLEANUP_SAVE:%.*]] = alloca i8**
+
+  // CHECK: store i1 true, i1* [[CLEANUP_COND]]
+  // CHECK-NEXT: store i8** {{.*}}, i8*** [[CLEANUP_SAVE]]
+
+  // CHECK: invoke void @test1_fn(
+  // CHECK-NEXT: to label %[[INVOKE_CONT:.*]] unwind label %[[LANDING_PAD_LAB:.*]]
+
+  // CHECK: [[INVOKE_CONT]]:
+  // CHECK-NEXT: [[LOAD:%.*]] = load i1, i1* [[CLEANUP_COND]]
+  // CHECK-NEXT: br i1 [[LOAD]], label %[[END_OF_SCOPE_LAB:.*]], label
+
+  // CHECK: [[END_OF_SCOPE_LAB]]:
+  // CHECK-NEXT: [[LOAD:%.*]] = load i8**, i8*** [[CLEANUP_SAVE]]
+  // CHECK-NEXT: call void @llvm.objc.destroyWeak(i8** [[LOAD]])
+  // CHECK-NEXT: br label
+
+  // CHECK: [[LANDING_PAD_LAB]]:
+  //   /* some EH stuff */
+  // CHECK:      [[LOAD:%.*]] = load i1, i1* [[CLEANUP_COND]]
+  // CHECK-NEXT: br i1 [[LOAD]], label %[[EH_CLEANUP_LAB:.*]], label
+
+  // CHECK: [[EH_CLEANUP_LAB]]:
+  // CHECK-NEXT: [[LOAD:%.*]] = load i8**, i8*** [[CLEANUP_SAVE]]
+  // CHECK-NEXT: call void @llvm.objc.destroyWeak(i8** [[LOAD]])
+  // CHECK-NEXT: br label
+}