naked functions: emit .private_extern on macos

[folkertdev]

fixes #148307

Emit .private_extern on macos when the naked function uses Linkage::Internal. Failing to do so can cause issues with LTO.

The documentation on this directive is kind of sparse, but I believe this is at least not incorrect, and does fix the issue.

r? @Amanieu
cc @bjorn3

[rustbot]

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

[bjorn3]

Is this really only an issue on macOS? I would have assumes this is an issue on all platforms.

[folkertdev]

The reproduction given in the issue builds successfully for me on linux.

I did just notice that we do already have

rust/compiler/rustc_codegen_ssa/src/mir/naked_asm.rs

Lines 245 to 248 in 73e6c9e

	match item_data.visibility {
	Visibility::Default | Visibility::Protected => {}
	Visibility::Hidden => writeln!(begin, ".private_extern {asm_name}").unwrap(),
	}

That clearly did not actually trigger in this case. Is that right?

[bjorn3]

Yeah, the issue here I believe is that rustc correctly computes the symbol visibility for the naked function as private, but then when ThinLTO decides to inline the caller of the naked function, it is unable to change the naked function to be public (+ add a .llvm.<somehash> suffix to the symbol name to prevent conflicts) like it would do for regular functions. So we have to mark all naked functions as visible within the same DSO rather than just within the same object file, aka use external linkage + hidden visibility on ELF and equivalent with other object file formats.

[folkertdev]

aka use external linkage + hidden visibility on ELF and equivalent with other object file formats.

Hmm, we do emit the following:

rust/compiler/rustc_middle/src/mir/mono.rs

Lines 151 to 159 in 6a884ad

	// If the function is #[naked] or contains any other attribute that requires exactly-once
	// instantiation:
	// We emit an unused_attributes lint for this case, which should be kept in sync if possible.
	let codegen_fn_attrs = tcx.codegen_instance_attrs(instance.def);
	if codegen_fn_attrs.contains_extern_indicator()
	|| codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::NAKED)
	{
	return InstantiationMode::GloballyShared { may_conflict: false };
	}

but apparently that is not enough.

Is compiler/rustc_monomorphize/src/partitioning.rs the right place to fix this?

[bjorn3]

That gets overridden at

rust/compiler/rustc_monomorphize/src/partitioning.rs

Lines 583 to 586 in 6a884ad

	// If we got here, we did not find any uses from other CGUs, so
	// it's fine to make this monomorphization internal.
	data.linkage = Linkage::Internal;
	data.visibility = Visibility::Default;

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[folkertdev]

is this too strict? We could allow it, but I think this is only reachable now with an explicit linkage attribute (which is unstable).

[Amanieu]

I think the issue with inlining is bigger than that and we effectively have to always use global visibility for naked functions. Consider dylib crates where the naked function is private but is called by an inlinable function. If that function gets inlined into another crate then the naked function needs to have global visibility so that it can be referenced from another dylib.

[bjorn3]

Consider dylib crates where the naked function is private but is called by an inlinable function. If that function gets inlined into another crate then the naked function needs to have global visibility so that it can be referenced from another dylib.

If rustc considers the caller to be cross-crate codegenable, it would already mark the callee as public be it a regular function be it a naked function. The problematic case is where the codegen backend decides to inline a function into another codegen unit when rustc didn't consider it cross-crate codegenable. This can only happen within a single dylib through ThinLTO, not between dylibs. And it is only an issue for naked functions as ThinLTO can't make the function public itself as it would do for regular functions.