[CodeGen] Avoid destructing a struct type that has already been destructed by a delegated constructor

[ahatanak]

Summary

This patch fixes a bug where a struct with an ObjC __weak field gets destructed after it has already been destructed. This bug was introduced in r328731, which made changes to the ABI that caused structs with ObjC pointer fields to be destructed in the callee in some cases.

This happens in two cases:

1. C++11 inheriting constructors.
2. When EmitConstructorBody does complete->base constructor delegation optimization.

I fixed the first case by making changes to canEmitDelegateCallArgs so that it returns false when the constructor has a parameter that is destructed in the callee.

For the second case, I made changes so that EmitParmDecl doesn't push the destructor cleanup for the struct parameter if the function is a constructor that is going to delegate to the base constructor. Alternatively, I think it's possible to just disable the optimization in EmitConstructorBody if canEmitDelegateCallArgs returns false.

rdar://problem/39194693

[rjmccall]

Hmm. I'm not actually sure *why* it's not okay to forward callee-cleanup arguments. Do we just not have the necessary logic to disable the cleanup in the caller?

[ahatanak]

In D45382#1060163, @rjmccall wrote:

Hmm. I'm not actually sure *why* it's not okay to forward callee-cleanup arguments. Do we just not have the necessary logic to disable the cleanup in the caller?

It seems that it would be OK if there was a way to disable the cleanup in the caller when we know that we are delegating to another constructor, possibly by setting the DelegateCXXConstructorCall here too. But maybe there are other reasons for not doing so.

Perhaps Richard (who committed r274049) knows why we can't call the delegated constructor when there are callee-cleanup arguments?

[rjmccall]

If that's the problem, then I think the right design is for CallArg to include an optional cleanup reference for a cleanup that can be deactivated at the instant of the call (we should assert that this exists for parameters that are destroyed in the callee), and then for forwarding it's just a matter of getting that cleanup from parameter emission to forwarding-argument emission.

[ahatanak]

Deactivate the cleanups for callee-destructed parameters that are being forwarded to a delegated constructor.

I also added a new member (CurrentCleanupStackDepth) to CodeGenFunction that tracks the stack depth of the most recent RunCleanupsScope. This is needed to prevent popping the cleanup at the top of the stack in DeactivateCleanupBlock if the cleanup doesn't belong to the current RunCleanupsScope. Without this, the current RunCleanupsScope's CleanupStackDepth can point to a cleanup that doesn't exist on the stack anymore (CleanupStackDepth.encloses(EHStack.stable_begin()) doesn't hold true anymore), which can cause CodeGenFunction::PopCleanupBlocks to pop all the cleanups before it hits the bottom of the stack and crash.

[rjmccall]

lib/CodeGen/CodeGenFunction.h

851

You don't need (or want) these accessors, I think; this is just private state of the CGF object, and nobody else should be using it.

1108

It's too bad that we need this DenseMap in every CGF when actually only a very specific set of thunk functions will actually use it. But I guess DenseMap is at least trivial to construct/destroy when empty, which will be the most common case.

1112

How about CurrentCleanupScopeDepth? The current name makes it sound like it's the active depth of the cleanup stack.

[ahatanak]

Address review comments.

[rjmccall]

lib/CodeGen/CGCall.cpp

3069

I wonder if this is something we should be taking from the CGFunctionInfo instead. It does seem plausible that it could vary, e.g. according to the calling convention. But maybe that's something we can handle in a separate patch?

lib/CodeGen/CodeGenFunction.h

590

Please rename this variable to something like OldCleanupScopeDepth.

[rsmith]

In D45382#1060452, @ahatanak wrote:

In D45382#1060163, @rjmccall wrote:

Hmm. I'm not actually sure *why* it's not okay to forward callee-cleanup arguments. Do we just not have the necessary logic to disable the cleanup in the caller?

It seems that it would be OK if there was a way to disable the cleanup in the caller when we know that we are delegating to another constructor, possibly by setting the DelegateCXXConstructorCall here too. But maybe there are other reasons for not doing so.

Perhaps Richard (who committed r274049) knows why we can't call the delegated constructor when there are callee-cleanup arguments?

I think the only reason was that we didn't have a good way to handle cleanup emission in the thunk (as @rjmccall points out, we do need parameter cleanups in the thunk, but we need to disable them at the point of the call to the inherited constructor). I cannot think of any correctness reason that prevents using a thunk for the callee-cleanup case.

[ahatanak]

x.

lib/CodeGen/CGCall.cpp

3069

I assume you are talking about the call to isParamDestroyedInCallee? If so, yes, I think we can discuss it in a separate patch. I have plans to clean up the way ParamDestroyedInCallee is handled in Sema and IRGen.

[rjmccall]

lib/CodeGen/CGCall.cpp

3069

I assume you are talking about the call to isParamDestroyedInCallee?

Yeah.

I assume you are talking about the call to isParamDestroyedInCallee? If so, yes, I think we can discuss it in a separate patch.

Okay, great, thanks.

[ahatanak]

Rename variable to something OldCleanupScopeDepth.

[rjmccall]

Thanks, LGTM.

lib/CodeGen/CGCall.cpp

// performed exactly once for a set of arguments, but that should be safe.
     Builder.CreateStore(null, local);
     args.add(RValue::get(ptr), type);
 
   // For the most part, we just need to load the alloca, except that
   // aggregate r-values are actually pointers to temporaries.
   } else {
     args.add(convertTempToRValue(local, type, loc), type);
   }
+
+  // Deactivate the cleanup for the callee-destructed param that was pushed.
+  if (hasAggregateEvaluationKind(type) &&
+      getContext().isParamDestroyedInCallee(type)) {
+    EHScopeStack::stable_iterator cleanup =
+        CalleeDestructedParamCleanups.lookup(cast<ParmVarDecl>(param));
+    if (cleanup.isValid()) {
+      // This unreachable is a temporary marker which will be removed later.
+      llvm::Instruction *isActive = Builder.CreateUnreachable();
+      args.addArgCleanupDeactivation(cleanup, isActive);
+    } else
+      // A param cleanup should have been pushed unless we are code-generating
+      // a thunk.
+      assert(CurFuncIsThunk &&
+             "cleanup for callee-destructed param not recorded");
+  }
 }
 
 static bool isProvablyNull(llvm::Value *addr) {
   return isa<llvm::ConstantPointerNull>(addr);
 }
 
 /// Emit the actual writing-back of a writeback.
 static void emitWriteback(CodeGenFunction &CGF,

lib/CodeGen/CGCleanup.cpp

 
 /// Deactive a cleanup that was created in an active state.
 void CodeGenFunction::DeactivateCleanupBlock(EHScopeStack::stable_iterator C,
                                              llvm::Instruction *dominatingIP) {
   assert(C != EHStack.stable_end() && "deactivating bottom of stack?");
   EHCleanupScope &Scope = cast<EHCleanupScope>(*EHStack.find(C));
   assert(Scope.isActive() && "double deactivation");
 
-  // If it's the top of the stack, just pop it.
-  if (C == EHStack.stable_begin()) {
+  // If it's the top of the stack, just pop it, but do so only if it belongs
+  // to the current RunCleanupsScope.
+  if (C == EHStack.stable_begin() &&
+      CurrentCleanupScopeDepth.strictlyEncloses(C)) {
     // If it's a normal cleanup, we need to pretend that the
     // fallthrough is unreachable.
     CGBuilderTy::InsertPoint SavedIP = Builder.saveAndClearIP();
     PopCleanupBlock();
     Builder.restoreIP(SavedIP);
     return;
   }
 

lib/CodeGen/CGDecl.cpp

if (Arg.isIndirect()) {
     // cleanup.
     if (!IsScalar && !CurFuncIsThunk &&
         getContext().isParamDestroyedInCallee(Ty)) {
       if (QualType::DestructionKind DtorKind = Ty.isDestructedType()) {
         assert((DtorKind == QualType::DK_cxx_destructor ||
                 DtorKind == QualType::DK_nontrivial_c_struct) &&
                "unexpected destructor type");
         pushDestroy(DtorKind, DeclPtr, Ty);
+        CalleeDestructedParamCleanups[cast<ParmVarDecl>(&D)] =
+            EHStack.stable_begin();
       }
     }
   } else {
     // Check if the parameter address is controlled by OpenMP runtime.
     Address OpenMPLocalAddr =
         getLangOpts().OpenMP
             ? CGM.getOpenMPRuntime().getAddressOfLocalVariable(*this, &D)
             : Address::invalid();

lib/CodeGen/CodeGenFunction.h

public:
   ///   all paths of execution between the current IP and the
   ///   the point at which the cleanup comes into scope.
   void ActivateCleanupBlock(EHScopeStack::stable_iterator Cleanup,
                             llvm::Instruction *DominatingIP);
 
   /// \brief Enters a new scope for capturing cleanups, all of which
   /// will be executed once the scope is exited.
   class RunCleanupsScope {
-    EHScopeStack::stable_iterator CleanupStackDepth;
+    EHScopeStack::stable_iterator CleanupStackDepth, OldCleanupScopeDepth;
     size_t LifetimeExtendedCleanupStackSize;
     bool OldDidCallStackSave;
   protected:
     bool PerformCleanup;
   private:
 
     RunCleanupsScope(const RunCleanupsScope &) = delete;
     void operator=(const RunCleanupsScope &) = delete;
 
   protected:
     CodeGenFunction& CGF;
 
   public:
     /// \brief Enter a new cleanup scope.
     explicit RunCleanupsScope(CodeGenFunction &CGF)
       : PerformCleanup(true), CGF(CGF)
     {
       CleanupStackDepth = CGF.EHStack.stable_begin();
       LifetimeExtendedCleanupStackSize =
           CGF.LifetimeExtendedCleanupStack.size();
       OldDidCallStackSave = CGF.DidCallStackSave;
       CGF.DidCallStackSave = false;
+      OldCleanupScopeDepth = CGF.CurrentCleanupScopeDepth;
+      CGF.CurrentCleanupScopeDepth = CleanupStackDepth;
     }
 
     /// \brief Exit this cleanup scope, emitting any accumulated cleanups.
     ~RunCleanupsScope() {
       if (PerformCleanup)
         ForceCleanup();
     }
 
public:
     /// a shared cleanup block, these value pointers will be rewritten.
     /// Otherwise, they not will be modified.
     void ForceCleanup(std::initializer_list<llvm::Value**> ValuesToReload = {}) {
       assert(PerformCleanup && "Already forced cleanup");
       CGF.DidCallStackSave = OldDidCallStackSave;
       CGF.PopCleanupBlocks(CleanupStackDepth, LifetimeExtendedCleanupStackSize,
                            ValuesToReload);
       PerformCleanup = false;
+      CGF.CurrentCleanupScopeDepth = OldCleanupScopeDepth;
     }
   };
 
+  // Cleanup stack depth of the RunCleanupsScope that was pushed most recently.
+  EHScopeStack::stable_iterator CurrentCleanupScopeDepth =
+      EHScopeStack::stable_end();
+
   class LexicalScope : public RunCleanupsScope {
     SourceRange Range;
     SmallVector<const LabelDecl*, 4> Labels;
     LexicalScope *ParentScope;
 
     LexicalScope(const LexicalScope &) = delete;
     void operator=(const LexicalScope &) = delete;
 
PopCleanupBlocks(EHScopeStack::stable_iterator OldCleanupStackSize,
                    std::initializer_list<llvm::Value **> ValuesToReload = {});
 
   void ResolveBranchFixups(llvm::BasicBlock *Target);
 
   /// The given basic block lies in the current EH scope, but may be a
   /// target of a potentially scope-crossing jump; get a stable handle
   /// to which we can perform this jump later.
   JumpDest getJumpDestInCurrentScope(llvm::BasicBlock *Target) {
     return JumpDest(Target,
                     EHStack.getInnermostNormalCleanup(),
                     NextCleanupDestIndex++);
   }
 
   /// The given basic block lies in the current EH scope, but may be a
   /// target of a potentially scope-crossing jump; get a stable handle
   /// to which we can perform this jump later.
   JumpDest getJumpDestInCurrentScope(StringRef Name = StringRef()) {
private:
   /// we add the label to the indirect goto.  Every subsequent indirect goto is
   /// codegen'd as a jump to the IndirectBranch's basic block.
   llvm::IndirectBrInst *IndirectBranch;
 
   /// LocalDeclMap - This keeps track of the LLVM allocas or globals for local C
   /// decls.
   DeclMapTy LocalDeclMap;
 
+  // Keep track of the cleanups for callee-destructed parameters pushed to the
+  // cleanup stack so that they can be deactivated later.
+  llvm::DenseMap<const ParmVarDecl *, EHScopeStack::stable_iterator>
+      CalleeDestructedParamCleanups;
+
   /// SizeArguments - If a ParmVarDecl had the pass_object_size attribute, this
   /// will contain a mapping from said ParmVarDecl to its implicit "object_size"
   /// parameter.
   llvm::SmallDenseMap<const ParmVarDecl *, const ImplicitParamDecl *, 2>
       SizeArguments;
 
   /// Track escaped local variables with auto storage. Used during SEH
   /// outlining to produce a call to llvm.localescape.
   llvm::DenseMap<llvm::AllocaInst *, int> EscapedLocals;
 
   /// LabelMap - This keeps track of the LLVM basic block for each C label.

test/CodeGenObjCXX/arc-forwarded-lambda-call.mm

 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.12.0 -emit-llvm -disable-llvm-passes -O3 -fblocks -fobjc-arc -fobjc-runtime-has-weak -std=c++11 -o - %s | FileCheck %s
 
 void test0(id x) {
   extern void test0_helper(id (^)(void));
   test0_helper([=]() { return x; });
   // CHECK-LABEL: define internal i8* @___Z5test0P11objc_object_block_invoke
   // CHECK: [[T0:%.*]] = call i8* @"_ZZ5test0P11objc_objectENK3$_0clEv"
   // CHECK-NEXT: [[T1:%.*]] = call i8* @objc_retainAutoreleasedReturnValue(i8* [[T0]])
   // CHECK-NEXT: [[T2:%.*]] = tail call i8* @objc_autoreleaseReturnValue(i8* [[T1]])
   // CHECK-NEXT: ret i8* [[T2]]
 }
 
+// Check that the delegating block invoke function doesn't destruct the Weak
+// object that is passed.
+
+// CHECK-LABEL: define internal void @___Z8testWeakv_block_invoke(
+// CHECK: call void @"_ZZ8testWeakvENK3$_2clE4Weak"(
+// CHECK-NEXT: ret void
+
+// CHECK-LABEL: define internal void @"_ZZ8testWeakvENK3$_2clE4Weak"(
+// CHECK: call void @_ZN4WeakD1Ev(
+// CHECK-NEXT: ret void
+
 id test1_rv;
 
 void test1() {
   extern void test1_helper(id (*)(void));
   test1_helper([](){ return test1_rv; });
   // CHECK-LABEL: define internal i8* @"_ZZ5test1vEN3$_18__invokeEv"
   // CHECK: [[T0:%.*]] = call i8* @"_ZZ5test1vENK3$_1clEv"
   // CHECK-NEXT: [[T1:%.*]] = call i8* @objc_retainAutoreleasedReturnValue(i8* [[T0]])
   // CHECK-NEXT: [[T2:%.*]] = tail call i8* @objc_autoreleaseReturnValue(i8* [[T1]])
   // CHECK-NEXT: ret i8* [[T2]]
 }
+
+struct Weak {
+  __weak id x;
+};
+
+void testWeak() {
+  extern void testWeak_helper(void (^)(Weak));
+  testWeak_helper([](Weak){});
+}

test/CodeGenObjCXX/arc-special-member-functions.mm

-// RUN: %clang_cc1 -fobjc-arc -fblocks -triple x86_64-apple-darwin10.0.0 -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -std=c++11 -fobjc-arc -fblocks -triple x86_64-apple-darwin10.0.0 -fobjc-runtime-has-weak -emit-llvm -o - %s | FileCheck %s
 
 struct ObjCMember {
   id member;
 };
 
 struct ObjCArrayMember {
   id member[2][3];
 };
 
 struct ObjCBlockMember {
   int (^bp)(int);
 };
 
+// CHECK: %[[STRUCT_CONTAINSWEAK:.*]] = type { %[[STRUCT_WEAK:.*]] }
+// CHECK: %[[STRUCT_WEAK]] = type { i8* }
+
+// The Weak object that is passed is destructed in this constructor.
+
+// CHECK: define void @_ZN12ContainsWeakC2E4Weak(
+// CHECK: call void @_ZN4WeakC1ERKS_(
+// CHECK: call void @_ZN4WeakD1Ev(
+
+// Check that the Weak object passed to this constructor is not destructed after
+// the delegate constructor is called.
+
+// CHECK: define void @_ZN12ContainsWeakC1E4Weak(
+// CHECK: call void @_ZN12ContainsWeakC2E4Weak(
+// CHECK-NEXT: ret void
+
+struct Weak {
+  Weak(id);
+  __weak id x;
+};
+
+struct ContainsWeak {
+  ContainsWeak(Weak);
+  Weak w;
+};
+
+ContainsWeak::ContainsWeak(Weak a) : w(a) {}
+
+// The Weak object that is passed is destructed in this constructor.
+
+// CHECK: define void @_ZN4BaseC2E4Weak(
+// CHECK: call void @_ZN4WeakD1Ev(
+// CHECK: ret void
+
+// Check that the Weak object passed to this constructor is not destructed after
+// the delegate constructor is called.
+
+// CHECK: define linkonce_odr void @_ZN7DerivedCI14BaseE4Weak(
+// CHECK: call void @_ZN7DerivedCI24BaseE4Weak(
+// CHECK-NEXT: ret void
+
+struct Base {
+  Base(Weak);
+};
+
+Base::Base(Weak a) {}
+
+struct Derived : Base {
+  using Base::Base;
+};
+
+Derived d(Weak(0));
+
 // CHECK-LABEL: define void @_Z42test_ObjCMember_default_construct_destructv(
 void test_ObjCMember_default_construct_destruct() {
   // CHECK: call void @_ZN10ObjCMemberC1Ev
   // CHECK: call void @_ZN10ObjCMemberD1Ev
   ObjCMember m1;
 }
 
 // CHECK-LABEL: define void @_Z39test_ObjCMember_copy_construct_destruct10ObjCMember

 // CHECK-NEXT: [[T4:%.*]] = bitcast i8* [[T3]] to i32 (i32)*
 // CHECK-NEXT: [[T5:%.*]] = getelementptr inbounds [[T]], [[T]]* {{%.*}}, i32 0, i32 0
 // CHECK-NEXT: [[T6:%.*]] = load i32 (i32)*, i32 (i32)** [[T5]], align 8
 // CHECK-NEXT: store i32 (i32)* [[T4]], i32 (i32)** [[T5]]
 // CHECK-NEXT: [[T7:%.*]] = bitcast i32 (i32)* [[T6]] to i8*
 // CHECK-NEXT: call void @objc_release(i8* [[T7]])
 // CHECK-NEXT: ret
 
+// Check that the Weak object passed to this constructor is not destructed after
+// the delegate constructor is called.
+
+// CHECK: define linkonce_odr void @_ZN7DerivedCI24BaseE4Weak(
+// CHECK: call void @_ZN4BaseC2E4Weak(
+// CHECK-NEXT: ret void
+
 // Implicitly-generated default constructor for ObjCMember
 // CHECK-LABEL: define linkonce_odr void @_ZN10ObjCMemberC2Ev
 // CHECK-NOT: objc_release
 // CHECK: store i8* null
 // CHECK-NEXT: ret void
 
 // Implicitly-generated destructor for ObjCMember
 // CHECK-LABEL: define linkonce_odr void @_ZN10ObjCMemberD2Ev

test/CodeGenObjCXX/lambda-expressions.mm

-// RUN: %clang_cc1 -triple x86_64-apple-darwin10.0.0 -emit-llvm -o - %s -fexceptions -std=c++11 -fblocks -fobjc-arc | FileCheck -check-prefix=ARC %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10.0.0 -emit-llvm -o - %s -fexceptions -std=c++11 -fblocks -fobjc-arc -fobjc-runtime-has-weak -DWEAK_SUPPORTED | FileCheck -check-prefix=ARC %s
 // RUN: %clang_cc1 -triple x86_64-apple-darwin10.0.0 -emit-llvm -o - %s -fexceptions -std=c++11 -fblocks | FileCheck -check-prefix=MRC %s
 
 typedef int (^fp)();
 fp f() { auto x = []{ return 3; }; return x; }
 
 // ARC: %[[LAMBDACLASS:.*]] = type { i32 }
 
 // MRC: @OBJC_METH_VAR_NAME{{.*}} = private unnamed_addr constant [5 x i8] c"copy\00"

 @implementation Foo
 - (void)foo {
   [&] {
     ^{ (void)self; }();
   }();
 }
 @end
 
+// Check that the delegating invoke function doesn't destruct the Weak object
+// that is passed.
+
+// ARC-LABEL: define internal void @"_ZZN14LambdaDelegate4testEvEN3$_58__invokeENS_4WeakE"(
+// ARC: call void @"_ZZN14LambdaDelegate4testEvENK3$_5clENS_4WeakE"(
+// ARC-NEXT: ret void
+
+// ARC-LABEL: define internal void @"_ZZN14LambdaDelegate4testEvENK3$_5clENS_4WeakE"(
+// ARC: call void @_ZN14LambdaDelegate4WeakD1Ev(
+
+#ifdef WEAK_SUPPORTED
+
+namespace LambdaDelegate {
+
+struct Weak {
+  __weak id x;
+};
+
+void test() {
+  void (*p)(Weak) = [](Weak a) { };
+}
+
+};
+
+#endif
+
 // ARC: attributes [[NUW]] = { noinline nounwind{{.*}} }
 // MRC: attributes [[NUW]] = { noinline nounwind{{.*}} }