commit 43226e1091ebbdf3e028df04fb8acaab7185b39f
author Arthur Sonzogni <arthursonzogni@chromium.org> Fri Jan 12 18:57:38 2024 +0000
committer Dawn LUCI CQ <dawn-scoped@luci-project-accounts.iam.gserviceaccount.com> Fri Jan 12 18:57:38 2024 +0000
tree ea5a3c7a4d41150ad9fc5e86cf085122eedff84f
parent aa7748413b909e531206c3bacc9bc8b495809840

SlabAllocator: Remove dangling pointers.

The `LeakedDanglingUntriaged` was caused by "placement delete" not being
called.
The `LeakedDanglingUntriaged` was only about the ordering in between
releasing memory and clearing the pointer.

Bug: dawn:2341
Fixed: dawn:2341
Change-Id: I338189737a6862056c253e95d0161319e32a1513
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/169382
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Auto-Submit: Arthur Sonzogni <arthursonzogni@chromium.org>

src/dawn/common/SlabAllocator.cpp

diff --git a/src/dawn/common/SlabAllocator.cpp b/src/dawn/common/SlabAllocator.cpp
index f092c03..7e92555 100644
--- a/src/dawn/common/SlabAllocator.cpp
+++ b/src/dawn/common/SlabAllocator.cpp
@@ -31,7 +31,6 @@
 #include <cstdlib>
 #include <limits>
 #include <new>
-
 #include "dawn/common/Assert.h"
 #include "dawn/common/Math.h"
 
@@ -44,23 +43,26 @@
 
 // Slab
 
+SlabAllocatorImpl::Slab::Slab() = default;
 SlabAllocatorImpl::Slab::Slab(char allocation[], IndexLinkNode* head)
-    : allocation(allocation), freeList(head), prev(nullptr), next(nullptr), blocksInUse(0) {}
+    : allocation(allocation), freeList(head) {}
 
 SlabAllocatorImpl::Slab::Slab(Slab&& rhs) = default;
 
-SlabAllocatorImpl::SentinelSlab::SentinelSlab() : Slab(nullptr, nullptr) {}
+// SentinelSlab
 
+SlabAllocatorImpl::SentinelSlab::SentinelSlab() = default;
 SlabAllocatorImpl::SentinelSlab::SentinelSlab(SentinelSlab&& rhs) = default;
 
 SlabAllocatorImpl::SentinelSlab::~SentinelSlab() {
-    Slab* slab = this->next;
-    while (slab != nullptr) {
-        Slab* next = slab->next;
+    // Delete the full linked list.
+    while (next) {
+        Slab* slab = next;
+        slab->Splice();
         DAWN_ASSERT(slab->blocksInUse == 0);
-        // Delete the slab's allocation. The slab is allocated inside slab->allocation.
-        delete[] slab->allocation;
-        slab = next;
+        char* allocation = slab->allocation;
+        slab->~Slab();  // Placement delete.
+        delete allocation;
     }
 }
 
@@ -156,30 +158,22 @@
 }
 
 void SlabAllocatorImpl::SentinelSlab::Prepend(SlabAllocatorImpl::Slab* slab) {
-    if (this->next != nullptr) {
-        this->next->prev = slab;
+    if (next != nullptr) {
+        next->prev = slab;
     }
     slab->prev = this;
-    slab->next = this->next;
-    this->next = slab;
+    slab->next = next;
+    next = slab;
 }
 
 void SlabAllocatorImpl::Slab::Splice() {
-    SlabAllocatorImpl::Slab* originalPrev = this->prev;
-    SlabAllocatorImpl::Slab* originalNext = this->next;
-
-    this->prev = nullptr;
-    this->next = nullptr;
-
-    DAWN_ASSERT(originalPrev != nullptr);
-
-    // Set the originalNext's prev pointer.
-    if (originalNext != nullptr) {
-        originalNext->prev = originalPrev;
+    DAWN_ASSERT(prev != nullptr);
+    prev->next = next;
+    if (next != nullptr) {
+        next->prev = prev;
     }
-
-    // Now, set the originalNext as the originalPrev's new next.
-    originalPrev->next = originalNext;
+    prev = nullptr;
+    next = nullptr;
 }
 
 void* SlabAllocatorImpl::Allocate() {

src/dawn/common/SlabAllocator.h

diff --git a/src/dawn/common/SlabAllocator.h b/src/dawn/common/SlabAllocator.h
index a20718f..9196c10 100644
--- a/src/dawn/common/SlabAllocator.h
+++ b/src/dawn/common/SlabAllocator.h
@@ -91,25 +91,21 @@
 
     struct Slab : PlacementAllocated {
         // A slab is placement-allocated into an aligned pointer from a separate allocation.
-        // Ownership of the allocation is transferred to the slab on creation.
         // | ---------- allocation --------- |
         // | pad | Slab | data ------------> |
+        Slab();
         Slab(char allocation[], IndexLinkNode* head);
         Slab(Slab&& rhs);
 
+        // Extract the Slab from the linked list.
         void Splice();
 
-        // TODO(crbug.com/dawn/2341): Both pointer are dangling in
-        // `dawn_perf_tests`, and `this` is not released before the end of the
-        // test. A side effect from one test to the next might cause difficult
-        // to reproduce flakes. Test:
-        // DrawCallPerf.Run/Vulkan_llvmpipe__LLVM_16_0_6__256_bits__MultipleVertexBuffers
-        raw_ptr<char, LeakedDanglingUntriaged> allocation;
-        raw_ptr<IndexLinkNode, LeakedDanglingUntriaged> freeList;
+        raw_ptr<char> allocation = nullptr;
+        raw_ptr<IndexLinkNode> freeList = nullptr;
 
-        raw_ptr<Slab, DanglingUntriaged> prev;
-        raw_ptr<Slab, DanglingUntriaged> next;
-        Index blocksInUse;
+        raw_ptr<Slab> prev = nullptr;
+        raw_ptr<Slab> next = nullptr;
+        Index blocksInUse = 0;
     };
 
     SlabAllocatorImpl(Index blocksPerSlab, uint32_t objectSize, uint32_t objectAlignment);