Skip to content

fix for multiple #[repr(align(N))] on functions #139917

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

Merged
merged 1 commit into from
Apr 17, 2025

Conversation

folkertdev
Copy link
Contributor

@folkertdev folkertdev commented Apr 16, 2025

tracking issue: #82232
fixes #132464

The behavior of align is specified at https://doc.rust-lang.org/reference/type-layout.html#r-layout.repr.alignment.align

For align, if the specified alignment is less than the alignment of the type without the align modifier, then the alignment is unaffected.

So in effect that means that the maximum of the specified alignments should be chosen. That is also the current behavior for align on ADTs:

#![feature(fn_align)]

#[repr(C,  align(32), align(64))]
struct Foo {
    x: u64,
}

const _: () = assert!(core::mem::align_of::<Foo>() == 64);

// See the godbolt LLVM output: the alignment of this function is 32
#[no_mangle]
#[repr(align(32))]
#[repr(align(64))]
fn foo() {}

// The current logic just picks the first alignment: the alignment of this function is 64
#[no_mangle]
#[repr(align(64))]
#[repr(align(32))]
fn bar() {}

https://godbolt.org/z/scco435jE

attr::ReprAlign(align) => {
max_align = max_align.max(Some(align));
ReprFlags::empty()
}

The #132464 issue is really about parsing/representing the attribute, which has already been improved and now uses the "parse, don't validate" attribute approach. That means the behavior is already different from what the issue describes: on current main, the first value is chosen. This PR fixes a logic error, where we just did not check for the effect of two or more align modifiers. In combination, that fixes the issue.

cc @jdonszelmann if you do have further thoughs here

@rustbot
Copy link
Collaborator

rustbot commented Apr 16, 2025

r? @nnethercote

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

@rustbot rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) 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 16, 2025
@rustbot
Copy link
Collaborator

rustbot commented Apr 16, 2025

Some changes occurred in compiler/rustc_codegen_ssa/src/codegen_attrs.rs

cc @jdonszelmann

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

@jdonszelmann
Copy link
Contributor

I'll give this a review in a sec :)

@jdonszelmann
Copy link
Contributor

jdonszelmann commented Apr 16, 2025

This looks good! Simple change, and also closes the issue. Nice :) @bors r+ rollup

@bors
Copy link
Collaborator

bors commented Apr 16, 2025

📌 Commit a6dcd51 has been approved by jdonszelmann

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 16, 2025
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 16, 2025
…onszelmann

fix for multiple `#[repr(align(N))]` on functions

tracking issue: rust-lang#82232
fixes rust-lang#132464

The behavior of align is specified at https://doc.rust-lang.org/reference/type-layout.html#r-layout.repr.alignment.align

> For align, if the specified alignment is less than the alignment of the type without the align modifier, then the alignment is unaffected.

So in effect that means that the maximum of the specified alignments should be chosen. That is also the current behavior for `align` on ADTs:

```rust
#![feature(fn_align)]

#[repr(C,  align(32), align(64))]
struct Foo {
    x: u64,
}

const _: () = assert!(core::mem::align_of::<Foo>() == 64);

// See the godbolt LLVM output: the alignment of this function is 32
#[no_mangle]
#[repr(align(32))]
#[repr(align(64))]
fn foo() {}

// The current logic just picks the first alignment: the alignment of this function is 64
#[no_mangle]
#[repr(align(64))]
#[repr(align(32))]
fn bar() {}
```

https://godbolt.org/z/scco435jE

https://github.com./rust-lang/rust/blob/afa859f8121bf2985362a2c8414dc71a825ccf2d/compiler/rustc_middle/src/ty/mod.rs#L1529-L1532

The rust-lang#132464 issue is really about parsing/representing the attribute, which has already been improved and now uses the "parse, don't validate" attribute approach. That means the behavior is already different from what the issue describes: on current `main`, the first value is chosen. This PR fixes a logic error, where we just did not check for the effect of two or more `align` modifiers. In combination, that fixes the issue.

cc `@jdonszelmann` if you do have further thoughs here
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 17, 2025
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#139084 (hygiene: Rename semi-transparent to semi-opaque)
 - rust-lang#139236 (Use a session counter to make anon dep nodes unique)
 - rust-lang#139650 (Fix `register_group_alias` for tools)
 - rust-lang#139770 (Rename `LifetimeName` as `LifetimeKind`.)
 - rust-lang#139846 (Remove `kw::Empty` uses in rustdoc)
 - rust-lang#139891 (Include optional dso_local marker for functions in `enum-match.rs`)
 - rust-lang#139908 (parser: Remove old diagnostic notes for type ascription syntax)
 - rust-lang#139917 (fix for multiple `#[repr(align(N))]` on functions)

Failed merges:

 - rust-lang#139615 (Remove `name_or_empty`)

r? `@ghost`
`@rustbot` modify labels: rollup
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 17, 2025
Rollup merge of rust-lang#139917 - folkertdev:fn-align-multiple, r=jdonszelmann

fix for multiple `#[repr(align(N))]` on functions

tracking issue: rust-lang#82232
fixes rust-lang#132464

The behavior of align is specified at https://doc.rust-lang.org/reference/type-layout.html#r-layout.repr.alignment.align

> For align, if the specified alignment is less than the alignment of the type without the align modifier, then the alignment is unaffected.

So in effect that means that the maximum of the specified alignments should be chosen. That is also the current behavior for `align` on ADTs:

```rust
#![feature(fn_align)]

#[repr(C,  align(32), align(64))]
struct Foo {
    x: u64,
}

const _: () = assert!(core::mem::align_of::<Foo>() == 64);

// See the godbolt LLVM output: the alignment of this function is 32
#[no_mangle]
#[repr(align(32))]
#[repr(align(64))]
fn foo() {}

// The current logic just picks the first alignment: the alignment of this function is 64
#[no_mangle]
#[repr(align(64))]
#[repr(align(32))]
fn bar() {}
```

https://godbolt.org/z/scco435jE

https://github.com./rust-lang/rust/blob/afa859f8121bf2985362a2c8414dc71a825ccf2d/compiler/rustc_middle/src/ty/mod.rs#L1529-L1532

The rust-lang#132464 issue is really about parsing/representing the attribute, which has already been improved and now uses the "parse, don't validate" attribute approach. That means the behavior is already different from what the issue describes: on current `main`, the first value is chosen. This PR fixes a logic error, where we just did not check for the effect of two or more `align` modifiers. In combination, that fixes the issue.

cc ``@jdonszelmann`` if you do have further thoughs here
@bors bors merged commit cbe469a into rust-lang:master Apr 17, 2025
6 checks passed
@rustbot rustbot added this to the 1.88.0 milestone Apr 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) 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.

Multiple alignments on functions (#![feature(fn_align)])
5 participants