llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Timm Baeder (tbaederr) <details> <summary>Changes</summary> 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<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 `````````` </details> https://github.com/llvm/llvm-project/pull/152672 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
