[PATCH] D135411: Add generic KCFI operand bundle lowering

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/KCFI.cpp:61
+  if (F.hasFnAttribute("patchable-function-prefix"))
+    report_fatal_error("-fpatchable-function-entry=N,M, where M>0 is not "
+                       "compatible with -fsanitize=kcfi on this target.",
----------------
samitolvanen wrote:
> MaskRay wrote:
> > Try avoiding report_fatal_error. Use a proper error handling mechanism like `Ctx.diagnose(...)` with a `DiagnosticInfo` subclass. Remove the trailing period for the message.
> > Try avoiding report_fatal_error. Use a proper error handling mechanism like `Ctx.diagnose(...)` with a `DiagnosticInfo` subclass.
> 
> I was looking at this, but I feel like adding a new class might be a bit of an overkill for this situation. Looking at the code in `lib/Transforms`, there are 70 uses of `report_fatal_error` and 17 uses of `Ctx.diagnose(...)`. The latter seem to all be more complex diagnostics than just printing out an error message. Do you think I should nevertheless use `Ctx.diagnose` for this message?
> 
> > Remove the trailing period for the message.
> 
> Done.
A lot of `report_fatal_error` uses are used similar to unreachable for both assertion and non-assertion builds. The code is pretty much reachable (if a user has a patchable_function_entry attribute and use -fsanitize=kcfi). It's bad interface to surface a crash-style diagnostic.

You may add a `namespace { class ... : public DiagnosticInfo }` in this file. It just needs to overload `print` which is not much boilerplate.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135411