-
Notifications
You must be signed in to change notification settings - Fork 502
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
naked functions #1689
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.
also maybe make links work?
Co-authored-by: bjorn3 <17426603+bjorn3@users.noreply.github.com>
5deb3b5
to
54695c9
Compare
Co-authored-by: Laine Taffin Altman <alexanderaltman@me.com>
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.
cc @bstrie @traviscross @Amanieu @bjorn3