Skip to content
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

sanity check field offsets in unsizeable structs #113245

Merged
merged 3 commits into from
Jul 7, 2023

Conversation

lukas-code
Copy link
Member

@lukas-code lukas-code commented Jul 1, 2023

As promised in #112062 (comment), this PR extends the layout sanity checks to ensure that structs fields don't move around when unsizing and prevent issues like #112048 in the future. Like most other layout sanity checks, this only runs on compilers with debug assertions enabled.

Here is how it looks when it fails:

error: internal compiler error: compiler/rustc_ty_utils/src/layout.rs:533:21: unsizing GcNode<std::boxed::Box<i32>> changed field order!
                                Layout { size: Size(32 bytes), align: AbiAndPrefAlign { abi: Align(8 bytes), pref: Align(8 bytes) }, abi: Aggregate { sized: true }, fields: Arbitrary { offsets: [Size(0 bytes), Size(8 bytes), Size(24 bytes)], memory_index: [0, 1, 2] }, largest_niche: Some(Niche { offset: Size(24 bytes), value: Pointer(AddressSpace(0)), valid_range: 1..=18446744073709551615 }), variants: Single { index: 0 } }
                                Layout { size: Size(24 bytes), align: AbiAndPrefAlign { abi: Align(8 bytes), pref: Align(8 bytes) }, abi: Aggregate { sized: false }, fields: Arbitrary { offsets: [Size(16 bytes), Size(0 bytes), Size(24 bytes)], memory_index: [1, 0, 2] }, largest_niche: None, variants: Single { index: 0 } }

r? @the8472

@rustbot 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 Jul 1, 2023
@rust-log-analyzer

This comment has been minimized.

@lukas-code lukas-code marked this pull request as draft July 1, 2023 20:57
@lukas-code
Copy link
Member Author

@rustbot author

@rustbot 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 Jul 1, 2023
@rust-log-analyzer

This comment has been minimized.

@lukas-code lukas-code force-pushed the unsizing-sanity-check branch 2 times, most recently from c9059e9 to 5e9cb60 Compare July 2, 2023 14:47
Comment on lines 481 to 486
let maybe_unsized = def.is_struct()
&& def.non_enum_variant().fields.raw.last().is_some_and(|last_field| {
let param_env = tcx.param_env(def.did());
!tcx.type_of(last_field.did).subst_identity().is_sized(tcx, param_env)
});
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will now pass always_sized = true to layout_of_struct_or_enum for enums and structs with no fields, but it shouldn't change behavior.

@lukas-code lukas-code marked this pull request as ready for review July 2, 2023 15:22
@lukas-code
Copy link
Member Author

I didn't find a good solution for #113245 (comment). Instead I moved this check back to where the actual layout computation happens, so we can directly replace the tail type instead of messing with the generic parameters (substs) of the struct. This also means that replacing the tail will not change the other struct fields.

@rustbot ready

@rustbot 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 2, 2023
@lukas-code
Copy link
Member Author

side note: The value passed for always_sized from rust-analyzer seems wrong, I think is_unsized should be is_sized instead.

!matches!(def, AdtId::EnumId(..))
&& variants
.iter()
.next()
.and_then(|x| x.last().map(|x| x.is_unsized()))
.unwrap_or(true),

@rustbot rustbot added the WG-trait-system-refactor The Rustc Trait System Refactor Initiative label Jul 5, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jul 5, 2023

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

@lukas-code lukas-code force-pushed the unsizing-sanity-check branch 2 times, most recently from 4e2ca12 to a28bb14 Compare July 5, 2023 19:15
@the8472
Copy link
Member

the8472 commented Jul 5, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Jul 5, 2023

📌 Commit a28bb14982a9b469a998279ce0bc60a37c58d959 has been approved by the8472

It is now in the queue for this repository.

@bors bors added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 5, 2023
@the8472
Copy link
Member

the8472 commented Jul 6, 2023

Hrmm, that test looks like it might be hitting the network and maybe the number of requests differ because it had to retry one, not sure.
Let's see if it's spurious.

@bors r+ rollup=iffy

@bors
Copy link
Contributor

bors commented Jul 6, 2023

💡 This pull request was already approved, no need to approve it again.

  • This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.
  • There's another pull request that is currently being tested, blocking this pull request: Remove some unnecessary(?) normalization #113348

@bors
Copy link
Contributor

bors commented Jul 6, 2023

📌 Commit a28bb14982a9b469a998279ce0bc60a37c58d959 has been approved by the8472

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 Jul 6, 2023
@bors
Copy link
Contributor

bors commented Jul 6, 2023

⌛ Testing commit a28bb14982a9b469a998279ce0bc60a37c58d959 with merge e3f0b86d0879138b8f4e8b755205c729f45870fc...

@bors
Copy link
Contributor

bors commented Jul 6, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 6, 2023
@rust-log-analyzer

This comment has been minimized.

@lukas-code
Copy link
Member Author

rebased onto #113377

@the8472
Copy link
Member

the8472 commented Jul 6, 2023

@bors retry

@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 Jul 6, 2023
@lukas-code
Copy link
Member Author

I don't see this PR in the queue.

I think you need to r+ again, because I pushed new commits.

@the8472
Copy link
Member

the8472 commented Jul 7, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Jul 7, 2023

📌 Commit 7aa5f39 has been approved by the8472

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jul 7, 2023

⌛ Testing commit 7aa5f39 with merge cb80ff1...

@bors
Copy link
Contributor

bors commented Jul 7, 2023

☀️ Test successful - checks-actions
Approved by: the8472
Pushing cb80ff1 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 7, 2023
@bors bors merged commit cb80ff1 into rust-lang:master Jul 7, 2023
11 checks passed
@rustbot rustbot added this to the 1.72.0 milestone Jul 7, 2023
@lukas-code lukas-code deleted the unsizing-sanity-check branch July 7, 2023 18:42
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (cb80ff1): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
1.6% [1.6%, 1.6%] 1
Improvements ✅
(primary)
-0.6% [-0.6%, -0.6%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.6% [-0.6%, -0.6%] 1

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)
2.2% [2.2%, 2.2%] 1
Regressions ❌
(secondary)
2.7% [2.5%, 2.9%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.2% [2.2%, 2.2%] 1

Cycles

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)
3.1% [2.4%, 4.2%] 7
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 3.1% [2.4%, 4.2%] 7

Binary size

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

Bootstrap: 657.311s -> 655.85s (-0.22%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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. WG-trait-system-refactor The Rustc Trait System Refactor Initiative
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants