Skip to content

Commit

Permalink
Auto merge of rust-lang#119627 - oli-obk:const_prop_lint_n̵o̵n̵sense,…
Browse files Browse the repository at this point in the history
… r=<try>

Remove all ConstPropNonsense

We track all locals and projections on them ourselves within the const propagator and only use the InterpCx to actually do some low level operations or read from constants (via `OpTy` we get for said constants).

r? `@RalfJung`
  • Loading branch information
bors committed Jan 5, 2024
2 parents 432fffa + 878f72f commit 9d5fcd0
Show file tree
Hide file tree
Showing 10 changed files with 412 additions and 252 deletions.
7 changes: 1 addition & 6 deletions compiler/rustc_const_eval/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -860,9 +860,6 @@ impl<'tcx> ReportErrorExt for InvalidProgramInfo<'tcx> {
InvalidProgramInfo::FnAbiAdjustForForeignAbi(_) => {
rustc_middle::error::middle_adjust_for_foreign_abi_error
}
InvalidProgramInfo::ConstPropNonsense => {
panic!("We had const-prop nonsense, this should never be printed")
}
}
}
fn add_args<G: EmissionGuarantee>(
Expand All @@ -871,9 +868,7 @@ impl<'tcx> ReportErrorExt for InvalidProgramInfo<'tcx> {
builder: &mut DiagnosticBuilder<'_, G>,
) {
match self {
InvalidProgramInfo::TooGeneric
| InvalidProgramInfo::AlreadyReported(_)
| InvalidProgramInfo::ConstPropNonsense => {}
InvalidProgramInfo::TooGeneric | InvalidProgramInfo::AlreadyReported(_) => {}
InvalidProgramInfo::Layout(e) => {
// The level doesn't matter, `diag` is consumed without it being used.
let dummy_level = Level::Bug;
Expand Down
6 changes: 1 addition & 5 deletions compiler/rustc_const_eval/src/interpret/operand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -643,11 +643,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
let layout = self.layout_of_local(frame, local, layout)?;
let op = *frame.locals[local].access()?;
if matches!(op, Operand::Immediate(_)) {
if layout.is_unsized() {
// ConstProp marks *all* locals as `Immediate::Uninit` since it cannot
// efficiently check whether they are sized. We have to catch that case here.
throw_inval!(ConstPropNonsense);
}
assert!(!layout.is_unsized());
}
Ok(OpTy { op, layout })
}
Expand Down
19 changes: 3 additions & 16 deletions compiler/rustc_const_eval/src/interpret/place.rs
Original file line number Diff line number Diff line change
Expand Up @@ -519,11 +519,7 @@ where
} else {
// Unsized `Local` isn't okay (we cannot store the metadata).
match frame_ref.locals[local].access()? {
Operand::Immediate(_) => {
// ConstProp marks *all* locals as `Immediate::Uninit` since it cannot
// efficiently check whether they are sized. We have to catch that case here.
throw_inval!(ConstPropNonsense);
}
Operand::Immediate(_) => bug!(),
Operand::Indirect(mplace) => Place::Ptr(*mplace),
}
};
Expand Down Expand Up @@ -816,17 +812,8 @@ where
// avoid force_allocation.
let src = match self.read_immediate_raw(src)? {
Right(src_val) => {
// FIXME(const_prop): Const-prop can possibly evaluate an
// unsized copy operation when it thinks that the type is
// actually sized, due to a trivially false where-clause
// predicate like `where Self: Sized` with `Self = dyn Trait`.
// See #102553 for an example of such a predicate.
if src.layout().is_unsized() {
throw_inval!(ConstPropNonsense);
}
if dest.layout().is_unsized() {
throw_inval!(ConstPropNonsense);
}
assert!(!src.layout().is_unsized());
assert!(!dest.layout().is_unsized());
assert_eq!(src.layout().size, dest.layout().size);
// Yay, we got a value that we can write directly.
return if layout_compat {
Expand Down
49 changes: 21 additions & 28 deletions compiler/rustc_const_eval/src/interpret/projection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -153,11 +153,7 @@ where

// Offset may need adjustment for unsized fields.
let (meta, offset) = if field_layout.is_unsized() {
if base.layout().is_sized() {
// An unsized field of a sized type? Sure...
// But const-prop actually feeds us such nonsense MIR! (see test `const_prop/issue-86351.rs`)
throw_inval!(ConstPropNonsense);
}
assert!(!base.layout().is_sized());
let base_meta = base.meta();
// Re-use parent metadata to determine dynamic field layout.
// With custom DSTS, this *will* execute user-defined code, but the same
Expand Down Expand Up @@ -205,29 +201,26 @@ where
// see https://github.com/rust-lang/rust/issues/93688#issuecomment-1032929496.)
// So we just "offset" by 0.
let layout = base.layout().for_variant(self, variant);
if layout.abi.is_uninhabited() {
// `read_discriminant` should have excluded uninhabited variants... but ConstProp calls
// us on dead code.
// In the future we might want to allow this to permit code like this:
// (this is a Rust/MIR pseudocode mix)
// ```
// enum Option2 {
// Some(i32, !),
// None,
// }
//
// fn panic() -> ! { panic!() }
//
// let x: Option2;
// x.Some.0 = 42;
// x.Some.1 = panic();
// SetDiscriminant(x, Some);
// ```
// However, for now we don't generate such MIR, and this check here *has* found real
// bugs (see https://github.com/rust-lang/rust/issues/115145), so we will keep rejecting
// it.
throw_inval!(ConstPropNonsense)
}
// In the future we might want to allow this to permit code like this:
// (this is a Rust/MIR pseudocode mix)
// ```
// enum Option2 {
// Some(i32, !),
// None,
// }
//
// fn panic() -> ! { panic!() }
//
// let x: Option2;
// x.Some.0 = 42;
// x.Some.1 = panic();
// SetDiscriminant(x, Some);
// ```
// However, for now we don't generate such MIR, and this check here *has* found real
// bugs (see https://github.com/rust-lang/rust/issues/115145), so we will keep rejecting
// it.
assert!(!layout.abi.is_uninhabited());

// This cannot be `transmute` as variants *can* have a smaller size than the entire enum.
base.offset(Size::ZERO, layout, self)
}
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_const_eval/src/util/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ pub use self::type_name::type_name;
/// Classify whether an operator is "left-homogeneous", i.e., the LHS has the
/// same type as the result.
#[inline]
pub(crate) fn binop_left_homogeneous(op: mir::BinOp) -> bool {
pub fn binop_left_homogeneous(op: mir::BinOp) -> bool {
use rustc_middle::mir::BinOp::*;
match op {
Add | AddUnchecked | Sub | SubUnchecked | Mul | MulUnchecked | Div | Rem | BitXor
Expand All @@ -26,7 +26,7 @@ pub(crate) fn binop_left_homogeneous(op: mir::BinOp) -> bool {
/// Classify whether an operator is "right-homogeneous", i.e., the RHS has the
/// same type as the LHS.
#[inline]
pub(crate) fn binop_right_homogeneous(op: mir::BinOp) -> bool {
pub fn binop_right_homogeneous(op: mir::BinOp) -> bool {
use rustc_middle::mir::BinOp::*;
match op {
Add | AddUnchecked | Sub | SubUnchecked | Mul | MulUnchecked | Div | Rem | BitXor
Expand Down
2 changes: 0 additions & 2 deletions compiler/rustc_middle/src/mir/interpret/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -208,8 +208,6 @@ pub enum InvalidProgramInfo<'tcx> {
/// (which unfortunately typeck does not reject).
/// Not using `FnAbiError` as that contains a nested `LayoutError`.
FnAbiAdjustForForeignAbi(call::AdjustForForeignAbiError),
/// We are runnning into a nonsense situation due to ConstProp violating our invariants.
ConstPropNonsense,
}

/// Details of why a pointer had to be in-bounds.
Expand Down
48 changes: 10 additions & 38 deletions compiler/rustc_mir_transform/src/const_prop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ use rustc_const_eval::interpret::{
self, compile_time_machine, AllocId, ConstAllocation, FnArg, Frame, ImmTy, InterpCx,
InterpResult, OpTy, PlaceTy, Pointer,
};
use rustc_data_structures::fx::FxHashSet;
use rustc_index::bit_set::BitSet;
use rustc_index::IndexVec;
use rustc_middle::mir::visit::{MutatingUseContext, NonMutatingUseContext, PlaceContext, Visitor};
Expand Down Expand Up @@ -49,24 +48,9 @@ pub(crate) macro throw_machine_stop_str($($tt:tt)*) {{
throw_machine_stop!(Zst)
}}

pub(crate) struct ConstPropMachine<'mir, 'tcx> {
/// The virtual call stack.
stack: Vec<Frame<'mir, 'tcx>>,
pub written_only_inside_own_block_locals: FxHashSet<Local>,
pub can_const_prop: IndexVec<Local, ConstPropMode>,
}

impl ConstPropMachine<'_, '_> {
pub fn new(can_const_prop: IndexVec<Local, ConstPropMode>) -> Self {
Self {
stack: Vec::new(),
written_only_inside_own_block_locals: Default::default(),
can_const_prop,
}
}
}
pub(crate) struct ConstPropMachine;

impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for ConstPropMachine<'mir, 'tcx> {
impl<'mir, 'tcx: 'mir> interpret::Machine<'mir, 'tcx> for ConstPropMachine {
compile_time_machine!(<'mir, 'tcx>);

const PANIC_ON_ALLOC_FAIL: bool = true; // all allocations are small (see `MAX_ALLOC_LIMIT`)
Expand Down Expand Up @@ -138,23 +122,11 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for ConstPropMachine<'mir, 'tcx>
}

fn before_access_local_mut<'a>(
ecx: &'a mut InterpCx<'mir, 'tcx, Self>,
frame: usize,
local: Local,
_ecx: &'a mut InterpCx<'mir, 'tcx, Self>,
_frame: usize,
_local: Local,
) -> InterpResult<'tcx> {
assert_eq!(frame, 0);
match ecx.machine.can_const_prop[local] {
ConstPropMode::NoPropagation => {
throw_machine_stop_str!(
"tried to write to a local that is marked as not propagatable"
)
}
ConstPropMode::OnlyInsideOwnBlock => {
ecx.machine.written_only_inside_own_block_locals.insert(local);
}
ConstPropMode::FullConstProp => {}
}
Ok(())
unreachable!()
}

fn before_access_global(
Expand Down Expand Up @@ -192,16 +164,16 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for ConstPropMachine<'mir, 'tcx>

#[inline(always)]
fn stack<'a>(
ecx: &'a InterpCx<'mir, 'tcx, Self>,
_ecx: &'a InterpCx<'mir, 'tcx, Self>,
) -> &'a [Frame<'mir, 'tcx, Self::Provenance, Self::FrameExtra>] {
&ecx.machine.stack
&[]
}

#[inline(always)]
fn stack_mut<'a>(
ecx: &'a mut InterpCx<'mir, 'tcx, Self>,
_ecx: &'a mut InterpCx<'mir, 'tcx, Self>,
) -> &'a mut Vec<Frame<'mir, 'tcx, Self::Provenance, Self::FrameExtra>> {
&mut ecx.machine.stack
unreachable!()
}
}

Expand Down
Loading

0 comments on commit 9d5fcd0

Please sign in to comment.