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

Update the alignment checks to match rust-lang/reference#1387 #113343

Merged
merged 1 commit into from
Nov 4, 2023

Conversation

saethlin
Copy link
Member

@saethlin saethlin commented Jul 4, 2023

Previously, we had a special case to not check Rvalue::AddressOf in this pass because we weren't quite sure if pointers needed to be aligned in the Place passed to it: #112026

Since rust-lang/reference#1387 merged, this PR updates this pass to match. The behavior of the check is nearly unchanged, except we also avoid inserting a check for creating references. Most of the changes in this PR are cleanup and new tests.

@saethlin saethlin added the T-opsem Relevant to the opsem team label Jul 4, 2023
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 4, 2023
@saethlin
Copy link
Member Author

saethlin commented Jul 7, 2023

I really don't know who is a good reviewer for this. My question in the PR description needs an answer and I'm not good enough at MIR to know it. Alternatively, a reviewer could decide that "check all place except X" is too fragile and we should go the other direction and only check the operands in a Rvalue::Use statement and the place passed to Rvalue::Ref.

r? @RalfJung

Or maybe @cjgillot ? I'd appreciate your input on this, if you have any.

cc @rust-lang/opsem generally

@saethlin saethlin marked this pull request as ready for review July 7, 2023 22:00
@rustbot
Copy link
Collaborator

rustbot commented Jul 7, 2023

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@RalfJung
Copy link
Member

RalfJung commented Jul 8, 2023

I don't know if this is complete yet or not. Is there anywhere else we construct a Place but don't require it to be aligned apart from Rvalue::AddressOf?

Yes, PlaceMention.

@RalfJung
Copy link
Member

RalfJung commented Jul 8, 2023

Alternatively, a reviewer could decide that "check all place except X" is too fragile and we should go the other direction and only check the operands in a Rvalue::Use statement and the place passed to Rvalue::Ref.

Yeah I think that would be clearer.

Reviewing this would take me a lot of time as currently I find the entire structure of this pass very unclear -- I have no idea what that diff does. So maybe it'd be better if whoever reviewed the pass when it landed could also take this one.

@saethlin
Copy link
Member Author

saethlin commented Jul 8, 2023

Reviewing this would take me a lot of time as currently I find the entire structure of this pass very unclear -- I have no idea what that diff does.

I added a note to the PR description; I looked at the diff myself and I find it quite choppy and it would be easier to consider this all new code. I agree that this pass is pretty unique, maybe I'll factor its structure out of the pass before I do more passes like it.

So maybe it'd be better if whoever reviewed the pass when it landed could also take this one.

r? @oli-obk

@rustbot rustbot assigned oli-obk and unassigned RalfJung Jul 8, 2023
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 25, 2023

I've been trying to motivate myself to review this PR since two weeks, but I'm just dreading mir opts 🙁. I'll give it another shot this week

@oli-obk
Copy link
Contributor

oli-obk commented Jul 27, 2023

@saethlin met with me and explained the new pass to me.

@bors r+ rollup=never (for bisectability)

@bors
Copy link
Contributor

bors commented Jul 27, 2023

📌 Commit 6e6f455fced3b033f3d648de8daeeca675c4ba9a has been approved by oli-obk

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 27, 2023
@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 27, 2023

@bors r- looks like this triggers somewhere in boostrap now?

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 27, 2023
@saethlin
Copy link
Member Author

I'm going to rebase and squash this down then debug.

@thomcc
Copy link
Member

thomcc commented Jul 27, 2023

As mentioned elsewhere, this triggering on #[repr(C, packed)] struct PackedFoo(MoreAlignedFoo); breaks things like

// DWARF streams are packed, so e.g., a u32 would not necessarily be aligned
// on a 4-byte boundary. This may cause problems on platforms with strict
// alignment requirements. By wrapping data in a "packed" struct, we are
// telling the backend to generate "misalignment-safe" code.
pub unsafe fn read<T: Copy>(&mut self) -> T {
let Unaligned(result) = *(self.ptr as *const Unaligned<T>);
self.ptr = self.ptr.add(mem::size_of::<T>());
result
}

which was previously accepted (even by miri). I also have code (not currently open source) that would be broken by it, and would prefer the pattern be allowed, since at least in my code, it has simplified working with underaligned data.

@thomcc
Copy link
Member

thomcc commented Jul 27, 2023

A more problematic example is the following code, which is safe and compiles without warnings, but is broken under a rule like the one in this PR.

#[repr(C, packed)]
pub struct Blah {
    pub foo: u8,
    pub bar: i32,
}

pub fn baz(x: &Blah) -> i32 {
    x.bar
}

While this PR's check only applies to raw pointers, @saethlin indicated in the zulip that it not triggering on the above is basically a false-negative. My opinion is that it would be deeply confusing to allow this only for references to packed structs (and not pointers), and that it would be a breaking language change to forbid it for both.

@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member

RalfJung commented Nov 4, 2023

Are there tests ensuring that let _ = *ptr does not get alignment checks?

Looks like the answer is "yes" now.

r? @RalfJung
@bors r+

@bors
Copy link
Contributor

bors commented Nov 4, 2023

📌 Commit 87b1bef has been approved by RalfJung

It is now in the queue for this repository.

@rustbot rustbot assigned RalfJung and unassigned oli-obk Nov 4, 2023
@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 Nov 4, 2023
@bors
Copy link
Contributor

bors commented Nov 4, 2023

⌛ Testing commit 87b1bef with merge a18ec64...

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 4, 2023
Update the alignment checks to match rust-lang/reference#1387

Previously, we had a special case to not check `Rvalue::AddressOf` in this pass because we weren't quite sure if pointers needed to be aligned in the Place passed to it: rust-lang#112026

Since rust-lang/reference#1387 merged, this PR updates this pass to match. The behavior of the check is nearly unchanged, except we also avoid inserting a check for creating references. Most of the changes in this PR are cleanup and new tests.
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Nov 4, 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 Nov 4, 2023
@saethlin
Copy link
Member Author

saethlin commented Nov 4, 2023

@bors r=RalfJung

@bors
Copy link
Contributor

bors commented Nov 4, 2023

📌 Commit f9bd7da has been approved by RalfJung

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 Nov 4, 2023
@bors
Copy link
Contributor

bors commented Nov 4, 2023

⌛ Testing commit f9bd7da with merge a42d94e...

@bors
Copy link
Contributor

bors commented Nov 4, 2023

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing a42d94e to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 4, 2023
@bors bors merged commit a42d94e into rust-lang:master Nov 4, 2023
12 checks passed
@rustbot rustbot added this to the 1.75.0 milestone Nov 4, 2023
@saethlin saethlin deleted the looser-alignment branch November 4, 2023 21:27
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (a42d94e): comparison URL.

Overall result: ✅ 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)
- - 0
Improvements ✅
(primary)
-0.5% [-0.6%, -0.4%] 2
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.5% [-0.6%, -0.4%] 2

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)
3.3% [0.6%, 6.3%] 4
Regressions ❌
(secondary)
2.0% [1.3%, 2.7%] 2
Improvements ✅
(primary)
-3.3% [-5.8%, -0.1%] 6
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.7% [-5.8%, 6.3%] 10

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)
0.5% [0.5%, 0.5%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.6% [-0.6%, -0.5%] 3
Improvements ✅
(secondary)
-1.6% [-1.6%, -1.6%] 1
All ❌✅ (primary) -0.3% [-0.6%, 0.5%] 4

Binary size

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.3% [0.0%, 0.6%] 9
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.1% [-0.4%, -0.0%] 36
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.0% [-0.4%, 0.6%] 45

Bootstrap: 635.113s -> 634.557s (-0.09%)
Artifact size: 304.36 MiB -> 304.51 MiB (0.05%)

bors-ferrocene bot added a commit to ferrocene/ferrocene that referenced this pull request Nov 6, 2023
84: Automated pull from upstream `master` r=Dajamante a=github-actions[bot]


This PR pulls the following changes from the upstream repository:

* rust-lang/rust#117585
* rust-lang/rust#117576
* rust-lang/rust#96979
* rust-lang/rust#117191
* rust-lang/rust#117179
* rust-lang/rust#117574
* rust-lang/rust#117537
* rust-lang/rust#117608
  * rust-lang/rust#117596
  * rust-lang/rust#117588
  * rust-lang/rust#117524
  * rust-lang/rust#116017
* rust-lang/rust#117504
* rust-lang/rust#117469
* rust-lang/rust#116218
* rust-lang/rust#117589
* rust-lang/rust#117581
* rust-lang/rust#117503
* rust-lang/rust#117590
  * rust-lang/rust#117583
  * rust-lang/rust#117570
  * rust-lang/rust#117562
  * rust-lang/rust#117534
  * rust-lang/rust#116894
  * rust-lang/rust#110340
* rust-lang/rust#113343
* rust-lang/rust#117579
* rust-lang/rust#117094
* rust-lang/rust#117566
* rust-lang/rust#117564
  * rust-lang/rust#117554
  * rust-lang/rust#117550
  * rust-lang/rust#117343
* rust-lang/rust#115274
* rust-lang/rust#117540
* rust-lang/rust#116412
* rust-lang/rust#115333
* rust-lang/rust#117507
* rust-lang/rust#117538
  * rust-lang/rust#117533
  * rust-lang/rust#117523
  * rust-lang/rust#117520
  * rust-lang/rust#117505
  * rust-lang/rust#117434
* rust-lang/rust#117535
* rust-lang/rust#117510
* rust-lang/rust#116439
* rust-lang/rust#117508



Co-authored-by: Ben Wiederhake <BenWiederhake.GitHub@gmx.de>
Co-authored-by: SabrinaJewson <sejewson@gmail.com>
Co-authored-by: J-ZhengLi <lizheng135@huawei.com>
Co-authored-by: koka <koka.code@gmail.com>
Co-authored-by: bjorn3 <17426603+bjorn3@users.noreply.github.com>
Co-authored-by: Joshua Liebow-Feeser <joshlf@users.noreply.github.com>
Co-authored-by: lengyijun <sjtu5140809011@gmail.com>
Co-authored-by: Zalathar <Zalathar@users.noreply.github.com>
Co-authored-by: Oli Scherer <git-spam-no-reply9815368754983@oli-obk.de>
Co-authored-by: Philipp Krones <hello@philkrones.com>
Co-authored-by: y21 <30553356+y21@users.noreply.github.com>
Co-authored-by: bors <bors@rust-lang.org>
Co-authored-by: bohan <bohan-zhang@foxmail.com>
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-opsem Relevant to the opsem team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants