Skip to content

Commit

Permalink
Auto merge of rust-lang#121557 - RalfJung:const-fn-call-promotion, r=…
Browse files Browse the repository at this point in the history
…<try>

restrict promotion of `const fn` calls

We only promote them in `const`/`static` initializers, but even that is still unfortunate -- we still cannot add promoteds to required_consts. But we should add them there to make sure it's always okay to evaluate every const we encounter in a MIR body.  That effort of not promoting things that can fail to evaluate is tracked in rust-lang#80619. These `const fn` calls are the last missing piece.

So I propose that we do not promote const-fn calls in const when that may fail without the entire const failing, thereby completing rust-lang#80619. Unfortunately we can't just reject promoting these functions outright due to backwards compatibility. So let's see if we can find a hack that makes crater happy...

For the record, this is the [crater analysis](rust-lang#80243 (comment)) from when I tried to entirely forbid this kind of promotion. It's a tiny amount of breakage and if we had a nice alternative for code like that, we could conceivably push it through... but sadly, inline const expressions are still blocked on t-lang concerns about post-monomorphization errors and we haven't yet figured out an implementation that can resolve those concerns. So we're forced to make progress via other means, such as terrible hacks like this.

Attempt one: only promote calls on the "safe path" at the beginning of a MIR block. This is the path that starts at the start block and continues via gotos and calls, but stops at the first branch. If we had imposed this restriction before stabilizing `if` and `match` in `const`, this would have definitely been sufficient...

EDIT: Turns out that works. :) Here's the t-lang [nomination comment](rust-lang#121557 (comment)).

r? `@oli-obk`
  • Loading branch information
bors committed Mar 13, 2024
2 parents 5a6c1aa + ad5c445 commit 2064e4e
Show file tree
Hide file tree
Showing 19 changed files with 242 additions and 294 deletions.
34 changes: 17 additions & 17 deletions compiler/rustc_const_eval/src/interpret/eval_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -792,15 +792,13 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
self.stack_mut().push(frame);

// Make sure all the constants required by this frame evaluate successfully (post-monomorphization check).
if M::POST_MONO_CHECKS {
for &const_ in &body.required_consts {
let c = self
.instantiate_from_current_frame_and_normalize_erasing_regions(const_.const_)?;
c.eval(*self.tcx, self.param_env, Some(const_.span)).map_err(|err| {
err.emit_note(*self.tcx);
err
})?;
}
for &const_ in &body.required_consts {
let c =
self.instantiate_from_current_frame_and_normalize_erasing_regions(const_.const_)?;
c.eval(*self.tcx, self.param_env, Some(const_.span)).map_err(|err| {
err.emit_note(*self.tcx);
err
})?;
}

// done
Expand Down Expand Up @@ -1141,18 +1139,20 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
self.raw_const_to_mplace(val)
}

pub fn eval_mir_constant(
/// Evaluate a constant from the current frame.
/// This function relies on the fact that when the frame got pushed, all `required_consts` have been checked.
pub(super) fn eval_frame_mir_constant(
&self,
val: &mir::Const<'tcx>,
const_: &mir::Const<'tcx>,
span: Option<Span>,
layout: Option<TyAndLayout<'tcx>>,
) -> InterpResult<'tcx, OpTy<'tcx, M::Provenance>> {
M::eval_mir_constant(self, *val, span, layout, |ecx, val, span, layout| {
let const_val = val.eval(*ecx.tcx, ecx.param_env, span).map_err(|err| {
// FIXME: somehow this is reachable even when POST_MONO_CHECKS is on.
// Are we not always populating `required_consts`?
err.emit_note(*ecx.tcx);
err
M::eval_mir_constant(self, *const_, span, layout, |ecx, val, span, layout| {
let const_val = val.eval(*ecx.tcx, ecx.param_env, span).map_err(|err| match err {
ErrorHandled::Reported(..) => {
bug!("interpret const eval failure of {val:?} which is not in required_consts")
}
ErrorHandled::TooGeneric(_) => err,
})?;
ecx.const_val_to_op(const_val, val.ty(), layout)
})
Expand Down
3 changes: 0 additions & 3 deletions compiler/rustc_const_eval/src/interpret/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,9 +140,6 @@ pub trait Machine<'mir, 'tcx: 'mir>: Sized {
/// Should the machine panic on allocation failures?
const PANIC_ON_ALLOC_FAIL: bool;

/// Should post-monomorphization checks be run when a stack frame is pushed?
const POST_MONO_CHECKS: bool = true;

/// Whether memory accesses should be alignment-checked.
fn enforce_alignment(ecx: &InterpCx<'mir, 'tcx, Self>) -> bool;

Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_const_eval/src/interpret/operand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -740,14 +740,14 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
// * During ConstProp, with `TooGeneric` or since the `required_consts` were not all
// checked yet.
// * During CTFE, since promoteds in `const`/`static` initializer bodies can fail.
self.eval_mir_constant(&c, Some(constant.span), layout)?
self.eval_frame_mir_constant(&c, Some(constant.span), layout)?
}
};
trace!("{:?}: {:?}", mir_op, op);
Ok(op)
}

pub(crate) fn const_val_to_op(
pub fn const_val_to_op(
&self,
val_val: mir::ConstValue<'tcx>,
ty: Ty<'tcx>,
Expand Down
15 changes: 15 additions & 0 deletions compiler/rustc_middle/src/mir/consts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,21 @@ impl<'tcx> Const<'tcx> {
))
}

#[inline]
pub fn is_required_const(&self) -> bool {
// Only unevalated consts must be added to `required_consts` as only those can possibly
// still have latent const-eval errors.
match self {
Const::Ty(c) => match c.kind() {
ty::ConstKind::Value(_) => false, // already a value, cannot error
ty::ConstKind::Param(_) | ty::ConstKind::Error(_) => true,
_ => bug!("only ConstKind::Param/Value should be encountered here, got {:#?}", c),
},
Const::Unevaluated(..) => true,
Const::Val(..) => false,
}
}

#[inline(always)]
pub fn ty(&self) -> Ty<'tcx> {
match self {
Expand Down
27 changes: 23 additions & 4 deletions compiler/rustc_mir_transform/src/dataflow_const_prop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,16 @@ use rustc_data_structures::fx::FxHashMap;
use rustc_hir::def::DefKind;
use rustc_middle::mir::interpret::{AllocId, ConstAllocation, InterpResult, Scalar};
use rustc_middle::mir::visit::{MutVisitor, PlaceContext, Visitor};
use rustc_middle::mir::*;
use rustc_middle::mir::{self, *};
use rustc_middle::query::TyCtxtAt;
use rustc_middle::ty::layout::{LayoutOf, TyAndLayout};
use rustc_middle::ty::layout::{HasParamEnv, LayoutOf, TyAndLayout};
use rustc_middle::ty::{self, Ty, TyCtxt};
use rustc_mir_dataflow::value_analysis::{
Map, PlaceIndex, State, TrackElem, ValueAnalysis, ValueAnalysisWrapper, ValueOrPlace,
};
use rustc_mir_dataflow::{lattice::FlatSet, Analysis, Results, ResultsVisitor};
use rustc_span::def_id::DefId;
use rustc_span::DUMMY_SP;
use rustc_span::{Span, DUMMY_SP};
use rustc_target::abi::{Abi, FieldIdx, Size, VariantIdx, FIRST_VARIANT};

/// Macro for machine-specific `InterpError` without allocation.
Expand Down Expand Up @@ -393,7 +393,9 @@ impl<'a, 'tcx> ConstAnalysis<'a, 'tcx> {
}
}
Operand::Constant(box constant) => {
if let Ok(constant) = self.ecx.eval_mir_constant(&constant.const_, None, None) {
if let Ok(constant) =
DummyMachine::eval_mir_constant(&self.ecx, &constant.const_, None)
{
self.assign_constant(state, place, constant, &[]);
}
}
Expand Down Expand Up @@ -889,6 +891,23 @@ impl<'tcx> Visitor<'tcx> for OperandCollector<'tcx, '_, '_, '_> {

pub(crate) struct DummyMachine;

impl DummyMachine {
/// Evaluate a MIR constant that doesn't have to be already pre-checked via `required_consts`.
pub fn eval_mir_constant<'tcx>(
ecx: &InterpCx<'_, 'tcx, Self>,
const_: &mir::Const<'tcx>,
span: Option<Span>,
) -> InterpResult<'tcx, OpTy<'tcx>> {
let const_val = const_.eval(*ecx.tcx, ecx.param_env(), span).map_err(|err| {
// This *may* be the only time we're actually evaluating this const, so show a note at
// the place it is used.
err.emit_note(*ecx.tcx);
err
})?;
ecx.const_val_to_op(const_val, const_.ty(), None)
}
}

impl HasStaticRootDefId for DummyMachine {
fn static_def_id(&self) -> Option<rustc_hir::def_id::LocalDefId> {
None
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir_transform/src/gvn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -367,7 +367,7 @@ impl<'body, 'tcx> VnState<'body, 'tcx> {
Repeat(..) => return None,

Constant { ref value, disambiguator: _ } => {
self.ecx.eval_mir_constant(value, None, None).ok()?
DummyMachine::eval_mir_constant(&self.ecx, value, None).ok()?
}
Aggregate(kind, variant, ref fields) => {
let fields = fields
Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_mir_transform/src/jump_threading.rs
Original file line number Diff line number Diff line change
Expand Up @@ -416,7 +416,8 @@ impl<'tcx, 'a> TOFinder<'tcx, 'a> {
match rhs {
// If we expect `lhs ?= A`, we have an opportunity if we assume `constant == A`.
Operand::Constant(constant) => {
let constant = self.ecx.eval_mir_constant(&constant.const_, None, None).ok()?;
let constant =
DummyMachine::eval_mir_constant(&self.ecx, &constant.const_, None).ok()?;
self.process_constant(bb, lhs, constant, state);
}
// Transfer the conditions on the copied rhs.
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_mir_transform/src/known_panics_lint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -259,9 +259,9 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
// that the `RevealAll` pass has happened and that the body's consts
// are normalized, so any call to resolve before that needs to be
// manually normalized.
let val = self.tcx.try_normalize_erasing_regions(self.param_env, c.const_).ok()?;
let const_ = self.tcx.try_normalize_erasing_regions(self.param_env, c.const_).ok()?;

self.use_ecx(|this| this.ecx.eval_mir_constant(&val, Some(c.span), None))?
self.use_ecx(|this| DummyMachine::eval_mir_constant(&this.ecx, &const_, Some(c.span)))?
.as_mplace_or_imm()
.right()
}
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_mir_transform/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,8 @@ fn mir_promoted(
body.tainted_by_errors = Some(error_reported);
}

// Collect `required_consts` *before* promotion, so if there are any consts being promoted
// we still add them to the list in the outer MIR body.
let mut required_consts = Vec::new();
let mut required_consts_visitor = RequiredConstsVisitor::new(&mut required_consts);
for (bb, bb_data) in traversal::reverse_postorder(&body) {
Expand Down
Loading

0 comments on commit 2064e4e

Please sign in to comment.