Align attr fixes #143206

Jules-Bertholet · 2025-06-29T17:05:48Z

Remove references to the superseded repr(align) syntax
Allow the attribute on fn items in extern blocks
Test attribute in combination with async fn and dyn

r? workingjubilee

Tracking issue: #82232
@rustbot label A-attributes F-fn_align T-compiler

rustbot · 2025-06-29T17:05:53Z

rustbot has assigned @WaffleLapkin.
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 · 2025-06-29T17:05:55Z

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

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

cc @jdonszelmann

rustbot · 2025-06-29T19:13:40Z

workingjubilee is currently at their maximum review capacity.
They may take a while to respond.

bors · 2025-07-01T09:35:37Z

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

workingjubilee · 2025-07-01T18:44:54Z

This requires rebase.

workingjubilee · 2025-07-01T18:45:11Z

@rustbot author

rustbot · 2025-07-01T18:45:14Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

Jules-Bertholet · 2025-07-01T19:00:11Z

@rustbot ready

workingjubilee

This is good as-is but I would like to see more tests. You can add them now or as a followup PR or open an issue instead. r=me either way.

tests/codegen/align-fn.rs

workingjubilee · 2025-07-01T19:35:45Z

tests/codegen/align-fn.rs

+    #[align(32)]
+    #[align(256)]
+    extern "C" fn align_unmangled() {}


Weren't we going to warn on applying align twice?

I mean that shouldn't affect the codegen test, I'm just realizing that I don't know for sure if we have a test that does check that we warn.

It would be nice to have a warning, but it’s not required for the MVP. It hasn’t been implemented yet.

workingjubilee · 2025-07-01T19:38:33Z

tests/ui/attributes/fn-align-dyn.rs

+#![feature(fn_align)]
+
+trait Test {
+    #[align(4096)]


4096 is pretty big but I am realizing I don't see a test for oversized aligns that we should definitely reject, I think.

4096 I believe is the highest align that should work more or less everywhere. Because this test relies on converting to a function pointer, choosing a high value minimizes the chance of “getting lucky”.

Rejecting high alignments on platforms that don’t support them is being worked on by others, I think.

tests/ui/attributes/malformed-fn-align.rs

workingjubilee · 2025-07-01T19:44:58Z

@bors delegate+

bors · 2025-07-01T19:45:02Z

✌️ @Jules-Bertholet, you can now approve this pull request!

If @workingjubilee told you to "r=me" after making some further change, please make that change, then do @bors r=@workingjubilee

workingjubilee · 2025-07-01T19:50:00Z

It's also possible that the tests already exist and I was just too ferrisClueless.jpg to find them

Jules-Bertholet · 2025-07-01T20:16:43Z

@bors r=@workingjubilee

bors · 2025-07-01T20:16:46Z

📌 Commit 440bf29 has been approved by workingjubilee

It is now in the queue for this repository.

…r=workingjubilee Align attr fixes - Remove references to the superseded `repr(align)` syntax - Allow the attribute on fn items in `extern` blocks - Test attribute in combination with `async fn` and `dyn` r? workingjubilee `@rustbot` label A-attributes F-fn_align T-compiler

RalfJung · 2025-07-03T06:16:31Z

In wasm, functions don't have an "address", it's not a von-Neumann architecture. Instead, there is a dedicated table and one can pick an integer index into the table when doing a dynamic call.

I don't know how exactly LLVM translates fn ptrs to wasm. Is the fn ptr directly the index into that table? Then indeed #[align] on functions just doesn't make sense... LLVM would have to put functions at even indices into the table, or something like that.

Cc @alexcrichton

Jules-Bertholet · 2025-07-03T11:54:42Z

Instead, there is a dedicated table and one can pick an integer index into the table when doing a dynamic call.

Yes. My thinking is that LLVM could fill the table with dummy entries in order to align things.

RalfJung · 2025-07-03T12:24:14Z

It's completely silly to "align" things in that table, though... isn't it?

Jules-Bertholet · 2025-07-03T12:45:35Z

If you are doing function pointer tagging, it might be necessary. But yes, the usefulness is limited.

folkertdev · 2025-07-03T12:49:33Z

So, should we just apply a target-specific minimum of #[align(1)] for wasm? (much like on windows targets the limit will be 8192)

RalfJung · 2025-07-03T12:52:45Z

We should definitely have an issue for this rather than discussing this in a PR, so I made one: #143368.

RalfJung · 2025-07-03T12:54:20Z

For this PR I think we can just mark the test as skipped for wasm with a link to the issue.

(I also added a link to the tracking issue, those are super useful when figuring out how a feature changed...)

alexcrichton · 2025-07-03T14:41:14Z

Looks like this has already mostly been answered, but can confirm:

I don't know how exactly LLVM translates fn ptrs to wasm. Is the fn ptr directly the index into that table?

This is correct, function pointers in wasm are table indices (e.g. 1, 2, 3, ...). The table index 0 is skipped to provide the guarantee that function pointers are never 0/null (also that 0/null is an invalid function pointer)

bors · 2025-07-06T20:50:08Z

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

workingjubilee · 2025-07-07T00:58:02Z

@bors r+

bors · 2025-07-07T00:58:04Z

📌 Commit 8f86c4a has been approved by workingjubilee

It is now in the queue for this repository.

Rollup of 9 pull requests Successful merges: - #143206 (Align attr fixes) - #143236 (Stabilize `mixed_integer_ops_unsigned_sub`) - #143344 (Port `#[path]` to the new attribute parsing infrastructure ) - #143359 (Link to 2024 edition page for `!` fallback changes) - #143456 (mbe: Change `unused_macro_rules` to a `DenseBitSet`) - #143529 (Renamed retain_mut to retain on LinkedList as mentioned in the ACP) - #143535 (Remove duplicate word) - #143544 (compiler: rename BareFn to FnPtr) - #143552 (lib: more eagerly return `self.len()` from `ceil_char_boundary`) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of #143206 - Jules-Bertholet:align-attr-fixes, r=workingjubilee Align attr fixes - Remove references to the superseded `repr(align)` syntax - Allow the attribute on fn items in `extern` blocks - Test attribute in combination with `async fn` and `dyn` r? workingjubilee Tracking issue: #82232 `@rustbot` label A-attributes F-fn_align T-compiler

rustbot assigned WaffleLapkin Jun 29, 2025

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 Jun 29, 2025

rustbot added the F-fn_align `#![feature(fn_align)]` label Jun 29, 2025

rustbot assigned workingjubilee Jun 29, 2025

rustbot unassigned WaffleLapkin Jun 29, 2025

rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 1, 2025

rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 1, 2025

Jules-Bertholet force-pushed the align-attr-fixes branch from 91791f5 to 0382357 Compare July 1, 2025 19:00

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 1, 2025

workingjubilee approved these changes Jul 1, 2025

View reviewed changes

Jules-Bertholet force-pushed the align-attr-fixes branch from 2e6c84a to 440bf29 Compare July 1, 2025 20:16

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 1, 2025

Jules-Bertholet mentioned this pull request Jul 2, 2025

Stabilize -Cmin-function-alignment #142824

Open

RalfJung mentioned this pull request Jul 3, 2025

Function alignment does not work on wasm #143368

Open

Jules-Bertholet added 6 commits July 6, 2025 16:56

Remove repr(align) code

97a7b9b

Support #[align(…)] on fns in extern blocks

a1cefee

Test async fn

ce8ab08

Add test for dyn alignment

a144fd6

Add FIXME for gen et al

1c93e16

Add more tests for invalid alignments

196e3ed

Jules-Bertholet force-pushed the align-attr-fixes branch from 440bf29 to 196e3ed Compare July 6, 2025 21:09

Skip align tests on wasm

8f86c4a

Jules-Bertholet force-pushed the align-attr-fixes branch from 1995282 to 8f86c4a Compare July 6, 2025 22:11

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 7, 2025

jhpratt mentioned this pull request Jul 7, 2025

Rollup of 9 pull requests #143556

Merged

bors merged commit d748e70 into rust-lang:master Jul 7, 2025
11 checks passed

rustbot added this to the 1.90.0 milestone Jul 7, 2025

Jules-Bertholet deleted the align-attr-fixes branch July 7, 2025 20:40

Align attr fixes #143206

Align attr fixes #143206

Conversation

Jules-Bertholet commented Jun 29, 2025 • edited by RalfJung Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Jun 29, 2025

Uh oh!

rustbot commented Jun 29, 2025

Uh oh!

rustbot commented Jun 29, 2025

Uh oh!

bors commented Jul 1, 2025

Uh oh!

workingjubilee commented Jul 1, 2025

Uh oh!

workingjubilee commented Jul 1, 2025

Uh oh!

rustbot commented Jul 1, 2025

Uh oh!

Jules-Bertholet commented Jul 1, 2025

Uh oh!

workingjubilee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

workingjubilee Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

Jules-Bertholet Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

workingjubilee Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

Jules-Bertholet Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

workingjubilee commented Jul 1, 2025

Uh oh!

bors commented Jul 1, 2025

Uh oh!

workingjubilee commented Jul 1, 2025

Uh oh!

Jules-Bertholet commented Jul 1, 2025

Uh oh!

bors commented Jul 1, 2025

Uh oh!

RalfJung commented Jul 3, 2025

Uh oh!

Jules-Bertholet commented Jul 3, 2025

Uh oh!

RalfJung commented Jul 3, 2025

Uh oh!

Jules-Bertholet commented Jul 3, 2025

Uh oh!

folkertdev commented Jul 3, 2025

Uh oh!

RalfJung commented Jul 3, 2025

Uh oh!

RalfJung commented Jul 3, 2025

Uh oh!

alexcrichton commented Jul 3, 2025

Uh oh!

bors commented Jul 6, 2025

Uh oh!

workingjubilee commented Jul 7, 2025

Uh oh!

bors commented Jul 7, 2025

Uh oh!

Uh oh!

Uh oh!

Jules-Bertholet commented Jun 29, 2025 •

edited by RalfJung

Loading