Avoid alignment mismatch between ABI and layout for unions. #104872

luqmana · 2022-11-25T07:15:09Z

compiler/rustc_abi/src/layout.rs

eddyb · 2022-11-28T08:23:00Z

I would prefer if this was fixed by #103926 - specifically, this comment: #103926 (comment).

If you can arrange with @oli-obk, you could take over in this PR - I don't care as long as both alignment sources are fixed at once without special-casing them.

Also, we need a test for #[repr(packed)] union Foo { zst: [u16; 0], byte: u8 }, which should, by virtue of #[repr(packed)] reducing alignment back, be able to be Abi::Scalar again!

luqmana · 2022-11-28T21:04:43Z

I would prefer if this was fixed by #103926 - specifically, this comment: #103926 (comment).

If you can arrange with @oli-obk, you could take over in this PR - I don't care as long as both alignment sources are fixed at once without special-casing them.

Ah, admittedly I did see that issue but didn't dig too deep into it. I'm ok with taking a stab at it or leaving it to @oli-obk

oli-obk · 2022-11-29T07:37:19Z

I'm ok with taking a stab at it or leaving it to @oli-obk

I won't get to it this week or next week, so feel free to take over!

bors · 2022-12-11T12:30:31Z

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

compiler/rustc_abi/src/layout.rs

compiler/rustc_abi/src/lib.rs

compiler/rustc_abi/src/layout.rs

apiraino · 2023-02-02T13:26:32Z

Switching to waiting on author to incorporate changes. Feel free to request a review with @rustbot ready, thanks!

@rustbot author

luqmana · 2023-02-20T00:45:00Z

@rustbot ready

wesleywiser · 2023-03-16T14:41:28Z

Visited during the compiler team triage meeting. It looks like @eddyb's feedback has been taken into account and resolved. Reassigning to @oli-obk for review. Feel free to reroll if you would prefer!

r? @oli-obk

oli-obk

just a nit, then this lgtm

oli-obk · 2023-03-16T16:22:24Z

compiler/rustc_abi/src/layout.rs

-                        Abi::Vector { element: x.to_union(), count }
+                let field_abi = field.abi().to_union();
+
+                if let Ok(Some((common_abi, common_align))) = &mut common_non_zst_abi_and_align {


This relies on the !is_err check above. Maybe replace that check with a let Ok(common_non_zst_abi_and_align) = &mut common_non_zst_abi_and_align to make sure these two things stay in sync

Suggested change

if let Ok(Some((common_abi, common_align))) = &mut common_non_zst_abi_and_align {

if let Some((common_abi, common_align)) = &mut common_non_zst_abi_and_align {

Sure, reordered it a bit to remove the is_err check.

bors · 2023-03-26T14:56:43Z

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

pnkfelix · 2023-05-04T14:34:17Z

@rustbot label: +S-waiting-on-author -S-waiting-on-review

just needs a rebase according to @oli-obk

…ick in.

luqmana · 2023-05-05T23:32:33Z

@rustbot ready

oli-obk · 2023-05-06T05:42:48Z

@bors r+

bors · 2023-05-06T05:42:50Z

📌 Commit 8e7714d has been approved by oli-obk

It is now in the queue for this repository.

bors · 2023-05-06T07:25:53Z

⌛ Testing commit 8e7714d with merge 151a070...

bors · 2023-05-06T10:08:49Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 151a070 to master...

rust-timer · 2023-05-06T11:24:54Z

Finished benchmarking commit (151a070): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.5%	[-2.7%, -2.2%]	2
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 656.275s -> 653.613s (-0.41%)

rustbot assigned eddyb Nov 25, 2022

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 Nov 25, 2022

RalfJung reviewed Nov 25, 2022

View reviewed changes

compiler/rustc_abi/src/layout.rs Outdated Show resolved Hide resolved

luqmana force-pushed the packed-union-align branch from 0faa074 to f5e8b81 Compare November 26, 2022 06:54

eddyb mentioned this pull request Nov 28, 2022

Do not use scalar layout if there are ZSTs with alignment > 1 #103926

Closed

luqmana force-pushed the packed-union-align branch from f5e8b81 to 13a61f5 Compare December 1, 2022 08:37

luqmana force-pushed the packed-union-align branch from 13a61f5 to b73c903 Compare December 19, 2022 02:45

This comment has been minimized.

Sign in to view

oli-obk requested a review from eddyb December 22, 2022 09:25

eddyb suggested changes Jan 10, 2023

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 Feb 2, 2023

luqmana force-pushed the packed-union-align branch from f31fea4 to 2a70829 Compare February 20, 2023 00:26

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 Feb 20, 2023

rustbot assigned oli-obk and unassigned eddyb Mar 16, 2023

oli-obk requested changes Mar 16, 2023

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 May 4, 2023

oli-obk and others added 9 commits May 5, 2023 16:00

Do not use scalar layout if there are ZSTs with alignment > 1

23d09ae

Add helper methods inherent_align and to_union on Abi.

a380053

Incorporate review feedback from 103926.

4f4f22b

Add test cases for rust-lang#104802.

d5ab3a0

Add additional test case for repr(packed) allowing union abi opt to k…

f2d81de

…ick in.

Factor out checks in layout check and add helper inherent_size.

3b1e535

Don't discard preferred alignment in scalar pair.

c63a204

Review feedback

012f9a3

Reorder to keep duplicate checks in sync.

8e7714d

luqmana force-pushed the packed-union-align branch from 2a70829 to 8e7714d Compare May 5, 2023 23: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 May 5, 2023

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 May 6, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label May 6, 2023

bors merged commit 151a070 into rust-lang:master May 6, 2023

rustbot added this to the 1.71.0 milestone May 6, 2023

luqmana deleted the packed-union-align branch May 7, 2023 06:18

	if let Ok(Some((common_abi, common_align))) = &mut common_non_zst_abi_and_align {
	if let Some((common_abi, common_align)) = &mut common_non_zst_abi_and_align {

Avoid alignment mismatch between ABI and layout for unions. #104872

Avoid alignment mismatch between ABI and layout for unions. #104872

Uh oh!

Conversation

luqmana commented Nov 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

eddyb commented Nov 28, 2022

Uh oh!

luqmana commented Nov 28, 2022

Uh oh!

oli-obk commented Nov 29, 2022

Uh oh!

bors commented Dec 11, 2022

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

apiraino commented Feb 2, 2023

Uh oh!

luqmana commented Feb 20, 2023

Uh oh!

wesleywiser commented Mar 16, 2023

Uh oh!

oli-obk left a comment

Choose a reason for hiding this comment

Uh oh!

oli-obk Mar 16, 2023

Choose a reason for hiding this comment

Uh oh!

luqmana May 5, 2023

Choose a reason for hiding this comment

Uh oh!

bors commented Mar 26, 2023

Uh oh!

pnkfelix commented May 4, 2023

Uh oh!

luqmana commented May 5, 2023

Uh oh!

oli-obk commented May 6, 2023

Uh oh!

bors commented May 6, 2023

Uh oh!

bors commented May 6, 2023

Uh oh!

bors commented May 6, 2023

Uh oh!

rust-timer commented May 6, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Uh oh!

Uh oh!

luqmana commented Nov 25, 2022 •

edited

Loading