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

[Joel Galenson via Phabricator via llvm-commits]

jgalenson marked an inline comment as done.
jgalenson added a comment.

Any other 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:
> eugenis wrote:
> > 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.
> Ah, that is indeed a good reason to keep it. I withdraw my request then.
Yes, I didn't actually see anything using this, but I figured it would be correct, so I left it there.


https://reviews.llvm.org/D49383