Skip to content

Make #[naked] an unsafe attribute #139753

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

folkertdev
Copy link
Contributor

@folkertdev folkertdev commented Apr 13, 2025

tracking issue: #138997

Per #134213 (comment), the #[naked] attribute is now an unsafe attribute (in any edition).

This can only be merged when the above PRs are merged, I'd just like to see if there are any CI surprises here, and maybe there is early review feedback too.

r? @traviscross

@rustbot rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) A-run-make Area: port run-make Makefiles to rmake.rs PG-exploit-mitigations Project group: Exploit mitigations S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 13, 2025
@rust-log-analyzer

This comment has been minimized.

@folkertdev
Copy link
Contributor Author

Hmm, how should this this error be resolved?

error: unsafe attribute used without unsafe
##[error]   --> /cargo/registry/src/index.crates.io-1949cf8c6b5b557f/compiler_builtins-0.1.153/src/macros.rs:436:15
    |
436 |               #[naked]
    |                 ^^^^^ usage of unsafe attribute
    |
   ::: /cargo/registry/src/index.crates.io-1949cf8c6b5b557f/compiler_builtins-0.1.153/src/x86_64.rs:10:1
    |
10  | / intrinsics! {
11  | |     #[naked]
12  | |     #[cfg(all(
13  | |         any(
...   |
42  | | }
    | |_- in this macro invocation
    |
    = note: this error originates in the macro `intrinsics` (in Nightly builds, run with -Z macro-backtrace for more info)
help: wrap the attribute in `unsafe(...)`
    |
436 |             #[unsafe(naked)]
    |               +++++++     +

It's kind of a cyclic dependency: I can't update the compiler without updating compiler-builtins, but I can't update compiler builtins either without an updated compiler. What is the right approach here?

@traviscross
Copy link
Contributor

Does #[cfg_attr(bootstrap, ..)] (docs) work here?

@bors
Copy link
Collaborator

bors commented Apr 14, 2025

☔ The latest upstream changes (presumably #139766) made this pull request unmergeable. Please resolve the merge conflicts.

@folkertdev folkertdev force-pushed the naked-function-unsafe-attribute branch from 3f4bcfb to 6a0c2ec Compare April 14, 2025 07:55
@rust-log-analyzer

This comment has been minimized.

@folkertdev
Copy link
Contributor Author

I thought that would not work because of the error that was emitted earlier (something with building miri), but it actually does appear to work (or at least, we get a different error now).

@bjorn3 do you see a way to work around the latest error? apparently a compiler where cfg(bootstrap) is true is used to run the mini_core_hello_world cranelift test?

@bjorn3
Copy link
Member

bjorn3 commented Apr 14, 2025

That shouldn't happen. cg_clif should be tested using a stage1 rustc.

@folkertdev folkertdev force-pushed the naked-function-unsafe-attribute branch from 6a0c2ec to f7f6d8d Compare April 14, 2025 10:32
@rust-log-analyzer

This comment has been minimized.

@folkertdev folkertdev force-pushed the naked-function-unsafe-attribute branch from f7f6d8d to b9f9059 Compare April 14, 2025 11:14
@rust-log-analyzer

This comment has been minimized.

@folkertdev
Copy link
Contributor Author

folkertdev commented Apr 14, 2025

well, the #[cfg(bootstrap)] thing does not work after all. So I think the only viable solution is to allow but not require unsafe(naked) for a brief period, so that compiler-builtins can be updated.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 14, 2025
…gross35

Allow (but don't require) `#[unsafe(naked)]` so that `compiler-builtins` can upgrade to it

tracking issue: rust-lang#138997

Per rust-lang#134213 (comment), we want to make the `#[naked]` attribute an unsafe attribute. Making that change runs into a cyclic dependency with `compiler-builtins` which uses `#[naked]`, where `rustc` needs an updated `compiler-builtins` and vice versa.

So based on rust-lang#139753 and [#t-compiler/help > updating &rust-lang#96;compiler-builtins&rust-lang#96; and &rust-lang#96;rustc&rust-lang#96;](https://rust-lang.zulipchat.com/#narrow/channel/182449-t-compiler.2Fhelp/topic/updating.20.60compiler-builtins.60.20and.20.60rustc.60), this PR allows, but does not require `#[unsafe(naked)]`, and makes that change for some of the tests to check that both `#[naked]` and `#[unsafe(naked)]` are accepted.

Then we can upgrade and synchronize `compiler-builtins`, and then make `#[naked]` (without `unsafe`) invalid.

r? `@traviscross` (or someone from t-compiler if you're faster and this look allright)
Zalathar added a commit to Zalathar/rust that referenced this pull request Apr 15, 2025
…gross35

Allow (but don't require) `#[unsafe(naked)]` so that `compiler-builtins` can upgrade to it

tracking issue: rust-lang#138997

Per rust-lang#134213 (comment), we want to make the `#[naked]` attribute an unsafe attribute. Making that change runs into a cyclic dependency with `compiler-builtins` which uses `#[naked]`, where `rustc` needs an updated `compiler-builtins` and vice versa.

So based on rust-lang#139753 and [#t-compiler/help > updating &rust-lang#96;compiler-builtins&rust-lang#96; and &rust-lang#96;rustc&rust-lang#96;](https://rust-lang.zulipchat.com/#narrow/channel/182449-t-compiler.2Fhelp/topic/updating.20.60compiler-builtins.60.20and.20.60rustc.60), this PR allows, but does not require `#[unsafe(naked)]`, and makes that change for some of the tests to check that both `#[naked]` and `#[unsafe(naked)]` are accepted.

Then we can upgrade and synchronize `compiler-builtins`, and then make `#[naked]` (without `unsafe`) invalid.

r? ``@traviscross`` (or someone from t-compiler if you're faster and this look allright)
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 15, 2025
Rollup merge of rust-lang#139797 - folkertdev:naked-allow-unsafe, r=tgross35

Allow (but don't require) `#[unsafe(naked)]` so that `compiler-builtins` can upgrade to it

tracking issue: rust-lang#138997

Per rust-lang#134213 (comment), we want to make the `#[naked]` attribute an unsafe attribute. Making that change runs into a cyclic dependency with `compiler-builtins` which uses `#[naked]`, where `rustc` needs an updated `compiler-builtins` and vice versa.

So based on rust-lang#139753 and [#t-compiler/help > updating &rust-lang#96;compiler-builtins&rust-lang#96; and &rust-lang#96;rustc&rust-lang#96;](https://rust-lang.zulipchat.com/#narrow/channel/182449-t-compiler.2Fhelp/topic/updating.20.60compiler-builtins.60.20and.20.60rustc.60), this PR allows, but does not require `#[unsafe(naked)]`, and makes that change for some of the tests to check that both `#[naked]` and `#[unsafe(naked)]` are accepted.

Then we can upgrade and synchronize `compiler-builtins`, and then make `#[naked]` (without `unsafe`) invalid.

r? `@traviscross` (or someone from t-compiler if you're faster and this look allright)
@bors

This comment was marked as outdated.

@folkertdev folkertdev force-pushed the naked-function-unsafe-attribute branch 2 times, most recently from 9258276 to a3c5a5d Compare April 18, 2025 20:24
@folkertdev folkertdev force-pushed the naked-function-unsafe-attribute branch from a3c5a5d to 0a99dc1 Compare April 18, 2025 21:01
@folkertdev folkertdev marked this pull request as ready for review April 18, 2025 21:04
@rustbot
Copy link
Collaborator

rustbot commented Apr 18, 2025

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in src/doc/unstable-book/src/compiler-flags/sanitizer.md

cc @rust-lang/project-exploit-mitigations, @rcvalle

This PR modifies run-make tests.

cc @jieyouxu

Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me once CI passes, unless you want to take a more thorough look @traviscross

@tgross35
Copy link
Contributor

Actually, would you mind also updating the diagnostic message mentions of #[naked] in compiler/rustc_passes/messages.ftl and compiler/rustc_builtin_macros/messages.ftl?

@rust-log-analyzer

This comment has been minimized.

@folkertdev folkertdev force-pushed the naked-function-unsafe-attribute branch from 0a99dc1 to 87dd3a0 Compare April 18, 2025 21:45
@folkertdev folkertdev force-pushed the naked-function-unsafe-attribute branch from 87dd3a0 to 41ddf86 Compare April 18, 2025 22:03
Copy link
Contributor

@traviscross traviscross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me also when CI passes, or I'll r= when I see that.

@traviscross
Copy link
Contributor

@bors r=tgross35,traviscross rollup

@bors
Copy link
Collaborator

bors commented Apr 18, 2025

📌 Commit 41ddf86 has been approved by tgross35,traviscross

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) A-run-make Area: port run-make Makefiles to rmake.rs PG-exploit-mitigations Project group: Exploit mitigations S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants