add repr to the allowlist for naked functions #129421

jdonszelmann · 2024-08-22T17:52:28Z

Fixes #129412 (combining unstable features #90957 (#![feature(naked_functions)]) and #82232 (#![feature(fn_align)])

rustbot · 2024-08-22T17:52:37Z

rustbot has assigned @wesleywiser.
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

jdonszelmann · 2024-08-22T17:57:18Z

r? @workingjubilee

tests/codegen/naked-fn/aligned.rs

workingjubilee

I found these two tests for the combinations of #[repr], #[naked], and things that accept one or the other:

rust/tests/ui/asm/naked-invalid-attr.rs

Lines 14 to 19 in f167efa

    
           #[naked] //~ ERROR should be applied to a function definition 
        
           #[repr(C)] 
        
           struct S { 
        
               a: u32, 
        
               b: u32, 
        
           }

rust/tests/ui/feature-gates/issue-43106-gating-of-builtin-attrs-error.rs

Line 152 in f167efa

#[repr(C)] fn f() { }

However, I wasn't able to find a UI test that checked the #[repr(C)] #[naked] fn, or perhaps something even sillier like #[repr(transparent)] #[naked] fn. I know it might be redundant, but I would prefer we did have a test to validate that specifically.

If it truly doesn't seem to be worth it then the fixes to the directives would be enough.

workingjubilee · 2024-08-24T21:39:57Z

tests/codegen/naked-fn/aligned.rs

@@ -0,0 +1,20 @@
+//@ compile-flags: -C no-prepopulate-passes -Copt-level=0
+//@ needs-asm-support
+//@ only-x86_64


Is there a reason this is x86_64 only? I believe ret should also be a valid instruction on:

x86

aarch64

avr

msp430

riscv32

riscv64

sparc

sparc64

xtensa

...it's true that AArch32, LoongArch, MIPS, Power ISA (AKA "PowerPC"), and Z/Architecture have chosen to be more... orthogonal (or just peculiar in the last case).

But it would be nice to see this test work on all of the ones that would support it without modification, and since the inclusions will be longer than the number of exclusions, we should probably reverse it. And of the exclusions, only one qualifies with needs-asm-support:

Suggested change

//@ only-x86_64

//@ ignore-arm no "ret" mnemonic

People maintaining the other ISA may wish to fix this test in a manner other than ignoring it when they turn on needs-asm-support, so we can simply let them do so.

workingjubilee · 2024-08-24T22:28:28Z

tests/codegen/naked-fn/aligned.rs

+#![feature(naked_functions, fn_align)]
+use std::arch::asm;
+
+// CHECK: Function Attrs: naked


Hmm, this is just checking a comment but that's probably fine since the check that validates the attribute is actually attached to the function is more annoying to write and LLVM doesn't emit these comments for no reason.

this is similar to how the tests for naked functions itself work. It may not be perfect but at least it's consistent as-is

workingjubilee · 2024-08-24T22:30:08Z

tests/codegen/naked-fn/aligned.rs

+#[no_mangle]
+#[naked]
+pub unsafe extern "C" fn naked_empty() {
+    // CHECK-NEXT: {{.+}}:


It's always going to be this label:

Suggested change

// CHECK-NEXT: {{.+}}:

// CHECK-NEXT: start:

jdonszelmann · 2024-08-24T22:35:15Z

fair comments, especially the one on isa support. The test was made by merging an existing test for #[naked] and #[repr(align(...))] so I'll see if the others actually need a fix too. I'll see what I can do; work for tomorrow though :)

jdonszelmann · 2024-08-27T13:34:50Z

@rustbot ready

@workingjubilee Note that I also changed a few diagnostics to make it hint at the right label :)

compiler-errors · 2024-08-27T14:26:46Z

tests/ui/asm/naked-with-invalid-repr-attr.stderr

+LL | | }
+   | |_- not a struct, enum, or union
+   |
+   = note: `#[repr(align(...))]` is valid on functions


Why are we noting this? It's extraneous -- I put repr(C) on an extern block; that has nothing to do with repr(align) or functions.

Er, misread that as an extern block. I would still probably rather not note this at all, though.

extern "C" fn example1() { isn't an extern block?

this isn't an extern block, it's a function. It wouldn't trigger if it wasn't a function. My reasoning was that the only valid repr on functions is align, so if you put repr(C) there you might like to know that align is a valid repr, but the one you used is not.

Er, misread that as an extern block.

This is a very silly language.

workingjubilee · 2024-08-27T14:32:50Z

tests/ui/asm/naked-with-invalid-repr-attr.stderr

+error[E0517]: attribute should be applied to a struct, enum, or union
+  --> $DIR/naked-with-invalid-repr-attr.rs:35:8
+   |
+LL |   #[repr(C, packed)]
+   |          ^
+...
+LL | / extern "C" fn example4() {
+LL | |
+LL | |
+LL | |     unsafe { asm!("", options(noreturn)) }
+LL | | }
+   | |_- not a struct, enum, or union
+   |
+   = note: `#[repr(align(...))]` is valid on functions
+
+error[E0517]: attribute should be applied to a struct or union
+  --> $DIR/naked-with-invalid-repr-attr.rs:35:11
+   |
+LL |   #[repr(C, packed)]
+   |             ^^^^^^
+...
+LL | / extern "C" fn example4() {


Is it possible to only emit 1 diagnostic for multiple errors on the same fn at least?

that's existing behavior of the repr attribute, got nothing to do with naked nor repr(align), but I agree it's not very nice. I can open a separate PR for that?

compiler-errors · 2024-08-27T14:37:31Z

compiler/rustc_passes/src/check_attr.rs

@@ -1782,6 +1783,17 @@ impl<'tcx> CheckAttrVisitor<'tcx> {
        let mut is_simd = false;
        let mut is_transparent = false;

+        let has_align =
+            hints.iter().find_map(|i| (i.name_or_empty() == sym::align).then(|| i.span()));
+        let (align_note, align_present_note) =


align_present_note and align_note are mutually exclusive, and both are only conditional on self.tcx.features().fn_align. I'd prefer if you represent this with an:

#[derive(Subdiagnostic)] enum FnAlignSuggestion { #[note(passes_align_present_note)] OnlyAlign, #[note(passes_align_note)] SupportsAlign, }

And then using Option<FnAlignSuggestion>. This avoid needing to add two fields to every diagnostic, and needing to repeat the note several times in the fluent grammar.

This is kinda unrelated to your change here, though -- I'd somewhat rather you break this into a separate PR so we can refine it separately.

yea that's reasonable, just a moment :)

thanks now I also understand properly how to use subdiagnostics like this :)

compiler-errors · 2024-08-27T14:38:26Z

One last thing -- the second and fourth commits should be squashed into the first IMO.

workingjubilee · 2024-08-27T16:22:19Z

( tests are implied. )

workingjubilee · 2024-08-27T16:23:05Z

( yes, this is stated with a long, meaningful gaze at people whomst do not add tests. )
( maybe a long meaningful gaze at jubilee. )

workingjubilee

Cool! I'm happy with this if errs is happy.

compiler-errors · 2024-08-27T17:09:04Z

@bors r=workingjubilee,compiler-errors

feel free to assign me any diagnostics tweak prs as follow-up

bors · 2024-08-27T17:09:07Z

📌 Commit a507ec6 has been approved by workingjubilee,compiler-errors

It is now in the queue for this repository.

compiler-errors · 2024-08-27T17:09:27Z

@bors rollup

…iaskrgr Rollup of 11 pull requests Successful merges: - rust-lang#129421 (add repr to the allowlist for naked functions) - rust-lang#129480 (docs: correct panic conditions for rem_euclid and similar functions) - rust-lang#129551 (ub_checks intrinsics: fall back to cfg(ub_checks)) - rust-lang#129608 (const-eval: do not make UbChecks behavior depend on current crate's flags) - rust-lang#129613 (interpret: do not make const-eval query result depend on tcx.sess) - rust-lang#129641 (rustdoc: fix missing resource suffix on `crates.js`) - rust-lang#129657 (Rename `BikeshedIntrinsicFrom` to `TransmuteFrom`) - rust-lang#129666 (interpret: add missing alignment check in raw_eq) - rust-lang#129667 (Rustc driver cleanup) - rust-lang#129668 (Fix Pin::set bounds regression) - rust-lang#129686 (coverage: Rename `CodeRegion` to `SourceRegion`) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#129421 - jdonszelmann:naked-repr-align-functions, r=workingjubilee,compiler-errors add repr to the allowlist for naked functions Fixes rust-lang#129412 (combining unstable features rust-lang#90957 (`#![feature(naked_functions)]`) and rust-lang#82232 (`#![feature(fn_align)]`)

rustbot assigned wesleywiser Aug 22, 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 Aug 22, 2024

rustbot assigned workingjubilee and unassigned wesleywiser Aug 22, 2024

compiler-errors reviewed Aug 22, 2024

View reviewed changes

tests/codegen/naked-fn/aligned.rs Show resolved Hide resolved

jdonszelmann requested a review from workingjubilee August 23, 2024 15:40

workingjubilee requested changes Aug 24, 2024

View reviewed changes

rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Aug 24, 2024

jdonszelmann force-pushed the naked-repr-align-functions branch from 45f8724 to 87beb43 Compare August 27, 2024 13:31

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 Aug 27, 2024

compiler-errors reviewed Aug 27, 2024

View reviewed changes

workingjubilee reviewed Aug 27, 2024

View reviewed changes

compiler-errors reviewed Aug 27, 2024

View reviewed changes

jdonszelmann force-pushed the naked-repr-align-functions branch from 59a53eb to dfd90d0 Compare August 27, 2024 15:17

jdonszelmann added 2 commits August 27, 2024 17:17

add repr to the allowlist for naked functions, and test that it works

c3000ad

add uitest for naked functions and the repr attr on functions

a507ec6

jdonszelmann force-pushed the naked-repr-align-functions branch from dfd90d0 to a507ec6 Compare August 27, 2024 15:18

workingjubilee changed the title ~~add repr to the allowlist for naked funcitons, and test that it works~~ add repr to the allowlist for naked functions Aug 27, 2024

workingjubilee approved these changes Aug 27, 2024

View reviewed changes

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 Aug 27, 2024

matthiaskrgr mentioned this pull request Aug 28, 2024

Rollup of 11 pull requests #129691

Merged

bors merged commit 99453ce into rust-lang:master Aug 28, 2024
6 checks passed

rustbot added this to the 1.82.0 milestone Aug 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add repr to the allowlist for naked functions #129421

add repr to the allowlist for naked functions #129421

jdonszelmann commented Aug 22, 2024

rustbot commented Aug 22, 2024

jdonszelmann commented Aug 22, 2024

workingjubilee left a comment

workingjubilee Aug 24, 2024

workingjubilee Aug 24, 2024

jdonszelmann Aug 27, 2024

workingjubilee Aug 24, 2024

jdonszelmann commented Aug 24, 2024 •

edited

Loading

jdonszelmann commented Aug 27, 2024 •

edited

Loading

compiler-errors Aug 27, 2024

compiler-errors Aug 27, 2024

workingjubilee Aug 27, 2024

jdonszelmann Aug 27, 2024 •

edited

Loading

compiler-errors Aug 27, 2024

workingjubilee Aug 27, 2024

workingjubilee Aug 27, 2024

jdonszelmann Aug 27, 2024

workingjubilee Aug 27, 2024

compiler-errors Aug 27, 2024 •

edited

Loading

jdonszelmann Aug 27, 2024 •

edited

Loading

jdonszelmann Aug 27, 2024

compiler-errors commented Aug 27, 2024

workingjubilee commented Aug 27, 2024

workingjubilee commented Aug 27, 2024

workingjubilee left a comment

compiler-errors commented Aug 27, 2024

bors commented Aug 27, 2024

compiler-errors commented Aug 27, 2024

	#[naked] //~ ERROR should be applied to a function definition
	#[repr(C)]
	struct S {
	a: u32,
	b: u32,
	}

add repr to the allowlist for naked functions #129421

add repr to the allowlist for naked functions #129421

Conversation

jdonszelmann commented Aug 22, 2024

rustbot commented Aug 22, 2024

jdonszelmann commented Aug 22, 2024

workingjubilee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdonszelmann commented Aug 24, 2024 • edited Loading

jdonszelmann commented Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdonszelmann Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

jdonszelmann Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors commented Aug 27, 2024

workingjubilee commented Aug 27, 2024

workingjubilee commented Aug 27, 2024

workingjubilee left a comment

Choose a reason for hiding this comment

compiler-errors commented Aug 27, 2024

bors commented Aug 27, 2024

compiler-errors commented Aug 27, 2024

jdonszelmann commented Aug 24, 2024 •

edited

Loading

jdonszelmann commented Aug 27, 2024 •

edited

Loading

jdonszelmann Aug 27, 2024 •

edited

Loading

compiler-errors Aug 27, 2024 •

edited

Loading

jdonszelmann Aug 27, 2024 •

edited

Loading