[PATCH] D119296: KCFI sanitizer

Thu Apr 28 18:49:06 PDT 2022

samitolvanen added a comment.

In D119296#3481573 <https://reviews.llvm.org/D119296#3481573>, @joaomoreira wrote:

> At first I was on the boat of KCFI being implemented in the IR level because being architecture agnostic is certainly a great plus.

The original plan was to just lower the intrinsic into IR without having to implement arch-specific lowering where not needed.

> Yet, once we end up needing additional passes (regardless of it being in IR or in the arch-specific back-end) to tie things and achieve the best possible instrumentation, I start to think we are over-complicating the design of the feature -- if we have a single pass at the very end of each supported arch back-end that just traverses the functions and adds KCFI checks to indirect branches that have a "PrototypeHash" attribute set (or an special operand as also suggested by @pcc ) we can get the best possible instrumentation while inherently dodging any pitfall due to unforeseen optimizations or code transformations that may happen between the KCFI checks placement and the last stage of the back-end.

I looked at your code quickly and I wonder if using operand bundles would be better than adding an attribute. Thoughts?

> I'm not an expert on LLVM's pipeline, but it just feels a little awkward and redundant that we need passes to fix what other passes messed up regarding a pass that executed before everything.

I agree that a separate pass wasn't ideal, but InstCombine seems to be full of code to "fix what other passes messed up".  :)  I'm not sure if messed up is the correct term though, these are checks that were necessary before optimizations, but are no longer needed.

> Also, I don't see clearly how it could not be in the back-end if we want to avoid all the call arguments setup in between the check and the call.

Yes, that's the strongest reason to avoid the IR level implementation, but it does make supporting other architectures quite a bit more painful.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119296