-
Notifications
You must be signed in to change notification settings - Fork 531
naked functions #1689
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: master
Are you sure you want to change the base?
naked functions #1689
Conversation
src/inline-assembly.md
Outdated
- The compiler cannot assume that the instructions in the asm are the ones that will actually end up executed. | ||
- This effectively means that the compiler must treat the `naked_asm!` as a black box and only take the interface specification into account, not the instructions themselves. | ||
- Runtime code patching is allowed, via target-specific mechanisms. | ||
- However there is no guarantee that each `naked_asm!` directly corresponds to a single instance of instructions in the object file: the compiler is free to duplicate or deduplicate `naked_asm!` blocks. |
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.
This is not true, naked functions cannot be duplicated/merged, unlike asm!
.
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.
and reading this over, in this case it seems important that we do actually guarantee (and hence the compiler can assume) that the instructions in the naked_asm!
invocation are exactly the ones that will be executed. or is that too strict?
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 that poses problems on SPIR-V or NVPTX targets. I seem to recall that the loader just inlines the functions at the Shader Assembly level.
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.
@chorman0773 just to be sure, you're referring to Amanieu's comment right?
What we do in rustc (or well, we will once that PR makes it through the queue) is that a naked function
#[naked]
extern "C" fn foo() {}
is emitted as something similar to
core::arch::global_asm!(
"foo:",
"ret"
);
extern "C" {
fn foo();
}
and we're relying on the compiler (rustc, llvm) not duplicating that bit of global assembly. I suspect we already rely on that in general, because global assembly can define symbols today.
If you still think this is problematic, how could we verify the behavior of this new codegen strategy for the targets you list?
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.
and reading this over, in this case it seems important that we do actually guarantee (and hence the compiler can assume) that the instructions in the
naked_asm!
invocation are exactly the ones that will be executed. or is that too strict?
No, they may not be the same instructions that are executed since you can still do runtime patching. We only guarantee that assembly code only appears once in the object file for the purposes of symbols.
src/attributes/codegen.md
Outdated
r[attributes.codegen.naked.no-duplication] | ||
The asm code may not be duplicated by the compiler. | ||
This property is important for naked functions that define symbols in the assembly code. |
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.
so, something I just thought of: we do duplicate the assembly when monomorphizing generic naked functions. So this phrasing should be a bit more specific I think.
5deb3b5
to
54695c9
Compare
This comment has been minimized.
This comment has been minimized.
b2ee83c
to
1a8abe7
Compare
This comment has been minimized.
This comment has been minimized.
1a8abe7
to
7fdadec
Compare
This comment has been minimized.
This comment has been minimized.
d1a523d
to
1caf564
Compare
f92f75f
to
54ddde1
Compare
src/inline-assembly.md
Outdated
# #[cfg(target_arch = "x86_64")] { | ||
#[naked] | ||
extern "C-unwind" fn naked_function() { | ||
unsafe { | ||
core::arch::naked_asm!( | ||
".cfi_startproc", | ||
"push rbp", | ||
".cfi_def_cfa_offset 16", | ||
".cfi_offset rbp, -16", | ||
"mov rbp, rsp", | ||
".cfi_def_cfa_register rbp", | ||
"", | ||
"call {function}", | ||
"", | ||
"pop rbp", | ||
".cfi_def_cfa rsp, 8", | ||
"ret", | ||
".cfi_endproc", | ||
function = sym function_that_panics, | ||
) | ||
} | ||
} | ||
|
||
extern "C-unwind" fn function_that_panics() { | ||
panic!("unwind!"); | ||
} | ||
# } |
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 don't think I can currently run this because #[naked]
is still unstable. So reminder to self: update this before merging.
cc @traviscross
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.
We won't merge this anyway until the stabilization lands, so you can just update it and leave the CI red until then (add the feature flag and test it locally though). That's what we normally do.
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.
Locally I still even get errors like this
Couldn't compile the test.
---- inline-assembly.md - Inline_assembly::Options (line 1360) stdout ----
error[E0658]: label operands for inline assembly are unstable
--> inline-assembly.md:1364:32
|
6 | core::arch::asm!("jmp {}", label {
| ________________________________^
7 | | println!();
8 | | }, options(noreturn));
| |_____^
|
= note: see issue #119364 <https://github.com/rust-lang/rust/issues/119364> for more information
with these versions in the reference
folder (so I don't think there is a rustup override active)
> cargo --version
cargo 1.88.0-nightly (d811228b1 2025-04-15)
> rustc --version
rustc 1.88.0-nightly (191df20fc 2025-04-18)
That feature has been stable on nightly for a while, so I must be missing something. Anyway, I believe the current code to be correct, and CI will catch it before merge if there are minor issues.
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've had trouble with overrides too on the Reference for reasons I haven't yet worked out. Perhaps if you rustup default nightly
(or rustup default stage1
or whatnot) it will work for you.
src/attributes/codegen.md
Outdated
The *`naked` [attribute]* prevents the compiler from emitting a function prologue and | ||
epilogue for the attributed function. |
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.
We're unwrapping lines on new things added, so go ahead and unwrap these all if you would.
This comment has been minimized.
This comment has been minimized.
54ddde1
to
81ad95f
Compare
also maybe make links work?
Co-authored-by: bjorn3 <[email protected]>
Co-authored-by: Laine Taffin Altman <[email protected]>
this only applies to `asm!` (even though the rule has been removed there since copying it for naked asm
81ad95f
to
c4d68c9
Compare
No function prologue or epilogue are generated for the attributed function: the contents of the `naked_asm!` invocation make up the full body of a naked function. | ||
|
||
r[attributes.codegen.naked.unsafe-attribute] | ||
The `naked` attribute is an [unsafe attribute]. Annotating a function with `#[unsafe(naked)]` comes with the safety obligation that the body respects the function's calling convention, and that the body either returns or diverges. |
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.
...and that the body either returns or diverges
Is it correct that this should be removed since we've determined that unwinding is OK?
tracking issue: rust-lang/rust#90957
earlier version: #1153
Pending on rust-lang/rust#128004. Once merged, we'll move for stabliziation.
I'm working off of the original PR to the reference, written 2+ years ago when the reference was less strict. I suspect some refinement and editing will be needed here to satisfy the quality requirements of the spec, and update the text to accurately reflect the current state and the exact guarantees/requirements.
Stabilization PR:
naked_functions
rust#134213cc @bstrie @traviscross @Amanieu @bjorn3