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

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.

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

saethlin · 2025-03-13T23:55:31Z

compiler/rustc_mir_transform/src/check_pointers.rs

+                if let Some(PlaceElem::Field(_, ty)) = place.projection.last() {
+                    *ty
+                } else {
+                    pointer_ty.builtin_deref(true).expect("no builtin_deref for an raw pointer")


Originally (when I first wrote this method) I thought we actually cared about whether or not the pointer involved in the Deref projection itself. That became not true pretty quickly and now with this PR it's really not true.

So I feel like all of this logic should be based on place.ty() now, and this checking of the last field projection is just a partial implementation of that method's logic. Do you agree?

I have to say I am not 100% sure I follow, but I agree that we should be always based on place.ty() and abstract from there. I reworked the code to make it a bit clearer, and the special case of deref projections for borrows is now just a special case. I put it into a separate commit for now, so that it becomes clearer.

The change that you made here isn't what I was thinking of. The sketchy code is not in the assumption above that the type of the Pointer is the type of the Local; that assumption is easily justified by local reasoning. We're creating a Place without projections which is just a Local.

The sketchy part is that you are special-casing computing the pointee type based on checking if the last projection is a field projection. If another projection is added that changes the type, this logic becomes incomplete in a subtle way. I believe the purpose of this code is to compute the type of the place in question, and that's what Place::ty is for.

Am I mistaken?

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.

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

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

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

Open

Rework the logic for PointerFinder::visit_place

db8b83e

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

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

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

saethlin Mar 13, 2025

1c3t3a Mar 17, 2025

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

Extend the alignment check to borrows #137940

Are you sure you want to change the base?

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

Instruction count

Max RSS (memory usage)

Cycles

Binary size

1c3t3a commented Mar 11, 2025

saethlin commented Mar 13, 2025

craterbot commented Mar 13, 2025

craterbot commented Mar 13, 2025

saethlin Mar 13, 2025

Choose a reason for hiding this comment

1c3t3a Mar 17, 2025

Choose a reason for hiding this comment

saethlin Mar 22, 2025

Choose a reason for hiding this comment

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