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

[Mitch Phillips via Phabricator via llvm-commits]

hctim marked 10 inline comments as done.
hctim 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(
----------------
eugenis wrote:
> If you move backtrace collection (but maybe not compression) to the caller, you can get rid of the ugly passing-by-reference of RecursiveGuard.
> 
Refactored a little here.


================
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();
----------------
eugenis wrote:
> 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?
> 
In case of `-DNDEBUG`.

Done.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:168
+  // servicing any further allocations permanently.
+  void stop();
+
----------------
eugenis wrote:
> Tries to stop. Not very hard.
> If we really wanted to, we could make the mutex recursive.
We need something to set `RecursiveGuard = true` in the in-process crash handler. The `tryLock` is mostly there as a best-effort as we might take some time uncompressing traces/getting the access backtrace, and it reduces the chance of our metadata getting corrupted.

Not sure what we could do to stop harder here :(


================
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
----------------
eugenis wrote:
> Would it be better to move this option to scudo then if it is unused?
My assumption is that some new consumer may chose to use the sanitizer_common flag parser (which we include optionally under `optional/options_parser.h`). In that case, it's nice to have this flag be user-controlled. I'm not super attached to it being here though, LMK.


================
Comment at: compiler-rt/lib/gwp_asan/tests/CMakeLists.txt:8
+  -g
+  -fno-optimize-sibling-calls # Used in crash_handler_api.cpp (GetAllocation())
+)
----------------
eugenis wrote:
> Don't understand the comment. Does this optimization break the code? Why? Where did  -O2 go?
It ensured that `call -> jmp` optimisation didn't take place, but `optnone` is a much better sol'n.


================
Comment at: compiler-rt/lib/gwp_asan/tests/crash_handler_api.cpp:1
+//===-- crash_handler_api.cpp -----------------------------------*- C++ -*-===//
+//
----------------
eugenis wrote:
> 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.
Let me refactor this to not go through the actual allocator + crashing stuff. I refactored this test from some working branch that had wildly different assumptions.


================
Comment at: compiler-rt/lib/gwp_asan/tests/crash_handler_api.cpp:216
+
+  uintptr_t IntPtr = reinterpret_cast<uintptr_t>(Ptr);
+
----------------
eugenis wrote:
> why not IntPtr = &Intptr ?
> 
Sorry, I don't follow...


================
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)
+
----------------
eugenis wrote:
> this can affect scudo performance.
@cryptoad 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73557