-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
add naked_asm!
macro for use in #[naked]
functions
#128651
Conversation
My own vote is that the noreturn option should not be placed by users into the naked_asm macro. The same option should not mean one thing for asm and then nearly the opposite in naked_asm. That's needlessly confusing. There are two reasonable designs that spring to mind:
|
This comment has been minimized.
This comment has been minimized.
7dbd972
to
bfa880f
Compare
Some changes occurred in src/tools/rustfmt cc @rust-lang/rustfmt |
I agree, and think we can get away with making the But also, I don't want to touch the validation logic in this PR, it can be handled when we disallow the use of |
bfa880f
to
f25a97c
Compare
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.
LGTM, but I would make one change and remove the noreturn
attribute from naked_asm
, changing it to always return !
.
The reason for this is that we want existing users of naked function to transition to using naked_asm
instead and we want to eliminate noreturn
as part of this transition.
r=me
cool, we discussed I did not implement that here yet because then But, I'll line up those changes and then we can see how to merge them: either in 2 parts or as one combined unit. |
It's probably fine to go ahead and disallow |
f25a97c
to
3de7452
Compare
This comment has been minimized.
This comment has been minimized.
3de7452
to
c1e29cb
Compare
This comment has been minimized.
This comment has been minimized.
c1e29cb
to
06dfdbe
Compare
This PR modifies cc @jieyouxu |
This comment has been minimized.
This comment has been minimized.
This PR changes MIR cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 |
well that pinged a bunch of people. The thing I had to solve was that the |
This comment has been minimized.
This comment has been minimized.
d2c6cbb
to
d2aa0e0
Compare
This comment has been minimized.
This comment has been minimized.
d2aa0e0
to
56b14b1
Compare
…tgross35 update `compiler-builtins` to 0.1.126 this requires the addition of a bootstrap variant of the new `naked_asm!` macro r? `@tgross35` extracted from rust-lang#128651
Rollup merge of rust-lang#130875 - folkertdev:naked-asm-bootstrap, r=tgross35 update `compiler-builtins` to 0.1.126 this requires the addition of a bootstrap variant of the new `naked_asm!` macro r? `@tgross35` extracted from rust-lang#128651
f8fe7bf
to
75f6160
Compare
This comment has been minimized.
This comment has been minimized.
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.
LGTM with one minor nit.
r=me
self.items.push((ItemKind::NakedAsm, span)); | ||
} | ||
rustc_ast::AsmMacro::GlobalAsm => { | ||
// not allowed in this position |
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.
Make this a span_bug!
.
@bors delegate+ |
✌️ @folkertdev, you can now approve this pull request! If @Amanieu told you to " |
also disallow the `noreturn` option, and infer `naked_asm!` as `!`
- fix for divergence - fix error message - fix another cranelift test - fix some cranelift things - don't set the NORETURN option for naked asm - fix use of naked_asm! in doc comment - fix use of naked_asm! in run-make test - use `span_bug` in unreachable branch
8a188bd
to
5fc60d1
Compare
☀️ Test successful - checks-actions |
Finished benchmarking commit (1b3b8e7): 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 (secondary 0.6%)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 7.9%)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: 774.699s -> 774.632s (-0.01%) |
…10-07 This feature has been added recently in this pull request: rust-lang/rust#128651 After this pull, `core::arch::asm!` can no longer be used in #[naked] functions. We should use `core::arch::naked_asm!` instead. Tested rustc version: ``` rustc -vV rustc 1.83.0-nightly (3ae715c8c 2024-10-07) binary: rustc commit-hash: 3ae715c8c63f9aeac47cbf7d8d9dadb3fa32c638 commit-date: 2024-10-07 host: x86_64-pc-windows-msvc release: 1.83.0-nightly LLVM version: 19.1.1 ``` Signed-off-by: Zhouqi Jiang <luojia@hust.edu.cn>
…r rustc nightly 2024-10-07 Ref: rust-lang/rust#128651 Signed-off-by: Zhouqi Jiang <luojia@hust.edu.cn>
… 2024-10-07 Ref: rust-lang/rust#128651 Signed-off-by: Zhouqi Jiang <luojia@hust.edu.cn>
tracking issue: #90957
Adds the
core::arch::naked_asm
macro, to be used in#[naked]
functions, but providing better error messages and a place to explain the restrictions on assembly in naked functions.This PR does not yet require that the
naked_asm!
macro is used inside of#[naked]
functions:asm!
macro can still be used in#[naked]
functions currently, with the same restrictions and error messages as before.naked_asm!
macro can be used outside of#[naked]
functions. It has not yet been decided whether that should be allowed long-term.In this PR, the parsing code of
naked_asm!
now enforces the restrictions on assembly in naked functions, with the exception of checking that thenoreturn
option is specified. It also has not currently been decided ifnoreturn
should be implicit or not.This PR looks large because it touches a bunch of tests. The code changes are mostly straightforward I think: we now have 3 flavors of assembly macro, and that information must be propagated through the parsing code and error messages.
cc @Lokathor
r? @Amanieu