Emit lifetime markers for temporary function parameter aggregates

[itessier]

Summary

Clang is not emitting lifetime markers for temporary function parameters, so more stack space is potentially being allocated than necessary.

A new parameter is added to the CreateAggTemp and EmitAnyExprToTemp functions to indicate whether to emit lifetime markers for aggregates, so that the EmitCallArg function can request them when evaluating an arg. The parameter is defaulted to false so that the behaviour of other callers is not affected.

[rjmccall]

lib/CodeGen/CGExpr.cpp

176

This is problematic because we're not necessarily in a scope that usefully limits the duration of cleanups — we don't push full-expression scopes when emitting an arbitrary statement. Probably we should, but we don't.

If you'd like to take a look at solving that problem first, that would be great.

[itessier]

This is problematic because we're not necessarily in a scope that usefully limits the duration of cleanups — we don't push full-expression scopes when emitting an arbitrary statement. Probably we should, but we don't.

If you'd like to take a look at solving that problem first, that would be great.

Sure I'd like to take a look at this, but I'm not familiar with the code gen module. Can you point me to where I should start looking to understand and figure out where to add scopes?

[rjmccall]

In D52440#1298816, @itessier wrote:

This is problematic because we're not necessarily in a scope that usefully limits the duration of cleanups — we don't push full-expression scopes when emitting an arbitrary statement. Probably we should, but we don't.

If you'd like to take a look at solving that problem first, that would be great.

Sure I'd like to take a look at this, but I'm not familiar with the code gen module. Can you point me to where I should start looking to understand and figure out where to add scopes?

It's not as well documented as I'd like. The main bits of infrastructure to understand are CodeGenFunction::RunCleanupsScope and the basic mechanics of pushing/popping cleanups in CGCleanups. Then the basic idea is that you want to find a way to push a scope around any sort of "top-level" expression emission (i.e. whenever we emit an expression that isn't part of an outer full-expression — which is a language concept you might need to look up). Fortunately, that's largely taken care of already because we have an ExprWithCleanups node that we create in Sema whenever there are certain kinds of semantic cleanups in a full-expression, and all the top-level places that emit expressions are probably already looking for that node to treat it specially. (As an example of that, look for the call to enterFullExpression in CodeGenFunction::EmitScalarInit.

So the general guidelines for the patch are:

1. Introduce a EmitIgnoredFullExpression function that checks for ExprWithCleanups, calls enterFullExpression if necessary, pushes a scope, and then calls EmitIgnoredExpr.

2. Change the expression case of CodeGenFunction::EmitStmt to call EmitIgnoredFullExpression. 1+2 should be a stable intermediate point where you can fix any test changes and submit a patch.

3. Change the general emission cases for ExprWithCleanups to unconditionally assert. (This means the case in EmitLValue, the cases in CGExprScalar, CGExprComplex, and CGExprAgg, and maybe a few others.) The idea here is that *nothing* should be relying for correctness on just encountering an ExprWithCleanups at an arbitrary position within the expression tree — they should all be pushing scopes anyway, and as part of that they all need to be checking for ExprWithCleanups, so they should never just find it alone.

4. Deal with any ramifications of (3).

5. Look at all the uses of ExprWithCleanups / enterFullExpression in CodeGen to make sure that they indeed unconditionally push a scope around expression-emission.

At that point, we should be reasonably confident that we're actually pushing scopes around every expression emission.

[itessier]

Thanks for the detailed info! I will try to get something out when I get some free cycles.

lib/CodeGen/CGCall.cpp

void CodeGenFunction::EmitCallArg(CallArgList &args, const Expr *E,
   if (HasAggregateEvalKind &&
       type->getAs<RecordType>()->getDecl()->isParamDestroyedInCallee()) {
     // If we're using inalloca, use the argument memory.  Otherwise, use a
     // temporary.
     AggValueSlot Slot;
     if (args.isUsingInAlloca())
       Slot = createPlaceholderSlot(*this, type);
     else
-      Slot = CreateAggTemp(type, "agg.tmp");
+      Slot = CreateAggTemp(type, "agg.tmp", /*EmitLifetimeMarkers=*/true);
 
     bool DestroyedInCallee = true, NeedsEHCleanup = true;
     if (const auto *RD = type->getAsCXXRecordDecl())
       DestroyedInCallee = RD->hasNonTrivialDestructor();
     else
       NeedsEHCleanup = needsEHCleanup(type.isDestructedType());
 
     if (DestroyedInCallee)
void CodeGenFunction::EmitCallArg(CallArgList &args, const Expr *E,
   if (HasAggregateEvalKind && isa<ImplicitCastExpr>(E) &&
       cast<CastExpr>(E)->getCastKind() == CK_LValueToRValue) {
     LValue L = EmitLValue(cast<CastExpr>(E)->getSubExpr());
     assert(L.isSimple());
     args.addUncopiedAggregate(L, type);
     return;
   }
 
-  args.add(EmitAnyExprToTemp(E), type);
+  args.add(EmitAnyExprToTemp(E, /*EmitLifetimeMarkers=*/true), type);
 }
 
 QualType CodeGenFunction::getVarArgType(const Expr *Arg) {
   // System headers on Windows define NULL to 0 instead of 0LL on Win64. MSVC
   // implicitly widens null pointer constants that are arguments to varargs
   // functions to pointer-sized ints.
   if (!getTarget().getTriple().isOSWindows())
     return Arg->getType();

lib/CodeGen/CGExpr.cpp

 }
 
 Address CodeGenFunction::CreateMemTempWithoutCast(QualType Ty,
                                                   const Twine &Name) {
   return CreateMemTempWithoutCast(Ty, getContext().getTypeAlignInChars(Ty),
                                   Name);
 }
 
+AggValueSlot CodeGenFunction::CreateAggTemp(QualType T, const Twine &Name,
+                                            bool EmitLifetimeMarkers) {
+  AggValueSlot Slot = AggValueSlot::forAddr(CreateMemTemp(T, Name),
+                               T.getQualifiers(),
+                               AggValueSlot::IsNotDestructed,
+                               AggValueSlot::DoesNotNeedGCBarriers,
+                               AggValueSlot::IsNotAliased,
+                               AggValueSlot::DoesNotOverlap);
+  if (ShouldEmitLifetimeMarkers && EmitLifetimeMarkers) {
+    uint64_t AllocSize =
+        CGM.getDataLayout().getTypeAllocSize(ConvertTypeForMem(T));
+    auto *Size = EmitLifetimeStart(AllocSize, Slot.getPointer());
+    if (Size) {
+      pushFullExprCleanup<CallLifetimeEnd>(NormalEHLifetimeMarker,
+                                           Slot.getAddress(), Size);
+    }
+  }
+  return Slot;
+}
+
 /// EvaluateExprAsBool - Perform the usual unary conversions on the specified
 /// expression and compare the result against zero, returning an Int1Ty value.
 llvm::Value *CodeGenFunction::EvaluateExprAsBool(const Expr *E) {
   PGO.setCurrentStmt(E);
   if (const MemberPointerType *MPT = E->getType()->getAs<MemberPointerType>()) {
     llvm::Value *MemPtr = EmitScalarExpr(E);
     return CGM.getCXXABI().EmitMemberPointerIsNotNull(*this, MemPtr, MPT);
   }
case TEK_Aggregate:
     EmitAggExpr(E, aggSlot);
     return aggSlot.asRValue();
   }
   llvm_unreachable("bad evaluation kind");
 }
 
 /// EmitAnyExprToTemp - Similar to EmitAnyExpr(), however, the result will
 /// always be accessible even if no aggregate location is provided.
-RValue CodeGenFunction::EmitAnyExprToTemp(const Expr *E) {
+RValue CodeGenFunction::EmitAnyExprToTemp(const Expr *E,
+                                          bool EmitLifetimeMarkers) {
   AggValueSlot AggSlot = AggValueSlot::ignored();
 
   if (hasAggregateEvaluationKind(E->getType()))
-    AggSlot = CreateAggTemp(E->getType(), "agg.tmp");
+    AggSlot = CreateAggTemp(E->getType(), "agg.tmp", EmitLifetimeMarkers);
   return EmitAnyExpr(E, AggSlot);
 }
 
 /// EmitAnyExprToMem - Evaluate an expression into a given memory
 /// location.
 void CodeGenFunction::EmitAnyExprToMem(const Expr *E,
                                        Address Location,
                                        Qualifiers Quals,

lib/CodeGen/CodeGenFunction.h

public:
   /// CreateMemTemp - Create a temporary memory object of the given type, with
   /// appropriate alignmen without casting it to the default address space.
   Address CreateMemTempWithoutCast(QualType T, const Twine &Name = "tmp");
   Address CreateMemTempWithoutCast(QualType T, CharUnits Align,
                                    const Twine &Name = "tmp");
 
   /// CreateAggTemp - Create a temporary memory object for the given
   /// aggregate type.
-  AggValueSlot CreateAggTemp(QualType T, const Twine &Name = "tmp") {
-    return AggValueSlot::forAddr(CreateMemTemp(T, Name),
-                                 T.getQualifiers(),
-                                 AggValueSlot::IsNotDestructed,
-                                 AggValueSlot::DoesNotNeedGCBarriers,
-                                 AggValueSlot::IsNotAliased,
-                                 AggValueSlot::DoesNotOverlap);
-  }
+  AggValueSlot CreateAggTemp(QualType T, const Twine &Name = "tmp",
+                             bool EmitLifetimeMarkers = false);
 
   /// Emit a cast to void* in the appropriate address space.
   llvm::Value *EmitCastToVoidPtr(llvm::Value *value);
 
   /// EvaluateExprAsBool - Perform the usual unary conversions on the specified
   /// expression and compare the result against zero, returning an Int1Ty value.
   llvm::Value *EvaluateExprAsBool(const Expr *E);
 
public:
 
   /// Emit a "reference" to a __builtin_ms_va_list; this is
   /// always the value of the expression, because a __builtin_ms_va_list is a
   /// pointer to a char.
   Address EmitMSVAListRef(const Expr *E);
 
   /// EmitAnyExprToTemp - Similarly to EmitAnyExpr(), however, the result will
   /// always be accessible even if no aggregate location is provided.
-  RValue EmitAnyExprToTemp(const Expr *E);
+  ///
+  /// \param EmitLifetimeMarkers True if lifetime markers should be emitted when
+  ///        creating an aggregrate location.
+  RValue EmitAnyExprToTemp(const Expr *E, bool EmitLifetimeMarkers = false);
 
   /// EmitAnyExprToMem - Emits the code necessary to evaluate an
   /// arbitrary expression into the given memory location.
   void EmitAnyExprToMem(const Expr *E, Address Location,
                         Qualifiers Quals, bool IsInitializer);
 
   void EmitAnyExprToExn(const Expr *E, Address Addr);
 

test/CodeGenCXX/microsoft-abi-eh-cleanups.cpp

 namespace PR37146 {
 // Check that IRGen doesn't emit calls to synthesized destructors for
 // non-trival C structs.
 
 // WIN32: define dso_local void @"?test@PR37146@@YAXXZ"()
 // WIN32: call void @llvm.memset.p0i8.i32(
 // WIN32: call i32 @"?getS@PR37146@@YA?AUS@1@XZ"(
 // WIN32: call void @"?func@PR37146@@YAXUS@1@0@Z"(
+// WIN32-NEXT: bitcast
+// WIN32-NEXT: call void @llvm.lifetime.end.p0i8(
+// WIN32-NEXT: bitcast
+// WIN32-NEXT: call void @llvm.lifetime.end.p0i8(
 // WIN32-NEXT: ret void
 // WIN32-NEXT: {{^}$}}
 
 struct S {
   int f;
 };
 
 void func(S, S);
 S getS();
 
 void test() {
   func(getS(), S());
 }
 
 }

test/CodeGenCXX/stack-reuse-miscompile.cpp

public:
   T(S);
 
   T concat(const T &Suffix) const;
   const char * str() const;
 };
 
 const char * f(S s)
 {
-// It's essential that the lifetimes of all three T temporaries here are
+// It's essential that the lifetimes of the first three T temporaries here are
 // overlapping. They must all remain alive through the call to str().
 //
 // CHECK: [[T1:%.*]] = alloca %class.T, align 4
 // CHECK: [[T2:%.*]] = alloca %class.T, align 4
 // CHECK: [[T3:%.*]] = alloca %class.T, align 4
+// CHECK: [[T4:%.*]] = alloca %class.S, align 4
 //
 // FIXME: We could defer starting the lifetime of the return object of concat
 // until the call.
 // CHECK: [[T1i8:%.*]] = bitcast %class.T* [[T1]] to i8*
 // CHECK: call void @llvm.lifetime.start.p0i8(i64 16, i8* [[T1i8]])
 //
 // CHECK: [[T2i8:%.*]] = bitcast %class.T* [[T2]] to i8*
 // CHECK: call void @llvm.lifetime.start.p0i8(i64 16, i8* [[T2i8]])
-// CHECK: [[T4:%.*]] = call %class.T* @_ZN1TC1EPKc(%class.T* [[T2]], i8* getelementptr inbounds ([2 x i8], [2 x i8]* @.str, i32 0, i32 0))
+// CHECK: [[T5:%.*]] = call %class.T* @_ZN1TC1EPKc(%class.T* [[T2]], i8* getelementptr inbounds ([2 x i8], [2 x i8]* @.str, i32 0, i32 0))
 //
 // CHECK: [[T3i8:%.*]] = bitcast %class.T* [[T3]] to i8*
 // CHECK: call void @llvm.lifetime.start.p0i8(i64 16, i8* [[T3i8]])
-// CHECK: [[T5:%.*]] = call %class.T* @_ZN1TC1E1S(%class.T* [[T3]], [2 x i32] %{{.*}})
+//
+// CHECK: [[T4i8:%.*]] = bitcast %class.S* [[T4]] to i8*
+// CHECK: call void @llvm.lifetime.start.p0i8(i64 8, i8* [[T4i8]])
+//
+// CHECK: [[T6:%.*]] = call %class.T* @_ZN1TC1E1S(%class.T* [[T3]], [2 x i32] %{{.*}})
 //
 // CHECK: call void @_ZNK1T6concatERKS_(%class.T* sret [[T1]], %class.T* [[T2]], %class.T* dereferenceable(16) [[T3]])
-// CHECK: [[T6:%.*]] = call i8* @_ZNK1T3strEv(%class.T* [[T1]])
+// CHECK: [[T7:%.*]] = call i8* @_ZNK1T3strEv(%class.T* [[T1]])
 //
 // CHECK: call void @llvm.lifetime.end.p0i8(
 // CHECK: call void @llvm.lifetime.end.p0i8(
 // CHECK: call void @llvm.lifetime.end.p0i8(
-// CHECK: ret i8* [[T6]]
+// CHECK: call void @llvm.lifetime.end.p0i8(
+// CHECK: ret i8* [[T7]]
 
   return T("[").concat(T(s)).str();
 }
 
 // CHECK: declare {{.*}}llvm.lifetime.start
 // CHECK: declare {{.*}}llvm.lifetime.end

test/CodeGenCXX/stack-reuse.cpp

struct Combiner {
 
   Combiner(S_large);
   Combiner f();
 };
 
 extern S_small foo_small();
 extern S_large foo_large();
 extern void bar_small(S_small*);
+extern void bar_small(S_small);
 extern void bar_large(S_large*);
+extern void bar_large(S_large);
 
 // Prevent mangling of function names.
 extern "C" {
 
 void small_rvoed_unnamed_temporary_object() {
 // CHECK-LABEL: define void @small_rvoed_unnamed_temporary_object
 // CHECK: call void @llvm.lifetime.start
 // CHECK: call void @_Z9foo_smallv
// CHECK: call void @llvm.lifetime.end
     bar_large(&s);
   }
   {
     S_large s;
     bar_large(&s);
   }
 }
 
+void small_temp_object() {
+// CHECK-LABEL: define void @small_temp_object
+// CHECK: call void @llvm.lifetime.start
+// CHECK: call void @_Z9foo_smallv
+// CHECK: call void @_Z9bar_small7S_small
+// CHECK: call void @llvm.lifetime.end
+// CHECK: call void @llvm.lifetime.start
+// CHECK: call void @_Z9foo_smallv
+// CHECK: call void @_Z9bar_small7S_small
+// CHECK: call void @llvm.lifetime.end
+
+  bar_small(foo_small());
+  bar_small(foo_small());
+}
+
+void large_temp_object() {
+// CHECK-LABEL: define void @large_temp_object
+// CHECK: call void @llvm.lifetime.start
+// CHECK: call void @_Z9foo_largev
+// CHECK: call void @_Z9bar_large7S_large
+// CHECK: call void @llvm.lifetime.end
+// CHECK: call void @llvm.lifetime.start
+// CHECK: call void @_Z9foo_largev
+// CHECK: call void @_Z9bar_large7S_large
+// CHECK: call void @llvm.lifetime.end
+
+  bar_large(foo_large());
+  bar_large(foo_large());
+}
+
 int large_combiner_test(S_large s) {
 // CHECK-LABEL: define i32 @large_combiner_test
 // CHECK: [[T2:%.*]] = alloca %struct.Combiner
 // CHECK: [[T1:%.*]] = alloca %struct.Combiner
 // CHECK: [[T3:%.*]] = call %struct.Combiner* @_ZN8CombinerC1E7S_large(%struct.Combiner* nonnull [[T1]], [9 x i32] %s.coerce)
 // CHECK: call void @_ZN8Combiner1fEv(%struct.Combiner* nonnull sret [[T2]], %struct.Combiner* nonnull [[T1]])
 // CHECK: [[T4:%.*]] = getelementptr inbounds %struct.Combiner, %struct.Combiner* [[T2]], i32 0, i32 0, i32 0, i32 0
 // CHECK: [[T5:%.*]] = load i32, i32* [[T4]]
 // CHECK: ret i32 [[T5]]
 
   return Combiner(s).f().a.a[0];
 }
 
 }