Extend the alignment check to borrows #137940

1c3t3a · 2025-03-03T13:59:14Z

The current alignment check does not include checks for creating misaligned references from raw pointers, which is now added in this patch.

When inserting the check we need to be careful with references to field projections (e.g. &(*ptr).a), in which case the resulting reference must be aligned according to the field type and not the type of the pointer.

r? @saethlin

cc @RalfJung, after our discussion in #134424

rustbot · 2025-03-03T13:59:19Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

RalfJung · 2025-03-10T12:59:58Z

When inserting the check we need to be careful with references to field projections (e.g. &(*ptr).a), in which case the resulting reference must be aligned according to the field type and not the type of the pointer.

Yes, that is the most subtle part. Please add a test ensuring we do not complain in a case where ptr: *const Align8Struct, then we do &(*ptr.u8_field), and the ptr points to an odd address.

1c3t3a · 2025-03-10T14:57:21Z

We actually already have a test for this (@saethlin added an amazing test-suite for this check): https://github.com/rust-lang/rust/blob/2b285cd5f0877e30ad1d83e04f8cc46254e43391/tests/ui/mir/alignment/place_computation.rs. I rename it as part of this PR to make it more clear what we actually test here in the context of the new pass.

saethlin · 2025-03-11T15:40:31Z

@bors try @rust-timer queue

bors · 2025-03-11T15:41:46Z

⌛ Trying commit d3ef125 with merge a7ce54d...

Extend the alignment check to borrows The current alignment check does not include checks for creating misaligned references from raw pointers, which is now added in this patch. When inserting the check we need to be careful with references to field projections (e.g. `&(*ptr).a`), in which case the resulting reference must be aligned according to the field type and not the type of the pointer. r? `@saethlin` cc `@RalfJung,` after our discussion in rust-lang#134424

bors · 2025-03-11T17:44:20Z

☀️ Try build successful - checks-actions
Build commit: a7ce54d (a7ce54db9b096bd1098d07c645b489195a667c9f)

rust-timer · 2025-03-11T20:14:25Z

Finished benchmarking commit (a7ce54d): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

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

Max RSS (memory usage)

Results (primary 1.8%)

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)	4.4%	[1.3%, 7.9%]	5
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.5%	[-2.7%, -2.4%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.8%	[-2.7%, 7.9%]	8

Cycles

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

Binary size

Results (primary 0.1%)

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.1%	[0.0%, 0.5%]	35
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.2%	[-0.4%, -0.0%]	6
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[-0.4%, 0.5%]	41

Bootstrap: 782.898s -> 782.134s (-0.10%)
Artifact size: 365.21 MiB -> 365.22 MiB (0.00%)

1c3t3a · 2025-03-11T21:06:00Z

The perf results look good I think, especially compared to the added null-check. What do you think @saethlin, @RalfJung?

saethlin · 2025-03-13T22:50:00Z

I'm going to review this and also start a crater run, just so that we can know what the impact is before t-release finds it.

@craterbot run mode=build-and-test

craterbot · 2025-03-13T22:50:26Z

👌 Experiment pr-137940 created and queued.
🤖 Automatically detected try build a7ce54d
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2025-03-13T22:50:43Z

🚧 Experiment pr-137940 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

compiler/rustc_mir_transform/src/check_pointers.rs

craterbot · 2025-03-15T00:38:44Z

🎉 Experiment pr-137940 is completed!
📊 155 regressed and 151 fixed (597197 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

saethlin · 2025-03-15T23:25:19Z

@craterbot run name=pr-137940-2 mode=build-and-test crates=https://crater-reports.s3.amazonaws.com/pr-137940/retry-regressed-list.txt

craterbot · 2025-03-15T23:25:26Z

👌 Experiment pr-137940-2 created and queued.
🤖 Automatically detected try build a7ce54d
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2025-03-17T08:33:49Z

🚧 Experiment pr-137940-2 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

1c3t3a · 2025-03-17T11:03:31Z

Thanks a lot for the Crater run! I started with a first analysis of the results (pre-rerun):

rg -U --multiline-dotall -e 'misaligned pointer deref' --files-with-matches | cut -d/ -f1 | counts
87 counts
(  1)       71 (81.6%, 81.6%): test-fail
(  2)        7 ( 8.0%, 89.7%): regressed
(  3)        4 ( 4.6%, 94.3%): spurious-regressed
(  4)        3 ( 3.4%, 97.7%): spurious-fixed
(  5)        1 ( 1.1%, 98.9%): error
(  6)        1 ( 1.1%,100.0%): fixed

That's 87 regressions which is way more than I initially expected. The null-check crater-numbers were in a similar ballpark, but ~3/4th of them were explainable by a bug in an old bindgen version. I will research if that is the case here as well.

Also qq: what is the reason for the rerun? Are you not certain about the quality of the regressed results because of a lot of spurious failures?

craterbot · 2025-03-17T11:18:32Z

🎉 Experiment pr-137940-2 is completed!
📊 24 regressed and 17 fixed (3530 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

RalfJung · 2025-03-17T11:24:37Z

Yeah crater has a lot of false regressions. Seems like in this case we're already down to 24. :)

1c3t3a · 2025-03-17T12:50:52Z

Ah, I completely forgot that there are existing crates that already fail the alignment check. Looking now only in the regressed folder we see 7 failures:

$ rg -U --multiline-dotall -e 'misaligned pointer deref' --files-with-matches
reg/rsmnl/0.1.0/try#a7ce54db9b096bd1098d07c645b489195a667c9f.txt
gh/LordGoatius/jalloc/57a91cf2ee09b6e83e9c8c24a6deb82d6b35ea57/try#a7ce54db9b096bd1098d07c645b489195a667c9f.txt
reg/ordered-pool-allocator/0.1.0/try#a7ce54db9b096bd1098d07c645b489195a667c9f.txt
gh/george-lim/pool-allocator/5c2d952d5917478aa6d0601d04942b0d68c4f3ed/try#a7ce54db9b096bd1098d07c645b489195a667c9f.txt
gh/fsommar/rustboi/af01ff0477843146622a23e85101e4f212f088e1/try#a7ce54db9b096bd1098d07c645b489195a667c9f.txt
gh/franciscosbf/lock-free-ll/9659f606be423f0d26489e13988f487b12514e45/try#a7ce54db9b096bd1098d07c645b489195a667c9f.txt
gh/JBourds/bitbuddy/fd7d0943dc0151095aedd002c72fef2bfd00eca3/try#a7ce54db9b096bd1098d07c645b489195a667c9f.txt

This involves two crates on crates.io, where one already has an open issue for the misalignment. (Due to @saethlin's ub.https://asan.saethlin.dev/ub, awesome 🥳 ). For the other one I'll open an issue.

Looking at the other regressions: There seems nothing related to this patch, the build failures come from the linker, the test failures are wrong assertions.

The current alignment check does not include checks for creating misaligned references from raw pointers, which is now added in this patch. When inserting the check we need to be careful with references to field projections (e.g. `&(*ptr).a`), in which case the resulting reference must be aligned according to the field type and not the type of the pointer.

This makes the implementation of our PointerFinder a bit more straightforward.

1c3t3a · 2025-04-28T12:40:53Z

@rustbot ready

saethlin · 2025-04-29T00:12:08Z

Sweet. Thanks for keeping the commit history clean!

@bors r+

bors · 2025-04-29T00:12:10Z

📌 Commit 7082fa2 has been approved by saethlin

It is now in the queue for this repository.

bors · 2025-04-29T05:36:48Z

⌛ Testing commit 7082fa2 with merge 4c83e55...

bors · 2025-04-29T08:43:13Z

☀️ Test successful - checks-actions
Approved by: saethlin
Pushing 4c83e55 to master...

github-actions · 2025-04-29T08:45:37Z

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing 1b8ab72 (parent) -> 4c83e55 (this PR)

Test differences

Show 13 test diffs

Stage 1

[ui] tests/ui/mir/alignment/borrow_aligned_field_projection.rs: [missing] -> pass (J2)
[ui] tests/ui/mir/alignment/borrow_misaligned_field_projection.rs: [missing] -> pass (J2)
[ui] tests/ui/mir/alignment/misaligned_borrow.rs: [missing] -> pass (J2)
[ui] tests/ui/mir/alignment/misaligned_mut_borrow.rs: [missing] -> pass (J2)
[ui] tests/ui/mir/alignment/place_computation.rs: pass -> [missing] (J2)

Stage 2

[ui] tests/ui/mir/alignment/borrow_misaligned_field_projection.rs: [missing] -> pass (J0)
[ui] tests/ui/mir/alignment/misaligned_borrow.rs: [missing] -> pass (J0)
[ui] tests/ui/mir/alignment/misaligned_mut_borrow.rs: [missing] -> pass (J0)
[ui] tests/ui/mir/alignment/borrow_misaligned_field_projection.rs: [missing] -> ignore (ignored when the target is i686-pc-windows-msvc (MSVC on x86-32 Windows fails to align variables to their required alignment #112480)) (J1)
[ui] tests/ui/mir/alignment/misaligned_borrow.rs: [missing] -> ignore (ignored when the target is i686-pc-windows-msvc (MSVC on x86-32 Windows fails to align variables to their required alignment #112480)) (J1)
[ui] tests/ui/mir/alignment/misaligned_mut_borrow.rs: [missing] -> ignore (ignored when the target is i686-pc-windows-msvc (MSVC on x86-32 Windows fails to align variables to their required alignment #112480)) (J1)
[ui] tests/ui/mir/alignment/borrow_aligned_field_projection.rs: [missing] -> pass (J3)
[ui] tests/ui/mir/alignment/place_computation.rs: pass -> [missing] (J3)

Job group index

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 4c83e55e2d88ff93155be2784b9f64b91b870e99 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

dist-apple-various: 8866.3s -> 5997.5s (-32.4%)
x86_64-apple-2: 3881.6s -> 5126.2s (32.1%)
x86_64-apple-1: 6257.3s -> 8246.4s (31.8%)
dist-x86_64-apple: 8236.2s -> 9565.5s (16.1%)
aarch64-gnu-debug: 4270.1s -> 3957.6s (-7.3%)
x86_64-gnu-nopt: 5433.1s -> 5791.5s (6.6%)
dist-various-2: 3431.0s -> 3239.4s (-5.6%)
x86_64-gnu: 6320.7s -> 6653.1s (5.3%)
dist-i686-mingw: 7722.3s -> 8126.7s (5.2%)
x86_64-msvc-2: 6914.3s -> 6569.7s (-5.0%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

rust-timer · 2025-04-29T12:43:46Z

Finished benchmarking commit (4c83e55): 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 (primary -0.4%)

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)	1.3%	[0.4%, 3.6%]	5
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.0%	[-4.4%, -0.6%]	5
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.4%	[-4.4%, 3.6%]	10

Cycles

Results (primary 0.2%, secondary 6.0%)

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%]	2
Regressions ❌ (secondary)	6.0%	[2.0%, 16.9%]	20
Improvements ✅ (primary)	-0.4%	[-0.4%, -0.4%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.2%	[-0.4%, 0.5%]	3

Binary size

Results (primary 0.1%)

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.2%	[0.0%, 0.5%]	26
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.2%	[-0.3%, -0.0%]	9
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[-0.3%, 0.5%]	35

Bootstrap: 764.09s -> 763.658s (-0.06%)
Artifact size: 365.39 MiB -> 365.42 MiB (0.01%)

rustbot assigned saethlin Mar 3, 2025

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 Mar 3, 2025

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 11, 2025

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 11, 2025

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 13, 2025

saethlin reviewed Mar 13, 2025

View reviewed changes

compiler/rustc_mir_transform/src/check_pointers.rs Outdated Show resolved Hide resolved

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Mar 15, 2025

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 15, 2025

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Mar 17, 2025

1c3t3a mentioned this pull request Mar 17, 2025

Misaligned pointer dereference george-lim/ordered-pool-allocator#1

Closed

1c3t3a force-pushed the alignment-borrows-check branch from 58d25ed to db8b83e Compare March 17, 2025 15:43

saethlin 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 Mar 22, 2025

1c3t3a mentioned this pull request Apr 3, 2025

Fix misaligned pointer dereference george-lim/ordered-pool-allocator#2

Merged

1c3t3a added 2 commits April 25, 2025 12:16

Rework the logic for PointerFinder::visit_place

7082fa2

This makes the implementation of our PointerFinder a bit more straightforward.

1c3t3a force-pushed the alignment-borrows-check branch from db8b83e to 7082fa2 Compare April 28, 2025 12:37

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 Apr 28, 2025

1c3t3a requested a review from saethlin April 28, 2025 12:41

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 Apr 29, 2025

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 29, 2025

bors merged commit 4c83e55 into rust-lang:master Apr 29, 2025
7 checks passed

rustbot added this to the 1.88.0 milestone Apr 29, 2025

bors mentioned this pull request Apr 29, 2025

Implement pin-project in pattern matching for &pin mut|const T #139751

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend the alignment check to borrows #137940

Extend the alignment check to borrows #137940

1c3t3a commented Mar 3, 2025

rustbot commented Mar 3, 2025

RalfJung commented Mar 10, 2025

1c3t3a commented Mar 10, 2025

saethlin commented Mar 11, 2025

This comment has been minimized.

bors commented Mar 11, 2025

bors commented Mar 11, 2025

This comment has been minimized.

rust-timer commented Mar 11, 2025

1c3t3a commented Mar 11, 2025

saethlin commented Mar 13, 2025

craterbot commented Mar 13, 2025

craterbot commented Mar 13, 2025

craterbot commented Mar 15, 2025

saethlin commented Mar 15, 2025

craterbot commented Mar 15, 2025

craterbot commented Mar 17, 2025

1c3t3a commented Mar 17, 2025

craterbot commented Mar 17, 2025

RalfJung commented Mar 17, 2025

1c3t3a commented Mar 17, 2025

1c3t3a commented Apr 28, 2025

saethlin commented Apr 29, 2025

bors commented Apr 29, 2025

bors commented Apr 29, 2025

bors commented Apr 29, 2025

github-actions bot commented Apr 29, 2025

Stage 1

Stage 2

rust-timer commented Apr 29, 2025

Extend the alignment check to borrows #137940

Extend the alignment check to borrows #137940

Conversation

1c3t3a commented Mar 3, 2025

rustbot commented Mar 3, 2025

RalfJung commented Mar 10, 2025

1c3t3a commented Mar 10, 2025

saethlin commented Mar 11, 2025

This comment has been minimized.

bors commented Mar 11, 2025

bors commented Mar 11, 2025

This comment has been minimized.

rust-timer commented Mar 11, 2025

Overall result: no relevant changes - no action needed

1c3t3a commented Mar 11, 2025

saethlin commented Mar 13, 2025

craterbot commented Mar 13, 2025

craterbot commented Mar 13, 2025

craterbot commented Mar 15, 2025

saethlin commented Mar 15, 2025

craterbot commented Mar 15, 2025

craterbot commented Mar 17, 2025

1c3t3a commented Mar 17, 2025

craterbot commented Mar 17, 2025

RalfJung commented Mar 17, 2025

1c3t3a commented Mar 17, 2025

1c3t3a commented Apr 28, 2025

saethlin commented Apr 29, 2025

bors commented Apr 29, 2025

bors commented Apr 29, 2025

bors commented Apr 29, 2025

github-actions bot commented Apr 29, 2025

Test differences

Stage 1

Stage 2

Job duration changes

rust-timer commented Apr 29, 2025

Overall result: no relevant changes - no action needed