Given https://reviews.llvm.org/D140363, this patch is being abandoned.

FWIIW, agreed on removing this until we figure out how to make it work properly. Thanks for the patch @MaskRay.

In D140035#3998954, @samitolvanen wrote:

In D140035#3996066, @joaomoreira wrote:

Regarding not being able to reproduce this in kernel -- never mind... I was misled by setup issues while running IBT kernels in QEMU. I managed to fix the setup and confirm that kernel won't boot.

OK, great. Thanks for double checking!

The patch itself looks good to me, but I suspect ibt-seal in general has the same issue as D138337 where it can drop endbr instructions from isUsedInRegularObj symbols that are not address-taken in the bitcode (e.g. functions whose address is only taken in stand-alone assembly). I saw this issue only in the arm64 Linux kernel, but there's always a chance a similar code pattern emerges on the x86 side at some point in future too. This can obviously be worked around in the kernel, but just something to keep in mind.

Weirdly enough, I double-tested the behavior for -flto=thin + -mibt-seal; the kernel did boot fine on my setup, but when dumped/grep'ed for ENDBRs, it had ~500 less ENDBRs throughout the binary

Did you confirm the issue with the reproducer in the CBL bug? It would be interesting to find out why you couldn't reproduce this in the kernel.

Regarding https://github.com/ClangBuiltLinux/linux/issues/1737:

I really like this revision. It removes the redundancy of having KCFI passes both in CodeGen and in the backend; it detangles CALL instructions from KCFI by creating a new MIR instruction; it fixes alignment while still supporting the -fpatchable-function-entry option; it doesn't add hashes/gadgets through the code as it was before needed by the use of cmp instructions. With all this said, the patch LGTM.

In D119296#3483176, @nickdesaulniers wrote:

In D119296#3481573, @joaomoreira wrote:

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 don't think so. Consider DCE; other passes leave behind garbage all the time; DCE is expected to clean up after them.

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

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.

This seems like a reasonable approach, and was also the approach taken for the PAuth ABI. The PAuth ABI attaches an operand bundle to the call instruction and arranges for the code for the check to be generated together with the call. This helps with avoiding spills of the verified function pointer between the check and the call. The code isn't upstream but is available on this branch: https://github.com/pcc/llvm-project/tree/apple-pac4

Grep for something like undle.*ptrauth and you should find the relevant code.

I think there are no more untied knots... @pengfei, do you think this is ready to merge? If yes, can you please merge it? tks!

Oh, one other tiny detail I forgot to mention. I noticed that the tag is pushing the functions 6 bytes forward, regardless of any prepending padding nops that were added to ensure 16b alignment. It would be cool to care about the proper function alignment and also to not emit dummy padding nops when the padding area can be filled with the tag itself.

I played a little bit with kcfi and here are some thoughts:

In the previous discussion, @joaomoreira pointed out that this is very similar to nocf_check and proposed reusing that attribute. In an offline discussion, @pcc was concerned that an attribute may not be the right approach here and suggested a __builtin_kcfi_unchecked(function(args)) built-in function to avoid changing the type system.

I did track down the problem to clang/lib/Frontend/CompilerInvocation.cpp -- RoundTrip method. There, we can se the following statement:

No problem! And thanks! The test looks better than the one I had half-way implemented. I'm also updating the test which got broken by this fix... let me know if you see a problem with it!

Hm. I got tests almost ready (fixed the broken one and am working on finishing a new one). I'll update the diff in a few minutes. I can also update the description, let me know if you have suggestions.

ping.

Hi Gabriel, thanks for the updated review. Here are the cmake flags as you asked:

Hm. First, thanks a lot for the detailed review. I double check on my end and I still don't get the flag as expected. Here are some of my outputs:

@pengfei or @craig.topper perhaps one of you could merge this? please, let me know if I'm missing any obvious requirement or detail.

Hi, is anything still preventing this from being merged?

Modularized needsPrologueENDBR function and removed missed comment.

FWIIW, looks correct to me too. Tks @DavidSpickett

It seems this was not merged yet. Is there anything else needed?

I noticed that this commit breaks MUSL 1.2.0. Here is an isolated test-case that illustrates the issue:

The test was updated in the last revision, diff was also updated for context. Is there anything else needed for this?

Fixes:

• Applied suggestions made by @RKSimon,
• Fixed "opcode" variable name capitalization.