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

[Evgenii Stepanov via Phabricator via llvm-commits]

eugenis added inline comments.


================
Comment at: compiler-rt/lib/gwp_asan/allocation_metadata.h:41
+  // Whether this allocation has been deallocated yet.
+  bool IsDeallocated = false;
+};
----------------
This feels wasteful. The bools could be kept together to save on alignment padding. The bool in "struct Trace" seems unnecessary - could we use Trace[0] == 0 to indicate a missing trace?


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:52
+  Metadata = reinterpret_cast<AllocationMetadata *>(
+      malloc(NumGuardedSlots * sizeof(AllocationMetadata)));
+
----------------
cryptoad wrote:
> `GuardedPoolAllocator::init` is called within the constructor of `Allocator` for Scudo, so `malloc` here doesn't sound like it would work?
> Any heap related function should probably be avoided.
Could full initialization be done lazily at the first sampled malloc?
At least, we should not allocate metadata array if the process is going to have sample rate of 0.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:55
+    if (UNLIKELY(SampleRate == 0))
+      return false;
+
----------------
morehouse wrote:
> It would be nice to avoid this check on the fast path.
> 
> Perhaps we can do `rand() % (SampleRate + 1)` and then check `SampleRate !=0` only just before we would return true.
Or it could be another special value of NextSampleCounter, which could also serve as a way to temporarily disable sampling in a thread.



================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:115
+  // pointer.
+  void *mapMemory(std::size_t Size) const;
+  void unmapMemory(void *Ptr, std::size_t Size) const;
----------------
Why are these functions in the public interface?


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:120
+
+protected:
+  // Get the current thread ID, or UINT64_MAX if failure. Note: This
----------------
why protected and not private?


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:190
+  // The number of guarded slots that this pool holds.
+  std::size_t NumGuardedSlots = 0;
+  // Record the number of currently used slots. We store this amount so that we
----------------
What's the distinction between slots and pages (as in FreePages below)?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60593/new/

https://reviews.llvm.org/D60593