[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

848

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.

1105

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.

1109

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 ↗

(On Diff #143624)

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

594

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 ↗

(On Diff #143624)

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 ↗

(On Diff #143624)

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/CGClass.cpp

void CodeGenFunction::EmitConstructorBody(FunctionArgList &Args) {
   CXXCtorType CtorType = CurGD.getCtorType();
 
   assert((CGM.getTarget().getCXXABI().hasConstructorVariants() ||
           CtorType == Ctor_Complete) &&
          "can only generate complete ctor for this ABI");
 
   // Before we go any further, try the complete->base constructor
   // delegation optimization.
-  if (CtorType == Ctor_Complete && IsConstructorDelegationValid(Ctor) &&
-      CGM.getTarget().getCXXABI().hasConstructorVariants()) {
+  if (DelegateCXXConstructorCall) {
     EmitDelegateCXXConstructorCall(Ctor, Ctor_Base, Args, Ctor->getLocEnd());
     return;
   }
 
   const FunctionDecl *Definition = nullptr;
   Stmt *Body = Ctor->getBody(Definition);
   assert(Definition == Ctor && "emitting wrong constructor body");
 

 
 static bool canEmitDelegateCallArgs(CodeGenFunction &CGF,
                                     const CXXConstructorDecl *Ctor,
                                     CXXCtorType Type, CallArgList &Args) {
   // We can't forward a variadic call.
   if (Ctor->isVariadic())
     return false;
 
-  if (CGF.getTarget().getCXXABI().areArgsDestroyedLeftToRightInCallee()) {
-    // If the parameters are callee-cleanup, it's not safe to forward.
-    for (auto *P : Ctor->parameters())
-      if (P->getType().isDestructedType())
-        return false;
+    if (CGF.getContext().isParamDestroyedInCallee(P->getType()) &&
+        P->getType().isDestructedType())
 
+  if (CGF.getTarget().getCXXABI().areArgsDestroyedLeftToRightInCallee()) {
     // Likewise if they're inalloca.
     const CGFunctionInfo &Info =
         CGF.CGM.getTypes().arrangeCXXConstructorCall(Args, Ctor, Type, 0, 0);
     if (Info.usesInAlloca())
       return false;
   }
 
   // Anything else should be OK.

lib/CodeGen/CGDecl.cpp

if (SrcLangAS != DestLangAS) {
       DeclPtr = Address(getTargetHooks().performAddrSpaceCast(
                             *this, V, SrcLangAS, DestLangAS, T, true),
                         DeclPtr.getAlignment());
     }
 
     // Push a destructor cleanup for this parameter if the ABI requires it.
     // Don't push a cleanup in a thunk for a method that will also emit a
     // cleanup.
-    if (!IsScalar && !CurFuncIsThunk &&
+    if (!IsScalar && !CurFuncIsThunk && !DelegateCXXConstructorCall &&
         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);
       }
     }

lib/CodeGen/CodeGenFunction.h

public:
     SanitizerScope(CodeGenFunction *CGF);
     ~SanitizerScope();
   };
 
   /// In C++, whether we are code generating a thunk.  This controls whether we
   /// should emit cleanups.
   bool CurFuncIsThunk;
 
+  /// In C++, whether we are code generating a constructor that is calling a
+  /// delegating constructor.
+  bool DelegateCXXConstructorCall;
+
   /// In ARC, whether we should autorelease the return value.
   bool AutoreleaseResult;
 
   /// Whether we processed a Microsoft-style asm block during CodeGen. These can
   /// potentially set the return value.
   bool SawAsmBlock;
 
   const FunctionDecl *CurSEHParent = nullptr;
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;
     size_t LifetimeExtendedCleanupStackSize;
     bool OldDidCallStackSave;
   protected:
     bool PerformCleanup;
   private:
 
     RunCleanupsScope(const RunCleanupsScope &) = delete;
     void operator=(const RunCleanupsScope &) = 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:
 
   /// LocalDeclMap - This keeps track of the LLVM allocas or globals for local C
   /// decls.
   DeclMapTy LocalDeclMap;
 
   /// 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.
   llvm::DenseMap<const LabelDecl*, JumpDest> LabelMap;
 
   // BreakContinueStack - This keeps track of where break and continue
   // statements should jump to.
   struct BreakContinue {

lib/CodeGen/CodeGenFunction.cpp

 }
 
 CodeGenFunction::CodeGenFunction(CodeGenModule &cgm, bool suppressNewContext)
     : CodeGenTypeCache(cgm), CGM(cgm), Target(cgm.getTarget()),
       Builder(cgm, cgm.getModule().getContext(), llvm::ConstantFolder(),
               CGBuilderInserterTy(this)),
       CurFn(nullptr), ReturnValue(Address::invalid()),
       CapturedStmtInfo(nullptr), SanOpts(CGM.getLangOpts().Sanitize),
-      IsSanitizerScope(false), CurFuncIsThunk(false), AutoreleaseResult(false),
+      IsSanitizerScope(false), CurFuncIsThunk(false),
+      DelegateCXXConstructorCall(false), AutoreleaseResult(false),
       SawAsmBlock(false), IsOutlinedSEHHelper(false), BlockInfo(nullptr),
       BlockPointer(nullptr), LambdaThisCaptureField(nullptr),
       NormalCleanupDest(Address::invalid()), NextCleanupDestIndex(1),
       FirstBlockInfo(nullptr), EHResumeBlock(nullptr), ExceptionSlot(nullptr),
       EHSelectorSlot(nullptr), DebugInfo(CGM.getModuleDebugInfo()),
       DisableDebugInfo(false), DidCallStackSave(false), IndirectBranch(nullptr),
       PGO(cgm), SwitchInsn(nullptr), SwitchWeights(nullptr),
       CaseRangeBlock(nullptr), UnreachableBlock(nullptr), NumReturnExprs(0),
void CodeGenFunction::GenerateCode(GlobalDecl GD, llvm::Function *Fn,
 
   Stmt *Body = FD->getBody();
 
   // Initialize helper which will detect jumps which can cause invalid lifetime
   // markers.
   if (Body && ShouldEmitLifetimeMarkers)
     Bypasses.Init(Body);
 
+  if (const auto *Ctor = dyn_cast<CXXConstructorDecl>(CurGD.getDecl())) {
+    if (CurGD.getCtorType() == Ctor_Complete &&
+        IsConstructorDelegationValid(Ctor) &&
+        CGM.getTarget().getCXXABI().hasConstructorVariants())
+      DelegateCXXConstructorCall = true;
+  }
+
   // Emit the standard function prologue.
   StartFunction(GD, ResTy, Fn, FnInfo, Args, Loc, BodyRange.getBegin());
 
   // Generate the body of the function.
   PGO.assignRegionCounters(GD, CurFn);
   if (isa<CXXDestructorDecl>(FD))
     EmitDestructorBody(Args);
   else if (isa<CXXConstructorDecl>(FD))

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 Weak's destructor is not called 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) {}
+
+// Check that the call to the inheriting constructor is inlined.
+
+// CHECK: define internal void @__cxx_global_var_init{{.*}}()
+// CHECK: call void @_ZN4WeakC1EP11objc_object(
+// CHECK-NOT: call
+// CHECK: call void @_ZN4BaseC2E4Weak(
+// 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