Skip to content

Preserve unused pointer to address casts #97597

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

Merged
merged 1 commit into from
Jun 8, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
29 changes: 27 additions & 2 deletions compiler/rustc_middle/src/mir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2605,9 +2605,34 @@ pub enum Rvalue<'tcx> {
static_assert_size!(Rvalue<'_>, 40);

impl<'tcx> Rvalue<'tcx> {
/// Returns true if rvalue can be safely removed when the result is unused.
#[inline]
pub fn is_pointer_int_cast(&self) -> bool {
matches!(self, Rvalue::Cast(CastKind::PointerExposeAddress, _, _))
pub fn is_safe_to_remove(&self) -> bool {
match self {
// Pointer to int casts may be side-effects due to exposing the provenance.
// While the model is undecided, we should be conservative. See
// <https://www.ralfj.de/blog/2022/04/11/provenance-exposed.html>
Rvalue::Cast(CastKind::PointerExposeAddress, _, _) => false,

Rvalue::Use(_)
Copy link
Member

@RalfJung RalfJung Jun 1, 2022

Choose a reason for hiding this comment

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

Use can potentially mutate memory if it uses an Operand::Move (which is proposed to de-initialize the memory it reads from).
Removing a Move with unused result is still okay (the optimized program simply has more defined memory than the unoptimized program), but e.g. reordering it with other reads/writes is not if they might overlap. So depending on what the caller cares about, Move needs to be considered effectful.

| Rvalue::Repeat(_, _)
| Rvalue::Ref(_, _, _)
| Rvalue::ThreadLocalRef(_)
| Rvalue::AddressOf(_, _)
| Rvalue::Len(_)
| Rvalue::Cast(
CastKind::Misc | CastKind::Pointer(_) | CastKind::PointerFromExposedAddress,
_,
_,
)
| Rvalue::BinaryOp(_, _)
| Rvalue::CheckedBinaryOp(_, _)
| Rvalue::NullaryOp(_, _)
| Rvalue::UnaryOp(_, _)
| Rvalue::Discriminant(_)
| Rvalue::Aggregate(_, _)
| Rvalue::ShallowInitBox(_, _) => true,
}
}
}

Expand Down
9 changes: 3 additions & 6 deletions compiler/rustc_mir_dataflow/src/impls/liveness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -244,13 +244,10 @@ impl<'a, 'tcx> Analysis<'tcx> for MaybeTransitiveLiveLocals<'a> {
// Compute the place that we are storing to, if any
let destination = match &statement.kind {
StatementKind::Assign(assign) => {
if assign.1.is_pointer_int_cast() {
// Pointer to int casts may be side-effects due to exposing the provenance.
// While the model is undecided, we should be conservative. See
// <https://www.ralfj.de/blog/2022/04/11/provenance-exposed.html>
None
} else {
if assign.1.is_safe_to_remove() {
Some(assign.0)
} else {
None
}
}
StatementKind::SetDiscriminant { place, .. } | StatementKind::Deinit(place) => {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir_transform/src/dead_store_elimination.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ pub fn eliminate<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>, borrowed: &BitS
for (statement_index, statement) in bb_data.statements.iter().enumerate().rev() {
let loc = Location { block: bb, statement_index };
if let StatementKind::Assign(assign) = &statement.kind {
if assign.1.is_pointer_int_cast() {
if !assign.1.is_safe_to_remove() {
continue;
}
}
Expand Down
8 changes: 6 additions & 2 deletions compiler/rustc_mir_transform/src/simplify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -494,8 +494,12 @@ impl<'tcx> Visitor<'tcx> for UsedLocals {
StatementKind::StorageLive(_local) | StatementKind::StorageDead(_local) => {}

StatementKind::Assign(box (ref place, ref rvalue)) => {
self.visit_lhs(place, location);
self.visit_rvalue(rvalue, location);
if rvalue.is_safe_to_remove() {
self.visit_lhs(place, location);
self.visit_rvalue(rvalue, location);
} else {
self.super_statement(statement, location);
}
}

StatementKind::SetDiscriminant { ref place, variant_index: _ }
Expand Down
7 changes: 7 additions & 0 deletions src/test/mir-opt/simplify-locals.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,12 @@ fn t4() -> u32 {
unsafe { X + 1 }
}

// EMIT_MIR simplify_locals.expose_addr.SimplifyLocals.diff
fn expose_addr(p: *const usize) {
// Used pointer to address cast. Has a side effect of exposing the provenance.
p as usize;
}

fn main() {
c();
d1();
Expand All @@ -71,4 +77,5 @@ fn main() {
t2();
t3();
t4();
expose_addr(&0);
}
21 changes: 21 additions & 0 deletions src/test/mir-opt/simplify_locals.expose_addr.SimplifyLocals.diff
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
- // MIR for `expose_addr` before SimplifyLocals
+ // MIR for `expose_addr` after SimplifyLocals

fn expose_addr(_1: *const usize) -> () {
debug p => _1; // in scope 0 at $DIR/simplify-locals.rs:66:16: 66:17
let mut _0: (); // return place in scope 0 at $DIR/simplify-locals.rs:66:33: 66:33
let _2: usize; // in scope 0 at $DIR/simplify-locals.rs:68:5: 68:15
let mut _3: *const usize; // in scope 0 at $DIR/simplify-locals.rs:68:5: 68:6

bb0: {
StorageLive(_2); // scope 0 at $DIR/simplify-locals.rs:68:5: 68:15
StorageLive(_3); // scope 0 at $DIR/simplify-locals.rs:68:5: 68:6
_3 = _1; // scope 0 at $DIR/simplify-locals.rs:68:5: 68:6
_2 = move _3 as usize (PointerExposeAddress); // scope 0 at $DIR/simplify-locals.rs:68:5: 68:15
StorageDead(_3); // scope 0 at $DIR/simplify-locals.rs:68:14: 68:15
StorageDead(_2); // scope 0 at $DIR/simplify-locals.rs:68:15: 68:16
_0 = const (); // scope 0 at $DIR/simplify-locals.rs:66:33: 69:2
return; // scope 0 at $DIR/simplify-locals.rs:69:2: 69:2
}
}