-
-
Notifications
You must be signed in to change notification settings - Fork 14.7k
naked functions: respect function-sections
#147811
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
base: main
Are you sure you want to change the base?
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 |
|---|---|---|
|
|
@@ -127,6 +127,8 @@ fn prefix_and_suffix<'tcx>( | |
|
|
||
| let is_arm = tcx.sess.target.arch == Arch::Arm; | ||
| let is_thumb = tcx.sess.unstable_target_features.contains(&sym::thumb_mode); | ||
| let function_sections = | ||
| tcx.sess.opts.unstable_opts.function_sections.unwrap_or(tcx.sess.target.function_sections); | ||
|
|
||
| let attrs = tcx.codegen_instance_attrs(instance.def); | ||
| let link_section = attrs.link_section.map(|symbol| symbol.as_str().to_string()); | ||
|
|
@@ -201,8 +203,6 @@ fn prefix_and_suffix<'tcx>( | |
| let mut end = String::new(); | ||
| match asm_binary_format { | ||
| BinaryFormat::Elf => { | ||
| let section = link_section.unwrap_or_else(|| format!(".text.{asm_name}")); | ||
|
|
||
| let progbits = match is_arm { | ||
| true => "%progbits", | ||
| false => "@progbits", | ||
|
|
@@ -213,7 +213,11 @@ fn prefix_and_suffix<'tcx>( | |
| false => "@function", | ||
| }; | ||
|
|
||
| writeln!(begin, ".pushsection {section},\"ax\", {progbits}").unwrap(); | ||
| if let Some(section) = &link_section { | ||
| writeln!(begin, ".pushsection {section},\"ax\", {progbits}").unwrap(); | ||
| } else if function_sections { | ||
| writeln!(begin, ".pushsection .text.{asm_name},\"ax\", {progbits}").unwrap(); | ||
| } | ||
| writeln!(begin, ".balign {align_bytes}").unwrap(); | ||
| write_linkage(&mut begin).unwrap(); | ||
| match item_data.visibility { | ||
|
|
@@ -232,14 +236,18 @@ fn prefix_and_suffix<'tcx>( | |
| // pattern match on assembly generated by LLVM. | ||
| writeln!(end, ".Lfunc_end_{asm_name}:").unwrap(); | ||
| writeln!(end, ".size {asm_name}, . - {asm_name}").unwrap(); | ||
| writeln!(end, ".popsection").unwrap(); | ||
| if link_section.is_some() || function_sections { | ||
| writeln!(end, ".popsection").unwrap(); | ||
| } | ||
| if !arch_suffix.is_empty() { | ||
| writeln!(end, "{}", arch_suffix).unwrap(); | ||
| } | ||
| } | ||
| BinaryFormat::MachO => { | ||
| let section = link_section.unwrap_or_else(|| "__TEXT,__text".to_string()); | ||
| writeln!(begin, ".pushsection {},regular,pure_instructions", section).unwrap(); | ||
| // NOTE: LLVM ignores `-Zfunction-sections` on macos. | ||
| if let Some(section) = &link_section { | ||
| writeln!(begin, ".pushsection {section},regular,pure_instructions").unwrap(); | ||
| } | ||
| writeln!(begin, ".balign {align_bytes}").unwrap(); | ||
| write_linkage(&mut begin).unwrap(); | ||
| match item_data.visibility { | ||
|
|
@@ -250,14 +258,19 @@ fn prefix_and_suffix<'tcx>( | |
|
|
||
| writeln!(end).unwrap(); | ||
| writeln!(end, ".Lfunc_end_{asm_name}:").unwrap(); | ||
| writeln!(end, ".popsection").unwrap(); | ||
| if link_section.is_some() { | ||
| writeln!(end, ".popsection").unwrap(); | ||
| } | ||
| if !arch_suffix.is_empty() { | ||
| writeln!(end, "{}", arch_suffix).unwrap(); | ||
| } | ||
| } | ||
| BinaryFormat::Coff => { | ||
| let section = link_section.unwrap_or_else(|| format!(".text.{asm_name}")); | ||
| writeln!(begin, ".pushsection {},\"xr\"", section).unwrap(); | ||
| if let Some(section) = &link_section { | ||
| writeln!(begin, ".pushsection {section},\"xr\"").unwrap() | ||
| } else if function_sections { | ||
| writeln!(begin, ".pushsection .text${asm_name},\"xr\"").unwrap() | ||
|
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. On -msvc targets, function_sections is ignored.
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. Also LLVM currently generates the following line on COFF targets: Should we be doing the same here?
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.
Meaning that it's just always assumed to be on? https://godbolt.org/z/a6ofW49YK
It should still work for msvc, no?
I don't think we can reliably emit the
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. Actually using
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. No, it's always assumed to be off on msvc. You can see it always uses .text.
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. It does always use i.e. it uses a subsection which, as far as I understand, does allow DCE, with So then DCE is impossible
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. The I just don't see how we can generate the unique ID in a reliable way. Also because we would generate a function per section, the function name (which should be unique?) should be sufficient to disambiguate the sections. In short, I think the implementation in this PR is the best we can reliably do. |
||
| } | ||
| writeln!(begin, ".balign {align_bytes}").unwrap(); | ||
| write_linkage(&mut begin).unwrap(); | ||
| writeln!(begin, ".def {asm_name}").unwrap(); | ||
|
|
@@ -268,7 +281,9 @@ fn prefix_and_suffix<'tcx>( | |
|
|
||
| writeln!(end).unwrap(); | ||
| writeln!(end, ".Lfunc_end_{asm_name}:").unwrap(); | ||
| writeln!(end, ".popsection").unwrap(); | ||
| if link_section.is_some() || function_sections { | ||
| writeln!(end, ".popsection").unwrap(); | ||
| } | ||
| if !arch_suffix.is_empty() { | ||
| writeln!(end, "{}", arch_suffix).unwrap(); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| use std::arch::naked_asm; | ||
|
|
||
| #[unsafe(naked)] | ||
| #[no_mangle] | ||
| extern "C" fn used() { | ||
| naked_asm!("ret") | ||
| } | ||
|
|
||
| #[unsafe(naked)] | ||
| #[no_mangle] | ||
| extern "C" fn unused() { | ||
| naked_asm!("ret") | ||
| } | ||
|
|
||
| #[unsafe(naked)] | ||
| #[link_section = "foobar"] | ||
| #[no_mangle] | ||
| extern "C" fn unused_link_section() { | ||
| naked_asm!("ret") | ||
| } | ||
|
Comment on lines
+9
to
+20
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 does it make sense to you that these functions are not removed at link time on windows? They are not present on linux at least, but with windows they are. So either we're missing something or the windows linker just keeps these functions despite them being unreachable.
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. I bet they'd get removed before the changes in this PR. Since function sections are disabled for windows-gnu, beginning with this PR they are no longer output in separate sections, so ld.bfd doesn't GC them.
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. In that case I'd expect I'm using cross-compilation here so maybe the issue is there? But it looks like unused sections just aren't removed. The same is true when I use normal (i.e. non-naked) functions.
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. You are right they aren't removed fully. Cross-compilation should be irrelevant. Before this PR or with With this PR but without Linking the same code with LLD ( In fact LLD didn't even discard
With GNU ld yes, but with LLD the normal function gets removed. If I add: #[no_mangle]
extern "C" fn unused_regular() {}
DetailsQuite a mess nonetheless.
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. Thanks for that detective work!
So, that is different from the Regardless, what should we test for windows here. Like obviously we could ignore it entirely, that would be simplest. Alternatively we could add regular function equivalents and assert that the naked function is treated the same as a non-naked function?
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.
Correct.
COMDAT usage for regular function is the most striking difference to me: With C example: ❯ cat main.c
void used() {}
void unused() {}
int main() { used(); return 0; }build with Once again this seems to boil down to Perhaps memory fails me but I think GNU ld used to remove such symbols in the past. Anyway for this PR this solution seems nice if it doesn't add too much work:
CI only tests regular x86_64-pc-windows-gnu which gives us no function sections and GNU ld as the linker, so in both cases the symbols will be undefined (assuming that the older ld installed on CI also doesn't get rid of the symbol). |
||
|
|
||
| fn main() { | ||
| used(); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| //@ needs-asm-support | ||
|
|
||
| use run_make_support::symbols::object_contains_any_symbol; | ||
| use run_make_support::{bin_name, rustc}; | ||
|
|
||
| fn main() { | ||
| rustc().input("main.rs").opt().run(); | ||
| let mut unused = vec!["unused", "unused_link_section"]; | ||
| assert!(!object_contains_any_symbol(bin_name("main"), &unused)); | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.