[clang] [clang][bytecode] Don't deallocate dynamic blocks with pointers (PR #152672)
https://github.com/tbaederr closed https://github.com/llvm/llvm-project/pull/152672 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][bytecode] Don't deallocate dynamic blocks with pointers (PR #152672)
llvmbot wrote:
@llvm/pr-subscribers-clang
Author: Timm Baeder (tbaederr)
Changes
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.
---
Full diff: https://github.com/llvm/llvm-project/pull/152672.diff
4 Files Affected:
- (modified) clang/lib/AST/ByteCode/DynamicAllocator.cpp (+31-25)
- (modified) clang/lib/AST/ByteCode/DynamicAllocator.h (+4)
- (modified) clang/lib/AST/ByteCode/InterpBlock.cpp (+1-1)
- (modified) clang/lib/AST/ByteCode/InterpState.cpp (+1)
``diff
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(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(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(A.Memory.get());
-return BlockToDelete == B;
+return BlockToDelete == A.block();
});
assert(AllocIt != Site.Allocations.end());
- Block *B = reinterpret_cast(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 Memory;
Allocation(std::unique_ptr Memory)
: Memory(std::move(Memory)) {}
+Block *block() const { return reinterpret_cast(Memory.get()); }
};
struct AllocationSite {
@@ -99,6 +100,9 @@ class DynamicAllocator final {
private:
llvm::DenseMap AllocationSites;
+ //
[clang] [clang][bytecode] Don't deallocate dynamic blocks with pointers (PR #152672)
https://github.com/tbaederr created
https://github.com/llvm/llvm-project/pull/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.
>From 12ecaadb1b3041ee42b2cbe2cce12a4bba5c24c4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Timm=20B=C3=A4der?=
Date: Fri, 8 Aug 2025 09:33:23 +0200
Subject: [PATCH] [clang][bytecode] Don't deallocate dynamic blocks with
pointers
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.
---
clang/lib/AST/ByteCode/DynamicAllocator.cpp | 56 -
clang/lib/AST/ByteCode/DynamicAllocator.h | 4 ++
clang/lib/AST/ByteCode/InterpBlock.cpp | 2 +-
clang/lib/AST/ByteCode/InterpState.cpp | 1 +
4 files changed, 37 insertions(+), 26 deletions(-)
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(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(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(A.Memory.get());
-return BlockToDelete == B;
+return BlockToDelete == A.block();
});
assert(AllocIt != Site.Allocations.end());
- Block *B = reinterpret_cast(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/Byte
