From 6d312d7bd17598153c018e4879ccaa657eb48853 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 1 Aug 2024 14:01:17 +0200 Subject: [PATCH] MIR required_consts, mentioned_items: ensure we do not forget to fill these lists --- .../src/interpret/eval_context.rs | 2 +- compiler/rustc_middle/src/mir/mod.rs | 48 ++++++++++++++++--- compiler/rustc_middle/src/mir/visit.rs | 8 ++-- .../rustc_mir_build/src/build/custom/mod.rs | 4 +- compiler/rustc_mir_transform/src/inline.rs | 13 +++-- compiler/rustc_mir_transform/src/lib.rs | 36 +++++++++----- .../src/mentioned_items.rs | 3 +- .../rustc_mir_transform/src/promote_consts.rs | 14 ++++-- .../src/required_consts.rs | 13 ++++- compiler/rustc_mir_transform/src/shim.rs | 12 +++-- compiler/rustc_monomorphize/src/collector.rs | 4 +- 11 files changed, 113 insertions(+), 44 deletions(-) diff --git a/compiler/rustc_const_eval/src/interpret/eval_context.rs b/compiler/rustc_const_eval/src/interpret/eval_context.rs index 85f9b2341d9..9b4a437bf77 100644 --- a/compiler/rustc_const_eval/src/interpret/eval_context.rs +++ b/compiler/rustc_const_eval/src/interpret/eval_context.rs @@ -888,7 +888,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { body: &'tcx mir::Body<'tcx>, ) -> InterpResult<'tcx> { // Make sure all the constants required by this frame evaluate successfully (post-monomorphization check). - for &const_ in &body.required_consts { + 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, const_.span).map_err(|err| { diff --git a/compiler/rustc_middle/src/mir/mod.rs b/compiler/rustc_middle/src/mir/mod.rs index b9c93edcd80..46c4d586f6a 100644 --- a/compiler/rustc_middle/src/mir/mod.rs +++ b/compiler/rustc_middle/src/mir/mod.rs @@ -383,15 +383,17 @@ pub struct Body<'tcx> { /// Constants that are required to evaluate successfully for this MIR to be well-formed. /// We hold in this field all the constants we are not able to evaluate yet. + /// `None` indicates that the list has not been computed yet. /// /// This is soundness-critical, we make a guarantee that all consts syntactically mentioned in a /// function have successfully evaluated if the function ever gets executed at runtime. - pub required_consts: Vec>, + pub required_consts: Option>>, /// Further items that were mentioned in this function and hence *may* become monomorphized, /// depending on optimizations. We use this to avoid optimization-dependent compile errors: the /// collector recursively traverses all "mentioned" items and evaluates all their /// `required_consts`. + /// `None` indicates that the list has not been computed yet. /// /// This is *not* soundness-critical and the contents of this list are *not* a stable guarantee. /// All that's relevant is that this set is optimization-level-independent, and that it includes @@ -399,7 +401,7 @@ pub struct Body<'tcx> { /// set after drop elaboration, so some drop calls that can never be reached are not considered /// "mentioned".) See the documentation of `CollectionMode` in /// `compiler/rustc_monomorphize/src/collector.rs` for more context. - pub mentioned_items: Vec>>, + pub mentioned_items: Option>>>, /// Does this body use generic parameters. This is used for the `ConstEvaluatable` check. /// @@ -477,8 +479,8 @@ impl<'tcx> Body<'tcx> { spread_arg: None, var_debug_info, span, - required_consts: Vec::new(), - mentioned_items: Vec::new(), + required_consts: None, + mentioned_items: None, is_polymorphic: false, injection_phase: None, tainted_by_errors, @@ -507,8 +509,8 @@ impl<'tcx> Body<'tcx> { arg_count: 0, spread_arg: None, span: DUMMY_SP, - required_consts: Vec::new(), - mentioned_items: Vec::new(), + required_consts: None, + mentioned_items: None, var_debug_info: Vec::new(), is_polymorphic: false, injection_phase: None, @@ -785,6 +787,40 @@ impl<'tcx> Body<'tcx> { // No inlined `SourceScope`s, or all of them were `#[track_caller]`. caller_location.unwrap_or_else(|| from_span(source_info.span)) } + + #[track_caller] + pub fn set_required_consts(&mut self, required_consts: Vec>) { + assert!( + self.required_consts.is_none(), + "required_consts for {:?} have already been set", + self.source.def_id() + ); + self.required_consts = Some(required_consts); + } + #[track_caller] + pub fn required_consts(&self) -> &[ConstOperand<'tcx>] { + match &self.required_consts { + Some(l) => l, + None => panic!("required_consts for {:?} have not yet been set", self.source.def_id()), + } + } + + #[track_caller] + pub fn set_mentioned_items(&mut self, mentioned_items: Vec>>) { + assert!( + self.mentioned_items.is_none(), + "mentioned_items for {:?} have already been set", + self.source.def_id() + ); + self.mentioned_items = Some(mentioned_items); + } + #[track_caller] + pub fn mentioned_items(&self) -> &[Spanned>] { + match &self.mentioned_items { + Some(l) => l, + None => panic!("mentioned_items for {:?} have not yet been set", self.source.def_id()), + } + } } impl<'tcx> Index for Body<'tcx> { diff --git a/compiler/rustc_middle/src/mir/visit.rs b/compiler/rustc_middle/src/mir/visit.rs index 0031ded2440..3921176873c 100644 --- a/compiler/rustc_middle/src/mir/visit.rs +++ b/compiler/rustc_middle/src/mir/visit.rs @@ -1066,9 +1066,11 @@ macro_rules! super_body { $self.visit_span($(& $mutability)? $body.span); - for const_ in &$($mutability)? $body.required_consts { - let location = Location::START; - $self.visit_const_operand(const_, location); + if let Some(required_consts) = &$($mutability)? $body.required_consts { + for const_ in required_consts { + let location = Location::START; + $self.visit_const_operand(const_, location); + } } } } diff --git a/compiler/rustc_mir_build/src/build/custom/mod.rs b/compiler/rustc_mir_build/src/build/custom/mod.rs index 38acb6f44f9..28477e527c7 100644 --- a/compiler/rustc_mir_build/src/build/custom/mod.rs +++ b/compiler/rustc_mir_build/src/build/custom/mod.rs @@ -54,8 +54,8 @@ pub(super) fn build_custom_mir<'tcx>( spread_arg: None, var_debug_info: Vec::new(), span, - required_consts: Vec::new(), - mentioned_items: Vec::new(), + required_consts: None, + mentioned_items: None, is_polymorphic: false, tainted_by_errors: None, injection_phase: None, diff --git a/compiler/rustc_mir_transform/src/inline.rs b/compiler/rustc_mir_transform/src/inline.rs index 36b2b3b7c44..f30732e6aaf 100644 --- a/compiler/rustc_mir_transform/src/inline.rs +++ b/compiler/rustc_mir_transform/src/inline.rs @@ -744,8 +744,8 @@ impl<'tcx> Inliner<'tcx> { // Copy required constants from the callee_body into the caller_body. Although we are only // pushing unevaluated consts to `required_consts`, here they may have been evaluated // because we are calling `instantiate_and_normalize_erasing_regions` -- so we filter again. - caller_body.required_consts.extend( - callee_body.required_consts.into_iter().filter(|ct| ct.const_.is_required_const()), + caller_body.required_consts.as_mut().unwrap().extend( + callee_body.required_consts().into_iter().filter(|ct| ct.const_.is_required_const()), ); // Now that we incorporated the callee's `required_consts`, we can remove the callee from // `mentioned_items` -- but we have to take their `mentioned_items` in return. This does @@ -755,12 +755,11 @@ impl<'tcx> Inliner<'tcx> { // We need to reconstruct the `required_item` for the callee so that we can find and // remove it. let callee_item = MentionedItem::Fn(func.ty(caller_body, self.tcx)); - if let Some(idx) = - caller_body.mentioned_items.iter().position(|item| item.node == callee_item) - { + let caller_mentioned_items = caller_body.mentioned_items.as_mut().unwrap(); + if let Some(idx) = caller_mentioned_items.iter().position(|item| item.node == callee_item) { // We found the callee, so remove it and add its items instead. - caller_body.mentioned_items.remove(idx); - caller_body.mentioned_items.extend(callee_body.mentioned_items); + caller_mentioned_items.remove(idx); + caller_mentioned_items.extend(callee_body.mentioned_items()); } else { // If we can't find the callee, there's no point in adding its items. Probably it // already got removed by being inlined elsewhere in the same function, so we already diff --git a/compiler/rustc_mir_transform/src/lib.rs b/compiler/rustc_mir_transform/src/lib.rs index ac3a44c803a..1f214bc42cb 100644 --- a/compiler/rustc_mir_transform/src/lib.rs +++ b/compiler/rustc_mir_transform/src/lib.rs @@ -28,11 +28,10 @@ use rustc_hir::def::DefKind; use rustc_hir::def_id::LocalDefId; use rustc_hir::intravisit::{self, Visitor}; use rustc_index::IndexVec; -use rustc_middle::mir::visit::Visitor as _; use rustc_middle::mir::{ - traversal, AnalysisPhase, Body, CallSource, ClearCrossCrate, ConstOperand, ConstQualifs, - LocalDecl, MirPass, MirPhase, Operand, Place, ProjectionElem, Promoted, RuntimePhase, Rvalue, - SourceInfo, Statement, StatementKind, TerminatorKind, START_BLOCK, + AnalysisPhase, Body, CallSource, ClearCrossCrate, ConstOperand, ConstQualifs, LocalDecl, + MirPass, MirPhase, Operand, Place, ProjectionElem, Promoted, RuntimePhase, Rvalue, SourceInfo, + Statement, StatementKind, TerminatorKind, START_BLOCK, }; use rustc_middle::ty::{self, TyCtxt, TypeVisitableExt}; use rustc_middle::util::Providers; @@ -339,12 +338,15 @@ fn mir_promoted( // 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) { - required_consts_visitor.visit_basic_block_data(bb, bb_data); + RequiredConstsVisitor::compute_required_consts(&mut body); + // If this has an associated by-move async closure body, that doesn't get run through these + // passes itself, it gets "tagged along" by the pass manager. `RequiredConstsVisitor` is not + // a regular pass so we have to also apply it manually to the other body. + if let Some(coroutine) = body.coroutine.as_mut() { + if let Some(by_move_body) = coroutine.by_move_body.as_mut() { + RequiredConstsVisitor::compute_required_consts(by_move_body); + } } - body.required_consts = required_consts; // What we need to run borrowck etc. let promote_pass = promote_consts::PromoteTemps::default(); @@ -561,9 +563,6 @@ fn run_optimization_passes<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) { tcx, body, &[ - // Before doing anything, remember which items are being mentioned so that the set of items - // visited does not depend on the optimization level. - &mentioned_items::MentionedItems, // Add some UB checks before any UB gets optimized away. &check_alignment::CheckAlignment, // Before inlining: trim down MIR with passes to reduce inlining work. @@ -657,6 +656,19 @@ fn inner_optimized_mir(tcx: TyCtxt<'_>, did: LocalDefId) -> Body<'_> { return body; } + // Before doing anything, remember which items are being mentioned so that the set of items + // visited does not depend on the optimization level. + // We do not use `run_passes` for this as that might skip the pass if `injection_phase` is set. + mentioned_items::MentionedItems.run_pass(tcx, &mut body); + // If this has an associated by-move async closure body, that doesn't get run through these + // passes itself, it gets "tagged along" by the pass manager. Since we're not using the pass + // manager we have to do this by hand. + if let Some(coroutine) = body.coroutine.as_mut() { + if let Some(by_move_body) = coroutine.by_move_body.as_mut() { + mentioned_items::MentionedItems.run_pass(tcx, by_move_body); + } + } + // If `mir_drops_elaborated_and_const_checked` found that the current body has unsatisfiable // predicates, it will shrink the MIR to a single `unreachable` terminator. // More generally, if MIR is a lone `unreachable`, there is nothing to optimize. diff --git a/compiler/rustc_mir_transform/src/mentioned_items.rs b/compiler/rustc_mir_transform/src/mentioned_items.rs index e33bdd99421..32c8064ebca 100644 --- a/compiler/rustc_mir_transform/src/mentioned_items.rs +++ b/compiler/rustc_mir_transform/src/mentioned_items.rs @@ -23,10 +23,9 @@ impl<'tcx> MirPass<'tcx> for MentionedItems { } fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut mir::Body<'tcx>) { - debug_assert!(body.mentioned_items.is_empty()); let mut mentioned_items = Vec::new(); MentionedItemsVisitor { tcx, body, mentioned_items: &mut mentioned_items }.visit_body(body); - body.mentioned_items = mentioned_items; + body.set_mentioned_items(mentioned_items); } } diff --git a/compiler/rustc_mir_transform/src/promote_consts.rs b/compiler/rustc_mir_transform/src/promote_consts.rs index f8971387ea4..dc8e50ac8cd 100644 --- a/compiler/rustc_mir_transform/src/promote_consts.rs +++ b/compiler/rustc_mir_transform/src/promote_consts.rs @@ -702,6 +702,9 @@ struct Promoter<'a, 'tcx> { temps: &'a mut IndexVec, extra_statements: &'a mut Vec<(Location, Statement<'tcx>)>, + /// Used to assemble the required_consts list while building the promoted. + required_consts: Vec>, + /// If true, all nested temps are also kept in the /// source MIR, not moved to the promoted MIR. keep_original: bool, @@ -924,11 +927,14 @@ impl<'a, 'tcx> Promoter<'a, 'tcx> { let span = self.promoted.span; self.assign(RETURN_PLACE, rvalue, span); - // Now that we did promotion, we know whether we'll want to add this to `required_consts`. + // Now that we did promotion, we know whether we'll want to add this to `required_consts` of + // the surrounding MIR body. if self.add_to_required { - self.source.required_consts.push(promoted_op); + self.source.required_consts.as_mut().unwrap().push(promoted_op); } + self.promoted.set_required_consts(self.required_consts); + self.promoted } } @@ -947,7 +953,7 @@ impl<'a, 'tcx> MutVisitor<'tcx> for Promoter<'a, 'tcx> { fn visit_const_operand(&mut self, constant: &mut ConstOperand<'tcx>, _location: Location) { if constant.const_.is_required_const() { - self.promoted.required_consts.push(*constant); + self.required_consts.push(*constant); } // Skipping `super_constant` as the visitor is otherwise only looking for locals. @@ -1011,9 +1017,9 @@ fn promote_candidates<'tcx>( extra_statements: &mut extra_statements, keep_original: false, add_to_required: false, + required_consts: Vec::new(), }; - // `required_consts` of the promoted itself gets filled while building the MIR body. let mut promoted = promoter.promote_candidate(candidate, promotions.len()); promoted.source.promoted = Some(promotions.next_index()); promotions.push(promoted); diff --git a/compiler/rustc_mir_transform/src/required_consts.rs b/compiler/rustc_mir_transform/src/required_consts.rs index 00bfb5e6600..50637e2ac03 100644 --- a/compiler/rustc_mir_transform/src/required_consts.rs +++ b/compiler/rustc_mir_transform/src/required_consts.rs @@ -1,14 +1,23 @@ use rustc_middle::mir::visit::Visitor; -use rustc_middle::mir::{ConstOperand, Location}; +use rustc_middle::mir::{traversal, Body, ConstOperand, Location}; pub struct RequiredConstsVisitor<'a, 'tcx> { required_consts: &'a mut Vec>, } impl<'a, 'tcx> RequiredConstsVisitor<'a, 'tcx> { - pub fn new(required_consts: &'a mut Vec>) -> Self { + fn new(required_consts: &'a mut Vec>) -> Self { RequiredConstsVisitor { required_consts } } + + pub fn compute_required_consts(body: &mut Body<'tcx>) { + 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) { + required_consts_visitor.visit_basic_block_data(bb, bb_data); + } + body.set_required_consts(required_consts); + } } impl<'tcx> Visitor<'tcx> for RequiredConstsVisitor<'_, 'tcx> { diff --git a/compiler/rustc_mir_transform/src/shim.rs b/compiler/rustc_mir_transform/src/shim.rs index e2fafa3a1a3..f41f3ef656c 100644 --- a/compiler/rustc_mir_transform/src/shim.rs +++ b/compiler/rustc_mir_transform/src/shim.rs @@ -305,7 +305,7 @@ fn new_body<'tcx>( arg_count: usize, span: Span, ) -> Body<'tcx> { - Body::new( + let mut body = Body::new( source, basic_blocks, IndexVec::from_elem_n( @@ -326,7 +326,10 @@ fn new_body<'tcx>( None, // FIXME(compiler-errors): is this correct? None, - ) + ); + // Shims do not directly mention any consts. + body.set_required_consts(Vec::new()); + body } pub struct DropShimElaborator<'a, 'tcx> { @@ -969,13 +972,16 @@ pub fn build_adt_ctor(tcx: TyCtxt<'_>, ctor_id: DefId) -> Body<'_> { }; let source = MirSource::item(ctor_id); - let body = new_body( + let mut body = new_body( source, IndexVec::from_elem_n(start_block, 1), local_decls, sig.inputs().len(), span, ); + // A constructor doesn't mention any other items (and we don't run the usual optimization passes + // so this would otherwise not get filled). + body.set_mentioned_items(Vec::new()); crate::pass_manager::dump_mir_for_phase_change(tcx, &body); diff --git a/compiler/rustc_monomorphize/src/collector.rs b/compiler/rustc_monomorphize/src/collector.rs index 99cac67f5b1..df2abf6dc9c 100644 --- a/compiler/rustc_monomorphize/src/collector.rs +++ b/compiler/rustc_monomorphize/src/collector.rs @@ -1229,7 +1229,7 @@ fn collect_items_of_instance<'tcx>( // Always visit all `required_consts`, so that we evaluate them and abort compilation if any of // them errors. - for const_op in &body.required_consts { + for const_op in body.required_consts() { if let Some(val) = collector.eval_constant(const_op) { collect_const_value(tcx, val, mentioned_items); } @@ -1237,7 +1237,7 @@ fn collect_items_of_instance<'tcx>( // Always gather mentioned items. We try to avoid processing items that we have already added to // `used_items` above. - for item in &body.mentioned_items { + for item in body.mentioned_items() { if !collector.used_mentioned_items.contains(&item.node) { let item_mono = collector.monomorphize(item.node); visit_mentioned_item(tcx, &item_mono, item.span, mentioned_items);