Skip to content

[scudo] Add static vector functionality. #98986

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

Merged
merged 15 commits into from
Jul 17, 2024
Merged

Conversation

JoshuaMBa
Copy link
Contributor

@JoshuaMBa JoshuaMBa commented Jul 16, 2024

The scudo vector implementation maintains static local data before
switching to dynamically allocated data as the array size grows.
Users of the vector must now specify the size of the static local data
through the vector template (the default size has been removed).
If 0 is specified for the size of the static local data, an assertion will
be triggered.

JoshuaMBa and others added 5 commits June 14, 2024 19:34
…e option handler.

Initially, the scudo allocator would return an error if the user attempted to set the cache
capacity (i.e. the number of possible entries in the cache) above the maximum cache capacity.
Now the allocator will resort to using the maximum cache capacity in this event. An
error will still be returned if the user attempts to set the number of entries to a
negative value.
The scudo vector implementation maintains static local data before
switching to dynamically allocated data as the array size grows.
Users of the vector can now specify the size of the
static local data through the vector template (the default size is
currently set to 10 * sizeof(T) where T is the vector element
type).
@JoshuaMBa JoshuaMBa changed the title [scudo] Add static vector specification. [scudo] Add static vector functionality. Jul 16, 2024
@llvmbot
Copy link
Member

llvmbot commented Jul 16, 2024

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: Joshua Baehring (JoshuaMBa)

Changes

The scudo vector implementation maintains static local data before
switching to dynamically allocated data as the array size grows.
Users of the vector can now specify the size of the static local data
through the vector template (the default size is currently set to
10 * sizeof(T) where T is the vector element type).


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

1 Files Affected:

  • (modified) compiler-rt/lib/scudo/standalone/vector.h (+9-6)
diff --git a/compiler-rt/lib/scudo/standalone/vector.h b/compiler-rt/lib/scudo/standalone/vector.h
index ca10cc281d770..d972ef068bee5 100644
--- a/compiler-rt/lib/scudo/standalone/vector.h
+++ b/compiler-rt/lib/scudo/standalone/vector.h
@@ -9,6 +9,7 @@
 #ifndef SCUDO_VECTOR_H_
 #define SCUDO_VECTOR_H_
 
+#include "common.h"
 #include "mem_map.h"
 
 #include <string.h>
@@ -21,7 +22,7 @@ namespace scudo {
 // implementation supports only POD types.
 //
 // NOTE: This class is not meant to be used directly, use Vector<T> instead.
-template <typename T> class VectorNoCtor {
+template <typename T, size_t StaticCapacity> class VectorNoCtor {
 public:
   T &operator[](uptr I) {
     DCHECK_LT(I, Size);
@@ -116,18 +117,19 @@ template <typename T> class VectorNoCtor {
   uptr CapacityBytes = 0;
   uptr Size = 0;
 
-  T LocalData[256 / sizeof(T)] = {};
+  T LocalData[StaticCapacity] = {};
   MemMapT ExternalBuffer;
 };
 
-template <typename T> class Vector : public VectorNoCtor<T> {
+template <typename T, size_t StaticCapacity = 10>
+class Vector : public VectorNoCtor<T, Max<size_t>(10, StaticCapacity)> {
 public:
-  constexpr Vector() { VectorNoCtor<T>::init(); }
+  constexpr Vector() { VectorNoCtor<T, StaticCapacity>::init(); }
   explicit Vector(uptr Count) {
-    VectorNoCtor<T>::init(Count);
+    VectorNoCtor<T, StaticCapacity>::init(Count);
     this->resize(Count);
   }
-  ~Vector() { VectorNoCtor<T>::destroy(); }
+  ~Vector() { VectorNoCtor<T, StaticCapacity>::destroy(); }
   // Disallow copies and moves.
   Vector(const Vector &) = delete;
   Vector &operator=(const Vector &) = delete;
@@ -138,3 +140,4 @@ template <typename T> class Vector : public VectorNoCtor<T> {
 } // namespace scudo
 
 #endif // SCUDO_VECTOR_H_
+

Copy link

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo.
Please turn off Keep my email addresses private setting in your account.
See LLVM Discourse for more information.

Copy link

github-actions bot commented Jul 16, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Contributor

@cferris1000 cferris1000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally, I though we might want to allow a zero length capacity. But thinking about it a bit more, I don't think we should since that would likely be a bug. If we ever decide to allow it, we can change the static assert.

Chia-hung, make sure the usage of bytes instead of items is okay with you.

@ChiaHungDuan
Copy link
Contributor

Originally, I though we might want to allow a zero length capacity. But thinking about it a bit more, I don't think we should since that would likely be a bug. If we ever decide to allow it, we can change the static assert.

Chia-hung, make sure the usage of bytes instead of items is okay with you.

Left a comment about the default size.

About the zero length, I told Jushua that he can either do default to 1 if it's 0 (like what llvm::SmallVector does) or static_assert that. Based on your suggestion, I think it's better to just disallow it.

@JoshuaMBa JoshuaMBa force-pushed the vector_update branch 2 times, most recently from 4e72ed8 to d8f1007 Compare July 17, 2024 21:04
@ChiaHungDuan ChiaHungDuan merged commit d08527e into llvm:main Jul 17, 2024
4 of 5 checks passed
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
Summary:
The scudo vector implementation maintains static local data before 
switching to dynamically allocated data as the array size grows.
Users of the vector must now specify the size of the static local data 
through the vector template (the default size has been removed). 
If 0 is specified for the size of the static local data, an assertion
will
be triggered.

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60250929
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.

4 participants