Skip to content

Commit 80d5fe3

Browse files
borsbors
committed
Auto merge of #121557 - RalfJung:const-fn-call-promotion, r=<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 #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 #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... 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. r? `@oli-obk`
2 parents 8f359be + 98c587a commit 80d5fe3

9 files changed

+107
-224
lines changed

‎compiler/rustc_mir_transform/src/lib.rs

+8-7
Original file line numberDiff line numberDiff line change
@@ -343,13 +343,6 @@ fn mir_promoted(
343343
body.tainted_by_errors = Some(error_reported);
344344
}
345345

346-
let mut required_consts = Vec::new();
347-
let mut required_consts_visitor = RequiredConstsVisitor::new(&mut required_consts);
348-
for (bb, bb_data) in traversal::reverse_postorder(&body) {
349-
required_consts_visitor.visit_basic_block_data(bb, bb_data);
350-
}
351-
body.required_consts = required_consts;
352-
353346
// What we need to run borrowck etc.
354347
let promote_pass = promote_consts::PromoteTemps::default();
355348
pm::run_passes(
@@ -359,6 +352,14 @@ fn mir_promoted(
359352
Some(MirPhase::Analysis(AnalysisPhase::Initial)),
360353
);
361354

355+
// Promotion generates new consts; we run this after promotion to ensure they are accounted for.
356+
let mut required_consts = Vec::new();
357+
let mut required_consts_visitor = RequiredConstsVisitor::new(&mut required_consts);
358+
for (bb, bb_data) in traversal::reverse_postorder(&body) {
359+
required_consts_visitor.visit_basic_block_data(bb, bb_data);
360+
}
361+
body.required_consts = required_consts;
362+
362363
let promoted = promote_pass.promoted_fragments.into_inner();
363364
(tcx.alloc_steal_mir(body), tcx.alloc_steal_promoted(promoted))
364365
}

‎compiler/rustc_mir_transform/src/promote_consts.rs

+57-5
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
//! move analysis runs after promotion on broken MIR.
1414
1515
use either::{Left, Right};
16+
use rustc_data_structures::fx::FxHashSet;
1617
use rustc_hir as hir;
1718
use rustc_middle::mir;
1819
use rustc_middle::mir::visit::{MutVisitor, MutatingUseContext, PlaceContext, Visitor};
@@ -175,6 +176,11 @@ fn collect_temps_and_candidates<'tcx>(
175176
struct Validator<'a, 'tcx> {
176177
ccx: &'a ConstCx<'a, 'tcx>,
177178
temps: &'a mut IndexSlice<Local, TempState>,
179+
/// For backwards compatibility, we are promoting function calls in `const`/`static`
180+
/// initializers. But we want to avoid evaluating code that otherwise would not have been
181+
/// evaluated, so we only do thos in basic blocks that are guaranteed to evaluate. Here we cache
182+
/// the result of computing that set of basic blocks.
183+
const_fn_safe_blocks: Option<FxHashSet<BasicBlock>>,
178184
}
179185

180186
impl<'a, 'tcx> std::ops::Deref for Validator<'a, 'tcx> {
@@ -260,7 +266,9 @@ impl<'tcx> Validator<'_, 'tcx> {
260266
self.validate_rvalue(rhs)
261267
}
262268
Right(terminator) => match &terminator.kind {
263-
TerminatorKind::Call { func, args, .. } => self.validate_call(func, args),
269+
TerminatorKind::Call { func, args, .. } => {
270+
self.validate_call(func, args, loc.block)
271+
}
264272
TerminatorKind::Yield { .. } => Err(Unpromotable),
265273
kind => {
266274
span_bug!(terminator.source_info.span, "{:?} not promotable", kind);
@@ -564,20 +572,59 @@ impl<'tcx> Validator<'_, 'tcx> {
564572
Ok(())
565573
}
566574

575+
/// Computes the sets of blocks of this MIR that are definitely going to be executed
576+
/// if the function returns successfully. That makes it safe to promote calls in them
577+
/// that might fail.
578+
fn const_fn_safe_blocks(body: &mir::Body<'tcx>) -> FxHashSet<BasicBlock> {
579+
let mut safe_blocks = FxHashSet::default();
580+
let mut safe_block = START_BLOCK;
581+
loop {
582+
safe_blocks.insert(safe_block);
583+
// Let's see if we can find another safe block.
584+
safe_block = match body.basic_blocks[safe_block].terminator().kind {
585+
TerminatorKind::Goto { target } => target,
586+
TerminatorKind::Call { target: Some(target), .. }
587+
| TerminatorKind::Drop { target, .. } => {
588+
// This calls a function or the destructor. `target` does not get executed if
589+
// the callee loops or panics. But in both cases the const already fails to
590+
// evaluate, so we are fine considering `target` a safe block for promotion.
591+
target
592+
}
593+
TerminatorKind::Assert { target, .. } => {
594+
// Similar to above, we only consider successful execution.
595+
target
596+
}
597+
_ => {
598+
// No next safe block.
599+
break;
600+
}
601+
};
602+
}
603+
safe_blocks
604+
}
605+
606+
fn is_const_fn_safe_block(&mut self, block: BasicBlock) -> bool {
607+
let body = self.body;
608+
let safe_blocks =
609+
self.const_fn_safe_blocks.get_or_insert_with(|| Self::const_fn_safe_blocks(body));
610+
safe_blocks.contains(&block)
611+
}
612+
567613
fn validate_call(
568614
&mut self,
569615
callee: &Operand<'tcx>,
570616
args: &[Spanned<Operand<'tcx>>],
617+
block: BasicBlock,
571618
) -> Result<(), Unpromotable> {
572619
let fn_ty = callee.ty(self.body, self.tcx);
573620

574-
// Inside const/static items, we promote all (eligible) function calls.
621+
// Inside const/static items, we may promote (eligible) function calls.
575622
// Everywhere else, we require `#[rustc_promotable]` on the callee.
576-
let promote_all_const_fn = matches!(
623+
let promote_const_fn = matches!(
577624
self.const_kind,
578625
Some(hir::ConstContext::Static(_) | hir::ConstContext::Const { inline: false })
579626
);
580-
if !promote_all_const_fn {
627+
if !promote_const_fn {
581628
if let ty::FnDef(def_id, _) = *fn_ty.kind() {
582629
// Never promote runtime `const fn` calls of
583630
// functions without `#[rustc_promotable]`.
@@ -595,6 +642,11 @@ impl<'tcx> Validator<'_, 'tcx> {
595642
return Err(Unpromotable);
596643
}
597644

645+
if !self.is_const_fn_safe_block(block) {
646+
// This function may error, and it may not even be executed as part of this `const`, so don't promote.
647+
return Err(Unpromotable);
648+
}
649+
598650
self.validate_operand(callee)?;
599651
for arg in args {
600652
self.validate_operand(&arg.node)?;
@@ -610,7 +662,7 @@ fn validate_candidates(
610662
temps: &mut IndexSlice<Local, TempState>,
611663
candidates: &[Candidate],
612664
) -> Vec<Candidate> {
613-
let mut validator = Validator { ccx, temps };
665+
let mut validator = Validator { ccx, temps, const_fn_safe_blocks: None };
614666

615667
candidates
616668
.iter()

‎tests/ui/consts/const-eval/promoted_errors.noopt.stderr

-44
This file was deleted.

‎tests/ui/consts/const-eval/promoted_errors.opt.stderr

-44
This file was deleted.

‎tests/ui/consts/const-eval/promoted_errors.opt_with_overflow_checks.stderr

-44
This file was deleted.

‎tests/ui/consts/const-eval/promoted_errors.rs

-52
This file was deleted.

‎tests/ui/consts/promote-not.rs

+9
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,15 @@ const TEST_INTERIOR_MUT: () = {
4141

4242
const TEST_DROP: String = String::new();
4343

44+
// We do not promote function calls in `const` initializers in dead code.
45+
const fn mk_panic() -> u32 { panic!() }
46+
const fn mk_false() -> bool { false }
47+
const Y: () = {
48+
if mk_false() {
49+
let _x: &'static u32 = &mk_panic(); //~ ERROR temporary value dropped while borrowed
50+
}
51+
};
52+
4453
fn main() {
4554
// We must not promote things with interior mutability. Not even if we "project it away".
4655
let _val: &'static _ = &(Cell::new(1), 2).0; //~ ERROR temporary value dropped while borrowed

0 commit comments

Comments
 (0)