Skip to content

Conversation

@ilovepi
Copy link
Contributor

@ilovepi ilovepi commented Dec 3, 2025

In C, consecutive statements in the same scope are under
CompoundStmt/CallExpr, while in C++ they typically fall under
CompoundStmt/ExprWithCleanup. This leads to different behavior with
respect to where pushFullExprCleanUp inserts the lifetime end markers
(e.g., at the end of scope).

For these cases, we can track and insert the lifetime end markers right
after the call completes. Allowing the stack space to be reused
immediately. This partially addresses #109204 and #43598 for improving
stack usage.

Co-authored-by: Nick Desaulniers [email protected]
Co-authored-by: Erik Pilkington [email protected]

Copy link
Contributor Author

ilovepi commented Dec 3, 2025

@ilovepi ilovepi marked this pull request as ready for review December 3, 2025 17:44
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:codegen IR generation bugs: mangling, exceptions, etc. labels Dec 3, 2025
@llvmbot
Copy link
Member

llvmbot commented Dec 3, 2025

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: Paul Kirth (ilovepi)

Changes

In C, consecutive statements in the same scope are under
CompoundStmt/CallExpr, while in C++ they typically fall under
CompoundStmt/ExprWithCleanup. This leads to different behavior with
respect to where pushFullExprCleanUp inserts the lifetime end markers
(e.g., at the end of scope).

For these cases, we can track and insert the lifetime end markers right
after the call completes. Allowing the stack space to be reused
immediately. This partially addresses #109204 and #43598 for improving
stack usage.

Co-authored-by: Nick Desaulniers <[email protected]>
Co-authored-by: Erik Pilkington <[email protected]>


Full diff: https://github.com/llvm/llvm-project/pull/170518.diff

4 Files Affected:

  • (modified) clang/lib/CodeGen/CGCall.cpp (+14-4)
  • (modified) clang/lib/CodeGen/CGCall.h (+19)
  • (modified) clang/test/CodeGen/stack-usage-lifetimes.c (+6-6)
  • (modified) clang/test/CodeGenCXX/stack-reuse-miscompile.cpp (+1-1)
diff --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp
index 92fe954fddcff..fc827c99c22b6 100644
--- a/clang/lib/CodeGen/CGCall.cpp
+++ b/clang/lib/CodeGen/CGCall.cpp
@@ -4957,11 +4957,16 @@ void CodeGenFunction::EmitCallArg(CallArgList &args, const Expr *E,
     RawAddress ArgSlotAlloca = Address::invalid();
     ArgSlot = CreateAggTemp(E->getType(), "agg.tmp", &ArgSlotAlloca);
 
-    // Emit a lifetime start/end for this temporary at the end of the full
-    // expression.
+    // Emit a lifetime start/end for this temporary. If the type has a
+    // destructor, then we need to keep it alive for the full expression.
     if (!CGM.getCodeGenOpts().NoLifetimeMarkersForTemporaries &&
-        EmitLifetimeStart(ArgSlotAlloca.getPointer()))
-      pushFullExprCleanup<CallLifetimeEnd>(NormalAndEHCleanup, ArgSlotAlloca);
+        EmitLifetimeStart(ArgSlotAlloca.getPointer())) {
+      if (E->getType().isDestructedType()) {
+        pushFullExprCleanup<CallLifetimeEnd>(NormalAndEHCleanup, ArgSlotAlloca);
+      } else {
+        args.addLifetimeCleanup({ArgSlotAlloca.getPointer()});
+      }
+    }
   }
 
   args.add(EmitAnyExpr(E, ArgSlot), type);
@@ -6291,6 +6296,11 @@ RValue CodeGenFunction::EmitCall(const CGFunctionInfo &CallInfo,
   for (CallLifetimeEnd &LifetimeEnd : CallLifetimeEndAfterCall)
     LifetimeEnd.Emit(*this, /*Flags=*/{});
 
+  if (!CGM.getCodeGenOpts().NoLifetimeMarkersForTemporaries)
+    for (const CallArgList::EndLifetimeInfo &LT :
+         CallArgs.getLifetimeCleanups())
+      EmitLifetimeEnd(LT.Addr);
+
   if (!ReturnValue.isExternallyDestructed() &&
       RetTy.isDestructedType() == QualType::DK_nontrivial_c_struct)
     pushDestroy(QualType::DK_nontrivial_c_struct, Ret.getAggregateAddress(),
diff --git a/clang/lib/CodeGen/CGCall.h b/clang/lib/CodeGen/CGCall.h
index 1ef8a3f114573..aab4b64d6a4a8 100644
--- a/clang/lib/CodeGen/CGCall.h
+++ b/clang/lib/CodeGen/CGCall.h
@@ -299,6 +299,10 @@ class CallArgList : public SmallVector<CallArg, 8> {
     llvm::Instruction *IsActiveIP;
   };
 
+  struct EndLifetimeInfo {
+    llvm::Value *Addr;
+  };
+
   void add(RValue rvalue, QualType type) { push_back(CallArg(rvalue, type)); }
 
   void addUncopiedAggregate(LValue LV, QualType type) {
@@ -312,6 +316,9 @@ class CallArgList : public SmallVector<CallArg, 8> {
     llvm::append_range(*this, other);
     llvm::append_range(Writebacks, other.Writebacks);
     llvm::append_range(CleanupsToDeactivate, other.CleanupsToDeactivate);
+    LifetimeCleanups.insert(LifetimeCleanups.end(),
+                            other.LifetimeCleanups.begin(),
+                            other.LifetimeCleanups.end());
     assert(!(StackBase && other.StackBase) && "can't merge stackbases");
     if (!StackBase)
       StackBase = other.StackBase;
@@ -352,6 +359,14 @@ class CallArgList : public SmallVector<CallArg, 8> {
   /// memory.
   bool isUsingInAlloca() const { return StackBase; }
 
+  void addLifetimeCleanup(EndLifetimeInfo Info) {
+    LifetimeCleanups.push_back(Info);
+  }
+
+  ArrayRef<EndLifetimeInfo> getLifetimeCleanups() const {
+    return LifetimeCleanups;
+  }
+
   // Support reversing writebacks for MSVC ABI.
   void reverseWritebacks() {
     std::reverse(Writebacks.begin(), Writebacks.end());
@@ -365,6 +380,10 @@ class CallArgList : public SmallVector<CallArg, 8> {
   /// occurs.
   SmallVector<CallArgCleanup, 1> CleanupsToDeactivate;
 
+  /// Lifetime information needed to call llvm.lifetime.end for any temporary
+  /// argument allocas.
+  SmallVector<EndLifetimeInfo, 2> LifetimeCleanups;
+
   /// The stacksave call.  It dominates all of the argument evaluation.
   llvm::CallInst *StackBase = nullptr;
 };
diff --git a/clang/test/CodeGen/stack-usage-lifetimes.c b/clang/test/CodeGen/stack-usage-lifetimes.c
index 3787a29e4ce7d..189bc9c229ca4 100644
--- a/clang/test/CodeGen/stack-usage-lifetimes.c
+++ b/clang/test/CodeGen/stack-usage-lifetimes.c
@@ -40,11 +40,11 @@ void t1(int c) {
 }
 
 void t2(void) {
-  // x86-precise-remark@-1 {{72 stack bytes}}
+  // x86-precise-remark@-1 {{40 stack bytes}}
   // x86-sloppy-remark@-2 {{72 stack bytes}}
-  // aarch64-precise-remark@-3 {{80 stack bytes}}
+  // aarch64-precise-remark@-3 {{48 stack bytes}}
   // aarch64-sloppy-remark@-4 {{80 stack bytes}}
-  // riscv-precise-remark@-5 {{80 stack bytes}}
+  // riscv-precise-remark@-5 {{48 stack bytes}}
   // riscv-sloppy-remark@-6 {{80 stack bytes}}
 
   useA(genA());
@@ -52,11 +52,11 @@ void t2(void) {
 }
 
 void t3(void) {
-  // x86-precise-remark@-1 {{72 stack bytes}}
+  // x86-precise-remark@-1 {{40 stack bytes}}
   // x86-sloppy-remark@-2 {{72 stack bytes}}
-  // aarch64-precise-remark@-3 {{80 stack bytes}}
+  // aarch64-precise-remark@-3 {{48 stack bytes}}
   // aarch64-sloppy-remark@-4 {{80 stack bytes}}
-  // riscv-precise-remark@-5 {{80 stack bytes}}
+  // riscv-precise-remark@-5 {{48 stack bytes}}
   // riscv-sloppy-remark@-6 {{80 stack bytes}}
 
   useB(genB());
diff --git a/clang/test/CodeGenCXX/stack-reuse-miscompile.cpp b/clang/test/CodeGenCXX/stack-reuse-miscompile.cpp
index 50c374d2710f4..4aef39119c94a 100644
--- a/clang/test/CodeGenCXX/stack-reuse-miscompile.cpp
+++ b/clang/test/CodeGenCXX/stack-reuse-miscompile.cpp
@@ -38,10 +38,10 @@ const char * f(S s)
 // CHECK: call void @llvm.lifetime.start.p0(ptr [[T3]])
 // CHECK: call void @llvm.lifetime.start.p0(ptr [[AGG]])
 // CHECK: [[T5:%.*]] = call noundef ptr @_ZN1TC1E1S(ptr {{[^,]*}} [[T3]], [2 x i32] %{{.*}})
+// CHECK: call void @llvm.lifetime.end.p0(ptr [[AGG]])
 //
 // CHECK: call void @_ZNK1T6concatERKS_(ptr dead_on_unwind writable sret(%class.T) align 4 [[T1]], ptr {{[^,]*}} [[T2]], ptr noundef nonnull align 4 dereferenceable(16) [[T3]])
 // CHECK: [[T6:%.*]] = call noundef ptr @_ZNK1T3strEv(ptr {{[^,]*}} [[T1]])
-// CHECK: call void @llvm.lifetime.end.p0(ptr [[AGG]])
 //
 // CHECK: call void @llvm.lifetime.end.p0(
 // CHECK: call void @llvm.lifetime.end.p0(

for (CallLifetimeEnd &LifetimeEnd : CallLifetimeEndAfterCall)
LifetimeEnd.Emit(*this, /*Flags=*/{});

if (!CGM.getCodeGenOpts().NoLifetimeMarkersForTemporaries)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NoLifetimeMarkersForTemporaries check is unnecessary: if it's disabled, getLifetimeCleanups() will be empty.

How do you handle inserting a lifetime marker on the exception path of an invoke?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a test specifically for invoke instructions. While it does make the bounds tighter, it does seem like we're missing the exception handling bits. I'll take a look at fixing that tomorrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the implementation (and tests). LMK if that's more in line with what you were thinking for the error handling paths.

@ilovepi ilovepi force-pushed the users/ilovepi/clang-lifetimes-2 branch from 6bd83b3 to 231df8a Compare December 3, 2025 21:56
@ilovepi ilovepi force-pushed the users/ilovepi/clang-lifetimes-temps branch from b4725eb to e0f9bcc Compare December 3, 2025 21:56
@ilovepi ilovepi force-pushed the users/ilovepi/clang-lifetimes-2 branch 2 times, most recently from 64b91cb to ba5d29e Compare December 4, 2025 00:52
Comment on lines 30 to 31
// CHECK-NOT: call void @llvm.lifetime.end.p0(ptr nonnull %[[AGG1]])
// CHECK-NOT: call void @llvm.lifetime.end.p0(ptr nonnull %[[AGG2]])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like we don't handle the exception case, which needs to be addressed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the new version covers this correctly now.

@ilovepi ilovepi force-pushed the users/ilovepi/clang-lifetimes-temps branch from 08189ad to 564e13b Compare December 4, 2025 21:48
@ilovepi ilovepi force-pushed the users/ilovepi/clang-lifetimes-2 branch from ba5d29e to 21b9837 Compare December 4, 2025 21:48
@github-actions
Copy link

github-actions bot commented Dec 4, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

} else {
args.addLifetimeCleanup({ArgSlotAlloca.getPointer()});
if (getInvokeDest())
pushFullExprCleanup<CallLifetimeEnd>(CleanupKind::EHCleanup,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this still doesn't work quite right.

Consider the case where you have two calls which can throw in the expression. Something like f2(f1(X{})). If f1 throws an exception, you correctly end the lifetime, but if f2 throws an exception, you end up calling lifetime.end on an alloca where the lifetime already ended.

You can probably fudge this by deactivating the cleanup after the call. But it would be cleaner to introduce a new scope for the call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, yeah, that's a good point. Thanks for the suggestion. That sounds promising.

@ilovepi ilovepi force-pushed the users/ilovepi/clang-lifetimes-2 branch 2 times, most recently from 0c900db to 4f50948 Compare December 4, 2025 22:52
@ilovepi ilovepi force-pushed the users/ilovepi/clang-lifetimes-temps branch from 17a82ce to 15c1888 Compare December 5, 2025 18:40
@ilovepi ilovepi force-pushed the users/ilovepi/clang-lifetimes-2 branch 2 times, most recently from 3dc61c1 to 1b4fb8b Compare December 5, 2025 18:50
Base automatically changed from users/ilovepi/clang-lifetimes-temps to main December 5, 2025 19:51
In C, consecutive statements in the same scope are under
CompoundStmt/CallExpr, while in C++ they typically fall under
CompoundStmt/ExprWithCleanup. This leads to different behavior with
respect to where pushFullExprCleanUp inserts the lifetime end markers
(e.g., at the end of scope).

For these cases, we can track and insert the lifetime end markers right
after the call completes. Allowing the stack space to be reused
immediately. This partially addresses #109204 and #43598 for improving
stack usage.
@ilovepi ilovepi force-pushed the users/ilovepi/clang-lifetimes-2 branch from 1b4fb8b to 2560fa6 Compare December 5, 2025 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clang:codegen IR generation bugs: mangling, exceptions, etc. clang Clang issues not falling into any other category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants