Changing the hash function breaks compatibility with the Rust KCFI implementation. I would personally like to avoid changing how KCFI type hashes are computed unless there's a very compelling reason to switch away from xxHash64.

Thanks for fixing this! This approach looks good to me.

Needs some thoughts to merge the above paragraphs with the existing one

Addressed feedback.

In D148385#4397995, @MaskRay wrote:

KCFI_CHECK lowering has some complexity to allocate a temporary register. This needs to follow the calling convention which can be modified by many compiler options and function attributes.

Looks good to me, but maybe worth waiting for someone more familiar with compiler-rt to take a look as well.

Dropped the arch-specific KCFI machine function pass.

Uploaded D149915 to remove the arch-specific KCFI passes. I'll update this patch once we clean that up.

Addressed most of the feedback.

In D148385#4275617, @jrtc27 wrote:

I can't help but feel the core of this should be target-independent, with TLI or similar hooks to actually do the target-specific bits?

LGTM. Folding D145979 into this one might also make sense.

Is this related to coverage? If not, should the test go to sanitizer-ld.c instead?

In D134831#4137408, @inglorion wrote:

Is this intended to warn on code that casts a function taking a pointer to some non-void type to a function that takes a void*?

Thanks for fixing the MSan issue, Ramon. There's still a clang-format error that trips the Debian build above, but it's trivial so I can fix it when relanding the patch.

Don't drop type hashes from VisibleToRegularObj symbols.

So, I looked into applying this to all object files with by adding similar code to Writer<ELFT>::finalizeSections(). This is not quite what we want in the kernel though. The primary goal is to better optimize bitcode during a relocatable link, and while we can conveniently generate a list of functions exported to kernel modules, we don't necessarily have a complete list of symbols (typically defined in stand-alone assembly) that are later needed in linker scripts or whose bindings objtool etc. assume to remain untouched, for example. In order to avoid additional hacks in the kernel, I would prefer to keep this flag specific to LTO. I do see the value in having a similar option for all object files, but perhaps that can be a separate option?

Don't apply to isUsedInRegularObj symbols.

Thanks, LGTM. @pcc, does this version look fine to you?

In D138337#4080628, @nickdesaulniers wrote:

If you're referring to ibt-seal, I assume a similar optimization could be added for BTI too.

Yes, please. Does it make sense to add to this patch, or a follow up on top?

In D138337#4080615, @nickdesaulniers wrote:

The test is for x86, does this also work for aarch64 with BTI?

In D138337#4078843, @nickdesaulniers wrote:

Is https://reviews.llvm.org/D142163 a blocker for this?

In D142163#4067630, @pcc wrote:

What do you think about making this apply not only to LTO but to other -r links as well?

In D139395#4067391, @pcc wrote:

I discussed this out of band with Ramon and we agreed that the new option should be marked as experimental because the rustc implementation is not yet finalized. I think that the criteria for removing the experimental marking should be that there is a full implementation of integer normalization for Rust (or some other language) that has been tested against a large codebase that uses FFI.

@pcc, @MaskRay, based on earlier discussions (e.g. https://github.com/ClangBuiltLinux/linux/issues/1737), does something like this look like a reasonable approach?

Thanks for the patch, Ramon. This looks like a reasonable approach to me, and just for reference, here appears to be the corresponding rustc change:

In D141444#4042006, @MaskRay wrote:

Ensuring that every instrumentation works with kcfi is a lot of work on the compiler side (think that another instrumentation technique comes up, we need to update O(n) existing instrumentation passes) and I don't quite agree that it's compiler's responsibility.
But if sanitizers and gcov are all, I think it's fine...

Addressed comments.

Addressed feedback.

I agree, it's probably best to temporarily revert this until Joao has time to address the issues you mentioned. The Linux kernel doesn't use -mibt-seal yet, so dropping the feature for now shouldn't be a problem.

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.

In D140035#3995703, @joaomoreira wrote:

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

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

In D138337#3972493, @pcc wrote:

Isn't that just a bug in the optimization? It shouldn't be applying this to symbols where VisibleToRegularObj is set.

In D138337#3972009, @pcc wrote:

Can't this be implicit if LTO is being used?

Really fix the indentation this time...

Addressed feedback.

Added a test.

Switched to [[#]].

Addressed feedback.

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