[clang] [clang][Interp] Add scopes to conditional operator subexpressions (PR #104418)

[Timm Baeder via cfe-commits]

https://github.com/tbaederr created https://github.com/llvm/llvm-project/pull/104418

We would otherwise try to destroy the variables from both branches after the conditional operator was done.

However, doing so breaks the case where we create internal temporaries. For those, add allocateTemporary(), which attaches the lifetime of the temporary to the topmost scope. Since they never have record type, this shouldn't create any issues.

>From 0cea714e807f4863237eaa04c221e29e3ff26ed2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Timm=20B=C3=A4der?= <tbaeder at redhat.com>
Date: Thu, 15 Aug 2024 07:05:09 +0200
Subject: [PATCH] [clang][Interp] Add scopes to conditional operator
 subexpressions

We would otherwise try to destroy the variables from both branches after
the conditional operator was done.

However, doing so breaks the case where we create internal temporaries.
For those, add allocateTemporary(), which attaches the lifetime of the
temporary to the topmost scope. Since they never have record type, this
shouldn't create any issues.
---
 clang/lib/AST/Interp/Compiler.cpp | 63 +++++++++++++++++++++----------
 clang/lib/AST/Interp/Compiler.h   |  1 +
 clang/test/AST/Interp/records.cpp | 26 +++++++++++++
 3 files changed, 70 insertions(+), 20 deletions(-)

diff --git a/clang/lib/AST/Interp/Compiler.cpp b/clang/lib/AST/Interp/Compiler.cpp
index d32b595f9f0724..5e1f507ca2b178 100644
--- a/clang/lib/AST/Interp/Compiler.cpp
+++ b/clang/lib/AST/Interp/Compiler.cpp
@@ -533,10 +533,8 @@ bool Compiler<Emitter>::VisitCastExpr(const CastExpr *CE) {
     // We're creating a complex value here, so we need to
     // allocate storage for it.
     if (!Initializing) {
-      std::optional<unsigned> LocalIndex = allocateLocal(CE);
-      if (!LocalIndex)
-        return false;
-      if (!this->emitGetPtrLocal(*LocalIndex, CE))
+      unsigned LocalIndex = allocateTemporary(CE);
+      if (!this->emitGetPtrLocal(LocalIndex, CE))
         return false;
     }
 
@@ -671,10 +669,8 @@ bool Compiler<Emitter>::VisitImaginaryLiteral(const ImaginaryLiteral *E) {
     return true;
 
   if (!Initializing) {
-    std::optional<unsigned> LocalIndex = allocateLocal(E);
-    if (!LocalIndex)
-      return false;
-    if (!this->emitGetPtrLocal(*LocalIndex, E))
+    unsigned LocalIndex = allocateTemporary(E);
+    if (!this->emitGetPtrLocal(LocalIndex, E))
       return false;
   }
 
@@ -986,10 +982,8 @@ template <class Emitter>
 bool Compiler<Emitter>::VisitComplexBinOp(const BinaryOperator *E) {
   // Prepare storage for result.
   if (!Initializing) {
-    std::optional<unsigned> LocalIndex = allocateLocal(E);
-    if (!LocalIndex)
-      return false;
-    if (!this->emitGetPtrLocal(*LocalIndex, E))
+    unsigned LocalIndex = allocateTemporary(E);
+    if (!this->emitGetPtrLocal(LocalIndex, E))
       return false;
   }
 
@@ -1045,10 +1039,7 @@ bool Compiler<Emitter>::VisitComplexBinOp(const BinaryOperator *E) {
 
     if (!LHSIsComplex) {
       // This is using the RHS type for the fake-complex LHS.
-      if (auto LHSO = allocateLocal(RHS))
-        LHSOffset = *LHSO;
-      else
-        return false;
+      LHSOffset = allocateTemporary(RHS);
 
       if (!this->emitGetPtrLocal(LHSOffset, E))
         return false;
@@ -1881,15 +1872,26 @@ bool Compiler<Emitter>::VisitAbstractConditionalOperator(
   if (!this->jumpFalse(LabelFalse))
     return false;
 
-  if (!this->delegate(TrueExpr))
-    return false;
+  {
+    LocalScope<Emitter> S(this);
+    if (!this->delegate(TrueExpr))
+      return false;
+    if (!S.destroyLocals())
+      return false;
+  }
+
   if (!this->jump(LabelEnd))
     return false;
 
   this->emitLabel(LabelFalse);
 
-  if (!this->delegate(FalseExpr))
-    return false;
+  {
+    LocalScope<Emitter> S(this);
+    if (!this->delegate(FalseExpr))
+      return false;
+    if (!S.destroyLocals())
+      return false;
+  }
 
   this->fallthrough(LabelEnd);
   this->emitLabel(LabelEnd);
@@ -3549,6 +3551,27 @@ Compiler<Emitter>::allocateLocal(DeclTy &&Src, const ValueDecl *ExtendingDecl) {
   return Local.Offset;
 }
 
+template <class Emitter>
+unsigned Compiler<Emitter>::allocateTemporary(const Expr *E) {
+  QualType Ty = E->getType();
+  assert(!Ty->isRecordType());
+
+  Descriptor *D = P.createDescriptor(
+      E, Ty.getTypePtr(), Descriptor::InlineDescMD, Ty.isConstQualified(),
+      /*IsTemporary=*/true, /*IsMutable=*/false, /*Init=*/nullptr);
+  assert(D);
+
+  Scope::Local Local = this->createLocal(D);
+  VariableScope<Emitter> *S = VarScope;
+  assert(S);
+  // Attach to topmost scope.
+  while (S->getParent())
+    S = S->getParent();
+  assert(S && !S->getParent());
+  S->addLocal(Local);
+  return Local.Offset;
+}
+
 template <class Emitter>
 const RecordType *Compiler<Emitter>::getRecordTy(QualType Ty) {
   if (const PointerType *PT = dyn_cast<PointerType>(Ty))
diff --git a/clang/lib/AST/Interp/Compiler.h b/clang/lib/AST/Interp/Compiler.h
index 112219c49e8bdd..74bfce5f241f28 100644
--- a/clang/lib/AST/Interp/Compiler.h
+++ b/clang/lib/AST/Interp/Compiler.h
@@ -297,6 +297,7 @@ class Compiler : public ConstStmtVisitor<Compiler<Emitter>, bool>,
   /// Allocates a space storing a local given its type.
   std::optional<unsigned>
   allocateLocal(DeclTy &&Decl, const ValueDecl *ExtendingDecl = nullptr);
+  unsigned allocateTemporary(const Expr *E);
 
 private:
   friend class VariableScope<Emitter>;
diff --git a/clang/test/AST/Interp/records.cpp b/clang/test/AST/Interp/records.cpp
index f51f9771b38aaa..7e3cf5b94518f7 100644
--- a/clang/test/AST/Interp/records.cpp
+++ b/clang/test/AST/Interp/records.cpp
@@ -1627,3 +1627,29 @@ namespace DtorDestroysFieldsAfterSelf {
   static_assert(foo() == 15);
 }
 #endif
+
+namespace ExprWithCleanups {
+  struct A { A(); ~A(); int get(); };
+  constexpr int get() {return false ? A().get() : 1;}
+  static_assert(get() == 1, "");
+
+
+  struct S {
+    int V;
+    constexpr S(int V) : V(V) {}
+    constexpr int get() {
+      return V;
+    }
+  };
+  constexpr int get(bool b) {
+    S a = b ? S(1) : S(2);
+
+    return a.get();
+  }
+  static_assert(get(true) == 1, "");
+  static_assert(get(false) == 2, "");
+
+
+  constexpr auto F = true ? 1i : 2i;
+  static_assert(F == 1i, "");
+}