[PATCH] D60593: [GwpAsan] Introduce GWP-ASan.
Vitaly Buka via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Apr 30 15:41:33 PDT 2019
vitalybuka added a comment.
you should try to extract obvious parts into smaller patches
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:51
+ // Round up memory mappings to the nearest page size.
+ std::size_t BytesRequired = NumGuardedSlots * sizeof(AllocationMetadata);
+ Metadata = reinterpret_cast<AllocationMetadata *>(mapMemory(BytesRequired));
----------------
sizeof(*Metadata)
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:56
+ // Allocate memory and set up the free pages queue.
+ BytesRequired = NumGuardedSlots * sizeof(std::size_t);
+ FreeSlots = reinterpret_cast<std::size_t *>(mapMemory(BytesRequired));
----------------
sizeof(*FreeSlots)
this is not Google style, but advice is still relevant https://google.github.io/styleguide/cppguide.html#sizeof
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:86
+ size_t Index;
+ if (reserveSlot(&Index) == false)
+ return nullptr;
----------------
...== false) -> if (!...
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:99
+ Meta->Size = Size;
+ Meta->Addr = Ptr;
+ Meta->IsDeallocated = false;
----------------
Meta->Init(Size, Ptr, .....)
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:124
+
+ Meta->IsDeallocated = true;
+ Meta->DeallocationTrace.ThreadID = getThreadID();
----------------
can you move all metadata manipulation logic into AllocationMetadata:: methods?
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:39
+
+ struct AllocationMetadata {
+ // Maximum number of stack trace frames to collect for allocations + frees.
----------------
can you hide this struct into cpp file and just keep forward declaration here?
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:50
+ // The thread ID for this trace, or UINT64_MAX if not available.
+ uint64_t ThreadID = UINT64_MAX;
+ };
----------------
can you please add kInvalidThreadID ?
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:78
+ // as this may cause a use-after-free on shutdown.
+ ~GuardedPoolAllocator() = default;
+
----------------
why do you need this this line at all "~GuardedPoolAllocator() = default;"?
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:178
+ // *SlotIndex. Returns false if there are no remaining slots, true otherwise.
+ bool reserveSlot(std::size_t *SlotIndex);
+
----------------
can you avoid output arg by:
std::size_t reserveSlot();
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:236
+ // should be sampled when it reaches zero.
+ static TLS_INITIAL_EXEC uint64_t NextSampleCounter;
+
----------------
"static private:" are not very useful
you can hide them into unnamed namespace of cpp file
================
Comment at: compiler-rt/lib/gwp_asan/mutex.h:42
+public:
+ ScopedLock(Mutex *Mx) : Mu(Mx) { Mu->lock(); }
+ ~ScopedLock() { Mu->unlock(); }
----------------
explicit?
================
Comment at: compiler-rt/lib/gwp_asan/mutex.h:63
+
+ALWAYS_INLINE void Mutex::yieldProcessor(uint8_t Count) {
+#if defined(__i386__) || defined(__x86_64__)
----------------
do you need yieldProcessor and lockSlow inline?
maybe move into cpp file to move this ifdefs from header?
================
Comment at: compiler-rt/lib/gwp_asan/random.h:24
+ // xorshift128+ (64-bit output). Avoids multiplication.
+ static ALWAYS_INLINE uint64_t getRandomUnsigned64() {
+ uint64_t A = RandomStateA;
----------------
it's called on slow path when NextSampleCounter == 0
I don't thins ALWAYS_INLINE will help here.
just move it into cpp file?
Also http://quick-bench.com/Jbi9nGqIlAfRkFGHjpJzBVR58Bw
Can you explain why you need own random?
================
Comment at: compiler-rt/lib/gwp_asan/random.h:49
+private:
+ static TLS_INITIAL_EXEC uint64_t RandomStateA;
+ static TLS_INITIAL_EXEC uint64_t RandomStateB;
----------------
they don't need to be in the header
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60593/new/
https://reviews.llvm.org/D60593
More information about the llvm-commits
mailing list