[clang] fde9ee1 - [clang][bytecode] Don't deallocate dynamic blocks with pointers (#152672)
via cfe-commits
cfe-commits at lists.llvm.org
Fri Aug 8 04:02:05 PDT 2025
Author: Timm Baeder
Date: 2025-08-08T13:02:01+02:00
New Revision: fde9ee1ac291652718124257796efff91a2d5baf
URL: https://github.com/llvm/llvm-project/commit/fde9ee1ac291652718124257796efff91a2d5baf
DIFF: https://github.com/llvm/llvm-project/commit/fde9ee1ac291652718124257796efff91a2d5baf.diff
LOG: [clang][bytecode] Don't deallocate dynamic blocks with pointers (#152672)
This fixes the edge case we had with variables pointing to dynamic
blocks, which forced us to convert basically *all* dynamic blocks to
DeadBlock when deallocating them.
We now don't run dynamic blocks through InterpState::deallocate() but
instead add them to a DeadAllocations list when they are deallocated but
still have pointers.
As a consequence, not all blocks with Block::IsDead = true are
DeadBlocks.
Added:
Modified:
clang/lib/AST/ByteCode/DynamicAllocator.cpp
clang/lib/AST/ByteCode/DynamicAllocator.h
clang/lib/AST/ByteCode/InterpBlock.cpp
clang/lib/AST/ByteCode/InterpState.cpp
Removed:
################################################################################
diff --git a/clang/lib/AST/ByteCode/DynamicAllocator.cpp b/clang/lib/AST/ByteCode/DynamicAllocator.cpp
index bbef94101b36f..f38d585336d96 100644
--- a/clang/lib/AST/ByteCode/DynamicAllocator.cpp
+++ b/clang/lib/AST/ByteCode/DynamicAllocator.cpp
@@ -13,25 +13,6 @@
using namespace clang;
using namespace clang::interp;
-// FIXME: There is a peculiar problem with the way we track pointers
-// to blocks and the way we allocate dynamic memory.
-//
-// When we have code like this:
-// while (true) {
-// char *buffer = new char[1024];
-// delete[] buffer;
-// }
-//
-// We have a local variable 'buffer' pointing to the heap allocated memory.
-// When deallocating the memory via delete[], that local variable still
-// points to the memory, which means we will create a DeadBlock for it and move
-// it over to that block, essentially duplicating the allocation. Moving
-// the data is also slow.
-//
-// However, when we actually try to access the allocation after it has been
-// freed, we need the block to still exist (alive or dead) so we can tell
-// that it's a dynamic allocation.
-
DynamicAllocator::~DynamicAllocator() { cleanup(); }
void DynamicAllocator::cleanup() {
@@ -42,8 +23,11 @@ void DynamicAllocator::cleanup() {
for (auto &Iter : AllocationSites) {
auto &AllocSite = Iter.second;
for (auto &Alloc : AllocSite.Allocations) {
- Block *B = reinterpret_cast<Block *>(Alloc.Memory.get());
+ Block *B = Alloc.block();
+ assert(!B->IsDead);
+ assert(B->isInitialized());
B->invokeDtor();
+
if (B->hasPointers()) {
while (B->Pointers) {
Pointer *Next = B->Pointers->asBlockPointer().Next;
@@ -89,6 +73,12 @@ Block *DynamicAllocator::allocate(const Descriptor *D, unsigned EvalID,
assert(D);
assert(D->asExpr());
+ // Garbage collection. Remove all dead allocations that don't have pointers to
+ // them anymore.
+ llvm::erase_if(DeadAllocations, [](Allocation &Alloc) -> bool {
+ return !Alloc.block()->hasPointers();
+ });
+
auto Memory =
std::make_unique<std::byte[]>(sizeof(Block) + D->getAllocSize());
auto *B = new (Memory.get()) Block(EvalID, D, /*isStatic=*/false);
@@ -132,18 +122,34 @@ bool DynamicAllocator::deallocate(const Expr *Source,
// Find the Block to delete.
auto AllocIt = llvm::find_if(Site.Allocations, [&](const Allocation &A) {
- const Block *B = reinterpret_cast<const Block *>(A.Memory.get());
- return BlockToDelete == B;
+ return BlockToDelete == A.block();
});
assert(AllocIt != Site.Allocations.end());
- Block *B = reinterpret_cast<Block *>(AllocIt->Memory.get());
+ Block *B = AllocIt->block();
+ assert(B->isInitialized());
+ assert(!B->IsDead);
B->invokeDtor();
- S.deallocate(B);
- Site.Allocations.erase(AllocIt);
+ // Almost all our dynamic allocations have a pointer pointing to them
+ // when we deallocate them, since otherwise we can't call delete() at all.
+ // This means that we would usually need to create DeadBlocks for all of them.
+ // To work around that, we instead mark them as dead without moving the data
+ // over to a DeadBlock and simply keep the block in a separate DeadAllocations
+ // list.
+ if (B->hasPointers()) {
+ B->IsDead = true;
+ DeadAllocations.push_back(std::move(*AllocIt));
+ Site.Allocations.erase(AllocIt);
+
+ if (Site.size() == 0)
+ AllocationSites.erase(It);
+ return true;
+ }
+ // Get rid of the allocation altogether.
+ Site.Allocations.erase(AllocIt);
if (Site.empty())
AllocationSites.erase(It);
diff --git a/clang/lib/AST/ByteCode/DynamicAllocator.h b/clang/lib/AST/ByteCode/DynamicAllocator.h
index cba5e347e950b..31d0e58667c11 100644
--- a/clang/lib/AST/ByteCode/DynamicAllocator.h
+++ b/clang/lib/AST/ByteCode/DynamicAllocator.h
@@ -43,6 +43,7 @@ class DynamicAllocator final {
std::unique_ptr<std::byte[]> Memory;
Allocation(std::unique_ptr<std::byte[]> Memory)
: Memory(std::move(Memory)) {}
+ Block *block() const { return reinterpret_cast<Block *>(Memory.get()); }
};
struct AllocationSite {
@@ -99,6 +100,9 @@ class DynamicAllocator final {
private:
llvm::DenseMap<const Expr *, AllocationSite> AllocationSites;
+ // Allocations that have already been deallocated but had pointers
+ // to them.
+ llvm::SmallVector<Allocation> DeadAllocations;
using PoolAllocTy = llvm::BumpPtrAllocator;
PoolAllocTy DescAllocator;
diff --git a/clang/lib/AST/ByteCode/InterpBlock.cpp b/clang/lib/AST/ByteCode/InterpBlock.cpp
index 963b54ec50cff..b0e048bc867e9 100644
--- a/clang/lib/AST/ByteCode/InterpBlock.cpp
+++ b/clang/lib/AST/ByteCode/InterpBlock.cpp
@@ -64,7 +64,7 @@ void Block::removePointer(Pointer *P) {
}
void Block::cleanup() {
- if (Pointers == nullptr && IsDead)
+ if (Pointers == nullptr && !IsDynamic && IsDead)
(reinterpret_cast<DeadBlock *>(this + 1) - 1)->free();
}
diff --git a/clang/lib/AST/ByteCode/InterpState.cpp b/clang/lib/AST/ByteCode/InterpState.cpp
index 49c9b543f25e1..f7f03e593301f 100644
--- a/clang/lib/AST/ByteCode/InterpState.cpp
+++ b/clang/lib/AST/ByteCode/InterpState.cpp
@@ -76,6 +76,7 @@ bool InterpState::reportOverflow(const Expr *E, const llvm::APSInt &Value) {
void InterpState::deallocate(Block *B) {
assert(B);
+ assert(!B->isDynamic());
// The block might have a pointer saved in a field in its data
// that points to the block itself. We call the dtor first,
// which will destroy all the data but leave InlineDescriptors
More information about the cfe-commits
mailing list