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

Don't check for misaligned raw pointer derefs inside Rvalue::AddressOf #112026

Merged
merged 2 commits into from
May 28, 2023

Conversation

saethlin
Copy link
Member

@saethlin saethlin commented May 27, 2023

From #112026 (comment):

rustc 1.70 (stable next week) added a Mir pass to add pointer alignment checks in debug mode. Adding these checks caused some crates to break, but that was expected, since they contain broken code (#111487) for tracking that.

However, the checks added are slightly more aggressive than they should have been. Specifically, they also check the place in an addr_of! expression. Whether lack of alignment there is or isn't UB is unclear. This PR modifies the pass to not affect those cases.

I spot checked the crater regressions and the ones I saw were not the case that this PR is modifying. It still seems good to not land anything overaggressive though

@rustbot
Copy link
Collaborator

rustbot commented May 27, 2023

r? @eholk

(rustbot has picked a reviewer for you, use r? to override)

@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 May 27, 2023
@rustbot
Copy link
Collaborator

rustbot commented May 27, 2023

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@thomcc
Copy link
Member

thomcc commented May 27, 2023

@thomcc wants this test case to be allowed

Some justification of why I think this should work:

let info = buffer.as_ptr().cast::<c::FILE_ID_BOTH_DIR_INFO>();
// While this is guaranteed to be aligned in documentation for
// https://docs.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-file_id_both_dir_info
// it does not seem that reality is so kind, and assuming this
// caused crashes in some cases (https://github.com/rust-lang/rust/issues/104530)
// presumably, this can be blamed on buggy filesystem drivers, but who knows.
let next_entry = ptr::addr_of!((*info).NextEntryOffset).read_unaligned() as usize;
let length = ptr::addr_of!((*info).FileNameLength).read_unaligned() as usize;
let attrs = ptr::addr_of!((*info).FileAttributes).read_unaligned();
let name = from_maybe_unaligned(
ptr::addr_of!((*info).FileName).cast::<u16>(),
length / size_of::<u16>(),
);
is an example of code that I believed to be correct.

@thomcc
Copy link
Member

thomcc commented May 27, 2023

@JakobDegen says that this would better match MiniRust, but may not justify our LLVM semantics.

My impression was that this related to a different aspect here, rather than this. Also note that it's entirely possible that there are a number of other places in windows fs/io code that make similar assumptions (I'll probably try to do an audit of it, but it uses addr_of! all over the place, and would need to change to e.g. info.cast::<u8>().add(offset_of!(FILE_ID_BOTH_DIR_INFO, NextEntryOffset)).cast::<DWORD>().read_unaligned()).

@JakobDegen
Copy link
Contributor

Hmm, I just double-checked codegen and we don't miss anything. We emit inbounds but not alignment information (example). That's not the most principled thing, but whatever.

We should definitely land this. I'll nominate for beta-backport too and ping Felix about getting it emergency-approved.

@JakobDegen JakobDegen added the beta-nominated Nominated for backporting to the compiler in the beta channel. label May 27, 2023
@JakobDegen
Copy link
Contributor

JakobDegen commented May 27, 2023

Because Felix correctly pointed out that this PR doesn't actually include the discord discussion that contained the explanation for why we needed this:

rustc 1.70 (stable next week) added a Mir pass to add pointer alignment checks in debug mode. Adding these checks caused some crates to break, but that was expected, since they contain broken code (#111487 for tracking that).

However, the checks added are slightly more aggressive than they should have been. Specifically, they also check the place in an addr_of! expression. Whether insufficient alignment there is or isn't UB is unclear. This PR modifies the pass to not affect those cases. The test contains an example of code that now doesn't panic.

I spot checked the crater regressions and the ones I saw were not the case that this PR is modifying. My guess is it would probably not be a disaster if we didn't backport. However, this PR is pretty straightforward and clearly erring onto the side of "more conservative" though, so it is a good candidate for backporting in general

@pnkfelix
Copy link
Member

We usually prefer to err on the side of reverting an injecting PR for backports, and then leaving fine-grained adjustments to the original PR to ride the trains.

Leaving aside the questions for the moment of 1. which tactic will be easier to deploy and 2. which tactic is likely to cause the least harm, i asked @JakobDegen if there is a single injecting PR here that we might consider reverting?

They pointed me at PR #98112 as the one that would need reverting. I'm going to do a spot comparison between this PR (#112026) and PR #98112 to try to arrive at a decision (or figure out whether I have to raise the question with the broader compiler team).

@pnkfelix
Copy link
Member

pnkfelix commented May 28, 2023

okay so this is clearly low risk on its own.

I'm a little nervous about some of the unknowns surrounding PR #98112; e.g. I don't think we yet have a clear answer as to the runtime impact of the change there to debug-builds. (See related discussion on #98112 (comment) ). But that's a quality control matter that we can afford to deal with later, after the release. And given that the crater run was already deemed "acceptable" in terms of the regressions that we associated with #98112, I think that all points to an argument for just beta-backporting this PR.

So, I'm unilaterally approving this for beta-backport to 1.70.

@rustbot label: beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label May 28, 2023
@pnkfelix
Copy link
Member

@bors r+ p=3

@bors
Copy link
Contributor

bors commented May 28, 2023

📌 Commit 783b1ce has been approved by pnkfelix

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 May 28, 2023
@pnkfelix
Copy link
Member

(landing with higher priority to push for this to be in nightly asap as to ensure we catch any issues with nightly integration before a beta-backport proceeds.)

@bors
Copy link
Contributor

bors commented May 28, 2023

⌛ Testing commit 783b1ce with merge 39c03fb...

@bors
Copy link
Contributor

bors commented May 28, 2023

☀️ Test successful - checks-actions
Approved by: pnkfelix
Pushing 39c03fb to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 28, 2023
@bors bors merged commit 39c03fb into rust-lang:master May 28, 2023
@rustbot rustbot added this to the 1.72.0 milestone May 28, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (39c03fb): 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

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.2% [3.2%, 3.2%] 1
Regressions ❌
(secondary)
1.9% [1.9%, 1.9%] 1
Improvements ✅
(primary)
-1.0% [-1.0%, -1.0%] 1
Improvements ✅
(secondary)
-2.4% [-2.4%, -2.4%] 1
All ❌✅ (primary) 1.1% [-1.0%, 3.2%] 2

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

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

Bootstrap: 647.057s -> 646.068s (-0.15%)

saethlin pushed a commit to saethlin/miri that referenced this pull request May 29, 2023
Don't check for misaligned raw pointer derefs inside Rvalue::AddressOf

From rust-lang/rust#112026 (comment):

rustc 1.70 (stable next week) added a Mir pass to add pointer alignment checks in debug mode. Adding these checks caused some crates to break, but that was expected, since they contain broken code (rust-lang/rust#111487) for tracking that.

However, the checks added are slightly more aggressive than they should have been. Specifically, they also check the place in an `addr_of!` expression. Whether lack of alignment there is or isn't UB is unclear. This PR modifies the pass to not affect those cases.

I spot checked the crater regressions and the ones I saw were not the case that this PR is modifying. It still seems good to not land anything overaggressive though
@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label May 29, 2023
@Mark-Simulacrum Mark-Simulacrum modified the milestones: 1.72.0, 1.70.0 May 29, 2023
@Mark-Simulacrum Mark-Simulacrum added the beta-nominated Nominated for backporting to the compiler in the beta channel. label May 29, 2023
@Mark-Simulacrum
Copy link
Member

Backporting to 1.70.0. Leaving beta-nominated so that we also backport this into 1.71.0, since it landed after the branch point for that release.

bors added a commit to rust-lang-ci/rust that referenced this pull request May 29, 2023
…mulacrum

[beta] 1.71.0 branch

* Swap out CURRENT_RUSTC_VERSION markers
* Bump CI channel
* Backport rust-lang#112026

r? `@Mark-Simulacrum`
@Mark-Simulacrum
Copy link
Member

Er, sounds like I confused myself despite trying to write that. #112066 backported this into 1.71, and we are missing a backport into 1.70 (stable branch as of promotion yesterday). Putting up a PR now to put this into 1.70.

@Mark-Simulacrum
Copy link
Member

#112107 takes care of the stable branch. Dropping beta-nominated since this is now in beta and will shortly be in the originally-targeted stable.

@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label May 30, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request May 30, 2023
…Simulacrum

[stable] 1.70.0 (backport)

Backport rust-lang#112026 into 1.70.0 stable. Will rebuild dev-static artifacts after this gets built.

r? `@Mark-Simulacrum`
@@ -75,6 +75,14 @@ struct PointerFinder<'tcx, 'a> {
}

impl<'tcx, 'a> Visitor<'tcx> for PointerFinder<'tcx, 'a> {
fn visit_rvalue(&mut self, rvalue: &Rvalue<'tcx>, location: Location) {
if let Rvalue::AddressOf(..) = rvalue {
// Ignore dereferences inside of an AddressOf
Copy link
Member

@RalfJung RalfJung Jun 5, 2023

Choose a reason for hiding this comment

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

Would be good to have a comment here that these derefs (if unaligned) are UB according to the reference, but we choose to skip them here because there are still debates about whether we want to relax this UB later or not.

Also (while I am here), IMO if let without any binder is better replaced by if matches!.

Copy link
Member

Choose a reason for hiding this comment

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

Would be good to have a comment here that these derefs (if unaligned) are UB according to the reference, but we choose to skip them here because there are still debates about whether we want to relax this UB later or not.

Can we start this discussion? I see the logic, but I have seen a good amount of code that does this (in addition to the code in the stdlib I linked above), and AFAICT there's very little benefit to this UB in terms of optimizations it allows.

It's easy to misunderstand the way addr_of avoids alignment issues as being slightly broader than it actually intends to be, and most of this code also would have insufficient test coverage for the unaligned case (pretty often it crops up when C code doesn't guarantee that it will give you an aligned pointer, even though in practice it tends to do so).

Copy link
Member

Choose a reason for hiding this comment

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

@thomcc would you be willing to create a meeting proposal at https://github.com/rust-lang/opsem-team/ ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@saethlin saethlin deleted the misaligned-addrof branch June 14, 2023 21:33
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.
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.
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Nov 15, 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/rust#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.
lnicola pushed a commit to lnicola/rust-analyzer that referenced this pull request Apr 7, 2024
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/rust#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.
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 27, 2024
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/rust#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants