[llvm-branch-commits] [clang] [clang] Use tighter lifetime bounds for C temporary arguments (PR #170518)

[Eli Friedman via llvm-branch-commits]

================
@@ -4963,21 +4963,28 @@ void CodeGenFunction::EmitCallArg(CallArgList &args, const Expr *E,
 
   AggValueSlot ArgSlot = AggValueSlot::ignored();
   // For arguments with aggregate type, create an alloca to store
-  // the value.  If the argument's type has a destructor, that destructor
+  // the value. If the argument's type has a destructor, that destructor
   // will run at the end of the full-expression; emit matching lifetime
-  // markers.
-  //
-  // FIXME: For types which don't have a destructor, consider using a
-  // narrower lifetime bound.
+  // markers. For types which don't have a destructor, we use a narrower
+  // lifetime bound.
   if (hasAggregateEvaluationKind(E->getType())) {
     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>(NormalEHLifetimeMarker,
+                                             ArgSlotAlloca);
+      } else {
+        args.addLifetimeCleanup({ArgSlotAlloca.getPointer()});
+        if (getInvokeDest())
+          pushFullExprCleanup<CallLifetimeEnd>(CleanupKind::EHCleanup,
----------------
efriedma-quic wrote:

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.

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