error on alignments greater than `isize::MAX` #131633

asquared31415 · 2024-10-13T00:15:02Z

On zulip someone had a concern that it was legal to create a type with alignment isize::MAX + 1, but I do not believe this should be allowed for repr(align), as repr(align) also increases the size of the type, and that would be larger than the maximum allowed size of objects.

(cc @workingjubilee #131276)

rustbot · 2024-10-13T00:15:10Z

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

compiler/rustc_passes/src/check_attr.rs

asquared31415 · 2024-10-13T02:38:06Z

"test compilation failed even though it shouldn't"... but it's a build-fail test???

jieyouxu · 2024-10-13T06:34:01Z

"test compilation failed even though it shouldn't"... but it's a build-fail test???

#130894

nnethercote · 2024-10-14T04:06:40Z

r? @workingjubilee

zachs18 · 2024-10-16T22:24:44Z

as repr(align) also increases the size of the type, and that would be larger than the maximum allowed size of objects.

repr(align) can be applied to unit/ZST structs without increasing their size, so #[repr(align(32768))] struct ZST; is possible on 16-bit targets and currently does not error. (though it might be enough of an edge case to disallow anyway).

#![no_std]

#[repr(align(32768))]
struct Hello;

pub fn foo() -> usize { core::mem::align_of::<Hello>() }

This currently compiles fine with cargo +nightly build -Z build-std=core --target avr-unknown-gnu-atmega328 --release, but fails on this branch with

error[E0589]: alignment must not be greater than `isize::MAX` bytes
 --> src/lib.rs:4:8
  |
4 | #[repr(align(32768))]
  |        ^^^^^^^^^^^^
  |
  = note: `isize::MAX` is 32767 for the current target

For more information about this error, try `rustc --explain E0589`.

workingjubilee · 2024-10-16T23:30:16Z

cc @Patryk27 as prospective maintainer for AVR
cc @cr1901 because you're the one who comes to mind re: MSP430

You are the only targets affected by this, as all other targets have a bound on repr(align(N)) that is already below isize::MAX, functionally imposed by our codegen backends.

Do you think this #[repr(align(32768))] struct Hello16BitAlign; has any real use-case on AVR? Even a somewhat silly one?

Note that #[repr(align(16384))] struct Hello16BitSize(u8); is the limit for actually-sized types.

asquared31415 · 2024-10-17T00:47:26Z

ah i see, i was under the impression repr(align) increased the size for structs that would otherwise be 1-ZSTs, i can probably add an exception if it's needed then? i do think this is a slightly weird use case, but i'm mostly neutral on this, and dont work with any 16 bit targets for rust to know what they might need.

cr1901 · 2024-10-17T05:40:09Z

I've never used Rust on an MSP430 with more than 10kB (8+2kB) of RAM. MSP430's max out at 512kB ROM and 66kB RAM.

However, from what I can gather, actual existing MSP430s top out at 18kB (16+2kB) of RAM without relying on stuff unsupported by LLVM. Given such constraints, #[repr(align(32768))]- or even #[repr(align(16384))], or #[repr(align(8192))], etc- isn't usable in practice.

If LLVM gets these features to access 20-bit addresses, I don't see how I'd use such functionality for RAM; you only have 64kB continguous RAM anyway- the other 2kB is a ~~marketing gimmick~~ "bonus" if you're not using the USB peripheral, at a much lower address than the rest of RAM. And having 20 ZSTs 32kB apart in ROM doesn't seem that useful either.

Patryk27 · 2024-10-17T05:59:03Z

Do you think this #[repr(align(32768))] struct Hello16BitAlign; has any real use-case on AVR? Even a somewhat silly one?

I don't think so - I mean, I think ideally we'd throw this error on instantiating the struct, so that a mere fact of such type existing wasn't a failure (as in @zachs18's example), but I think there's no point in doing more complex analysis considering it's already quite an edge case.

workingjubilee · 2024-10-17T06:06:35Z

Okay!

Then if people using the targets cannot think of a good reason for us to support an alignment greater than isize::MAX, zero-sized type or no: let us not, and keep the rule consistent.

workingjubilee · 2024-10-17T06:21:15Z

Unfortunately I don't know that this is the right place to emit this check, so having determined that, its implementation seems correct, but it would be good if someone with experience with rustc_passes could actually weigh in.

workingjubilee · 2024-10-18T03:07:58Z

hmm, recent history (and a coinflip) says...

r? @jieyouxu

asquared31415 · 2024-10-18T04:07:04Z

Unfortunately I don't know that this is the right place to emit this check [...]

I explored an alternative approach doing the error in the same location that the 2^64 check was emitted in rustc_attr::builtin::parse_alignment, but was unsatisfied with how the &'static str error type was being used and changing the call sites seemed non-trivial because it is used for repr(packed) as well.

... on further thought, should this isize::MAX check be done for repr(packed(N))? Since repr(packed) can only lower alignment, it's probably not necessary?

jieyouxu

This is a reasonable place to check the attribute. I have some nits, but looks good otherwise.

should this isize::MAX check be done for repr(packed(N))? Since repr(packed) can only lower alignment, it's probably not necessary?

That should be in a follow-up if that is determined desirable.

compiler/rustc_passes/src/check_attr.rs

jieyouxu · 2024-10-18T10:45:07Z

compiler/rustc_passes/src/check_attr.rs

+                        // ignore values greater than 2^29, a different error will be emitted
+                        if val <= 2_u64.pow(29) {


Problem: please use a self.dcx().delayed_bug("...") here to assert that an error is emitted

This is a strange one: lets you register an error without emitting it. If compilation ends without any other errors occurring, this will be emitted as a bug. Otherwise, it will be silently dropped. I.e. “expect other errors are emitted” semantics. Useful on code paths that should only be reached when compiling erroneous code.

-- https://doc.rust-lang.org/nightly/nightly-rustc/rustc_errors/enum.Level.html#variant.DelayedBug

Turns out this is good, actually. The delayed bug caught the fact that we didn't even properly check the attr on certain targets in the first place, lmao.

jieyouxu · 2024-10-18T10:47:29Z

@rustbot author

asquared31415 · 2024-10-25T18:27:06Z

@rustbot ready

will open a zulip thread about the potential follow up PR for repr(align), though personally i don't think it's necessary

jieyouxu

Thanks, one tiny nit then r=me.

tests/ui/repr/repr_align_greater_usize.rs

Co-authored-by: Jieyou Xu <jieyouxu@outlook.com>

jieyouxu · 2024-10-28T05:23:37Z

Dropped the redundant check-fail, no other changes.
@bors r+ rollup=iffy

bors · 2024-10-28T05:23:40Z

📌 Commit 6fc7ce4 has been approved by jieyouxu

It is now in the queue for this repository.

Rollup of 7 pull requests Successful merges: - rust-lang#131633 (error on alignments greater than `isize::MAX`) - rust-lang#132086 (Tweak E0277 highlighting and "long type" path printing) - rust-lang#132220 (Add GUI regression test for doc struct fields margins) - rust-lang#132225 (Dynamically link run-make support) - rust-lang#132227 (Pass constness with span into lower_poly_trait_ref) - rust-lang#132242 (Support `char::is_digit` in const contexts.) - rust-lang#132243 (Remove `ObligationCause::span()` method) r? `@ghost` `@rustbot` modify labels: rollup

bors · 2024-10-28T09:58:06Z

⌛ Testing commit 6fc7ce4 with merge 66701c4...

Rollup merge of rust-lang#131633 - asquared31415:align_isize_max, r=jieyouxu error on alignments greater than `isize::MAX` Fixes rust-lang#131122 On zulip someone had a concern that it was legal to create a type with alignment `isize::MAX + 1`, but I do not believe this should be allowed for `repr(align)`, as `repr(align)` also increases the *size* of the type, and that would be larger than the maximum allowed size of objects. (cc `@workingjubilee` rust-lang#131276)

rustbot assigned nnethercote Oct 13, 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 Oct 13, 2024

jieyouxu reviewed Oct 13, 2024

View reviewed changes

compiler/rustc_passes/src/check_attr.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

rust-cloud-vms bot force-pushed the align_isize_max branch from d11b576 to 0f982de Compare October 13, 2024 02:03

This comment has been minimized.

Sign in to view

rust-cloud-vms bot force-pushed the align_isize_max branch from 0f982de to aead2c5 Compare October 13, 2024 17:59

This comment has been minimized.

Sign in to view

rust-cloud-vms bot force-pushed the align_isize_max branch from aead2c5 to ec07317 Compare October 13, 2024 19:51

rustbot assigned workingjubilee and unassigned nnethercote Oct 14, 2024

rustbot assigned jieyouxu and unassigned workingjubilee Oct 18, 2024

jieyouxu reviewed Oct 18, 2024

View reviewed changes

rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 18, 2024

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

rust-cloud-vms bot force-pushed the align_isize_max branch from ec07317 to aa0b3c7 Compare October 25, 2024 16:19

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 Oct 25, 2024

jieyouxu approved these changes Oct 28, 2024

View reviewed changes

tests/ui/repr/repr_align_greater_usize.rs Outdated Show resolved Hide resolved

Error on alignments greater than isize::MAX

6fc7ce4

Co-authored-by: Jieyou Xu <jieyouxu@outlook.com>

jieyouxu force-pushed the align_isize_max branch from aa0b3c7 to 6fc7ce4 Compare October 28, 2024 05:22

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 Oct 28, 2024

jieyouxu mentioned this pull request Oct 28, 2024

Rollup of 7 pull requests #132251

Merged

bors merged commit 95a9d49 into rust-lang:master Oct 28, 2024
6 checks passed

rustbot added this to the 1.84.0 milestone Oct 28, 2024

jieyouxu mentioned this pull request Oct 31, 2024

ICE: malformed malformed repr(align(N)) #132391

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

error on alignments greater than `isize::MAX` #131633

error on alignments greater than `isize::MAX` #131633

asquared31415 commented Oct 13, 2024

rustbot commented Oct 13, 2024

This comment has been minimized.

This comment has been minimized.

asquared31415 commented Oct 13, 2024

jieyouxu commented Oct 13, 2024

This comment has been minimized.

nnethercote commented Oct 14, 2024

zachs18 commented Oct 16, 2024

workingjubilee commented Oct 16, 2024 •

edited

Loading

asquared31415 commented Oct 17, 2024

cr1901 commented Oct 17, 2024 •

edited

Loading

Patryk27 commented Oct 17, 2024

workingjubilee commented Oct 17, 2024

workingjubilee commented Oct 17, 2024

workingjubilee commented Oct 18, 2024

asquared31415 commented Oct 18, 2024 •

edited

Loading

jieyouxu left a comment •

edited

Loading

jieyouxu Oct 18, 2024

jieyouxu Oct 31, 2024

jieyouxu commented Oct 18, 2024

asquared31415 commented Oct 25, 2024

jieyouxu left a comment

jieyouxu commented Oct 28, 2024

bors commented Oct 28, 2024

bors commented Oct 28, 2024

		// ignore values greater than 2^29, a different error will be emitted
		if val <= 2_u64.pow(29) {

error on alignments greater than isize::MAX #131633

error on alignments greater than isize::MAX #131633

Conversation

asquared31415 commented Oct 13, 2024

rustbot commented Oct 13, 2024

This comment has been minimized.

This comment has been minimized.

asquared31415 commented Oct 13, 2024

jieyouxu commented Oct 13, 2024

This comment has been minimized.

nnethercote commented Oct 14, 2024

zachs18 commented Oct 16, 2024

workingjubilee commented Oct 16, 2024 • edited Loading

asquared31415 commented Oct 17, 2024

cr1901 commented Oct 17, 2024 • edited Loading

Patryk27 commented Oct 17, 2024

workingjubilee commented Oct 17, 2024

workingjubilee commented Oct 17, 2024

workingjubilee commented Oct 18, 2024

asquared31415 commented Oct 18, 2024 • edited Loading

jieyouxu left a comment • edited Loading

Choose a reason for hiding this comment

jieyouxu Oct 18, 2024

Choose a reason for hiding this comment

jieyouxu Oct 31, 2024

Choose a reason for hiding this comment

jieyouxu commented Oct 18, 2024

asquared31415 commented Oct 25, 2024

jieyouxu left a comment

Choose a reason for hiding this comment

jieyouxu commented Oct 28, 2024

bors commented Oct 28, 2024

bors commented Oct 28, 2024

error on alignments greater than `isize::MAX` #131633

error on alignments greater than `isize::MAX` #131633

workingjubilee commented Oct 16, 2024 •

edited

Loading

cr1901 commented Oct 17, 2024 •

edited

Loading

asquared31415 commented Oct 18, 2024 •

edited

Loading

jieyouxu left a comment •

edited

Loading