[clang] [clang] Fix a crash from nested ArrayInitLoopExpr (PR #67722)
via cfe-commits
cfe-commits at lists.llvm.org
Thu Sep 28 20:28:24 PDT 2023
https://github.com/isuckatcs updated https://github.com/llvm/llvm-project/pull/67722
>From fb0bf2fcc9d873ce9cf4269cfb6de8786ac6f343 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/3] [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 19c09fb7656ba3b2e170802adfd397dd9aa9fec9 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/3] 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 31ad08e46398c35043632870d92be0c285c10015 Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Fri, 29 Sep 2023 05:22:12 +0200
Subject: [PATCH 3/3] addressed comment
---
clang/lib/AST/ExprConstant.cpp | 16 +++++++++--
clang/lib/AST/Interp/ByteCodeExprGen.cpp | 8 ++++++
clang/lib/AST/Interp/ByteCodeExprGen.h | 4 ++-
clang/lib/AST/Interp/Descriptor.h | 5 ++++
clang/test/AST/Interp/cxx20.cpp | 35 ++++++++++++++++++++++--
5 files changed, 62 insertions(+), 6 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/lib/AST/Interp/ByteCodeExprGen.cpp b/clang/lib/AST/Interp/ByteCodeExprGen.cpp
index e813d4fa651ceaf..cc1ba77835fa807 100644
--- a/clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ b/clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -795,6 +795,14 @@ bool ByteCodeExprGen<Emitter>::VisitArrayInitLoopExpr(
return false;
if (!visitInitializer(SubExpr))
return false;
+
+ // FIXME: This is a workaround so that the destructors are evaluated when
+ // needed. For more details please see
+ // ArrayExprEvaluator::VisitArrayInitLoopExpr. Eventually this should be
+ // removed and temporary destruction should be handled properly.
+ for (VariableScope<Emitter> *C = VarScope; C; C = C->getParent())
+ C->emitDestructors();
+
if (!this->emitPopPtr(E))
return false;
}
diff --git a/clang/lib/AST/Interp/ByteCodeExprGen.h b/clang/lib/AST/Interp/ByteCodeExprGen.h
index 47a3f75f13459d0..e5a6500c6176230 100644
--- a/clang/lib/AST/Interp/ByteCodeExprGen.h
+++ b/clang/lib/AST/Interp/ByteCodeExprGen.h
@@ -382,9 +382,11 @@ template <class Emitter> class LocalScope : public VariableScope<Emitter> {
// Emit destructor calls for local variables of record
// type with a destructor.
for (Scope::Local &Local : this->Ctx->Descriptors[*Idx]) {
- if (!Local.Desc->isPrimitive() && !Local.Desc->isPrimitiveArray()) {
+ if (!Local.Desc->isPrimitive() && !Local.Desc->isPrimitiveArray() &&
+ !Local.Desc->IsEmitted) {
this->Ctx->emitGetPtrLocal(Local.Offset, SourceInfo{});
this->Ctx->emitRecordDestruction(Local.Desc);
+ Local.Desc->IsEmitted = true;
}
}
}
diff --git a/clang/lib/AST/Interp/Descriptor.h b/clang/lib/AST/Interp/Descriptor.h
index 55a754c3505cce7..c17e7a00f551932 100644
--- a/clang/lib/AST/Interp/Descriptor.h
+++ b/clang/lib/AST/Interp/Descriptor.h
@@ -110,6 +110,11 @@ struct Descriptor final {
/// Flag indicating if the block is an array.
const bool IsArray = false;
+ // FIXME: This is a part of the workaround in
+ // ByteCodeExprGen::VisitArrayInitLoopExpr.
+ /// Flag indicating if the descriptor has already been emitted.
+ bool IsEmitted = false;
+
/// Storage management methods.
const BlockCtorFn CtorFn = nullptr;
const BlockDtorFn DtorFn = nullptr;
diff --git a/clang/test/AST/Interp/cxx20.cpp b/clang/test/AST/Interp/cxx20.cpp
index df08bb75199d86a..e0cfc72875b6aa3 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;
@@ -646,3 +646,34 @@ namespace ImplicitFunction {
// expected-error {{not an integral constant expression}} \
// expected-note {{in call to 'callMe()'}}
}
+
+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"; }
+ 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);
+ }
+ };
+
+ constexpr int f() {
+ int n = 0;
+ X x[3] = {&n, &n, &n};
+ auto [a, b, c] = x;
+ return n;
+ }
+
+ static_assert(f() == 0);
+
+ int main() {
+ return f();
+ }
+}
More information about the cfe-commits
mailing list