[clang] 368a398 - Revert "[clang][Interp] Call destructors of local variables"
Timm Bäder via cfe-commits
cfe-commits at lists.llvm.org
Mon Oct 23 21:48:49 PDT 2023
Author: Timm Bäder
Date: 2023-10-24T06:48:29+02:00
New Revision: 368a39882d286c31975792f27e52e891e5a58ab9
URL: https://github.com/llvm/llvm-project/commit/368a39882d286c31975792f27e52e891e5a58ab9
DIFF: https://github.com/llvm/llvm-project/commit/368a39882d286c31975792f27e52e891e5a58ab9.diff
LOG: Revert "[clang][Interp] Call destructors of local variables"
This reverts commit 3671a69470f3fa708926a9e5ce579751c6b51dac.
This breaks builders, e.g.
https://lab.llvm.org/buildbot/#/builders/245/builds/15697
Added:
Modified:
clang/lib/AST/Interp/Descriptor.cpp
clang/lib/AST/Interp/Descriptor.h
clang/lib/AST/Interp/EvalEmitter.cpp
clang/lib/AST/Interp/EvalEmitter.h
clang/lib/AST/Interp/InterpBlock.h
clang/lib/AST/Interp/InterpFrame.cpp
clang/lib/AST/Interp/InterpState.cpp
clang/lib/AST/Interp/Pointer.cpp
clang/lib/AST/Interp/Pointer.h
clang/test/AST/Interp/arrays.cpp
Removed:
################################################################################
diff --git a/clang/lib/AST/Interp/Descriptor.cpp b/clang/lib/AST/Interp/Descriptor.cpp
index abd75a8d67bbd3f..3990282686fe3d6 100644
--- a/clang/lib/AST/Interp/Descriptor.cpp
+++ b/clang/lib/AST/Interp/Descriptor.cpp
@@ -40,9 +40,6 @@ static void moveTy(Block *, const std::byte *Src, std::byte *Dst,
template <typename T>
static void ctorArrayTy(Block *, std::byte *Ptr, bool, bool, bool,
const Descriptor *D) {
- new (Ptr) InitMapPtr(std::nullopt);
-
- Ptr += sizeof(InitMapPtr);
for (unsigned I = 0, NE = D->getNumElems(); I < NE; ++I) {
new (&reinterpret_cast<T *>(Ptr)[I]) T();
}
@@ -50,11 +47,11 @@ static void ctorArrayTy(Block *, std::byte *Ptr, bool, bool, bool,
template <typename T>
static void dtorArrayTy(Block *, std::byte *Ptr, const Descriptor *D) {
- InitMapPtr &IMP = *reinterpret_cast<InitMapPtr *>(Ptr);
+ InitMap *IM = *reinterpret_cast<InitMap **>(Ptr);
+ if (IM != (InitMap *)-1)
+ free(IM);
- if (IMP)
- IMP = std::nullopt;
- Ptr += sizeof(InitMapPtr);
+ Ptr += sizeof(InitMap *);
for (unsigned I = 0, NE = D->getNumElems(); I < NE; ++I) {
reinterpret_cast<T *>(Ptr)[I].~T();
}
@@ -212,8 +209,7 @@ static BlockMoveFn getMovePrim(PrimType Type) {
}
static BlockCtorFn getCtorArrayPrim(PrimType Type) {
- TYPE_SWITCH(Type, return ctorArrayTy<T>);
- llvm_unreachable("unknown Expr");
+ COMPOSITE_TYPE_SWITCH(Type, return ctorArrayTy<T>, return nullptr);
}
static BlockDtorFn getDtorArrayPrim(PrimType Type) {
@@ -222,8 +218,7 @@ static BlockDtorFn getDtorArrayPrim(PrimType Type) {
}
static BlockMoveFn getMoveArrayPrim(PrimType Type) {
- TYPE_SWITCH(Type, return moveArrayTy<T>);
- llvm_unreachable("unknown Expr");
+ COMPOSITE_TYPE_SWITCH(Type, return moveArrayTy<T>, return nullptr);
}
/// Primitives.
@@ -243,7 +238,7 @@ Descriptor::Descriptor(const DeclTy &D, PrimType Type, MetadataSize MD,
bool IsMutable)
: Source(D), ElemSize(primSize(Type)), Size(ElemSize * NumElems),
MDSize(MD.value_or(0)),
- AllocSize(align(Size) + sizeof(InitMapPtr) + MDSize), IsConst(IsConst),
+ AllocSize(align(Size) + sizeof(InitMap *) + MDSize), IsConst(IsConst),
IsMutable(IsMutable), IsTemporary(IsTemporary), IsArray(true),
CtorFn(getCtorArrayPrim(Type)), DtorFn(getDtorArrayPrim(Type)),
MoveFn(getMoveArrayPrim(Type)) {
@@ -254,10 +249,9 @@ Descriptor::Descriptor(const DeclTy &D, PrimType Type, MetadataSize MD,
Descriptor::Descriptor(const DeclTy &D, PrimType Type, bool IsTemporary,
UnknownSize)
: Source(D), ElemSize(primSize(Type)), Size(UnknownSizeMark), MDSize(0),
- AllocSize(alignof(void *) + sizeof(InitMapPtr)), IsConst(true),
- IsMutable(false), IsTemporary(IsTemporary), IsArray(true),
- CtorFn(getCtorArrayPrim(Type)), DtorFn(getDtorArrayPrim(Type)),
- MoveFn(getMoveArrayPrim(Type)) {
+ AllocSize(alignof(void *)), IsConst(true), IsMutable(false),
+ IsTemporary(IsTemporary), IsArray(true), CtorFn(getCtorArrayPrim(Type)),
+ DtorFn(getDtorArrayPrim(Type)), MoveFn(getMoveArrayPrim(Type)) {
assert(Source && "Missing source");
}
@@ -278,10 +272,10 @@ Descriptor::Descriptor(const DeclTy &D, Descriptor *Elem, MetadataSize MD,
Descriptor::Descriptor(const DeclTy &D, Descriptor *Elem, bool IsTemporary,
UnknownSize)
: Source(D), ElemSize(Elem->getAllocSize() + sizeof(InlineDescriptor)),
- Size(UnknownSizeMark), MDSize(0),
- AllocSize(alignof(void *) + sizeof(InitMapPtr)), ElemDesc(Elem),
- IsConst(true), IsMutable(false), IsTemporary(IsTemporary), IsArray(true),
- CtorFn(ctorArrayDesc), DtorFn(dtorArrayDesc), MoveFn(moveArrayDesc) {
+ Size(UnknownSizeMark), MDSize(0), AllocSize(alignof(void *)),
+ ElemDesc(Elem), IsConst(true), IsMutable(false), IsTemporary(IsTemporary),
+ IsArray(true), CtorFn(ctorArrayDesc), DtorFn(dtorArrayDesc),
+ MoveFn(moveArrayDesc) {
assert(Source && "Missing source");
}
@@ -320,12 +314,21 @@ SourceLocation Descriptor::getLocation() const {
llvm_unreachable("Invalid descriptor type");
}
-InitMap::InitMap(unsigned N)
- : UninitFields(N), Data(std::make_unique<T[]>(numFields(N))) {
- std::fill_n(data(), numFields(N), 0);
+InitMap::InitMap(unsigned N) : UninitFields(N) {
+ std::fill_n(data(), (N + PER_FIELD - 1) / PER_FIELD, 0);
}
-bool InitMap::initializeElement(unsigned I) {
+InitMap::T *InitMap::data() {
+ auto *Start = reinterpret_cast<char *>(this) + align(sizeof(InitMap));
+ return reinterpret_cast<T *>(Start);
+}
+
+const InitMap::T *InitMap::data() const {
+ auto *Start = reinterpret_cast<const char *>(this) + align(sizeof(InitMap));
+ return reinterpret_cast<const T *>(Start);
+}
+
+bool InitMap::initialize(unsigned I) {
unsigned Bucket = I / PER_FIELD;
T Mask = T(1) << (I % PER_FIELD);
if (!(data()[Bucket] & Mask)) {
@@ -335,7 +338,13 @@ bool InitMap::initializeElement(unsigned I) {
return UninitFields == 0;
}
-bool InitMap::isElementInitialized(unsigned I) const {
+bool InitMap::isInitialized(unsigned I) const {
unsigned Bucket = I / PER_FIELD;
return data()[Bucket] & (T(1) << (I % PER_FIELD));
}
+
+InitMap *InitMap::allocate(unsigned N) {
+ const size_t NumFields = ((N + PER_FIELD - 1) / PER_FIELD);
+ const size_t Size = align(sizeof(InitMap)) + NumFields * PER_FIELD;
+ return new (malloc(Size)) InitMap(N);
+}
diff --git a/clang/lib/AST/Interp/Descriptor.h b/clang/lib/AST/Interp/Descriptor.h
index be9a380138a7b11..55a754c3505cce7 100644
--- a/clang/lib/AST/Interp/Descriptor.h
+++ b/clang/lib/AST/Interp/Descriptor.h
@@ -20,12 +20,10 @@ namespace clang {
namespace interp {
class Block;
class Record;
-struct InitMap;
struct Descriptor;
enum PrimType : unsigned;
using DeclTy = llvm::PointerUnion<const Decl *, const Expr *>;
-using InitMapPtr = std::optional<std::pair<bool, std::shared_ptr<InitMap>>>;
/// Invoked whenever a block is created. The constructor method fills in the
/// inline descriptors of all fields and array elements. It also initializes
@@ -195,6 +193,9 @@ struct Descriptor final {
};
/// Bitfield tracking the initialisation status of elements of primitive arrays.
+/// A pointer to this is embedded at the end of all primitive arrays.
+/// If the map was not yet created and nothing was initialized, the pointer to
+/// this structure is 0. If the object was fully initialized, the pointer is -1.
struct InitMap final {
private:
/// Type packing bits.
@@ -202,29 +203,26 @@ struct InitMap final {
/// Bits stored in a single field.
static constexpr uint64_t PER_FIELD = sizeof(T) * CHAR_BIT;
-public:
/// Initializes the map with no fields set.
- explicit InitMap(unsigned N);
-
-private:
- friend class Pointer;
+ InitMap(unsigned N);
/// Returns a pointer to storage.
- T *data() { return Data.get(); }
- const T *data() const { return Data.get(); }
+ T *data();
+ const T *data() const;
+public:
/// Initializes an element. Returns true when object if fully initialized.
- bool initializeElement(unsigned I);
+ bool initialize(unsigned I);
/// Checks if an element was initialized.
- bool isElementInitialized(unsigned I) const;
+ bool isInitialized(unsigned I) const;
- static constexpr size_t numFields(unsigned N) {
- return (N + PER_FIELD - 1) / PER_FIELD;
- }
- /// Number of fields not initialized.
+ /// Allocates a map holding N elements.
+ static InitMap *allocate(unsigned N);
+
+private:
+ /// Number of fields initialized.
unsigned UninitFields;
- std::unique_ptr<T[]> Data;
};
} // namespace interp
diff --git a/clang/lib/AST/Interp/EvalEmitter.cpp b/clang/lib/AST/Interp/EvalEmitter.cpp
index f8942291b3b162d..f46ef1067cf52a0 100644
--- a/clang/lib/AST/Interp/EvalEmitter.cpp
+++ b/clang/lib/AST/Interp/EvalEmitter.cpp
@@ -25,14 +25,6 @@ EvalEmitter::EvalEmitter(Context &Ctx, Program &P, State &Parent,
new InterpFrame(S, /*Func=*/nullptr, /*Caller=*/nullptr, CodePtr());
}
-EvalEmitter::~EvalEmitter() {
- for (auto &[K, V] : Locals) {
- Block *B = reinterpret_cast<Block *>(V.get());
- if (B->isInitialized())
- B->invokeDtor();
- }
-}
-
llvm::Expected<bool> EvalEmitter::interpretExpr(const Expr *E) {
if (this->visitExpr(E))
return true;
diff --git a/clang/lib/AST/Interp/EvalEmitter.h b/clang/lib/AST/Interp/EvalEmitter.h
index 5a9be18c34a03bc..1128e0fb09f0de8 100644
--- a/clang/lib/AST/Interp/EvalEmitter.h
+++ b/clang/lib/AST/Interp/EvalEmitter.h
@@ -40,7 +40,7 @@ class EvalEmitter : public SourceMapper {
EvalEmitter(Context &Ctx, Program &P, State &Parent, InterpStack &Stk,
APValue &Result);
- virtual ~EvalEmitter();
+ virtual ~EvalEmitter() {}
/// Define a label.
void emitLabel(LabelTy Label);
diff --git a/clang/lib/AST/Interp/InterpBlock.h b/clang/lib/AST/Interp/InterpBlock.h
index 89f937b588fb678..5112108e6f010a6 100644
--- a/clang/lib/AST/Interp/InterpBlock.h
+++ b/clang/lib/AST/Interp/InterpBlock.h
@@ -71,7 +71,6 @@ class Block final {
unsigned getSize() const { return Desc->getAllocSize(); }
/// Returns the declaration ID.
std::optional<unsigned> getDeclID() const { return DeclID; }
- bool isInitialized() const { return IsInitialized; }
/// Returns a pointer to the stored data.
/// You are allowed to read Desc->getSize() bytes from this address.
@@ -105,14 +104,12 @@ class Block final {
if (Desc->CtorFn)
Desc->CtorFn(this, data(), Desc->IsConst, Desc->IsMutable,
/*isActive=*/true, Desc);
- IsInitialized = true;
}
/// Invokes the Destructor.
void invokeDtor() {
if (Desc->DtorFn)
Desc->DtorFn(this, data(), Desc);
- IsInitialized = false;
}
protected:
@@ -142,12 +139,8 @@ class Block final {
bool IsStatic = false;
/// Flag indicating if the block is an extern.
bool IsExtern = false;
- /// Flag indicating if the pointer is dead. This is only ever
- /// set once, when converting the Block to a DeadBlock.
+ /// Flag indicating if the pointer is dead.
bool IsDead = false;
- /// Flag indicating if the block contents have been initialized
- /// via invokeCtor.
- bool IsInitialized = false;
/// Pointer to the stack slot descriptor.
Descriptor *Desc;
};
diff --git a/clang/lib/AST/Interp/InterpFrame.cpp b/clang/lib/AST/Interp/InterpFrame.cpp
index b06923114c7a24e..49db83a965ab001 100644
--- a/clang/lib/AST/Interp/InterpFrame.cpp
+++ b/clang/lib/AST/Interp/InterpFrame.cpp
@@ -72,19 +72,6 @@ InterpFrame::InterpFrame(InterpState &S, const Function *Func, CodePtr RetPC)
InterpFrame::~InterpFrame() {
for (auto &Param : Params)
S.deallocate(reinterpret_cast<Block *>(Param.second.get()));
-
- // When destroying the InterpFrame, call the Dtor for all block
- // that haven't been destroyed via a destroy() op yet.
- // This happens when the execution is interruped midway-through.
- if (Func) {
- for (auto &Scope : Func->scopes()) {
- for (auto &Local : Scope.locals()) {
- Block *B = localBlock(Local.Offset);
- if (B->isInitialized())
- B->invokeDtor();
- }
- }
- }
}
void InterpFrame::destroy(unsigned Idx) {
diff --git a/clang/lib/AST/Interp/InterpState.cpp b/clang/lib/AST/Interp/InterpState.cpp
index 2cb87ef07fe5882..4f050baea39e79a 100644
--- a/clang/lib/AST/Interp/InterpState.cpp
+++ b/clang/lib/AST/Interp/InterpState.cpp
@@ -65,9 +65,9 @@ void InterpState::deallocate(Block *B) {
std::memcpy(D->rawData(), B->rawData(), Desc->getMetadataSize());
}
- // We moved the contents over to the DeadBlock.
- B->IsInitialized = false;
} else {
- B->invokeDtor();
+ // Free storage, if necessary.
+ if (Desc->DtorFn)
+ Desc->DtorFn(B, B->data(), Desc);
}
}
diff --git a/clang/lib/AST/Interp/Pointer.cpp b/clang/lib/AST/Interp/Pointer.cpp
index d1af58203bec64d..e04d29b8bd81be4 100644
--- a/clang/lib/AST/Interp/Pointer.cpp
+++ b/clang/lib/AST/Interp/Pointer.cpp
@@ -164,16 +164,13 @@ bool Pointer::isInitialized() const {
if (Desc->isPrimitiveArray()) {
if (isStatic() && Base == 0)
return true;
-
- InitMapPtr &IM = getInitMap();
-
- if (!IM)
+ // Primitive array field are stored in a bitset.
+ InitMap *Map = getInitMap();
+ if (!Map)
return false;
-
- if (IM->first)
+ if (Map == (InitMap *)-1)
return true;
-
- return IM->second->isElementInitialized(getIndex());
+ return Map->isInitialized(getIndex());
}
// Field has its bit in an inline descriptor.
@@ -190,20 +187,15 @@ void Pointer::initialize() const {
if (isStatic() && Base == 0)
return;
- InitMapPtr &IM = getInitMap();
- if (!IM)
- IM =
- std::make_pair(false, std::make_shared<InitMap>(Desc->getNumElems()));
-
- assert(IM);
-
- // All initialized.
- if (IM->first)
+ // Primitive array initializer.
+ InitMap *&Map = getInitMap();
+ if (Map == (InitMap *)-1)
return;
-
- if (IM->second->initializeElement(getIndex())) {
- IM->first = true;
- IM->second.reset();
+ if (Map == nullptr)
+ Map = InitMap::allocate(Desc->getNumElems());
+ if (Map->initialize(getIndex())) {
+ free(Map);
+ Map = (InitMap *)-1;
}
return;
}
diff --git a/clang/lib/AST/Interp/Pointer.h b/clang/lib/AST/Interp/Pointer.h
index 3b21290332a9d2f..d5279e757f04764 100644
--- a/clang/lib/AST/Interp/Pointer.h
+++ b/clang/lib/AST/Interp/Pointer.h
@@ -108,7 +108,7 @@ class Pointer {
if (getFieldDesc()->ElemDesc)
Off += sizeof(InlineDescriptor);
else
- Off += sizeof(InitMapPtr);
+ Off += sizeof(InitMap *);
return Pointer(Pointee, Base, Base + Off);
}
@@ -147,7 +147,7 @@ class Pointer {
if (inPrimitiveArray()) {
if (Offset != Base)
return *this;
- return Pointer(Pointee, Base, Offset + sizeof(InitMapPtr));
+ return Pointer(Pointee, Base, Offset + sizeof(InitMap *));
}
// Pointer is to a field or array element - enter it.
@@ -168,7 +168,7 @@ class Pointer {
// Revert to an outer one-past-end pointer.
unsigned Adjust;
if (inPrimitiveArray())
- Adjust = sizeof(InitMapPtr);
+ Adjust = sizeof(InitMap *);
else
Adjust = sizeof(InlineDescriptor);
return Pointer(Pointee, Base, Base + getSize() + Adjust);
@@ -257,7 +257,7 @@ class Pointer {
if (getFieldDesc()->ElemDesc)
Adjust = sizeof(InlineDescriptor);
else
- Adjust = sizeof(InitMapPtr);
+ Adjust = sizeof(InitMap *);
}
return Offset - Base - Adjust;
}
@@ -359,7 +359,7 @@ class Pointer {
assert(isLive() && "Invalid pointer");
if (isArrayRoot())
return *reinterpret_cast<T *>(Pointee->rawData() + Base +
- sizeof(InitMapPtr));
+ sizeof(InitMap *));
return *reinterpret_cast<T *>(Pointee->rawData() + Offset);
}
@@ -367,7 +367,7 @@ class Pointer {
/// Dereferences a primitive element.
template <typename T> T &elem(unsigned I) const {
assert(I < getNumElems());
- return reinterpret_cast<T *>(Pointee->data() + sizeof(InitMapPtr))[I];
+ return reinterpret_cast<T *>(Pointee->data() + sizeof(InitMap *))[I];
}
/// Initializes a field.
@@ -418,7 +418,6 @@ class Pointer {
private:
friend class Block;
friend class DeadBlock;
- friend struct InitMap;
Pointer(Block *Pointee, unsigned Base, unsigned Offset);
@@ -432,9 +431,9 @@ class Pointer {
1;
}
- /// Returns a reference to the InitMapPtr which stores the initialization map.
- InitMapPtr &getInitMap() const {
- return *reinterpret_cast<InitMapPtr *>(Pointee->rawData() + Base);
+ /// Returns a reference to the pointer which stores the initialization map.
+ InitMap *&getInitMap() const {
+ return *reinterpret_cast<InitMap **>(Pointee->rawData() + Base);
}
/// The block the pointer is pointing to.
diff --git a/clang/test/AST/Interp/arrays.cpp b/clang/test/AST/Interp/arrays.cpp
index 7110785ea4c662a..ba29632e97c30a6 100644
--- a/clang/test/AST/Interp/arrays.cpp
+++ b/clang/test/AST/Interp/arrays.cpp
@@ -406,52 +406,3 @@ namespace StringZeroFill {
static_assert(b[4] == '\0', "");
static_assert(b[5] == '\0', "");
}
-
-namespace NoInitMapLeak {
-#pragma clang diagnostic push
-#pragma clang diagnostic ignored "-Wdivision-by-zero"
-#pragma clang diagnostic ignored "-Wc++20-extensions"
- constexpr int testLeak() { // expected-error {{never produces a constant expression}} \
- // ref-error {{never produces a constant expression}}
- int a[2];
- a[0] = 1;
- // interrupts interpretation.
- (void)(1 / 0); // expected-note 2{{division by zero}} \
- // ref-note 2{{division by zero}}
-
-
- return 1;
- }
-#pragma clang diagnostic pop
- static_assert(testLeak() == 1, ""); // expected-error {{not an integral constant expression}} \
- // expected-note {{in call to 'testLeak()'}} \
- // ref-error {{not an integral constant expression}} \
- // ref-note {{in call to 'testLeak()'}}
-
-
- constexpr int a[] = {1,2,3,4/0,5}; // expected-error {{must be initialized by a constant expression}} \
- // expected-note {{division by zero}} \
- // ref-error {{must be initialized by a constant expression}} \
- // ref-note {{division by zero}} \
- // ref-note {{declared here}}
-
- /// FIXME: This should fail in the new interpreter as well.
- constexpr int b = a[0]; // ref-error {{must be initialized by a constant expression}} \
- // ref-note {{is not a constant expression}} \
- // ref-note {{declared here}}
- static_assert(b == 1, ""); // ref-error {{not an integral constant expression}} \
- // ref-note {{not a constant expression}}
-
- constexpr int f() { // expected-error {{never produces a constant expression}} \
- // ref-error {{never produces a constant expression}}
- int a[] = {19,2,3/0,4}; // expected-note 2{{division by zero}} \
- // expected-warning {{is undefined}} \
- // ref-note 2{{division by zero}} \
- // ref-warning {{is undefined}}
- return 1;
- }
- static_assert(f() == 1, ""); // expected-error {{not an integral constant expression}} \
- // expected-note {{in call to}} \
- // ref-error {{not an integral constant expression}} \
- // ref-note {{in call to}}
-}
More information about the cfe-commits
mailing list