[clang][bytecode] Don't deallocate dynamic blocks with pointers

[tbaederr]

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.

[llvmbot]

@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 --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