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

[Mitch Phillips via Phabricator via llvm-commits]

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


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:149
   }
-  uninstallSignalHandlers();
+  crash_handler::uninstallSignalHandlers();
+}
----------------
eugenis wrote:
> if init() does not set the signal handlers, maybe uninit should not remove them.
> Also, weak declarations are not very portable.
Removed it, and updated the tests elsewhere.


================
Comment at: compiler-rt/lib/gwp_asan/tests/crash_handler_api.cpp:158
+TEST_F(CrashHandlerAPITest, InvalidFree) {
+  uintptr_t FailureAddress = 0x7001;
+
----------------
eugenis wrote:
> It's usually better to be a bit more explicit in tests and setup the program state in each test case. In this case, I'd need to look ~2 pages up to understand what 0x7001 refers to. The test class could provide metadata setup helpers, and test case could look something like
> 
> metadata(0, 0x7000, 0x20, /*allocated*/false);
> State.FailureAddress = 0x7001;
> State.FailureType = Error::INVALID_FREE;
> EXPECT_TRUE(__gwp_asan_error_is_mine(&State));
> 
Great idea, thanks!


================
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)
+
----------------
hctim wrote:
> eugenis wrote:
> > this can affect scudo performance.
> @cryptoad 
Flagged it behind `COMPILER_RT_HAS_GWP_ASAN` - but still worth assessing for @cryptoad 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73557