Skip to content

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

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

Closed
jdonszelmann opened this issue Nov 1, 2024 · 2 comments · Fixed by #139917
Closed

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

jdonszelmann opened this issue Nov 1, 2024 · 2 comments · Fixed by #139917
Assignees
Labels
A-align Area: alignment control (`repr(align(N))` and so on) A-repr Area: the `#[repr(stuff)]` attribute C-bug Category: This is a bug. F-fn_align `#![feature(fn_align)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@jdonszelmann
Copy link
Contributor

jdonszelmann commented Nov 1, 2024

This code specifies two alignments, but applies none. A single align(256) does work

#![feature(fn_align)]


#[repr(align(256), align(256))]
pub fn main() {
    let ptr = main as *const u8;
    println!("{ptr:?}");

    assert_eq!(ptr.align_offset(256), 0);
}

See here: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=f841ae6318f0f7abdab285ea9ec641ab

CC: #82232

The culprit is this line here matching on slices of length 1:

codegen_fn_attrs.alignment = if let Some(items) = attr.meta_item_list()
&& let [item] = items.as_slice()
&& let Some((sym::align, literal)) = item.singleton_lit_list()

It's a one line fix, but honestly this is trivially resolved with rust-lang/compiler-team#796 which I'm working on. I'll make it a separate PR at some point, but I'll assign myself since it makes sure changes conflict a little less :)

@rustbot claim

@jdonszelmann jdonszelmann added the C-bug Category: This is a bug. label Nov 1, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Nov 1, 2024
@jieyouxu jieyouxu added F-fn_align `#![feature(fn_align)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Nov 1, 2024
@workingjubilee workingjubilee added A-align Area: alignment control (`repr(align(N))` and so on) and removed F-fn_align `#![feature(fn_align)]` labels Nov 1, 2024
@jdonszelmann
Copy link
Contributor Author

@rustbot label +A-repr +A-align

@rustbot rustbot added the A-repr Area: the `#[repr(stuff)]` attribute label Nov 1, 2024
@workingjubilee workingjubilee added F-fn_align `#![feature(fn_align)]` A-align Area: alignment control (`repr(align(N))` and so on) and removed A-align Area: alignment control (`repr(align(N))` and so on) labels Nov 1, 2024
@folkertdev
Copy link
Contributor

This problem has been partially solved, but there is still a logic error when more than one alignment is specified.

Currently, the logic picks the first alignment that is given. That is inconsistent with alignment on data structures which picks the highest specified alignment.

#![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() {}!("{}", core::mem::size_of::<Foo>());
}

https://godbolt.org/z/scco435jE

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 value should be chosen.

@jdonszelmann is it OK if I open a PR fixing this final problem (given that you've claimed the issue)? Maybe something attribute-related is not done yet?

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue 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 bors closed this as completed in cbe469a Apr 17, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this issue 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
github-actions bot pushed a commit to rust-lang/rustc-dev-guide that referenced this issue Apr 19, 2025
fix for multiple `#[repr(align(N))]` on functions

tracking issue: rust-lang/rust#82232
fixes rust-lang/rust#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/rust#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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-align Area: alignment control (`repr(align(N))` and so on) A-repr Area: the `#[repr(stuff)]` attribute C-bug Category: This is a bug. F-fn_align `#![feature(fn_align)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants