[clang] [clang][Interp] Fix `ArrayInitLoopExpr` handling (PR #67886)

via cfe-commits cfe-commits at lists.llvm.org
Mon Oct 23 11:13:50 PDT 2023


https://github.com/isuckatcs updated https://github.com/llvm/llvm-project/pull/67886

>From e623c9713dfaecd83aa8bcbd606b0a6e4c593c61 Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Sat, 30 Sep 2023 17:05:02 +0200
Subject: [PATCH 1/3] implement fix in interp

---
 clang/lib/AST/Interp/ByteCodeExprGen.cpp |  1 +
 clang/test/AST/Interp/cxx20.cpp          | 10 ++++------
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/clang/lib/AST/Interp/ByteCodeExprGen.cpp b/clang/lib/AST/Interp/ByteCodeExprGen.cpp
index d3e0d1112935a98..2860956bb99e972 100644
--- a/clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ b/clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -827,6 +827,7 @@ bool ByteCodeExprGen<Emitter>::VisitArrayInitLoopExpr(
   // where the LHS is on the stack (the target array)
   // and the RHS is our SubExpr.
   for (size_t I = 0; I != Size; ++I) {
+    BlockScope<Emitter> Scope(this);
     ArrayIndexScope<Emitter> IndexScope(this, I);
 
     if (ElemT) {
diff --git a/clang/test/AST/Interp/cxx20.cpp b/clang/test/AST/Interp/cxx20.cpp
index 553bc6eb4d5244f..8e54b6afd2982d3 100644
--- a/clang/test/AST/Interp/cxx20.cpp
+++ b/clang/test/AST/Interp/cxx20.cpp
@@ -701,13 +701,12 @@ namespace ThreeWayCmp {
   static_assert(pa2 <=> pa1 == 1, "");
 }
 
-// FIXME: Interp should also be able to evaluate this snippet properly.
 namespace ConstexprArrayInitLoopExprDestructors
 {
   struct Highlander {
       int *p = 0;
       constexpr Highlander() {}
-      constexpr void set(int *p) { this->p = p; ++*p; if (*p != 1) throw "there can be only one"; } // expected-note {{not valid in a constant expression}}
+      constexpr void set(int *p) { this->p = p; ++*p; if (*p != 1) throw "there can be only one"; }
       constexpr ~Highlander() { --*p; }
   };
 
@@ -715,19 +714,18 @@ namespace ConstexprArrayInitLoopExprDestructors
       int *p;
       constexpr X(int *p) : p(p) {}
       constexpr X(const X &x, Highlander &&h = Highlander()) : p(x.p) {
-          h.set(p); // expected-note {{in call to '&Highlander()->set(&n)'}}
+          h.set(p);
       }
   };
 
   constexpr int f() {
       int n = 0;
       X x[3] = {&n, &n, &n};
-      auto [a, b, c] = x; // expected-note {{in call to 'X(x[0], Highlander())'}}
+      auto [a, b, c] = x;
       return n;
   }
 
-  static_assert(f() == 0); // expected-error {{not an integral constant expression}} \
-                           // expected-note {{in call to 'f()'}}
+  static_assert(f() == 0);
 
   int main() {
       return f();

>From adb673278a95468511705c9fd901f5fafedc09a8 Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Mon, 23 Oct 2023 19:37:57 +0200
Subject: [PATCH 2/3] rebase

---
 clang/lib/AST/Interp/ByteCodeExprGen.cpp | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/clang/lib/AST/Interp/ByteCodeExprGen.cpp b/clang/lib/AST/Interp/ByteCodeExprGen.cpp
index 2860956bb99e972..056d85facb6843d 100644
--- a/clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ b/clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -816,9 +816,17 @@ bool ByteCodeExprGen<Emitter>::VisitArrayInitLoopExpr(
     const ArrayInitLoopExpr *E) {
   assert(Initializing);
   assert(!DiscardResult);
+
+  // Make sure the common expression is evaluated, and the result is cached in
+  // the current scope.
+  {
+    OptionScope<Emitter> Scope(this, /*NewDiscardResult=*/true,
+                               /*NewInitializing=*/false);
+    this->VisitOpaqueValueExpr(E->getCommonExpr());
+  }
+
   // TODO: This compiles to quite a lot of bytecode if the array is larger.
   //   Investigate compiling this to a loop.
-
   const Expr *SubExpr = E->getSubExpr();
   size_t Size = E->getArraySize().getZExtValue();
   std::optional<PrimType> ElemT = classify(SubExpr->getType());
@@ -873,9 +881,11 @@ bool ByteCodeExprGen<Emitter>::VisitOpaqueValueExpr(const OpaqueValueExpr *E) {
     return false;
 
   // Here the local variable is created but the value is removed from the stack,
-  // so we put it back, because the caller might need it.
-  if (!this->emitGetLocal(SubExprT, *LocalIndex, E))
-    return false;
+  // so we put it back if the caller needs it.
+  if (!DiscardResult) {
+    if (!this->emitGetLocal(SubExprT, *LocalIndex, E))
+      return false;
+  }
 
   // FIXME: Ideally the cached value should be cleaned up later.
   OpaqueExprs.insert({E, *LocalIndex});

>From 6087f37d0eb28d939a91df8af2a51239142afa52 Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Mon, 23 Oct 2023 20:07:41 +0200
Subject: [PATCH 3/3] ove cleanup

---
 clang/lib/AST/Interp/ByteCodeExprGen.cpp |  2 +-
 clang/lib/AST/Interp/ByteCodeExprGen.h   | 20 ++++++++++++++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/clang/lib/AST/Interp/ByteCodeExprGen.cpp b/clang/lib/AST/Interp/ByteCodeExprGen.cpp
index 056d85facb6843d..ec9cce5e018c537 100644
--- a/clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ b/clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -887,7 +887,7 @@ bool ByteCodeExprGen<Emitter>::VisitOpaqueValueExpr(const OpaqueValueExpr *E) {
       return false;
   }
 
-  // FIXME: Ideally the cached value should be cleaned up later.
+  // This is cleaned up when the local variable is destroyed.
   OpaqueExprs.insert({E, *LocalIndex});
 
   return true;
diff --git a/clang/lib/AST/Interp/ByteCodeExprGen.h b/clang/lib/AST/Interp/ByteCodeExprGen.h
index 83986d3dd579ed6..774d0b25f4ad56d 100644
--- a/clang/lib/AST/Interp/ByteCodeExprGen.h
+++ b/clang/lib/AST/Interp/ByteCodeExprGen.h
@@ -360,6 +360,7 @@ template <class Emitter> class LocalScope : public VariableScope<Emitter> {
     if (!Idx)
       return;
     this->Ctx->emitDestroy(*Idx, SourceInfo{});
+    removeStoredOpaqueValues();
   }
 
   /// Overriden to support explicit destruction.
@@ -368,6 +369,7 @@ template <class Emitter> class LocalScope : public VariableScope<Emitter> {
       return;
     this->emitDestructors();
     this->Ctx->emitDestroy(*Idx, SourceInfo{});
+    removeStoredOpaqueValues();
     this->Idx = std::nullopt;
   }
 
@@ -389,10 +391,28 @@ template <class Emitter> class LocalScope : public VariableScope<Emitter> {
       if (!Local.Desc->isPrimitive() && !Local.Desc->isPrimitiveArray()) {
         this->Ctx->emitGetPtrLocal(Local.Offset, SourceInfo{});
         this->Ctx->emitRecordDestruction(Local.Desc);
+        removeIfStoredOpaqueValue(Local);
       }
     }
   }
 
+  void removeStoredOpaqueValues() {
+    if (!Idx)
+      return;
+
+    for (Scope::Local &Local : this->Ctx->Descriptors[*Idx]) {
+      removeIfStoredOpaqueValue(Local);
+    }
+  }
+
+  void removeIfStoredOpaqueValue(const Scope::Local &Local) {
+    if (auto *OVE =
+            llvm::dyn_cast_or_null<OpaqueValueExpr>(Local.Desc->asExpr());
+        OVE && this->Ctx->OpaqueExprs.contains(OVE)) {
+      this->Ctx->OpaqueExprs.erase(OVE);
+    };
+  }
+
   /// Index of the scope in the chain.
   std::optional<unsigned> Idx;
 };



More information about the cfe-commits mailing list