[clang] [clang] Fix a crash from nested ArrayInitLoopExpr (PR #67722)

via cfe-commits cfe-commits at lists.llvm.org
Fri Sep 29 05:31:47 PDT 2023


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

>From a6d3c27ea0b35e32198e1652fe3f6d647bc8cec4 Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Thu, 28 Sep 2023 20:43:23 +0200
Subject: [PATCH 1/4] [clang] Fix a crash from nested ArrayInitLoopExpr

When visiting an ArrayInitLoopExpr, clang creates a temporary
variable for the source array, however in a nested AILE this
variable is created multiple times, in every iteration as they
have the same AST node and clang is unable to differentiate them.

Fixes #57135
---
 clang/lib/AST/ExprConstant.cpp                      | 11 ++++++-----
 .../nested-array-init-loop-in-lambda-capture.cpp    | 13 +++++++++++++
 2 files changed, 19 insertions(+), 5 deletions(-)
 create mode 100644 clang/test/AST/nested-array-init-loop-in-lambda-capture.cpp

diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index fea06b97259fe31..a1f5c262ca10be3 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -10967,19 +10967,20 @@ bool ArrayExprEvaluator::VisitArrayInitLoopExpr(const ArrayInitLoopExpr *E) {
   LValue Subobject = This;
   Subobject.addArray(Info, E, CAT);
 
-  bool Success = true;
   for (EvalInfo::ArrayInitLoopIndex Index(Info); Index != Elements; ++Index) {
     if (!EvaluateInPlace(Result.getArrayInitializedElt(Index),
                          Info, Subobject, E->getSubExpr()) ||
         !HandleLValueArrayAdjustment(Info, E, Subobject,
                                      CAT->getElementType(), 1)) {
-      if (!Info.noteFailure())
-        return false;
-      Success = false;
+
+      // There's no need to try and evaluate the rest, as those are the exact
+      // same expressions.
+      std::ignore = Info.noteFailure();
+      return false;
     }
   }
 
-  return Success;
+  return true;
 }
 
 bool ArrayExprEvaluator::VisitCXXConstructExpr(const CXXConstructExpr *E) {
diff --git a/clang/test/AST/nested-array-init-loop-in-lambda-capture.cpp b/clang/test/AST/nested-array-init-loop-in-lambda-capture.cpp
new file mode 100644
index 000000000000000..82d27380b637d03
--- /dev/null
+++ b/clang/test/AST/nested-array-init-loop-in-lambda-capture.cpp
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -fexperimental-new-constant-interpreter -std=c++17 -verify %s
+// RUN: %clang_cc1 -std=c++17 -verify=ref %s
+
+// ref-no-diagnostics
+// expected-no-diagnostics
+
+void used_to_crash() {
+  int s[2][2];
+
+  int arr[4];
+
+  arr[0] = [s] { return s[0][0]; }();
+}

>From 8f8f2b9ce187a5479b7c6bced51bff97c38f36bc Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Thu, 28 Sep 2023 21:26:40 +0200
Subject: [PATCH 2/4] wrapping the temporary into a scope

---
 clang/lib/AST/ExprConstant.cpp | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index a1f5c262ca10be3..12b4d6b22c2c2bc 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -10950,6 +10950,9 @@ bool ArrayExprEvaluator::VisitCXXParenListOrInitListExpr(
 }
 
 bool ArrayExprEvaluator::VisitArrayInitLoopExpr(const ArrayInitLoopExpr *E) {
+
+  FullExpressionRAII Scope(Info);
+
   LValue CommonLV;
   if (E->getCommonExpr() &&
       !Evaluate(Info.CurrentCall->createTemporary(
@@ -10967,20 +10970,19 @@ bool ArrayExprEvaluator::VisitArrayInitLoopExpr(const ArrayInitLoopExpr *E) {
   LValue Subobject = This;
   Subobject.addArray(Info, E, CAT);
 
+  bool Success = true;
   for (EvalInfo::ArrayInitLoopIndex Index(Info); Index != Elements; ++Index) {
     if (!EvaluateInPlace(Result.getArrayInitializedElt(Index),
                          Info, Subobject, E->getSubExpr()) ||
         !HandleLValueArrayAdjustment(Info, E, Subobject,
                                      CAT->getElementType(), 1)) {
-
-      // There's no need to try and evaluate the rest, as those are the exact
-      // same expressions.
-      std::ignore = Info.noteFailure();
-      return false;
+      if (!Info.noteFailure())
+        return false;
+      Success = false;
     }
   }
 
-  return true;
+  return Success;
 }
 
 bool ArrayExprEvaluator::VisitCXXConstructExpr(const CXXConstructExpr *E) {

>From fb659cb7620d396882964ab628319643212a75b8 Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Fri, 29 Sep 2023 07:30:12 +0200
Subject: [PATCH 3/4] addressed comments

---
 clang/lib/AST/ExprConstant.cpp  | 16 +++++++++++---
 clang/test/AST/Interp/cxx20.cpp | 37 +++++++++++++++++++++++++++++++--
 2 files changed, 48 insertions(+), 5 deletions(-)

diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 12b4d6b22c2c2bc..1c0fa3ad2b6fb3e 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -10950,9 +10950,6 @@ bool ArrayExprEvaluator::VisitCXXParenListOrInitListExpr(
 }
 
 bool ArrayExprEvaluator::VisitArrayInitLoopExpr(const ArrayInitLoopExpr *E) {
-
-  FullExpressionRAII Scope(Info);
-
   LValue CommonLV;
   if (E->getCommonExpr() &&
       !Evaluate(Info.CurrentCall->createTemporary(
@@ -10972,6 +10969,16 @@ bool ArrayExprEvaluator::VisitArrayInitLoopExpr(const ArrayInitLoopExpr *E) {
 
   bool Success = true;
   for (EvalInfo::ArrayInitLoopIndex Index(Info); Index != Elements; ++Index) {
+    // C++ [class.temporary]/5
+    // There are four contexts in which temporaries are destroyed at a different
+    // point than the end of the full-expression. [...] The second context is
+    // when a copy constructor is called to copy an element of an array while
+    // the entire array is copied [...]. In either case, if the constructor has
+    // one or more default arguments, the destruction of every temporary created
+    // in a default argument is sequenced before the construction of the next
+    // array element, if any.
+    FullExpressionRAII Scope(Info);
+
     if (!EvaluateInPlace(Result.getArrayInitializedElt(Index),
                          Info, Subobject, E->getSubExpr()) ||
         !HandleLValueArrayAdjustment(Info, E, Subobject,
@@ -10980,6 +10987,9 @@ bool ArrayExprEvaluator::VisitArrayInitLoopExpr(const ArrayInitLoopExpr *E) {
         return false;
       Success = false;
     }
+
+    // Make sure we run the destructors too.
+    Scope.destroy();
   }
 
   return Success;
diff --git a/clang/test/AST/Interp/cxx20.cpp b/clang/test/AST/Interp/cxx20.cpp
index 0b13f41270a95b8..197090b0a37d9df 100644
--- a/clang/test/AST/Interp/cxx20.cpp
+++ b/clang/test/AST/Interp/cxx20.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -fexperimental-new-constant-interpreter -std=c++20 -verify %s
-// RUN: %clang_cc1 -std=c++20 -verify=ref %s
+// RUN: %clang_cc1 -fcxx-exceptions -fexperimental-new-constant-interpreter -std=c++20 -verify %s
+// RUN: %clang_cc1 -fcxx-exceptions -std=c++20 -verify=ref %s
 
 void test_alignas_operand() {
   alignas(8) char dummy;
@@ -700,3 +700,36 @@ namespace ThreeWayCmp {
   static_assert(pa1 <=> pa2 == -1, "");
   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 ~Highlander() { --*p; }
+  };
+
+  struct X {
+      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)'}}
+      }
+  };
+
+  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())'}}
+      return n;
+  }
+
+  static_assert(f() == 0); // expected-error {{not an integral constant expression}} \
+                           // expected-note {{in call to 'f()'}}
+
+  int main() {
+      return f();
+  }
+}

>From bedfe40d2bab344d8f2720a2ac47dfc991b2f403 Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Fri, 29 Sep 2023 13:25:25 +0200
Subject: [PATCH 4/4] addressed comments

---
 clang/docs/ReleaseNotes.rst      | 5 +++++
 clang/test/AST/Interp/arrays.cpp | 7 +------
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 09302040a3510b6..47d409eadf5b30e 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -269,6 +269,8 @@ Bug Fixes in This Version
 - Fixes crash when trying to obtain the common sugared type of
   `decltype(instantiation-dependent-expr)`.
   Fixes (`#67603 <https://github.com/llvm/llvm-project/issues/67603>`_)
+- Fixes a crash caused by a multidimensional array being captured by a lambda
+  (`#67722 <https://github.com/llvm/llvm-project/issues/67722>`_).
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -370,6 +372,9 @@ Bug Fixes to C++ Support
   argument. Fixes:
   (`#67395 <https://github.com/llvm/llvm-project/issues/67395>`_)
 
+- Fix a bug when destructors in a ``constexpr`` structured binding were
+  called at the wrong place.
+
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
 - Fixed an import failure of recursive friend class template.
diff --git a/clang/test/AST/Interp/arrays.cpp b/clang/test/AST/Interp/arrays.cpp
index 5640f57f6aeb826..303ad6839208e43 100644
--- a/clang/test/AST/Interp/arrays.cpp
+++ b/clang/test/AST/Interp/arrays.cpp
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 -fexperimental-new-constant-interpreter -verify %s
-// RUN: %clang_cc1 -verify=ref -DCUR_INTERP %s
+// RUN: %clang_cc1 -verify=ref %s
 
 constexpr int m = 3;
 constexpr const int *foo[][5] = {
@@ -352,10 +352,6 @@ namespace ZeroInit {
 }
 
 namespace ArrayInitLoop {
-  /// FIXME: The ArrayInitLoop for the decomposition initializer in g() has
-  /// f(n) as its CommonExpr. We need to evaluate that exactly once and not
-  /// N times as we do right now.
-#ifndef CUR_INTERP
   struct X {
       int arr[3];
   };
@@ -369,5 +365,4 @@ namespace ArrayInitLoop {
   }
   static_assert(g() == 6); // expected-error {{failed}} \
                            // expected-note {{15 == 6}}
-#endif
 }



More information about the cfe-commits mailing list