[PATCH] D73557: [GWP-ASan] Crash Handler API.

[Evgenii Stepanov via Phabricator via llvm-commits]

eugenis added inline comments.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:113
     size_t BacktraceLength =
         Backtrace(UncompressedBuffer, kMaxTraceLengthToCollect);
     DeallocationTrace.TraceSize = compression::pack(
----------------
If you move backtrace collection (but maybe not compression) to the caller, you can get rid of the ugly passing-by-reference of RecursiveGuard.



================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:130
   if (Opts.SampleRate < 0) {
-    Opts.Printf("GWP-ASan Error: SampleRate is < 0.\n");
-    exit(EXIT_FAILURE);
+    assert(false && "GWP-ASan Error: SampleRate is < 0.");
+    __builtin_trap();
----------------
hctim wrote:
> No `Printf` maintained internally any more, so we assert and trap instead.
why do you need trap after assert?

Factor out CHECK() implemented (on Android) as set_abort_message() + abort?



================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:168
+  // servicing any further allocations permanently.
+  void stop();
+
----------------
Tries to stop. Not very hard.
If we really wanted to, we could make the mutex recursive.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:260
+  // order to tell the crash handler what happened. Used when errors are
+  // detected internall (Double Free, Invalid Free).
+  void trapOnAddress(uintptr_t Address, Error E);
----------------
internally


================
Comment at: compiler-rt/lib/gwp_asan/options.inc:33
 
+// Developer note - This option is not actually processed by GWP-ASan itself. It
+// is included here so that a user can specify whether they want signal handlers
----------------
Would it be better to move this option to scudo then if it is unused?


================
Comment at: compiler-rt/lib/gwp_asan/tests/CMakeLists.txt:8
+  -g
+  -fno-optimize-sibling-calls # Used in crash_handler_api.cpp (GetAllocation())
+)
----------------
Don't understand the comment. Does this optimization break the code? Why? Where did  -O2 go?


================
Comment at: compiler-rt/lib/gwp_asan/tests/crash_handler_api.cpp:1
+//===-- crash_handler_api.cpp -----------------------------------*- C++ -*-===//
+//
----------------
hctim wrote:
> New tests for the crash handler API.
This test makes my head hurt :(
I don't see why it needs to actually send and receive signals to test this API.
Also, the fuzzy match logic seems to be testing an implementation of backtrace. This also seems irrelevant. I'd simply supply a mock backtrace callback.


================
Comment at: compiler-rt/lib/gwp_asan/tests/crash_handler_api.cpp:23
+// We also disable sibling call optimisations in the build file to ensure that
+// the call inside this function doesn't get optimised to a jump.
+__attribute__((noinline)) void *GetAllocation(GuardedPoolAllocator *GPA,
----------------
or can just do __attribute__((optnone))


================
Comment at: compiler-rt/lib/gwp_asan/tests/crash_handler_api.cpp:216
+
+  uintptr_t IntPtr = reinterpret_cast<uintptr_t>(Ptr);
+
----------------
why not IntPtr = &Intptr ?



================
Comment at: compiler-rt/lib/scudo/standalone/CMakeLists.txt:25
+append_list_if(COMPILER_RT_HAS_OMIT_FRAME_POINTER_FLAG
+               -mno-omit-leaf-frame-pointer SCUDO_CFLAGS)
+
----------------
this can affect scudo performance.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73557