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

[saethlin]

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]

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

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[RalfJung]

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]

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]

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

[oli-obk]

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]

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

@bors r+ rollup=never (for bisectability)

[bors]

📌 Commit 6e6f455fced3b033f3d648de8daeeca675c4ba9a has been approved by oli-obk

It is now in the queue for this repository.

[oli-obk]

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

[saethlin]

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

[thomcc]

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

rust/library/std/src/sys/personality/dwarf/mod.rs

Lines 28 to 36 in 9339f44

	// 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]

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.

[RalfJung]

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

Looks like the answer is "yes" now.

r? @RalfJung
@bors r+

[bors]

📌 Commit 87b1bef has been approved by RalfJung

It is now in the queue for this repository.

[bors]

⌛ Testing commit 87b1bef with merge a18ec64...

[bors]

💔 Test failed - checks-actions

[saethlin]

@bors r=RalfJung

[bors]

📌 Commit f9bd7da has been approved by RalfJung

It is now in the queue for this repository.

[bors]

⌛ Testing commit f9bd7da with merge a42d94e...

[bors]

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

[rust-timer]

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%)