-
-
Notifications
You must be signed in to change notification settings - Fork 14.5k
naked functions: emit .private_extern on macos
#148339
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -580,6 +580,16 @@ fn internalize_symbols<'tcx>( | |
| } | ||
| } | ||
|
|
||
| // When LTO inlines the caller of a naked function, it will attempt but fail to make the | ||
| // naked function symbol visible. To ensure that LTO works correctly, do not default | ||
| // naked functions to internal linkage and default visibility. | ||
| if let MonoItem::Fn(instance) = item { | ||
| let flags = cx.tcx.codegen_instance_attrs(instance.def).flags; | ||
| if flags.contains(CodegenFnAttrFlags::NAKED) { | ||
| continue; | ||
| } | ||
| } | ||
|
Comment on lines
+583
to
+591
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bjorn3 why would we handle naked functions differently from some other external symbol defined in global assembly? extern "C" {
fn foo();
}
global_asm!(
"foo:", "ret"
);is it just that nobody ran into this issue with global assembly so far? In any case this fix is causing issues elsewhere, see #151946
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. global_asm!(
"foo:", "ret"
);this defines a local symbol, which means that only if |
||
|
|
||
| // 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; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| //@ revisions: macos-x86 macos-aarch64 linux-x86 | ||
| //@ add-minicore | ||
| //@ assembly-output: emit-asm | ||
| // | ||
| //@[macos-aarch64] compile-flags: --target aarch64-apple-darwin | ||
| //@[macos-aarch64] needs-llvm-components: aarch64 | ||
| // | ||
| //@[macos-x86] compile-flags: --target x86_64-apple-darwin | ||
| //@[macos-x86] needs-llvm-components: x86 | ||
| // | ||
| //@[linux-x86] compile-flags: --target x86_64-unknown-linux-gnu | ||
| //@[linux-x86] needs-llvm-components: x86 | ||
|
|
||
| #![crate_type = "lib"] | ||
| #![feature(no_core, asm_experimental_arch)] | ||
| #![no_core] | ||
|
|
||
| // 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`. | ||
| // | ||
| // Without this directive, LTO may fail because the symbol is not visible. | ||
| // See also https://github.com/rust-lang/rust/issues/148307. | ||
|
|
||
| extern crate minicore; | ||
| use minicore::*; | ||
|
|
||
| // CHECK: .p2align 2 | ||
| // macos-x86,macos-aarch64: .private_extern | ||
| // linux-x86: .globl | ||
| // linux-x86: .hidden | ||
| // CHECK: ret | ||
| #[unsafe(naked)] | ||
| extern "C" fn ret() { | ||
| naked_asm!("ret") | ||
| } | ||
|
|
||
| // CHECK-LABEL: entry | ||
| #[no_mangle] | ||
| pub fn entry() { | ||
| ret() | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this too strict? We could allow it, but I think this is only reachable now with an explicit linkage attribute (which is unstable).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fine for now. We can always revise it later if it becomes a problem.