Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Support] Recycler: Match dealloc size and enforce min size #121889

Conversation

optimisan
Copy link
Contributor

@optimisan optimisan commented Jan 7, 2025

Address sanitizer found mismatching deallocation size in Recycler.

@optimisan optimisan marked this pull request as ready for review January 7, 2025 06:29
@llvmbot
Copy link
Member

llvmbot commented Jan 7, 2025

@llvm/pr-subscribers-llvm-support

Author: Akshat Oke (optimisan)

Changes

Address sanitizer found mismatching deallocation size in Recycler.


Full diff: https://github.com/llvm/llvm-project/pull/121889.diff

2 Files Affected:

  • (modified) llvm/include/llvm/Support/Recycler.h (+3-1)
  • (modified) llvm/unittests/Support/RecyclerTest.cpp (+24)
diff --git a/llvm/include/llvm/Support/Recycler.h b/llvm/include/llvm/Support/Recycler.h
index 693c6559ff2fdc3..e531e235ee78f87 100644
--- a/llvm/include/llvm/Support/Recycler.h
+++ b/llvm/include/llvm/Support/Recycler.h
@@ -72,7 +72,7 @@ class Recycler {
   void clear(AllocatorType &Allocator) {
     while (FreeList) {
       T *t = reinterpret_cast<T *>(pop_val());
-      Allocator.Deallocate(t);
+      Allocator.Deallocate(t, Size, Align);
     }
   }
 
@@ -89,6 +89,8 @@ class Recycler {
                   "Recycler allocation alignment is less than object align!");
     static_assert(sizeof(SubClass) <= Size,
                   "Recycler allocation size is less than object size!");
+    static_assert(Size >= sizeof(FreeNode) &&
+                  "Recycler allocation size must be at least sizeof(FreeNode)");
     return FreeList ? reinterpret_cast<SubClass *>(pop_val())
                     : static_cast<SubClass *>(Allocator.Allocate(Size, Align));
   }
diff --git a/llvm/unittests/Support/RecyclerTest.cpp b/llvm/unittests/Support/RecyclerTest.cpp
index a33506b47ebeae3..696e397d3f10edd 100644
--- a/llvm/unittests/Support/RecyclerTest.cpp
+++ b/llvm/unittests/Support/RecyclerTest.cpp
@@ -14,6 +14,10 @@ using namespace llvm;
 
 namespace {
 
+struct Object1 {
+  char Data[1];
+};
+
 struct Object8 {
   char Data[8];
 };
@@ -22,12 +26,32 @@ class DecoratedMallocAllocator : public MallocAllocator {
 public:
   int DeallocCount = 0;
 
+  void Deallocate(const void *Ptr, size_t Size, size_t Alignment) {
+    DeallocCount++;
+    MallocAllocator::Deallocate(Ptr, Size, Alignment);
+  }
+
   template <typename T> void Deallocate(T *Ptr) {
     DeallocCount++;
     MallocAllocator::Deallocate(Ptr);
   }
 };
 
+TEST(RecyclerTest, RecycleAllocation) {
+  DecoratedMallocAllocator Allocator;
+  // Recycler needs size to be atleast 8 bytes.
+  Recycler<Object1, 8, 8> R;
+  Object1 *A1 = R.Allocate(Allocator);
+  Object1 *A2 = R.Allocate(Allocator);
+  R.Deallocate(Allocator, A2);
+  Object1 *A3 = R.Allocate(Allocator);
+  EXPECT_EQ(A2, A3); // reuse the deallocated object.
+  R.Deallocate(Allocator, A1);
+  R.Deallocate(Allocator, A3);
+  R.clear(Allocator); // Should deallocate A1 and A3.
+  EXPECT_EQ(Allocator.DeallocCount, 2);
+}
+
 TEST(RecyclerTest, MoveConstructor) {
   DecoratedMallocAllocator Allocator;
   Recycler<Object8> R;

Base automatically changed from users/Akshat-Oke/12-19-_support_recycler_implement_move_constructor to main January 7, 2025 10:17
@optimisan optimisan force-pushed the users/Akshat-Oke/01-07-_support_recycler_match_dealloc_size_and_enforce_min_size branch from 0f46720 to 03ca3f5 Compare January 7, 2025 10:22
@optimisan optimisan force-pushed the users/Akshat-Oke/01-07-_support_recycler_match_dealloc_size_and_enforce_min_size branch from 03ca3f5 to e31e945 Compare January 8, 2025 05:09
@optimisan optimisan merged commit f07b10b into main Jan 9, 2025
6 of 8 checks passed
@optimisan optimisan deleted the users/Akshat-Oke/01-07-_support_recycler_match_dealloc_size_and_enforce_min_size branch January 9, 2025 08:52
DKLoehr pushed a commit to DKLoehr/llvm-project that referenced this pull request Jan 17, 2025
)

Address sanitizer found mismatching deallocation size in Recycler.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants