Stabilize naked_functions
#134213
Stabilize naked_functions
#134213
Conversation
rustbot
added
A-run-make
|
Some changes occurred in src/doc/unstable-book/src/compiler-flags/sanitizer.md cc @rust-lang/project-exploit-mitigations, @rcvalle This PR modifies cc @jieyouxu rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead. cc @rust-lang/rust-analyzer Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 |
traviscross
added
T-lang
|
☔ The latest upstream changes (presumably #134395) made this pull request unmergeable. Please resolve the merge conflicts. |
folkertdev
force-pushed
the
stabilize-naked-functions
branch
from
6183807 to
ed0d0b9
Compare
folkertdev
force-pushed
the
stabilize-naked-functions
branch
from
ed0d0b9 to
ae2fa18
Compare
tgross35
added
the
T-libs-api
|
r? lang |
|
This probably needs a new lang FCP since the old one is probably outdated (the implementation of naked function has changed signficantly). |
|
Thanks for the thorough report @folkertdev! @rfcbot fcp merge |
|
Team member @tmandry has proposed to merge this. The next step is review by the rest of the tagged team members: Concerns:
Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns. |
rfcbot
added
proposed-final-comment-period
|
Actually, @rust-lang/libs-api does this need your FCP? I think the path of |
|
RFC 2972 specifies that naked functions:
This doesn't seem to be required in nightly and isn't mentioned in the stabilization report. What's the story here? In addition to it not be required, it's not even allowed. Why is that? #![feature(naked_functions)]
#[unsafe(no_mangle)]
pub extern "sysv64" fn g() -> ! {
loop {}
}
#[naked]
pub extern "sysv64" fn f() -> ! {
unsafe {
core::arch::naked_asm!(
"jmp {}",
"ud2",
sym g,
options(noreturn), //~ ERROR
)
}
}
fn main() {
f();
}@rfcbot concern question-about-noreturn |
|
@rfcbot concern question-about-rust-abi |
|
@rfcbot concern mention-nop-padding-and-ud2 There was a long discussion over in the tracking issue about nop padding and the emission of |
|
I don't really see any benefit to The lint could be deny-by-default but that probably doesn't gain much. |
Interesting. Probably we should update the Reference PR then, and it'd be good for that to have an example of what it looks like to do this correctly. |
This was decided to be implicit in #128651, when it was switched from |
|
Thanks for the detailed notes! I'll update the stabilization reports with notes on Something that I think should also be brought up here is the handling of target features. The current implementation silently ignores target features on but
So, I'm not exactly sure yet what to do with all that yet. I think we can make it work well for the targets where One option is to split out target feature support and have that remain unstable on naked functions for the time being? Users of naked functions are writing assembly anyway, so adding the required assembler directives likely is something they can live with. |
|
About |
|
Thanks for raising that question about @rfcbot concern what-to-do-for-target-feature About WASM here... |
|
For Wasm, IIRC naked functions require the use of inline assembly which is already unstable on WebAssembly anyway, so this stability wouldn't end up affecting wasm would it? Regardless though my guess is that the wasm assembler in LLVM doesn't have stateful features like other architectures do. In that sense I (a) don't know enough about the assembler which is another reason to keep it unstable on wasm, which I think it already is, and (b) I suspect this probably isn't an issue for wasm and if it ever were we could add similar directives as other native platforms too. |
|
I used wasm just as an example of a target for which we can't set the target features in global inline assembly. There are also some platforms where it would be complex (i.e. high maintenance burden). We need a policy for what to do in those cases. As a concrete example, it is unclear what this code should do, given that we just can't apply the target feature: https://godbolt.org/z/esP11x8j8 #![feature(naked_functions, asm_experimental_arch)]
#[no_mangle]
#[naked]
#[cfg(target_arch = "wasm32")]
#[target_feature(enable = "simd128")]
unsafe extern "C" fn simd128() {
std::arch::naked_asm!(
"i8x16.shuffle 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15",
"return"
);
}On nightly today, That the target feature doesn't work is surprising, because standard (clothed?) functions can be defined with a target feature just fine (even though on wasm this does not make much sense) use std::arch::wasm32::*;
#[no_mangle]
#[target_feature(enable = "simd128")]
unsafe extern "C" fn simd128_intrinsic(v: v128) -> v128 {
i8x16_add(v, v)
}For webassembly this issue is not acute because inline assembly for it is unstable (but hopefully that won't be forever), and also LLVM might just implement a way to toggle target features using assembler directives. But at least right now there are targets for which we can't guarantee that we can toggle target features (correctly). Currently the option I prefer is to disallow That route unblocks stabilization of |
|
Do we need to allow If we do allow it, I think it should be up to whoever writes the The main question IMO is to figure out how a naked function taking i.e. a Because of this, I would propose that we allow |
|
@folkertdev ah that all makes sense, I was unaware of this assembler behavior for wasm! My guess is that given that it's a relatively new assembler there just isn't support for dynamically enabling/disabling features like there is on other platforms. Regardless your proposed route sounds quite reasonable to me 👍 |
|
@rfcbot concern maybe-unsafe-attribute When we accepted the relevant RFC, we hadn't yet done unsafe attributes. Looking at it now, I wonder if it shouldn't be
That sounds rather unsafe! Of course, we could argue that, "well, you can't actually create unsafety without the inner More radically, we could ask whether maybe #[unsafe(naked)]
#[unsafe(no_mangle)]
pub extern "sysv64" fn f() -> ! {
core::arch::naked_asm!(
"ud2",
)
}What do we think? |
There was a problem hiding this comment.
Question: does #![feature(naked_functions)] have any iteractions with #![feature(fn_align)]? In particular, are there any potential caveats or gotchas that stabilizing naked_functions may have for fn_align in the future?
(The test is irrelevant, I just wanted to force an inline comment)
There was a problem hiding this comment.
Combining #[naked] with #[repr(align(N))] works well, and is tested e.g. here
- https://github.com/rust-lang/rust/blob/master/tests/codegen/naked-fn/aligned.rs
- https://github.com/rust-lang/rust/blob/master/tests/codegen/naked-fn/min-function-alignment.rs
It's tested extensively because we do need to explicitly support the repr(align) attribute (and make sure we e.g. don't mistake powers of two for number of bytes).
There was a problem hiding this comment.
It'd be worth including what you just mentioned here in the stabilization report.
My 2 cents: +1 on making the attribute unsafe; I am not sure if I would then want to see the naked_asm macro not require unsafe (it's somewhat redundant, but as this is wildly unsafe...) Also, just to make sure I understand correctly, the following would not compile, right? #[naked]
pub extern "sysv64" fn f() {}It is obviously unsound, without actually containing an unsafe block, as calling this function would start executing padding bytes, so it should be disallowed. If that's the case, then it's impossible to write a naked function without writing unsafe inside it, which sounds like a pretty convincing argument to make it an unsafe attribute... |
|
Could someone explain the interaction with target features? I would have expected that attribute to be completely useless on a naked function -- if I want to use AVX instructions, I just put them in the asm block, and that implicitly imposes a requirement that AVX must be available, but the compiler doesn't actually have to know about this? |
|
@rfcbot resolve question-about-unwind As @Amanieu mentioned, naked functions do support unwinding, and the Reference PR has been updated to reflect and describe this. It looks like this: #![feature(naked_functions)]
#[unsafe(no_mangle)]
pub extern "sysv64-unwind" fn g() {
panic!();
}
#[cfg(target_arch = "x86_64")]
#[naked]
#[unsafe(no_mangle)]
pub extern "sysv64-unwind" fn f() {
unsafe {
core::arch::naked_asm!(
".cfi_startproc",
"push rbp",
".cfi_adjust_cfa_offset 8",
".cfi_offset rbp, -16",
"mov rbp, rsp",
".cfi_def_cfa_register rbp",
"call {f}",
"pop rbp",
".cfi_def_cfa rsp, 8",
"ret",
".cfi_endproc",
f = sym g,
)
}
}Playground link (with further annotations) |
On many targets LLVM refuses to assemble instructions unless the target feature that they got introduced with is enabled. Target features can also affect the exact encoding of instructions I believe. |
Isn't that something that can be toggled within a |
In most cases yes, using assembler directives. Those depend on the architecture. E.g.
To me, the appeal of It does not seem feasible to me to make target features work seamlessly for naked functions: on wasm LLVM gives us no tools for it, for arm it's horribly complex, and even for the other targets every target feature would need to be tested to make sure it actually works (because rust target features don't map one-to-one with what the assembler understands). But (and I overlooked this before) So I think that leaves 2 plausible options
I have concerns about option 1 because it can cause some extremely confusing errors, e.g. this would error https://godbolt.org/z/89GhGzqW4 #[naked]
#[no_mangle]
#[target_feature(enable = "aes")]
unsafe extern "C" fn aes_encrypt(a: uint8x16_t, round_key: uint8x16_t) -> uint8x16_t {
core::arch::naked_asm!(
"aese v0.16b, v1.16b",
"ret",
);
}Saying that moreover, the non-naked version of this function does work! #[no_mangle]
#[target_feature(enable = "aes")]
unsafe extern "C" fn non_naked_inline_asm(a: uint8x16_t, round_key: uint8x16_t) -> uint8x16_t {
core::arch::asm!(
"aese {0:v}.16b, {1:v}.16b",
"ret",
in(vreg) a, in(vreg) round_key, options(noreturn)
);
}I suppose we could try to parse the LLVM error and try to emit a custom error message mentioning target features (and specifically how they behave on naked functions)? I brought up parsing the error before though #121496 and others seemed sceptical. |
|
I'm afraid the situation is somewhat more complex than that -- if you use a SIMD type by value without declaring the target feature on the function, nowadays you get a warning, but we're planning for that to become a hard error. I'd argue that it should be sufficient to write in the documentation of |
|
Is there any reason for not just moving |
|
Okay this seem to mostly be about the technical difficulties of passing targey features to the assemblee then, I have no idea at all about things like that.
We can map our tatget feature names to the ones LLVM uses easily, we already do that. Is that not enough?
If LLVM does not let us set the targey features on naked functions the same way we set it on all other functions, then we'd have to duplicate a lot of the backend specific logic inside LLVM, right? That sounds terrible.
But (and I overlooked this before) #[target_feature] is also relevant for ABI reasons: it influences which registers are used to pass arguments and return values, e.g. https://godbolt.org/z/xW4cW3dor. That is highly relevant for naked functions which rely on the ABI of the function to get at their arguments and put the return value in the right place.
Every such case is a soundness bug (and we should we emitting a future compat warning already). Some types require certain target features to be passable, but the presence or absence of a target feature must only affect whether the code builds.
|
|
catching up to some comments
Yes that is what I would argue :) from the edition guide
There are no soundness requirements to a naked function itself. The requirements are on the
Correct, that is tested here: rust/tests/ui/asm/naked-functions.rs Lines 79 to 81 in ed897d5
Sadly not. LLVM target features are also just made-up names that don't always map to the names that the assembler uses. E.g. the arm
That is my proposed course of action. So far nobody seems to really be against this approach, so if things stay that way I'll make a PR. |
And I guess we can't send the naked function "through" LLVM to let it do the translation? That might be a good feature request for them...
Sounds good to me. |
No that was the original design, but it was rejected for various reasons:
Some further context from the tracking issue: |
As they should. They have to deal with target feature names anyway for regular code. Anyway, I am mostly clueless here, just poking a bit in the dark to feel out the design space. @Amanieu knows a lot better than me how to make inline assembly work well. :) |
tracking issue: #90957
request for stabilization on tracking issue: #90957 (comment)
reference PR: rust-lang/reference#1689
Request for Stabilization
Two years later, we're ready to try this again. Even though this issue is already marked as having passed FCP, given the amount of time that has passed and the changes in implementation strategy, we should follow the process again.
Summary
The
naked_functionsfeature has two main parts: the#[naked]function attribute, and thenaked_asm!macro.An example of a naked function:
When the
#[naked]attribute is applied to a function, the compiler won't emit a function prologue or epilogue when generating code for this function. This attribute is analogous to__attribute__((naked))in C. The use of this feature allows the programmer to have precise control over the assembly that is generated for a given function.The body of a naked function must consist of a single
naked_asm!invocation, a heavily restricted variant of theasm!macro: the only legal operands areconstandsym, and the only legal options arerawandatt_syntax. In lieu of specifying operands, thenaked_asm!within a naked function relies on the function's calling convention to determine the validity of registers.Documentation
The Rust Reference: rust-lang/reference#1153
Tests
pub,#[no_mangle]and#[linkage = "..."]work correctly for naked functionsnaked_asm!, etcInteraction with other (unstable) features
fn_align
Combining
#[naked]with#[repr(align(N))]works well, and is tested e.g. hereIt's tested extensively because we do need to explicitly support the
repr(align)attribute (and make sure we e.g. don't mistake powers of two for number of bytes).History
This feature was originally proposed in RFC 1201, filed on 2015-07-10 and accepted on 2016-03-21. Support for this feature was added in #32410, landing on 2016-03-23. Development languished for several years as it was realized that the semantics given in RFC 1201 were insufficiently specific. To address this, a minimal subset of naked functions was specified by RFC 2972, filed on 2020-08-07 and accepted on 2021-11-16. Prior to the acceptance of RFC 2972, all of the stricter behavior specified by RFC 2972 was implemented as a series of warn-by-default lints that would trigger on existing uses of the
nakedattribute; these lints became hard errors in #93153 on 2022-01-22. As a result, today RFC 2972 has completely superseded RFC 1201 in describing the semantics of thenakedattribute.More recently, the
naked_asm!macro was added to replace the earlier use of a heavily restrictedasm!invocation. Thenaked_asm!name is clearer in error messages, and provides a place for documenting the specific requirements of inline assembly in naked functions.The implementation strategy was changed to emitting a global assembly block. In effect, an extern function
is emitted as something similar to
The codegen approach was chosen over the llvm naked function attribute because:
Finally, there is now an allow list of compatible attributes on naked functions, so that e.g.
#[inline]is rejected with an error.relevant PRs for these recent changes
#[naked]: report incompatible attributes #127853naked_asm!macro for use in#[naked]functions #128651#[naked]functions using global asm #128004Various historical notes
RFC 2972 mentions that naked functions
Instead of
asm!, the current implementation mandates that the body contain a singlenaked_asm!statement. Thenaked_asm!macro is a heavily restricted version of theasm!macro, making it easier to talk about and document the rules of assembly in naked functions and give dedicated error messages.For
naked_asm!, the behavior of theasm!'snoreturnoption is implicit. Thenoreturnoption means that it is UB for control flow to fall through the end of the assembly block. Withasm!, this option is usually used for blocks that diverge (and thus have no return and can be typed as!). Withnaked_asm!, the intent is different: usually naked funtions do return, but they must do so from within the assembly block. Thenoreturnoption was used so that the compiler would not itself also insert aretinstruction at the very end.A
naked_asm!block that violates the safety assumption that control flow must not fall through the end of the assembly block is UB. Because no return instruction is emitted, whatever bytes follow the naked function will be executed, resulting in truly undefined behavior. There has been discussion whether rustc should emit an invalid instruction after thenaked_asm!block to at least fail early in the case of an invalidnaked_asm!. It was however decided that it is more useful to guarantee that#[naked]functions NEVER contain any instructions besides those in thenaked_asm!block.unresolved questions
None
r? @Amanieu
I've validated the tests on x86_64 and aarch64