fix -Zsanitizer=kcfi on #[naked] functions

[folkertdev]

fixes #143266

With -Zsanitizer=kcfi, indirect calls happen via generated intermediate shim that forwards the call. The generated shim preserves the attributes of the original, including #[unsafe(naked)]. The shim is not a naked function though, and violates its invariants (like having a body that consists of a single naked_asm! call).

My fix here is to match on the InstanceKind, and only use codegen_naked_asm when the instance is not a ReifyShim. That does beg the question whether there are other InstanceKinds that could come up. As far as I can tell the answer is no: calling via dyn seems to work find, and #[track_caller] is disallowed in combination with #[naked].

r? codegen
@rustbot label +A-naked
cc @maurer @rcvalle

[rustbot]

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

Some changes occurred in tests/codegen/sanitizer

cc @rcvalle

[compiler-errors]

reify shims are not the only way to trigger this ICE. For example, vtable shims will trigger this too:

use std::arch::naked_asm;

trait MyTrait {
    #[unsafe(naked)]
    extern "C" fn foo(&self) {
        naked_asm!("ret")
    }
}

impl MyTrait for i32 {}

fn main() {
    let x: extern "C" fn(&_) = <dyn MyTrait as MyTrait>::foo;
    x(&1);
}

I think this check should be limited to InstanceKind::Item, like:

if let InstanceKind::Item(def_id) = instance.kind
    && cx.tcx().codegen_fn_attrs(instance.def_id()).flags.contains(NAKED)
{}

Rather than filtering out shims explicitly.

[compiler-errors]

r? compiler-errors

[compiler-errors]

Please also grep for other instances of .contains(CodegenFnAttrFlags::NAKED) in the codebase. They may need to be adjusted similarly.

[compiler-errors]

This also feels like it deserves a larger refactor, though I wouldn't block this fix on that: I think the codegen_fn_attrs query being keyed on DefId is a bit of a footgun. Although two InstanceKinds may correspond to the same def id, the shims that some of them generate sometimes don't have the same characteristics. Like in this case, both the vtable shim and reify shims should probably not be inheriting the naked attr.

[compiler-errors]

(edit: the code I shared above actually is just a non-CFI instance of a reify shim, not a vtable shim, but I think that the point still remains that we may add new shim InstanceKinds so codegen attrs being keyed on def id is still a footgun)

[maurer]

Came here to post that I think we should have a codegen_instance_attrs that takes an instance instead of a DefId, that way we could have a different "effective" set of attributes compared to the real ones. I think that's what @compiler-errors has already suggested though.

If we made a codegen_instance_attrs, we could then see if it's easy to migrate codegen_fn_attrs calls that get tested for NAKED to the new method.

Other attributes that might make sense to have an effective flag strip for some instance kinds include NO_MANGLE, USED_COMPILER, USED_LINKER.

[compiler-errors]

I think that's what @compiler-errors has already suggested though.

Yeah, I agree.

I think it's worth to just migrate all codegen_fn_attrs calls. All existing calls (that care about the item itself) would just use InstanceKind::Item(def_id), or they'd migrate to using the instance itself if it has one at hand.

And it should probably take InstanceKind, not Instance; there's probably no reason for it to need to have its args included.