Set hidden visibility on naked functions in compiler-builtins

[zmodem]

88b4646 made builtin functions hidden, but it doesn't apply to naked functions, which are generated through a different code path.

This was discovered in #151486 where aarch64 outline atomics showed up in shared objects, overriding the symbols from compiler-rt.

[rustbot]

r? @davidtwco

rustbot has assigned @davidtwco.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[zmodem]

r? @tgross35

Also @bjorn3 and @folkertdev from rust-lang/compiler-builtins#349 (comment)

I wasn't sure how to test this. Looking at the non-naked case for inspiration didn't help, since that doesn't seem tested either :-) I did verify locally that the symbols in libcompiler_builtins_compiler_builtins.rlib do get hidden.

[rustbot]

tgross35 is currently at their maximum review capacity.
They may take a while to respond.

[folkertdev]

We have something to figure out here. Since #148339 the visibility (in LLVM IR) is actually hidden, but the assembly itself does not reflect that. So there is some sort of mismatch.

A special-case for compiler-builtins seems a little hacky to me, though it is special of course. Can we do this in a more principled way?

[zmodem]

Since #148339 the visibility (in LLVM IR) is actually hidden, but the assembly itself does not reflect that. So there is some sort of mismatch.

I'm not sure where it happens in the code, but based on this comment:

rust/tests/assembly-llvm/naked-functions/hidden.rs

Lines 18 to 19 in f60a0f1

	// Tests that naked functions that are not externally linked (e.g. via `no_mangle`)
	// are marked as `Visibility::Hidden` and emit `.private_extern` or `.hidden`.

it sounds intentional that the "hidden" part does not apply to no_mangle functions, like the compiler-builtins.

A special-case for compiler-builtins seems a little hacky to me, though it is special of course. Can we do this in a more principled way?

Agreed. It's a continuation of 88b4646.

I think the ultimate principled way would be to have a function attribute for controlling visibility (#151425) and use it on the builtins.

[folkertdev]

Right, I'd rather wait for that attribute I think, assuming that has a decent chance of making it in. If it's an urgent problem for compiler-builtins we could ask T-lang to bump its priority.

[bjorn3]

I think the ultimate principled way would be to have a function attribute for controlling visibility (#151425) and use it on the builtins.

This would not help for compiler-builtins. The special case for compiler-builtins needs to apply to all functions codegened as part of the compiler-builtins rlib, not just #[no_mangle] functions. This because unlike every other crate, compiler-builtins is duplicated between dylibs. This means that it is not allowed to export any symbol from said dylib, which is done by applying hidden visibility unconditionally while codegening for the compiler-builtins crate. #151425 would only apply to #[no_mangle]functions, with rustc using default visibility for all other functions (unless-Zdefault-visibilityis used, but-Zdefault-visibility=hidden` did have to disallow linking the crate into a rust dylib, which is too restrictive for compiler-builtins)

IMO copying the same logic as we already have for regular functions like this PR does is the right approach.

[tgross35]

I think this is probably the correct approach if we're doing it for regular functions, but

r? @saethlin

is probably the most familiar with these details and how this should be tested.

There is also a check for symbol properties in compiler-builtins https://github.com/rust-lang/compiler-builtins/blob/0e5f78cd1963b89bf0b715c810919917e3fdce6b/crates/symbol-check/src/main.rs. If you're able, would you mind also submitting a PR there adding another verify_* function to check this to make sure we don't run into other edge cases?

[rustbot]

compiler-builtins is developed in its own repository. If possible, consider making this change to rust-lang/compiler-builtins instead.

cc @tgross35

[zmodem]

Sorry, git had me confused for a bit.

If you're able, would you mind also submitting a PR there adding another verify_* function to check this to make sure we don't run into other edge cases?

Thanks for the pointer! That seems like a good way to test this. I've sent rust-lang/compiler-builtins#1073

[zmodem]

I've updated the PR to handle statics as well, as symbol-check discovered that RSQRT_TAB has default visibility. Or maybe it would be better to do that in a separate PR?