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

Evaluate place expression in PlaceMention #104844

Merged
merged 2 commits into from
Apr 22, 2023
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
8 changes: 5 additions & 3 deletions compiler/rustc_borrowck/src/def_use.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,16 @@ pub fn categorize(context: PlaceContext) -> Option<DefUse> {
PlaceContext::NonMutatingUse(NonMutatingUseContext::ShallowBorrow) |
PlaceContext::NonMutatingUse(NonMutatingUseContext::UniqueBorrow) |

// `PlaceMention` and `AscribeUserType` both evaluate the place, which must not
// contain dangling references.
PlaceContext::NonUse(NonUseContext::PlaceMention) |
PlaceContext::NonUse(NonUseContext::AscribeUserTy) |
Comment on lines +55 to +58
Copy link
Member

Choose a reason for hiding this comment

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

I don't know borrowck, so someone else will have to review this part.
But doesn't this do a check that the place is actually initialized? Just to avoid UB it would be sufficient to check that the place is live, but maybe borrowck does not have such a check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checking that the place is initialized will break a lot of code, because we allow binding moved-from places to _. For instance in test src/test/ui/binding/issue-53114-borrow-checks.rs.

Copy link
Member

@RalfJung RalfJung Nov 26, 2022

Choose a reason for hiding this comment

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

Yes an init check is not needed. I just thought DefUse::Use does an init check (since usually when we use a place, we load from it, and it must be init). But it seems I am wrong about that?


PlaceContext::MutatingUse(MutatingUseContext::AddressOf) |
PlaceContext::NonMutatingUse(NonMutatingUseContext::AddressOf) |
PlaceContext::NonMutatingUse(NonMutatingUseContext::Inspect) |
PlaceContext::NonMutatingUse(NonMutatingUseContext::Copy) |
PlaceContext::NonMutatingUse(NonMutatingUseContext::Move) |
PlaceContext::NonUse(NonUseContext::AscribeUserTy) |
PlaceContext::MutatingUse(MutatingUseContext::Retag) =>
Some(DefUse::Use),

Expand All @@ -72,8 +76,6 @@ pub fn categorize(context: PlaceContext) -> Option<DefUse> {
PlaceContext::MutatingUse(MutatingUseContext::Drop) =>
Some(DefUse::Drop),

// This statement exists to help unsafeck. It does not require the place to be live.
PlaceContext::NonUse(NonUseContext::PlaceMention) => None,
// Debug info is neither def nor use.
PlaceContext::NonUse(NonUseContext::VarDebugInfo) => None,

Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_borrowck/src/invalidation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ impl<'cx, 'tcx> Visitor<'tcx> for InvalidationGenerator<'cx, 'tcx> {
}
// Only relevant for mir typeck
StatementKind::AscribeUserType(..)
// Only relevant for unsafeck
// Only relevant for liveness and unsafeck
| StatementKind::PlaceMention(..)
// Doesn't have any language semantics
| StatementKind::Coverage(..)
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_borrowck/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -665,7 +665,7 @@ impl<'cx, 'tcx> rustc_mir_dataflow::ResultsVisitor<'cx, 'tcx> for MirBorrowckCtx
}
// Only relevant for mir typeck
StatementKind::AscribeUserType(..)
// Only relevant for unsafeck
// Only relevant for liveness and unsafeck
| StatementKind::PlaceMention(..)
// Doesn't have any language semantics
| StatementKind::Coverage(..)
Expand Down
10 changes: 8 additions & 2 deletions compiler/rustc_const_eval/src/interpret/step.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,14 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {

Intrinsic(box intrinsic) => self.emulate_nondiverging_intrinsic(intrinsic)?,

// Statements we do not track.
PlaceMention(..) | AscribeUserType(..) => {}
// Evaluate the place expression, without reading from it.
PlaceMention(box place) => {
let _ = self.eval_place(*place)?;
}

// This exists purely to guide borrowck lifetime inference, and does not have
// an operational effect.
AscribeUserType(..) => {}

// Currently, Miri discards Coverage statements. Coverage statements are only injected
// via an optional compile time MIR pass and have no side effects. Since Coverage
Expand Down
9 changes: 1 addition & 8 deletions compiler/rustc_const_eval/src/transform/validate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -757,14 +757,6 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
}
}
}
StatementKind::PlaceMention(..) => {
if self.mir_phase >= MirPhase::Runtime(RuntimePhase::Initial) {
self.fail(
location,
"`PlaceMention` should have been removed after drop lowering phase",
);
}
}
StatementKind::AscribeUserType(..) => {
if self.mir_phase >= MirPhase::Runtime(RuntimePhase::Initial) {
self.fail(
Expand Down Expand Up @@ -874,6 +866,7 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
StatementKind::StorageDead(_)
| StatementKind::Coverage(_)
| StatementKind::ConstEvalCounter
| StatementKind::PlaceMention(..)
| StatementKind::Nop => {}
}

Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_interface/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -768,6 +768,7 @@ fn test_unstable_options_tracking_hash() {
tracked!(merge_functions, Some(MergeFunctions::Disabled));
tracked!(mir_emit_retag, true);
tracked!(mir_enable_passes, vec![("DestProp".to_string(), false)]);
tracked!(mir_keep_place_mention, true);
tracked!(mir_opt_level, Some(4));
tracked!(move_size_limit, Some(4096));
tracked!(mutable_noalias, false);
Expand Down
5 changes: 2 additions & 3 deletions compiler/rustc_middle/src/mir/syntax.rs
Original file line number Diff line number Diff line change
Expand Up @@ -331,9 +331,8 @@ pub enum StatementKind<'tcx> {
/// This is especially useful for `let _ = PLACE;` bindings that desugar to a single
/// `PlaceMention(PLACE)`.
///
/// When executed at runtime this is a nop.
///
/// Disallowed after drop elaboration.
/// When executed at runtime, this computes the given place, but then discards
/// it without doing a load. It is UB if the place is not pointing to live memory.
PlaceMention(Box<Place<'tcx>>),

/// Encodes a user's type ascription. These need to be preserved
Expand Down
1 change: 0 additions & 1 deletion compiler/rustc_mir_transform/src/cleanup_post_borrowck.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ impl<'tcx> MirPass<'tcx> for CleanupPostBorrowck {
for statement in basic_block.statements.iter_mut() {
match statement.kind {
StatementKind::AscribeUserType(..)
| StatementKind::PlaceMention(..)
| StatementKind::Assign(box (_, Rvalue::Ref(_, BorrowKind::Shallow, _)))
| StatementKind::FakeRead(..) => statement.make_nop(),
_ => (),
Expand Down
5 changes: 2 additions & 3 deletions compiler/rustc_mir_transform/src/dead_store_elimination.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,10 @@ pub fn eliminate<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>, borrowed: &BitS
| StatementKind::Coverage(_)
| StatementKind::Intrinsic(_)
| StatementKind::ConstEvalCounter
| StatementKind::PlaceMention(_)
| StatementKind::Nop => (),

StatementKind::FakeRead(_)
| StatementKind::PlaceMention(_)
| StatementKind::AscribeUserType(_, _) => {
StatementKind::FakeRead(_) | StatementKind::AscribeUserType(_, _) => {
bug!("{:?} not found in this MIR phase!", &statement.kind)
}
}
Expand Down
7 changes: 3 additions & 4 deletions compiler/rustc_mir_transform/src/dest_prop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -582,10 +582,9 @@ impl WriteInfo {
| StatementKind::Nop
| StatementKind::Coverage(_)
| StatementKind::StorageLive(_)
| StatementKind::StorageDead(_) => (),
StatementKind::FakeRead(_)
| StatementKind::AscribeUserType(_, _)
| StatementKind::PlaceMention(_) => {
| StatementKind::StorageDead(_)
| StatementKind::PlaceMention(_) => (),
StatementKind::FakeRead(_) | StatementKind::AscribeUserType(_, _) => {
bug!("{:?} not found in this MIR phase", statement)
}
}
Expand Down
8 changes: 6 additions & 2 deletions compiler/rustc_mir_transform/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ mod add_retag;
mod check_const_item_mutation;
mod check_packed_ref;
pub mod check_unsafety;
mod remove_place_mention;
// This pass is public to allow external drivers to perform MIR cleanup
pub mod cleanup_post_borrowck;
mod const_debuginfo;
Expand Down Expand Up @@ -460,8 +461,11 @@ fn run_runtime_lowering_passes<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {

/// Returns the sequence of passes that do the initial cleanup of runtime MIR.
fn run_runtime_cleanup_passes<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
let passes: &[&dyn MirPass<'tcx>] =
&[&lower_intrinsics::LowerIntrinsics, &simplify::SimplifyCfg::ElaborateDrops];
let passes: &[&dyn MirPass<'tcx>] = &[
&lower_intrinsics::LowerIntrinsics,
&remove_place_mention::RemovePlaceMention,
&simplify::SimplifyCfg::ElaborateDrops,
];

pm::run_passes(tcx, body, passes, Some(MirPhase::Runtime(RuntimePhase::PostCleanup)));

Expand Down
23 changes: 23 additions & 0 deletions compiler/rustc_mir_transform/src/remove_place_mention.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
//! This pass removes `PlaceMention` statement, which has no effect at codegen.
use crate::MirPass;
use rustc_middle::mir::*;
use rustc_middle::ty::TyCtxt;

pub struct RemovePlaceMention;

impl<'tcx> MirPass<'tcx> for RemovePlaceMention {
fn is_enabled(&self, sess: &rustc_session::Session) -> bool {
!sess.opts.unstable_opts.mir_keep_place_mention
}

fn run_pass(&self, _: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
trace!("Running RemovePlaceMention on {:?}", body.source);
for data in body.basic_blocks.as_mut_preserves_cfg() {
data.statements.retain(|statement| match statement.kind {
StatementKind::PlaceMention(..) | StatementKind::Nop => false,
_ => true,
})
}
}
}
3 changes: 3 additions & 0 deletions compiler/rustc_session/src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1558,6 +1558,9 @@ options! {
"use like `-Zmir-enable-passes=+DestProp,-InstCombine`. Forces the specified passes to be \
enabled, overriding all other checks. Passes that are not specified are enabled or \
disabled by other flags as usual."),
mir_keep_place_mention: bool = (false, parse_bool, [TRACKED],
"keep place mention MIR statements, interpreted e.g., by miri; implies -Zmir-opt-level=0 \
(default: no)"),
#[rustc_lint_opt_deny_field_access("use `Session::mir_opt_level` instead of this field")]
mir_opt_level: Option<usize> = (None, parse_opt_number, [TRACKED],
"MIR optimization level (0-4; default: 1 in non optimized builds and 2 in optimized builds)"),
Expand Down
4 changes: 2 additions & 2 deletions src/tools/clippy/tests/ui/option_if_let_else.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ fn else_if_option(string: Option<&str>) -> Option<(bool, &str)> {
fn unop_bad(string: &Option<&str>, mut num: Option<i32>) {
let _ = string.map_or(0, |s| s.len());
let _ = num.as_ref().map_or(&0, |s| s);
let _ = num.as_mut().map_or(&mut 0, |s| {
let _ = num.as_mut().map_or(&0, |s| {
*s += 1;
s
});
Expand All @@ -34,7 +34,7 @@ fn unop_bad(string: &Option<&str>, mut num: Option<i32>) {
s += 1;
s
});
let _ = num.as_mut().map_or(&mut 0, |s| {
let _ = num.as_mut().map_or(&0, |s| {
*s += 1;
s
});
Expand Down
4 changes: 2 additions & 2 deletions src/tools/clippy/tests/ui/option_if_let_else.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ fn unop_bad(string: &Option<&str>, mut num: Option<i32>) {
*s += 1;
s
} else {
&mut 0
&0
};
let _ = if let Some(ref s) = num { s } else { &0 };
let _ = if let Some(mut s) = num {
Expand All @@ -46,7 +46,7 @@ fn unop_bad(string: &Option<&str>, mut num: Option<i32>) {
*s += 1;
s
} else {
&mut 0
&0
Copy link
Member

@RalfJung RalfJung Mar 18, 2023

Choose a reason for hiding this comment

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

What are these clippy changes about?

EDIT: Oh I see, with &mut 0 these are temporaries that now lead to errors, but with &0 promotion kicks in.

};
}

Expand Down
8 changes: 4 additions & 4 deletions src/tools/clippy/tests/ui/option_if_let_else.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,13 @@ LL | let _ = if let Some(s) = &mut num {
LL | | *s += 1;
LL | | s
LL | | } else {
LL | | &mut 0
LL | | &0
LL | | };
| |_____^
|
help: try
|
LL ~ let _ = num.as_mut().map_or(&mut 0, |s| {
LL ~ let _ = num.as_mut().map_or(&0, |s| {
LL + *s += 1;
LL + s
LL ~ });
Expand Down Expand Up @@ -76,13 +76,13 @@ LL | let _ = if let Some(ref mut s) = num {
LL | | *s += 1;
LL | | s
LL | | } else {
LL | | &mut 0
LL | | &0
LL | | };
| |_____^
|
help: try
|
LL ~ let _ = num.as_mut().map_or(&mut 0, |s| {
LL ~ let _ = num.as_mut().map_or(&0, |s| {
LL + *s += 1;
LL + s
LL ~ });
Expand Down
1 change: 1 addition & 0 deletions src/tools/miri/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ pub const MIRI_DEFAULT_ARGS: &[&str] = &[
"-Zalways-encode-mir",
"-Zextra-const-ub-checks",
"-Zmir-emit-retag",
"-Zmir-keep-place-mention",
"-Zmir-opt-level=0",
"-Zmir-enable-passes=-CheckAlignment",
];
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Make sure we find these even with many checks disabled.
//@compile-flags: -Zmiri-disable-alignment-check -Zmiri-disable-stacked-borrows -Zmiri-disable-validation
RalfJung marked this conversation as resolved.
Show resolved Hide resolved

fn main() {
let p = {
let b = Box::new(42);
&*b as *const i32
};
unsafe {
let _ = *p; //~ ERROR: dereferenced after this allocation got freed
}
panic!("this should never print");
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
error: Undefined Behavior: pointer to ALLOC was dereferenced after this allocation got freed
--> $DIR/dangling_pointer_deref_underscore.rs:LL:CC
|
LL | let _ = *p;
| ^^ pointer to ALLOC was dereferenced after this allocation got freed
|
= help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
= help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
= note: BACKTRACE:
= note: inside `main` at $DIR/dangling_pointer_deref_underscore.rs:LL:CC

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to previous error

30 changes: 29 additions & 1 deletion tests/ui/borrowck/let_underscore_temporary.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// check-pass
// check-fail

fn let_underscore(string: &Option<&str>, mut num: Option<i32>) {
let _ = if let Some(s) = *string { s.len() } else { 0 };
Expand All @@ -8,6 +8,7 @@ fn let_underscore(string: &Option<&str>, mut num: Option<i32>) {
s
} else {
&mut 0
//~^ ERROR temporary value dropped while borrowed
};
let _ = if let Some(ref s) = num { s } else { &0 };
let _ = if let Some(mut s) = num {
Expand All @@ -21,6 +22,33 @@ fn let_underscore(string: &Option<&str>, mut num: Option<i32>) {
s
} else {
&mut 0
//~^ ERROR temporary value dropped while borrowed
};
}

fn let_ascribe(string: &Option<&str>, mut num: Option<i32>) {
let _: _ = if let Some(s) = *string { s.len() } else { 0 };
let _: _ = if let Some(s) = &num { s } else { &0 };
let _: _ = if let Some(s) = &mut num {
*s += 1;
s
} else {
&mut 0
//~^ ERROR temporary value dropped while borrowed
};
let _: _ = if let Some(ref s) = num { s } else { &0 };
let _: _ = if let Some(mut s) = num {
s += 1;
s
} else {
0
};
let _: _ = if let Some(ref mut s) = num {
*s += 1;
s
} else {
&mut 0
//~^ ERROR temporary value dropped while borrowed
};
}

Expand Down
Loading