[PATCH] D60593: [GwpAsan] Introduce GWP-ASan.

[Vitaly Buka via Phabricator via llvm-commits]

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