[PATCH] D49383: [cfi-verify] Support cross-DSO

[Evgenii Stepanov via Phabricator via llvm-commits]

eugenis added inline comments.


================
Comment at: tools/llvm-cfi-verify/lib/FileAnalysis.cpp:539
+  TrapOnFailFunctions.insert("__cfi_slowpath");
+  TrapOnFailFunctions.insert("abort");
+  TrapOnFailFunctions.insert("__cfi_slowpath at plt");
----------------
pcc wrote:
> jgalenson wrote:
> > pcc wrote:
> > > jgalenson wrote:
> > > > eugenis wrote:
> > > > > pcc wrote:
> > > > > > Where does `abort` come from? In the diagnostic mode the failure path will end up calling `__ubsan_handle_cfi_check_fail` or `__ubsan_handle_cfi_check_fail_abort`.
> > > > > I think it comes from a combination of -fsanitize-trap and -ftrap-function=abort.
> > > > Yes, I figured it was a common function that might get used instead of a trap instruction.  I can remove it.
> > > > 
> > > > I didn't add the diagnostic ones because I couldn't easily generate a test file using them and I didn't want to add them without a test.
> > > I'd remove it since we shouldn't be trapping directly in cross-DSO mode, so even if a trap function is specified we shouldn't be emitting a call to it in connection with a CFI check.
> > You mean remove `__cfi_slowpath_diag` from this list?  I actually want to keep it since I see calls to it in real binaries.  It's also specified in https://clang.llvm.org/docs/ControlFlowIntegrityDesign.html#cfi-slowpath just next to `__cfi_slowpath`, which apparently calls `__cfi_slowpath_diag`.
> Sorry, I meant `abort`.
Why remove abort from the list? This tool is intended to check if the binary is safe while relying on the cfi implementation details as little as possible. Any function that does not return when the call is unsafe is fine in this list, IMHO.


https://reviews.llvm.org/D49383