[PATCH] D70681: [GWP-ASan] Implementation of crash handler API.

[Vlad Tsyrklevich via Phabricator via llvm-commits]

vlad.tsyrklevich added a comment.

Is there a reason you want the API to be such that you can get the 'error string', 'allocation string', and metadata? The first two seem nice, though not especially useful without out the stack traces. The third seems like it might be exposing internals we don't want to expose and potentially lead to versioning issues. Also how does the crash reporter already knows the error type when calling these functions? I think perhaps it would make sense to have a getCrashReport() API that fills out a struct and perhaps having dumpReportInternal() only use the information from that struct to avoid duplicating work/ensuring all information we use is exposed? I may not be familiar enough with your crash handler case to understand the justification for why it is structured this way.



================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:222
+
+  // If the page containing the address isn't currently marked an inacessible,
+  // do that, so that we can trap on the address provided.
----------------
s/an/as/
s/inacessible/inaccessible/


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:230
+  volatile char ThisReadShouldSegv = *Ptr;
+  (void)(ThisReadShouldSegv); // Ignore unused variable warning.
+}
----------------
I used to do the same thing as you do here but ended up switching to just setting a global with the bad address and calling `__builtin_trap()`. You might consider doing the same here as you already set the global and would just need to move the if(pointerIsMine()) in diagnoseErrorInternal() after the check


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:397
+// the result was truncated.
+size_t errorString(Error E, char *Buffer, size_t BufferSize) {
   switch (E) {
----------------
This would be simpler if it returned a `const char*` instead of writing to a buffer, and then the code below that snprintf()s into dumpReportInternal() would also be simpler and clearer. This would add some code to getErrorString() but I think that's fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70681