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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
de4dddf
Add a test for misaligned pointer derefs inside addr_of!
saethlin May 27, 2023
783b1ce
Exclude Rvalue::AddressOf for raw pointer deref alignment checks
saethlin May 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions compiler/rustc_mir_transform/src/check_alignment.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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.

Done: rust-lang/opsem-team#9

return;
}
self.super_rvalue(rvalue, location);
}

fn visit_place(&mut self, place: &Place<'tcx>, context: PlaceContext, _location: Location) {
if let PlaceContext::NonUse(_) = context {
return;
Expand Down
15 changes: 15 additions & 0 deletions tests/ui/mir/addrof_alignment.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// run-pass
// ignore-wasm32-bare: No panic messages
// compile-flags: -C debug-assertions

struct Misalignment {
a: u32,
}

fn main() {
let items: [Misalignment; 2] = [Misalignment { a: 0 }, Misalignment { a: 1 }];
unsafe {
let ptr: *const Misalignment = items.as_ptr().cast::<u8>().add(1).cast::<Misalignment>();
let _ptr = core::ptr::addr_of!((*ptr).a);
}
}