[clang] 49c399c - [clang][Interp] Toplevel destructors may fail
Timm Bäder via cfe-commits
cfe-commits at lists.llvm.org
Wed Feb 28 00:48:35 PST 2024
Author: Timm Bäder
Date: 2024-02-28T09:48:27+01:00
New Revision: 49c399c2d113df1654b09c9b5afa38924829a8fe
URL: https://github.com/llvm/llvm-project/commit/49c399c2d113df1654b09c9b5afa38924829a8fe
DIFF: https://github.com/llvm/llvm-project/commit/49c399c2d113df1654b09c9b5afa38924829a8fe.diff
LOG: [clang][Interp] Toplevel destructors may fail
We used to run them, but not check if they failed. If they do,
the expression is invalid, even if we already have a result.
I do have a suspicion that we need to manually call destroyLocals()
in more places (everywhere basically?), but I'll wait with that
until I have a reproducer at hand.
Added:
Modified:
clang/lib/AST/Interp/ByteCodeExprGen.cpp
clang/lib/AST/Interp/ByteCodeExprGen.h
clang/lib/AST/Interp/EvalEmitter.cpp
clang/lib/AST/Interp/EvaluationResult.h
clang/test/AST/Interp/cxx20.cpp
Removed:
################################################################################
diff --git a/clang/lib/AST/Interp/ByteCodeExprGen.cpp b/clang/lib/AST/Interp/ByteCodeExprGen.cpp
index f193f959d3a6c0..b151f8d0d7a79c 100644
--- a/clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ b/clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -2587,7 +2587,10 @@ bool ByteCodeExprGen<Emitter>::visitExpr(const Expr *E) {
if (!this->emitFinishInit(E))
return false;
- return this->emitRetValue(E);
+ // We are destroying the locals AFTER the Ret op.
+ // The Ret op needs to copy the (alive) values, but the
+ // destructors may still turn the entire expression invalid.
+ return this->emitRetValue(E) && RootScope.destroyLocals();
}
return false;
@@ -3414,14 +3417,15 @@ bool ByteCodeExprGen<Emitter>::emitRecordDestruction(const Record *R) {
// Now emit the destructor and recurse into base classes.
if (const CXXDestructorDecl *Dtor = R->getDestructor();
Dtor && !Dtor->isTrivial()) {
- if (const Function *DtorFunc = getFunction(Dtor)) {
- assert(DtorFunc->hasThisPointer());
- assert(DtorFunc->getNumParams() == 1);
- if (!this->emitDupPtr(SourceInfo{}))
- return false;
- if (!this->emitCall(DtorFunc, 0, SourceInfo{}))
- return false;
- }
+ const Function *DtorFunc = getFunction(Dtor);
+ if (!DtorFunc)
+ return false;
+ assert(DtorFunc->hasThisPointer());
+ assert(DtorFunc->getNumParams() == 1);
+ if (!this->emitDupPtr(SourceInfo{}))
+ return false;
+ if (!this->emitCall(DtorFunc, 0, SourceInfo{}))
+ return false;
}
for (const Record::Base &Base : llvm::reverse(R->bases())) {
diff --git a/clang/lib/AST/Interp/ByteCodeExprGen.h b/clang/lib/AST/Interp/ByteCodeExprGen.h
index 5b3b533dba3877..acbbcc3dc9619a 100644
--- a/clang/lib/AST/Interp/ByteCodeExprGen.h
+++ b/clang/lib/AST/Interp/ByteCodeExprGen.h
@@ -332,7 +332,7 @@ template <class Emitter> class VariableScope {
}
virtual void emitDestruction() {}
- virtual void emitDestructors() {}
+ virtual bool emitDestructors() { return true; }
VariableScope *getParent() const { return Parent; }
protected:
@@ -356,13 +356,18 @@ template <class Emitter> class LocalScope : public VariableScope<Emitter> {
}
/// Overriden to support explicit destruction.
- void emitDestruction() override {
+ void emitDestruction() override { destroyLocals(); }
+
+ /// Explicit destruction of local variables.
+ bool destroyLocals() {
if (!Idx)
- return;
- this->emitDestructors();
+ return true;
+
+ bool Success = this->emitDestructors();
this->Ctx->emitDestroy(*Idx, SourceInfo{});
removeStoredOpaqueValues();
this->Idx = std::nullopt;
+ return Success;
}
void addLocal(const Scope::Local &Local) override {
@@ -374,19 +379,25 @@ template <class Emitter> class LocalScope : public VariableScope<Emitter> {
this->Ctx->Descriptors[*Idx].emplace_back(Local);
}
- void emitDestructors() override {
+ bool emitDestructors() override {
if (!Idx)
- return;
+ return true;
// 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()) {
- this->Ctx->emitGetPtrLocal(Local.Offset, SourceInfo{});
- this->Ctx->emitDestruction(Local.Desc);
- this->Ctx->emitPopPtr(SourceInfo{});
+ if (!this->Ctx->emitGetPtrLocal(Local.Offset, SourceInfo{}))
+ return false;
+
+ if (!this->Ctx->emitDestruction(Local.Desc))
+ return false;
+
+ if (!this->Ctx->emitPopPtr(SourceInfo{}))
+ return false;
removeIfStoredOpaqueValue(Local);
}
}
+ return true;
}
void removeStoredOpaqueValues() {
diff --git a/clang/lib/AST/Interp/EvalEmitter.cpp b/clang/lib/AST/Interp/EvalEmitter.cpp
index 9cae25f5c4d642..c9c2bf9b145b22 100644
--- a/clang/lib/AST/Interp/EvalEmitter.cpp
+++ b/clang/lib/AST/Interp/EvalEmitter.cpp
@@ -38,8 +38,11 @@ EvaluationResult EvalEmitter::interpretExpr(const Expr *E,
this->ConvertResultToRValue = ConvertResultToRValue;
EvalResult.setSource(E);
- if (!this->visitExpr(E) && EvalResult.empty())
+ if (!this->visitExpr(E)) {
+ // EvalResult may already have a result set, but something failed
+ // after that (e.g. evaluating destructors).
EvalResult.setInvalid();
+ }
return std::move(this->EvalResult);
}
diff --git a/clang/lib/AST/Interp/EvaluationResult.h b/clang/lib/AST/Interp/EvaluationResult.h
index 28e1ae6ba3e7ab..ecf2250074cc9e 100644
--- a/clang/lib/AST/Interp/EvaluationResult.h
+++ b/clang/lib/AST/Interp/EvaluationResult.h
@@ -72,7 +72,8 @@ class EvaluationResult final {
Kind = LValue;
}
void setInvalid() {
- assert(empty());
+ // We are NOT asserting empty() here, since setting it to invalid
+ // is allowed even if there is already a result.
Kind = Invalid;
}
void setValid() {
diff --git a/clang/test/AST/Interp/cxx20.cpp b/clang/test/AST/Interp/cxx20.cpp
index 5c9c6257965108..b24b0c8a3ba0ec 100644
--- a/clang/test/AST/Interp/cxx20.cpp
+++ b/clang/test/AST/Interp/cxx20.cpp
@@ -1,5 +1,5 @@
-// 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
+// RUN: %clang_cc1 -fcxx-exceptions -fexperimental-new-constant-interpreter -std=c++20 -verify=both,expected -fcxx-exceptions %s
+// RUN: %clang_cc1 -fcxx-exceptions -std=c++20 -verify=both,ref -fcxx-exceptions %s
void test_alignas_operand() {
alignas(8) char dummy;
@@ -799,3 +799,21 @@ void f2() {
// access info for unnamed bit-field
}
}
+
+namespace FailingDestructor {
+ struct D {
+ int n;
+ bool can_destroy;
+
+ constexpr ~D() {
+ if (!can_destroy)
+ throw "oh no";
+ }
+ };
+ template<D d>
+ void f() {} // both-note {{invalid explicitly-specified argument}}
+
+ void g() {
+ f<D{0, false}>(); // both-error {{no matching function}}
+ }
+}
More information about the cfe-commits
mailing list