`#[naked]`: report incompatible attributes #127853

folkertdev · 2024-07-17T09:09:33Z

tracking issue: #90957

this is a re-implementation of #93809 by @bstrie which was closed 2 years ago due to inactivity.

This PR takes some of the final comments into account, specifically providing a little more context in error messages, and using an allow list to determine which attributes are compatible with #[naked].

Notable attributes that are incompatible with #[naked] are:

#[inline]
#[track_caller]
~~#[target_feature]~~ (this is now allowed, see PR discussion)
#[test], #[ignore], #[should_panic]

These attributes just directly conflict with what #[naked] should do.

Naked functions are still important for systems programming, embedded, and operating systems, so I'd like to move them forward.

…nd `#[target-feature]``

rustbot · 2024-07-17T09:09:42Z

r? @compiler-errors

rustbot has assigned @compiler-errors.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

bjorn3 · 2024-07-17T10:21:57Z

compiler/rustc_passes/src/check_attr.rs

+        //
+        // * `#[inline]`
+        // * `#[track_caller]`
+        // * `#[target_feature]`


#[target_feature] affects the set of instructions allowed inside inline assembly as well as the exact encoding of certain instructions.

yes, I'm going off of #90957 (comment) where @joshtriplett suggests to exclude #[target_feature] initially.

that was 2 years ago though so maybe that should be re-evaluated. Do you have concerns with allowing #[target_feature] now?

I don't know what a reason for forbidding it would be, and why @joshtriplett thought it would be hard to get right

well a reason to be cautious is that I think to most (me included) it's not 100% clear what #[target_feature] could do. Are we absolutely sure that on no platform, present or future, a target feature would insert additional instructions that violate the assumptions of #[naked]?

What I said at the time was that it might take some care to get right. If we take that care, and commit to avoiding any potential issues that might arise (e.g. locking down what a given target_feature means for a naked function and never inserting any instructions for it), then supporting the combination seems fine.

With the implementation strategy in #128004 I don't think there is any risk of #[target_feature] inserting instructions. The user's inline assembly block is hoisted to a top-level global asm block, and the function itself becomes effectively an extern fn.

So I've now added target_feature to the allow list in a separate commit that can be reverted if there are any concerns.

compiler-errors · 2024-07-25T15:19:16Z

r? bjorn3

tests/ui/rfcs/rfc-2091-track-caller/error-with-naked.stderr

bjorn3 · 2024-07-26T19:22:38Z

@bors r+

bors · 2024-07-26T19:22:40Z

📌 Commit 45e943f has been approved by bjorn3

It is now in the queue for this repository.

…ssages, r=bjorn3 `#[naked]`: report incompatible attributes tracking issue: rust-lang#90957 this is a re-implementation of rust-lang#93809 by `@bstrie` which was closed 2 years ago due to inactivity. This PR takes some of the final comments into account, specifically providing a little more context in error messages, and using an allow list to determine which attributes are compatible with `#[naked]`. Notable attributes that are incompatible with `#[naked]` are: * `#[inline]` * `#[track_caller]` * ~~`#[target_feature]`~~ (this is now allowed, see discussion below) * `#[test]`, `#[ignore]`, `#[should_panic]` There may be valid use cases for `#[target_feature]` but for now it is disallowed. The others just directly conflict with what `#[naked]` should do. Naked functions are still important for systems programming, embedded and operating systems, so I'd like to move them forward.

Rollup of 9 pull requests Successful merges: - rust-lang#124941 (Stabilize const `{integer}::from_str_radix` i.e. `const_int_from_str`) - rust-lang#127853 (`#[naked]`: report incompatible attributes) - rust-lang#128210 (rustdoc: change title of search results) - rust-lang#128223 (Refactor complex conditions in `collect_tokens_trailing_token`) - rust-lang#128224 (Remove unnecessary range replacements) - rust-lang#128226 (Remove redundant option that was just encoding that a slice was empty) - rust-lang#128227 (CI: do not respect custom try jobs for unrolled perf builds) - rust-lang#128229 (Improve `extern "<abi>" unsafe fn()` error message) - rust-lang#128235 (Fix `Iterator::filter` docs) r? `@ghost` `@rustbot` modify labels: rollup

tgross35 · 2024-07-26T23:02:01Z

@bors r-

It looks like the changes here are breaking the compiler_builtins build #128248 (comment). It seems like link-related attributes (linkage here) shouldn't be incompatible?

folkertdev · 2024-07-26T23:05:48Z

right, that's a (perma-unstable, apparently) unstable attribute and only external linkage makes sense, but it makes sense to add to the allowlist I think. I'll add it

folkertdev · 2024-07-26T23:43:36Z

@rustbot ready

though are there other unstable attributes that may be missing from that list? is there a comprehensive list of such attributes?

tgross35 · 2024-07-27T03:17:26Z

I don't know of any list specific for attributes unfortunately, just the huge list in compiler/rustc_span/src/symbol.rs. link_ordinal seems to make sense too, maybe worth double checking the other link_* symbols to see if they are applicable.

Does this correctly flag on #[cfg_attr(foo, invalid_attribute)] when foo is true? Could probably use a test if so. (Also probably a squash).

folkertdev · 2024-07-27T17:21:52Z

I'm pretty sure bors will fail when running aarch64 tests. The last commit fixes those issues. Not sure what should happen but because the bors thing is tied to a commit I think some action is needed

tgross35 · 2024-07-27T17:28:48Z

Bors bumps you out of the queue when you push, just doesn't update the labels for some reason.

Your last change switched from att_syntax to raw, why wouldn't at&t syntax work on aarch64?

folkertdev · 2024-07-27T17:31:45Z

well, "the att_syntax option is only supported on x86" is an error that is given. I guess it is just the default on any non-x86 system, and you cannot be explicit about that. Now that you bring it up that does seem a little weird, but not something that this PR should change I think.

tgross35 · 2024-07-27T17:37:05Z

Ah weird, yeah seems reasonable.

@bors r=bjorn3

bors · 2024-07-27T17:37:07Z

📌 Commit c7e688e has been approved by bjorn3

It is now in the queue for this repository.

…ssages, r=bjorn3 `#[naked]`: report incompatible attributes tracking issue: rust-lang#90957 this is a re-implementation of rust-lang#93809 by `@bstrie` which was closed 2 years ago due to inactivity. This PR takes some of the final comments into account, specifically providing a little more context in error messages, and using an allow list to determine which attributes are compatible with `#[naked]`. Notable attributes that are incompatible with `#[naked]` are: * `#[inline]` * `#[track_caller]` * ~~`#[target_feature]`~~ (this is now allowed, see PR discussion) * `#[test]`, `#[ignore]`, `#[should_panic]` These attributes just directly conflict with what `#[naked]` should do. Naked functions are still important for systems programming, embedded, and operating systems, so I'd like to move them forward.

…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#127853 (`#[naked]`: report incompatible attributes) - rust-lang#128228 (Stabilize `const_waker`) - rust-lang#128276 (Add a README to rustbook to explain its purpose) - rust-lang#128279 (Stabilize `is_sorted`) - rust-lang#128282 (bitwise and bytewise methods on `NonZero`) - rust-lang#128285 (rustc book: document how the RUST_TARGET_PATH variable is used) r? `@ghost` `@rustbot` modify labels: rollup

…iaskrgr Rollup of 5 pull requests Successful merges: - rust-lang#127853 (`#[naked]`: report incompatible attributes) - rust-lang#128276 (Add a README to rustbook to explain its purpose) - rust-lang#128279 (Stabilize `is_sorted`) - rust-lang#128282 (bitwise and bytewise methods on `NonZero`) - rust-lang#128285 (rustc book: document how the RUST_TARGET_PATH variable is used) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#127853 - folkertdev:naked-function-error-messages, r=bjorn3 `#[naked]`: report incompatible attributes tracking issue: rust-lang#90957 this is a re-implementation of rust-lang#93809 by ``@bstrie`` which was closed 2 years ago due to inactivity. This PR takes some of the final comments into account, specifically providing a little more context in error messages, and using an allow list to determine which attributes are compatible with `#[naked]`. Notable attributes that are incompatible with `#[naked]` are: * `#[inline]` * `#[track_caller]` * ~~`#[target_feature]`~~ (this is now allowed, see PR discussion) * `#[test]`, `#[ignore]`, `#[should_panic]` These attributes just directly conflict with what `#[naked]` should do. Naked functions are still important for systems programming, embedded, and operating systems, so I'd like to move them forward.

hsanzg · 2024-07-30T02:09:20Z

I'm currently running into "error[E0736]: attribute incompatible with #[naked]" when compiling this function with rustc 1.82.0-nightly (612a33f 2024-07-29). The x86_64 module is under a target_arch cfg attribute, but this code used to work fine in 1.81.0-nightly.

…mment, r=bjorn3 make `///` doc comments compatible with naked functions tracking issue: rust-lang#90957 reported in rust-lang#127853 (comment) it turns out `/// doc comment` and `#[doc = "doc comment"]` are represented differently, at least at the point where we perform the check for what should be allowed. The `///` style doc comment is now also allowed. r? `@bjorn3` cc `@hsanzg`

portasynthinca3 · 2024-07-30T20:22:43Z

Having the same issue over here.

tgross35 · 2024-07-30T20:25:16Z

The fix is in #128380, unless your case is different @portasynthinca3. It will probably make it into either today or tomorrow's nightly.

…mment, r=bjorn3 make `///` doc comments compatible with naked functions tracking issue: rust-lang#90957 reported in rust-lang#127853 (comment) it turns out `/// doc comment` and `#[doc = "doc comment"]` are represented differently, at least at the point where we perform the check for what should be allowed. The `///` style doc comment is now also allowed. r? ``@bjorn3`` cc ``@hsanzg``

Rollup merge of rust-lang#128380 - folkertdev:naked-compatible-doc-comment, r=bjorn3 make `///` doc comments compatible with naked functions tracking issue: rust-lang#90957 reported in rust-lang#127853 (comment) it turns out `/// doc comment` and `#[doc = "doc comment"]` are represented differently, at least at the point where we perform the check for what should be allowed. The `///` style doc comment is now also allowed. r? ``@bjorn3`` cc ``@hsanzg``

rust-lang/rust#127853 (comment) ``` error[E0736]: attribute incompatible with `#[naked]` --> /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/gba-0.11.6/src/asm_runtime.rs:164:1 | 164 | /// Returns 0 in `r0`, while placing the `numerator` into `r1`. | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the `` attribute is incompatible with `#[naked]` ... 172 | #[naked] | -------- function marked with `#[naked]` here ```

…bjorn3 make `///` doc comments compatible with naked functions tracking issue: rust-lang/rust#90957 reported in rust-lang/rust#127853 (comment) it turns out `/// doc comment` and `#[doc = "doc comment"]` are represented differently, at least at the point where we perform the check for what should be allowed. The `///` style doc comment is now also allowed. r? ``@bjorn3`` cc ``@hsanzg``

Freax13 · 2024-08-17T15:22:35Z

unclear. Whether these make sense really depends on the outcome of #128004

sym::repr

sym::patchable_function_entry

sym::no_sanitize

Before this PR was merged, I was able to use #[no_sanitize(address)] on a #[naked] function with the calling convention "x86-interrupt". Omitting #[no_sanitize(address)] leads to a compiler crash. I tried looking at the generated LLVM IR and the function prologue contains a bunch of other KASAN-related stuff which I wouldn't expect for naked function. I'm not sure whether the right solution here is to stop sanitizing naked functions in the first place or to allow #[no_sanitize(...)] on them.

folkertdev · 2024-08-17T15:42:36Z

The idea of the linked PR is to implement naked functions like this

core::arch::global_asm!(".globl page_fault_handler","page_fault_handler:", "ud2");

extern "x86-interrupt" { 
    fn page_fault_handler(_: u64, _: u64);
}

but with support for (const) generics and with being able to export the symbol (which is tricky with extern function declarations in user space). I think in that world, the no_sanitize is no longer needed? At least on godbolt the above codegens fine.

However, in the meantime, we probably should not break anyone? @bjorn3 do you have thoughts on what to do?

tgross35 · 2024-08-17T17:43:09Z

Before this PR was merged, I was able to use #[no_sanitize(address)] on a #[naked] function with the calling convention "x86-interrupt". Omitting #[no_sanitize(address)] leads to a compiler crash. I tried looking at the generated LLVM IR and the function prologue contains a bunch of other KASAN-related stuff which I wouldn't expect for naked function. I'm not sure whether the right solution here is to stop sanitizing naked functions in the first place or to allow #[no_sanitize(...)] on them.

@Freax13 could you open a new bug for this crash? It seems like that very specific combination of extern "x86-interrupt", sanitizers, and naked functions is required to reproduce. And it's been around since before this PR, https://godbolt.org/z/EnhsWxejT (using RUSTC_BOOTSTRAP for the demo). Naked functions should probably just never get sanitizer annotations, but we can ping project-exploit-mitigations on the issue to confirm.

folkertdev added 4 commits July 16, 2024 22:06

update text for E0736 and E0739

ceec6dd

improve error message when #[naked] is used with #[inline]

4bd3632

improve error message when #[naked] is used with `#[track-caller] a…

7e6c083

…nd `#[target-feature]``

add error message when #[naked] is used with #[test]

4d082b7

rustbot assigned compiler-errors Jul 17, 2024

rustbot added 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 Jul 17, 2024

bjorn3 reviewed Jul 17, 2024

View reviewed changes

This was referenced Jul 20, 2024

codegen #[naked] functions using global asm #128004

Open

Tracking Issue for RFC #2972: Constrained Naked Functions #90957

Open

rustbot assigned bjorn3 and unassigned compiler-errors Jul 25, 2024

bjorn3 reviewed Jul 26, 2024

View reviewed changes

tests/ui/rfcs/rfc-2091-track-caller/error-with-naked.stderr Outdated Show resolved Hide resolved

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 Jul 26, 2024

tgross35 mentioned this pull request Jul 26, 2024

Rollup of 9 pull requests #128248

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 26, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 26, 2024

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jul 27, 2024

fix tests/ui/asm/naked-functions.rs for aarch64

c7e688e

matthiaskrgr mentioned this pull request Jul 28, 2024

Rollup of 6 pull requests #128297

Closed

matthiaskrgr mentioned this pull request Jul 28, 2024

Rollup of 5 pull requests #128298

Merged

bors merged commit a13f40d into rust-lang:master Jul 28, 2024
6 checks passed

rustbot added this to the 1.82.0 milestone Jul 28, 2024

folkertdev mentioned this pull request Jul 30, 2024

make /// doc comments compatible with naked functions #128380

Merged

Freax13 mentioned this pull request Aug 18, 2024

using extern "x86-interrupt" + #[naked] + kernel address sanitizer crashes compiler #129224

Closed

folkertdev mentioned this pull request Aug 23, 2024

#[naked] attribute excludes use of #[repr(align(N))] #129412

Closed

traviscross mentioned this pull request Sep 3, 2024

Add documentation for the naked function attribute rust-lang/reference#1153

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`#[naked]`: report incompatible attributes #127853

`#[naked]`: report incompatible attributes #127853

folkertdev commented Jul 17, 2024 •

edited by workingjubilee

Loading

rustbot commented Jul 17, 2024

bjorn3 Jul 17, 2024

folkertdev Jul 17, 2024

Noratrieb Jul 17, 2024

folkertdev Jul 18, 2024

joshtriplett Jul 22, 2024

folkertdev Jul 23, 2024

compiler-errors commented Jul 25, 2024

bjorn3 commented Jul 26, 2024

bors commented Jul 26, 2024

tgross35 commented Jul 26, 2024

folkertdev commented Jul 26, 2024 •

edited

Loading

folkertdev commented Jul 26, 2024

tgross35 commented Jul 27, 2024

folkertdev commented Jul 27, 2024

tgross35 commented Jul 27, 2024

folkertdev commented Jul 27, 2024

tgross35 commented Jul 27, 2024

bors commented Jul 27, 2024

hsanzg commented Jul 30, 2024 •

edited

Loading

portasynthinca3 commented Jul 30, 2024

tgross35 commented Jul 30, 2024 •

edited

Loading

Freax13 commented Aug 17, 2024

folkertdev commented Aug 17, 2024

tgross35 commented Aug 17, 2024

#[naked]: report incompatible attributes #127853

#[naked]: report incompatible attributes #127853

Conversation

folkertdev commented Jul 17, 2024 • edited by workingjubilee Loading

rustbot commented Jul 17, 2024

bjorn3 Jul 17, 2024

Choose a reason for hiding this comment

folkertdev Jul 17, 2024

Choose a reason for hiding this comment

Noratrieb Jul 17, 2024

Choose a reason for hiding this comment

folkertdev Jul 18, 2024

Choose a reason for hiding this comment

joshtriplett Jul 22, 2024

Choose a reason for hiding this comment

folkertdev Jul 23, 2024

Choose a reason for hiding this comment

compiler-errors commented Jul 25, 2024

bjorn3 commented Jul 26, 2024

bors commented Jul 26, 2024

tgross35 commented Jul 26, 2024

folkertdev commented Jul 26, 2024 • edited Loading

folkertdev commented Jul 26, 2024

tgross35 commented Jul 27, 2024

folkertdev commented Jul 27, 2024

tgross35 commented Jul 27, 2024

folkertdev commented Jul 27, 2024

tgross35 commented Jul 27, 2024

bors commented Jul 27, 2024

hsanzg commented Jul 30, 2024 • edited Loading

portasynthinca3 commented Jul 30, 2024

tgross35 commented Jul 30, 2024 • edited Loading

Freax13 commented Aug 17, 2024

folkertdev commented Aug 17, 2024

tgross35 commented Aug 17, 2024

`#[naked]`: report incompatible attributes #127853

`#[naked]`: report incompatible attributes #127853

folkertdev commented Jul 17, 2024 •

edited by workingjubilee

Loading

folkertdev commented Jul 26, 2024 •

edited

Loading

hsanzg commented Jul 30, 2024 •

edited

Loading

tgross35 commented Jul 30, 2024 •

edited

Loading