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]

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.

[rustbot]

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

[rustbot]

Some changes occurred in compiler/rustc_codegen_ssa/src/codegen_attrs.rs

cc @jdonszelmann

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

[folkertdev]

I've made an attempt at implementing the codegen_instance_attrs query. It does work (locally), but I think it's not quite right yet.

I've also updated the locations where an instance is easily available. That is generally the case in the backend, but in earlier stages it won't be. On the other hand, having codegen_fn_attrs available means it's likely to be used by accident. So either it needs a scarier name, or we do translate all occurences to use InstanceKind::Item, but that'll be quite verbose.

Anyway, my thinking is that resolving that can be its own PR, and here we can focus just on naked functions and some of the low-hanging fruit.

[folkertdev]

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

The only one that's still missing is here:

rust/compiler/rustc_mir_transform/src/coverage/query.rs

Lines 45 to 48 in f51c987

	if tcx.codegen_fn_attrs(def_id).flags.contains(CodegenFnAttrFlags::NAKED) {
	trace!("InstrumentCoverage skipped for {def_id:?} (`#[naked]`)");
	return false;
	}

Where I could make up a InstanceKind::Item but I suspect that is incorrect.

[bjorn3]

Maybe codegen_instance_attrs could be a regular function that calls the codegen_fn_attrs query and drops the naked flag (and any other flags like no_mangle) as necessary depending on the instance kind?

[folkertdev]

For this PR that is probably a nicer solution actually yeah.

[folkertdev]

A downside of that approach is that function has to clone the CodegenFnAttrs in order to modify it (the query method returns an immutable reference). I'm not sure how big of a deal that is though?

[bjorn3]

A Cow<'tcx, CodegenFnAttrs> could be returned I guess. So only when we actually need to modify the arguments is it necessary to clone it.