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

[Matt Morehouse via Phabricator via llvm-commits]

morehouse added inline comments.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:111
+  void *mapMemory(std::size_t Size) const;
+  void unmapMemory(void *Ptr, std::size_t Size) const;
+  void markReadWrite(void *Ptr, std::size_t Size) const;
----------------
This is unused.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator_posix.cpp:57
+  // against the RSS.
+  if (mmap(Ptr, Size, PROT_NONE, MAP_FIXED | MAP_ANONYMOUS | MAP_PRIVATE, 0,
+           0) == MAP_FAILED) {
----------------
s/0/-1 to be safe


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator_posix.cpp:71
+
+static void sigSegvHandler(int sig, siginfo_t *info, void *ucontext) {
+  if (sig != SIGSEGV)
----------------
This should probably forward to the previous SEGV handler after printing any GWP-ASan related things.

Doing this correctly is tricky, but you can look at how the original Google3 implementation did it for reference.


================
Comment at: compiler-rt/lib/gwp_asan/mutex.h:10
+// TODO(hctim): Move this implementation and Scudo's implementation into a
+// unified base implementaiton. We shouldn't need to have separate
+// implementations for this.
----------------
implementation


================
Comment at: compiler-rt/lib/gwp_asan/mutex.h:33
+
+  bool Semaphore = false;
+};
----------------
I don't normally think of a semaphore as a bool.  Maybe just call it `Locked`?


================
Comment at: compiler-rt/lib/gwp_asan/mutex.h:52
+ALWAYS_INLINE bool Mutex::tryLock() {
+  return __atomic_exchange_n(&Semaphore, true, __ATOMIC_ACQUIRE) == false;
+}
----------------
Nit:  Since bools, we can do `!__atomic_exchange_n(...)` instead.


================
Comment at: compiler-rt/lib/gwp_asan/mutex_posix.cpp:14
+namespace gwp_asan {
+void Mutex::yieldPlatform() { sched_yield(); }
+} // namespace gwp_asan
----------------
I'd prefer if we avoid creating an entire file just for `sched_yield()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60593