[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.

Author: tbaederr

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);
 

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;

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();
 }
 

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