codegen #[naked] functions using global asm
#128004
Conversation
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
|
On the tracking issue a The changes in this PR seem like good direction. |
|
I think that separately from any internal implementation change, the surface syntax of rust should use |
|
I agree that it is still a good idea to add But that is separate from how the codegen works, which is what this PR is for. |
| if let Visibility::Hidden = item_data.visibility { | ||
| writeln!(begin, ".hidden {asm_name}").unwrap(); | ||
| } |
There was a problem hiding this comment.
I've not been able to actually generate code that triggers this if. Visibility just seems to always be Default. So this is entirely untested at the moment.
There was a problem hiding this comment.
For anything like this, drop a question on Zulip https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp. Unfortunately that enum doesn't seem to be very well documented
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| } | ||
|
|
||
| fn inline_to_global_operand<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>( | ||
| cx: &'a Bx::CodegenCx, |
There was a problem hiding this comment.
This function can probably just take TyCtxt.
| cx.tcx(), | ||
| value.span, | ||
| const_value, | ||
| cx.layout_of(value.ty()), |
There was a problem hiding this comment.
Ah, this layout_of call uses the CodegenCx. This is during codegen though, so RevealAllLayoutCx (a wrapper around TyCtxt) is enough: https://github.com/rust-lang/rustc_codegen_cranelift/blob/b70ad2defd4bb5fba6af7958893e22be0f33dfdd/src/common.rs#L450-L518 Maybe it should be uplifted out of cg_clif?
There was a problem hiding this comment.
seems useful, should that be part of this PR though?
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
This comment has been minimized.
This comment has been minimized.
folkertdev
force-pushed
the
naked-fn-asm
branch
2 times, most recently
from
f54a458 to
6783e26
Compare
|
☔ The latest upstream changes (presumably #128298) made this pull request unmergeable. Please resolve the merge conflicts. |
|
☔ The latest upstream changes (presumably #125443) made this pull request unmergeable. Please resolve the merge conflicts. |
bors
added
S-waiting-on-bors
codegen `#[naked]` functions using global asm tracking issue: rust-lang#90957 Fixes rust-lang#124375 This implements the approach suggested in the tracking issue: use the existing global assembly infrastructure to emit the body of `#[naked]` functions. The main advantage is that we now have full control over what gets generated, and are no longer dependent on LLVM not sneakily messing with our output (inlining, adding extra instructions, etc). I discussed this approach with `@Amanieu` and while I think the general direction is correct, there is probably a bunch of stuff that needs to change or move around here. I'll leave some inline comments on things that I'm not sure about. Combined with rust-lang#127853, if both accepted, I think that resolves all steps from the tracking issue. r? `@Amanieu`
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
@bors retry For reference, the |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Question: My understanding of global_asm is that it can't be culled during linking, even if the global_asm is never called. Does |
|
Finished benchmarking commit (1daec06): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (primary 1.2%, secondary -4.0%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (secondary 3.2%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 771.021s -> 769.999s (-0.13%) |
That's not true, global_asm absolutely can be culled during linking. It usually isn't because it emits to |
|
This breaks use of CFI instructions in unwinding crate: nbdd0121/unwinding#41 |
|
@bjorn3 is this related to the We certainly can emit those directives, but I don't know whether we should, or leave it to the assembly author. |
|
These directives need to be provided by the assembly author. It is up to them to declare that the naked function has valid unwind info. |
|
Before this change the CFI instructions are emitted by LLVM and it's illegal to have EDIT: This was the behaviour for previous Rust compiler and C compilers: https://godbolt.org/z/qff8hjE8e |
|
The previous behavior was actually context-dependent: the compiler may or may not insert This makes writing code that uses CFI directives quite brittle and it's a problem I've hit before in libfringe (the predecessor of corosensei). It's much better to let users provide |
tracking issue: #90957
Fixes #124375
This implements the approach suggested in the tracking issue: use the existing global assembly infrastructure to emit the body of
#[naked]functions. The main advantage is that we now have full control over what gets generated, and are no longer dependent on LLVM not sneakily messing with our output (inlining, adding extra instructions, etc).I discussed this approach with @Amanieu and while I think the general direction is correct, there is probably a bunch of stuff that needs to change or move around here. I'll leave some inline comments on things that I'm not sure about.
Combined with #127853, if both accepted, I think that resolves all steps from the tracking issue.
r? @Amanieu