[Support] Recycler: Enforce minimum allocation size #121425
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Jan 1, 2025
This stack of pull requests is managed by Graphite. Learn more about stacking. |
optimisan
force-pushed
the
users/Akshat-Oke/01-01-_support_recycler_enforce_minimum_allocation_size
branch
from
600fd4c to
4496358
Compare
optimisan
changed the base branch from
users/Akshat-Oke/12-16-_newpm_codegen_record_parameterized_machine_pass_names_to_pic
to
users/Akshat-Oke/12-19-_codegen_liveregmatrix_use_allocator_through_a_unique_ptr
This was referenced Jan 1, 2025
optimisan
force-pushed
the
users/Akshat-Oke/01-01-_support_recycler_enforce_minimum_allocation_size
branch
from
4496358 to
ac8a44c
Compare
|
@llvm/pr-subscribers-llvm-support Author: Akshat Oke (optimisan) ChangesRecycler uses reinterpret_cast to an internal structure of size 8. Full diff: https://github.com/llvm/llvm-project/pull/121425.diff 3 Files Affected:
diff --git a/llvm/include/llvm/Support/Recycler.h b/llvm/include/llvm/Support/Recycler.h
index bbd9ae321ae30c..8cc882ea5fa058 100644
--- a/llvm/include/llvm/Support/Recycler.h
+++ b/llvm/include/llvm/Support/Recycler.h
@@ -85,6 +85,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 size must be atleast 8");
return FreeList ? reinterpret_cast<SubClass *>(pop_val())
: static_cast<SubClass *>(Allocator.Allocate(Size, Align));
}
diff --git a/llvm/unittests/Support/CMakeLists.txt b/llvm/unittests/Support/CMakeLists.txt
index d64f89847aa8e7..6de81658264420 100644
--- a/llvm/unittests/Support/CMakeLists.txt
+++ b/llvm/unittests/Support/CMakeLists.txt
@@ -69,6 +69,7 @@ add_llvm_unittest(SupportTests
PerThreadBumpPtrAllocatorTest.cpp
ProcessTest.cpp
ProgramTest.cpp
+ RecyclerTest.cpp
RegexTest.cpp
ReverseIterationTest.cpp
ReplaceFileTest.cpp
diff --git a/llvm/unittests/Support/RecyclerTest.cpp b/llvm/unittests/Support/RecyclerTest.cpp
new file mode 100644
index 00000000000000..8cd763c0b83f8a
--- /dev/null
+++ b/llvm/unittests/Support/RecyclerTest.cpp
@@ -0,0 +1,46 @@
+//===--- unittest/Support/RecyclerTest.cpp --------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/Support/Recycler.h"
+#include "llvm/Support/AllocatorBase.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+
+namespace {
+
+struct Object1 {
+ char Data[1];
+};
+
+class DecoratedMallocAllocator : public MallocAllocator {
+public:
+ int DeallocCount = 0;
+
+ 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);
+}
+
+} // end anonymous namespace
|
Base automatically changed from
users/Akshat-Oke/12-19-_codegen_liveregmatrix_use_allocator_through_a_unique_ptr
to
main
January 1, 2025 09:24
Recycler uses reinterpret_cast to an internal structure of size 8. Invalid write occurs if Recycler is used for objects with sizes less than 8.
optimisan
force-pushed
the
users/Akshat-Oke/01-01-_support_recycler_enforce_minimum_allocation_size
branch
from
ac8a44c to
5e6aefd
Compare
arsenm
reviewed
Jan 2, 2025
arsenm
reviewed
Jan 2, 2025
| R.clear(Allocator); // Should deallocate A1 and A3. | ||
| EXPECT_EQ(Allocator.DeallocCount, 2); | ||
| } | ||
|
|
There was a problem hiding this comment.
Is there a way to test the static_assert fails in the < 8 case?
There was a problem hiding this comment.
Don't think we have any infrastructure in LLVM for "this should fail to compile". If it were SFINAE'd then we could static_assert that something fails to compile, but static_asserts aren't themselves SFINAE, so it'd mean adding more metaprogramming infrastructure that's presumably not otherwise needed (& wouldn't test the static_assert itself, but the SFINAE trap)
Co-authored-by: Matt Arsenault <Matthew.Arsenault@amd.com>
arsenm
approved these changes
Jan 3, 2025
optimisan
deleted the
users/Akshat-Oke/01-01-_support_recycler_enforce_minimum_allocation_size
branch
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/190/builds/12239 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/169/builds/7088 Here is the relevant piece of the build log for the reference |
optimisan
added a commit
that referenced
this pull request
Jan 6, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/146/builds/2009 Here is the relevant piece of the build log for the reference |
github-actions bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Jan 10, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Recycler uses reinterpret_cast to an internal structure of size 8.
Invalid write occurs if Recycler is used for objects with sizes less
than 8.