Skip to content

Commit

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

This reverts commit 68411c9, reversing
changes made to 7ffc697.
  • Loading branch information
djkoloski committed Jan 25, 2024
1 parent 5bd5d21 commit 33c816b
Show file tree
Hide file tree
Showing 14 changed files with 437 additions and 404 deletions.
7 changes: 6 additions & 1 deletion compiler/rustc_const_eval/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -864,6 +864,9 @@ 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 @@ -872,7 +875,9 @@ impl<'tcx> ReportErrorExt for InvalidProgramInfo<'tcx> {
builder: &mut DiagnosticBuilder<'_, G>,
) {
match self {
InvalidProgramInfo::TooGeneric | InvalidProgramInfo::AlreadyReported(_) => {}
InvalidProgramInfo::TooGeneric
| InvalidProgramInfo::AlreadyReported(_)
| InvalidProgramInfo::ConstPropNonsense => {}
InvalidProgramInfo::Layout(e) => {
// The level doesn't matter, `diag` is consumed without it being used.
let dummy_level = Level::Bug;
Expand Down
6 changes: 5 additions & 1 deletion compiler/rustc_const_eval/src/interpret/operand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -643,7 +643,11 @@ 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(_)) {
assert!(!layout.is_unsized());
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);
}
}
Ok(OpTy { op, layout })
}
Expand Down
19 changes: 16 additions & 3 deletions compiler/rustc_const_eval/src/interpret/place.rs
Original file line number Diff line number Diff line change
Expand Up @@ -519,7 +519,11 @@ where
} else {
// Unsized `Local` isn't okay (we cannot store the metadata).
match frame_ref.locals[local].access()? {
Operand::Immediate(_) => bug!(),
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::Indirect(mplace) => Place::Ptr(*mplace),
}
};
Expand Down Expand Up @@ -812,8 +816,17 @@ where
// avoid force_allocation.
let src = match self.read_immediate_raw(src)? {
Right(src_val) => {
assert!(!src.layout().is_unsized());
assert!(!dest.layout().is_unsized());
// 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_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: 28 additions & 21 deletions compiler/rustc_const_eval/src/interpret/projection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,11 @@ where

// Offset may need adjustment for unsized fields.
let (meta, offset) = if field_layout.is_unsized() {
assert!(!base.layout().is_sized());
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);
}
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 @@ -201,26 +205,29 @@ 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);
// 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());

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)
}
// 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 fn binop_left_homogeneous(op: mir::BinOp) -> bool {
pub(crate) 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 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 fn binop_right_homogeneous(op: mir::BinOp) -> bool {
pub(crate) 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: 2 additions & 0 deletions compiler/rustc_middle/src/mir/interpret/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,8 @@ 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
167 changes: 166 additions & 1 deletion compiler/rustc_mir_transform/src/const_prop.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,21 @@
//! Propagates constants for early reporting of statically known
//! assertion failures

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};
use rustc_middle::mir::*;
use rustc_middle::ty::{ParamEnv, TyCtxt};
use rustc_middle::query::TyCtxtAt;
use rustc_middle::ty::layout::TyAndLayout;
use rustc_middle::ty::{self, ParamEnv, TyCtxt};
use rustc_span::def_id::DefId;
use rustc_target::abi::Size;
use rustc_target::spec::abi::Abi as CallAbi;

/// The maximum number of bytes that we'll allocate space for a local or the return value.
/// Needed for #66397, because otherwise we eval into large places and that can cause OOM or just
Expand Down Expand Up @@ -40,6 +49,162 @@ 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,
}
}
}

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

const PANIC_ON_ALLOC_FAIL: bool = true; // all allocations are small (see `MAX_ALLOC_LIMIT`)

const POST_MONO_CHECKS: bool = false; // this MIR is still generic!

type MemoryKind = !;

#[inline(always)]
fn enforce_alignment(_ecx: &InterpCx<'mir, 'tcx, Self>) -> bool {
false // no reason to enforce alignment
}

#[inline(always)]
fn enforce_validity(_ecx: &InterpCx<'mir, 'tcx, Self>, _layout: TyAndLayout<'tcx>) -> bool {
false // for now, we don't enforce validity
}

fn load_mir(
_ecx: &InterpCx<'mir, 'tcx, Self>,
_instance: ty::InstanceDef<'tcx>,
) -> InterpResult<'tcx, &'tcx Body<'tcx>> {
throw_machine_stop_str!("calling functions isn't supported in ConstProp")
}

fn panic_nounwind(_ecx: &mut InterpCx<'mir, 'tcx, Self>, _msg: &str) -> InterpResult<'tcx> {
throw_machine_stop_str!("panicking isn't supported in ConstProp")
}

fn find_mir_or_eval_fn(
_ecx: &mut InterpCx<'mir, 'tcx, Self>,
_instance: ty::Instance<'tcx>,
_abi: CallAbi,
_args: &[FnArg<'tcx>],
_destination: &PlaceTy<'tcx>,
_target: Option<BasicBlock>,
_unwind: UnwindAction,
) -> InterpResult<'tcx, Option<(&'mir Body<'tcx>, ty::Instance<'tcx>)>> {
Ok(None)
}

fn call_intrinsic(
_ecx: &mut InterpCx<'mir, 'tcx, Self>,
_instance: ty::Instance<'tcx>,
_args: &[OpTy<'tcx>],
_destination: &PlaceTy<'tcx>,
_target: Option<BasicBlock>,
_unwind: UnwindAction,
) -> InterpResult<'tcx> {
throw_machine_stop_str!("calling intrinsics isn't supported in ConstProp")
}

fn assert_panic(
_ecx: &mut InterpCx<'mir, 'tcx, Self>,
_msg: &rustc_middle::mir::AssertMessage<'tcx>,
_unwind: rustc_middle::mir::UnwindAction,
) -> InterpResult<'tcx> {
bug!("panics terminators are not evaluated in ConstProp")
}

fn binary_ptr_op(
_ecx: &InterpCx<'mir, 'tcx, Self>,
_bin_op: BinOp,
_left: &ImmTy<'tcx>,
_right: &ImmTy<'tcx>,
) -> InterpResult<'tcx, (ImmTy<'tcx>, bool)> {
// We can't do this because aliasing of memory can differ between const eval and llvm
throw_machine_stop_str!("pointer arithmetic or comparisons aren't supported in ConstProp")
}

fn before_access_local_mut<'a>(
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(())
}

fn before_access_global(
_tcx: TyCtxtAt<'tcx>,
_machine: &Self,
_alloc_id: AllocId,
alloc: ConstAllocation<'tcx>,
_static_def_id: Option<DefId>,
is_write: bool,
) -> InterpResult<'tcx> {
if is_write {
throw_machine_stop_str!("can't write to global");
}
// If the static allocation is mutable, then we can't const prop it as its content
// might be different at runtime.
if alloc.inner().mutability.is_mut() {
throw_machine_stop_str!("can't access mutable globals in ConstProp");
}

Ok(())
}

#[inline(always)]
fn expose_ptr(_ecx: &mut InterpCx<'mir, 'tcx, Self>, _ptr: Pointer) -> InterpResult<'tcx> {
throw_machine_stop_str!("exposing pointers isn't supported in ConstProp")
}

#[inline(always)]
fn init_frame_extra(
_ecx: &mut InterpCx<'mir, 'tcx, Self>,
frame: Frame<'mir, 'tcx>,
) -> InterpResult<'tcx, Frame<'mir, 'tcx>> {
Ok(frame)
}

#[inline(always)]
fn stack<'a>(
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>,
) -> &'a mut Vec<Frame<'mir, 'tcx, Self::Provenance, Self::FrameExtra>> {
&mut ecx.machine.stack
}
}

/// The mode that `ConstProp` is allowed to run in for a given `Local`.
#[derive(Clone, Copy, Debug, PartialEq)]
pub enum ConstPropMode {
Expand Down
Loading

0 comments on commit 33c816b

Please sign in to comment.