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

[Mitch Phillips via Phabricator via llvm-commits]

hctim marked 36 inline comments as done.
hctim added inline comments.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:114
+  uintptr_t UPtr = reinterpret_cast<uintptr_t>(Ptr);
+  uintptr_t SlotStart = slotToAddr(addrToSlot(UPtr));
+  AllocationMetadata *Meta = addrToMetadata(UPtr);
----------------
vlad.tsyrklevich wrote:
> getPageAddr()
Not equal if `sizeof(Slot) != sizeof(Page)`. Happy to introduce a helper fn for this if you want, but it's used in a single place.


================
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.
----------------
vitalybuka wrote:
> can you hide this struct into cpp file and just keep forward declaration here?
See https://reviews.llvm.org/D60593?id=195942#inline-541269. Happy to change.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:78
+  // as this may cause a use-after-free on shutdown.
+  ~GuardedPoolAllocator() = default;
+
----------------
vitalybuka wrote:
> why do you need this this line at all "~GuardedPoolAllocator() = default;"?
It's simply an explicit note to the reader that we have a trivial destructor. If they want to add a d'tor, they'll come to this line and read the note above.


================
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);
+
----------------
vitalybuka wrote:
> can you avoid output arg by:
> std::size_t reserveSlot();
See https://reviews.llvm.org/D60593#inline-542081


================
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;
+
----------------
vitalybuka wrote:
> "static private:" are not very useful 
> you can hide them into unnamed namespace of cpp file
`NextSampleCounter` are referenced in `shouldSample()`, which is implemented in the header for inlining reasons.
`SingletonPtr` has been moved.


================
Comment at: compiler-rt/lib/gwp_asan/mutex.h:63
+
+ALWAYS_INLINE void Mutex::yieldProcessor(uint8_t Count) {
+#if defined(__i386__) || defined(__x86_64__)
----------------
vitalybuka wrote:
> do you need yieldProcessor and lockSlow inline?
> maybe move into cpp file to move this ifdefs from header?
http://quick-bench.com/5oJNxyiBe-7KwZ0nMCRDGBaNa5s

Looks like with clang gets a slight perf improvement with inlining, which is extended (to 10% perf improvement) w/ libc++.

Also I'm hesitent to create small cpp files, as per https://reviews.llvm.org/D60593?id=195942#inline-542189.

Okay to leave as is?


================
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;
----------------
vitalybuka wrote:
> 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?
> 
Can't use PRNG from libc++, as we have to be c-compatible.

Can't use PRNG from sanitizer_common, as we don't depend on sanitizer_common in the core code.

libc rand() is too slow.

read from /dev/ is too slow.


================
Comment at: compiler-rt/test/gwp_asan/thread_contention.cpp:1
+// REQUIRES: gwp_asan
+// This test ensures that normal allocation/memory access/deallocation works
----------------
vlad.tsyrklevich wrote:
> This tests races in the allocation code but not in the deallocation (and reallocation) code.
I've also added a test that's pretty much ThreadedHighContention.


================
Comment at: compiler-rt/test/gwp_asan/thread_contention.cpp:9
+// RUN: %clangxx_gwp_asan %s -o %t
+// RUN: %env_gwp_asan_options=NumUsableGuardedSlots=32 %t 32
+// RUN: %env_gwp_asan_options=NumUsableGuardedSlots=127 %t 127
----------------
vlad.tsyrklevich wrote:
> I don't think the runtime configuration adds much here anyway. I think you could get rid of both options and just have it allocate a fixed amount (lets say 1024 times) twice (first w/o random eviction and second run with.) It simplifies the test.
> 
> If we had access to the underlying allocator we could be cuter and also verify that every available slot was used, but I wouldn't bother otherwise.
The runtime configuration is mostly to avoid the overhead of recompilation. The compilation of this file for a debug build of clang takes 3.5-4s on my machine, and I desperately wanted to avoid that overhead multiple times.

The additional runs have been deleted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60593