From aa7dd32343a7a265cfd1e3ed3a4466624318faaa Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 15 Mar 2024 21:06:40 +0100 Subject: [PATCH 1/5] collector: recursively traverse 'mentioned' items to evaluate their constants --- compiler/rustc_data_structures/src/sync.rs | 9 +- compiler/rustc_middle/src/mir/mod.rs | 23 +++ .../rustc_mir_build/src/build/custom/mod.rs | 1 + compiler/rustc_mir_transform/src/inline.rs | 26 ++- compiler/rustc_mir_transform/src/lib.rs | 5 + .../src/mentioned_items.rs | 56 ++++++ compiler/rustc_mir_transform/src/shim.rs | 4 +- compiler/rustc_monomorphize/src/collector.rs | 181 +++++++++++++++--- .../collect-in-dead-drop.noopt.stderr | 6 +- .../collect-in-dead-drop.opt.stderr | 17 ++ .../required-consts/collect-in-dead-drop.rs | 7 +- .../collect-in-dead-fn.noopt.stderr | 8 +- .../collect-in-dead-fn.opt.stderr | 17 ++ .../required-consts/collect-in-dead-fn.rs | 7 +- .../collect-in-dead-move.noopt.stderr | 6 +- .../collect-in-dead-move.opt.stderr | 17 ++ .../required-consts/collect-in-dead-move.rs | 7 +- 17 files changed, 343 insertions(+), 54 deletions(-) create mode 100644 compiler/rustc_mir_transform/src/mentioned_items.rs create mode 100644 tests/ui/consts/required-consts/collect-in-dead-drop.opt.stderr create mode 100644 tests/ui/consts/required-consts/collect-in-dead-fn.opt.stderr create mode 100644 tests/ui/consts/required-consts/collect-in-dead-move.opt.stderr diff --git a/compiler/rustc_data_structures/src/sync.rs b/compiler/rustc_data_structures/src/sync.rs index 32202ac3eded1..eab6d8168ca8e 100644 --- a/compiler/rustc_data_structures/src/sync.rs +++ b/compiler/rustc_data_structures/src/sync.rs @@ -20,6 +20,7 @@ //! | ----------------------- | ------------------- | ------------------------------- | //! | `Lrc` | `rc::Rc` | `sync::Arc` | //! |` Weak` | `rc::Weak` | `sync::Weak` | +//! | `LRef<'a, T>` [^2] | `&'a mut T` | `&'a T` | //! | | | | //! | `AtomicBool` | `Cell` | `atomic::AtomicBool` | //! | `AtomicU32` | `Cell` | `atomic::AtomicU32` | @@ -38,7 +39,7 @@ //! of a `RefCell`. This is appropriate when interior mutability is not //! required. //! -//! [^2] `MTLockRef` is a typedef. +//! [^2] `MTRef`, `MTLockRef` are type aliases. pub use crate::marker::*; use std::collections::HashMap; @@ -208,7 +209,7 @@ cfg_match! { use std::cell::RefCell as InnerRwLock; - pub type MTLockRef<'a, T> = &'a mut MTLock; + pub type LRef<'a, T> = &'a mut T; #[derive(Debug, Default)] pub struct MTLock(T); @@ -274,7 +275,7 @@ cfg_match! { pub use std::sync::Arc as Lrc; pub use std::sync::Weak as Weak; - pub type MTLockRef<'a, T> = &'a MTLock; + pub type LRef<'a, T> = &'a T; #[derive(Debug, Default)] pub struct MTLock(Lock); @@ -314,6 +315,8 @@ cfg_match! { } } +pub type MTLockRef<'a, T> = LRef<'a, MTLock>; + #[derive(Default)] #[cfg_attr(parallel_compiler, repr(align(64)))] pub struct CacheAligned(pub T); diff --git a/compiler/rustc_middle/src/mir/mod.rs b/compiler/rustc_middle/src/mir/mod.rs index d57ffc0f8b5c5..6c9d5c670825f 100644 --- a/compiler/rustc_middle/src/mir/mod.rs +++ b/compiler/rustc_middle/src/mir/mod.rs @@ -20,6 +20,7 @@ use rustc_hir::def_id::{DefId, CRATE_DEF_ID}; use rustc_hir::{self, CoroutineDesugaring, CoroutineKind, ImplicitSelfKind}; use rustc_hir::{self as hir, HirId}; use rustc_session::Session; +use rustc_span::source_map::Spanned; use rustc_target::abi::{FieldIdx, VariantIdx}; use polonius_engine::Atom; @@ -312,6 +313,16 @@ impl<'tcx> CoroutineInfo<'tcx> { } } +/// Some item that needs to monomorphize successfully for a MIR body to be considered well-formed. +#[derive(Copy, Clone, PartialEq, Debug, HashStable, TyEncodable, TyDecodable)] +#[derive(TypeFoldable, TypeVisitable)] +pub enum MentionedItem<'tcx> { + Fn(DefId, GenericArgsRef<'tcx>), + Drop(Ty<'tcx>), + // FIXME: add Vtable { source_ty: Ty<'tcx>, target_ty: Ty<'tcx> }, + // FIXME: do we have to add closures? +} + /// The lowered representation of a single function. #[derive(Clone, TyEncodable, TyDecodable, Debug, HashStable, TypeFoldable, TypeVisitable)] pub struct Body<'tcx> { @@ -375,8 +386,18 @@ 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. + /// + /// 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>, + /// Further items that were mentioned in this function and hence *may* become monomorphized, + /// depending on optimizations. We use this to ensure their consts evaluate successfully even if + /// they do not get monomorphized, to avoid optimization-dependent compile errors. + /// + /// This is *not* soundness-critical and the contents of this list are *not* a stable guarantee. + pub mentioned_items: Vec>>, + /// Does this body use generic parameters. This is used for the `ConstEvaluatable` check. /// /// Note that this does not actually mean that this body is not computable right now. @@ -453,6 +474,7 @@ impl<'tcx> Body<'tcx> { var_debug_info, span, required_consts: Vec::new(), + mentioned_items: Vec::new(), is_polymorphic: false, injection_phase: None, tainted_by_errors, @@ -482,6 +504,7 @@ impl<'tcx> Body<'tcx> { spread_arg: None, span: DUMMY_SP, required_consts: Vec::new(), + mentioned_items: Vec::new(), var_debug_info: Vec::new(), is_polymorphic: false, injection_phase: None, diff --git a/compiler/rustc_mir_build/src/build/custom/mod.rs b/compiler/rustc_mir_build/src/build/custom/mod.rs index 288b787798b49..109ffedec5506 100644 --- a/compiler/rustc_mir_build/src/build/custom/mod.rs +++ b/compiler/rustc_mir_build/src/build/custom/mod.rs @@ -56,6 +56,7 @@ pub(super) fn build_custom_mir<'tcx>( var_debug_info: Vec::new(), span, required_consts: Vec::new(), + mentioned_items: Vec::new(), 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 f6a0945c22297..aee2c99c929db 100644 --- a/compiler/rustc_mir_transform/src/inline.rs +++ b/compiler/rustc_mir_transform/src/inline.rs @@ -565,7 +565,8 @@ impl<'tcx> Inliner<'tcx> { mut callee_body: Body<'tcx>, ) { let terminator = caller_body[callsite.block].terminator.take().unwrap(); - let TerminatorKind::Call { args, destination, unwind, target, .. } = terminator.kind else { + let TerminatorKind::Call { func, args, destination, unwind, target, .. } = terminator.kind + else { bug!("unexpected terminator kind {:?}", terminator.kind); }; @@ -717,6 +718,29 @@ impl<'tcx> Inliner<'tcx> { Const::Val(..) | Const::Unevaluated(..) => true, }, )); + // 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 some extra work here to save the monomorphization collector work later. + // FIXME: benchmark which option is better. + let callee_item = { + // We need to reconstruct the `required_item` for the callee so that we can find and + // remove it. + let func_ty = func.ty(caller_body, self.tcx); + match func_ty.kind() { + ty::FnDef(def_id, args) => MentionedItem::Fn(*def_id, args), + _ => bug!(), + } + }; + if let Some(idx) = + caller_body.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); + } 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. + } } fn make_call_args( diff --git a/compiler/rustc_mir_transform/src/lib.rs b/compiler/rustc_mir_transform/src/lib.rs index 4551380152217..65bc2af2ad7eb 100644 --- a/compiler/rustc_mir_transform/src/lib.rs +++ b/compiler/rustc_mir_transform/src/lib.rs @@ -89,6 +89,7 @@ mod lint; mod lower_intrinsics; mod lower_slice_len; mod match_branches; +mod mentioned_items; mod multiple_return_terminators; mod normalize_array_len; mod nrvo; @@ -566,6 +567,10 @@ 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. diff --git a/compiler/rustc_mir_transform/src/mentioned_items.rs b/compiler/rustc_mir_transform/src/mentioned_items.rs new file mode 100644 index 0000000000000..b1b1ddbf665da --- /dev/null +++ b/compiler/rustc_mir_transform/src/mentioned_items.rs @@ -0,0 +1,56 @@ +use rustc_middle::mir::visit::Visitor; +use rustc_middle::mir::{self, ConstOperand, Location, MentionedItem, MirPass}; +use rustc_middle::ty::{self, TyCtxt}; +use rustc_session::Session; +use rustc_span::source_map::Spanned; + +pub struct MentionedItems; + +struct MentionedItemsVisitor<'a, 'tcx> { + tcx: TyCtxt<'tcx>, + body: &'a mir::Body<'tcx>, + mentioned_items: &'a mut Vec>>, +} + +impl<'tcx> MirPass<'tcx> for MentionedItems { + fn is_enabled(&self, _sess: &Session) -> bool { + // We need to do always do that as the collector will only check the 'mentioned' items of + // 'mentioned' items, so if some item X gets compiled without optimizations but later called + // from somewhere that has optimizations enabled, we need the `mentioned_items` of X to be + // filled. + true + } + + 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; + } +} + +impl<'tcx> Visitor<'tcx> for MentionedItemsVisitor<'_, 'tcx> { + fn visit_constant(&mut self, constant: &ConstOperand<'tcx>, _: Location) { + let const_ = constant.const_; + // This is how function items get referenced: via constants of `FnDef` type. + if let ty::FnDef(def_id, args) = const_.ty().kind() { + debug!("adding to required_items: {def_id:?}"); + self.mentioned_items + .push(Spanned { node: MentionedItem::Fn(*def_id, args), span: constant.span }); + } + } + + fn visit_terminator(&mut self, terminator: &mir::Terminator<'tcx>, location: Location) { + self.super_terminator(terminator, location); + match terminator.kind { + // We don't need to handle `Call` as we already handled all function type operands in + // `visit_constant`. But we do need to handle `Drop`. + mir::TerminatorKind::Drop { place, .. } => { + let ty = place.ty(self.body, self.tcx).ty; + let span = self.body.source_info(location).span; + self.mentioned_items.push(Spanned { node: MentionedItem::Drop(ty), span }); + } + _ => {} + } + } +} diff --git a/compiler/rustc_mir_transform/src/shim.rs b/compiler/rustc_mir_transform/src/shim.rs index 733e2f93b2535..dd2b24ad6691d 100644 --- a/compiler/rustc_mir_transform/src/shim.rs +++ b/compiler/rustc_mir_transform/src/shim.rs @@ -17,7 +17,7 @@ use std::iter; use crate::{ abort_unwinding_calls, add_call_guards, add_moves_for_packed_drops, deref_separator, - pass_manager as pm, remove_noop_landing_pads, simplify, + mentioned_items, pass_manager as pm, remove_noop_landing_pads, simplify, }; use rustc_middle::mir::patch::MirPatch; use rustc_mir_dataflow::elaborate_drops::{self, DropElaborator, DropFlagMode, DropStyle}; @@ -147,6 +147,7 @@ fn make_shim<'tcx>(tcx: TyCtxt<'tcx>, instance: ty::InstanceDef<'tcx>) -> Body<' tcx, &mut body, &[ + &mentioned_items::MentionedItems, &abort_unwinding_calls::AbortUnwindingCalls, &add_call_guards::CriticalCallEdges, ], @@ -178,6 +179,7 @@ fn make_shim<'tcx>(tcx: TyCtxt<'tcx>, instance: ty::InstanceDef<'tcx>) -> Body<' tcx, &mut result, &[ + &mentioned_items::MentionedItems, &add_moves_for_packed_drops::AddMovesForPackedDrops, &deref_separator::Derefer, &remove_noop_landing_pads::RemoveNoopLandingPads, diff --git a/compiler/rustc_monomorphize/src/collector.rs b/compiler/rustc_monomorphize/src/collector.rs index cd9eb4916ce5d..2bdfa069f49e2 100644 --- a/compiler/rustc_monomorphize/src/collector.rs +++ b/compiler/rustc_monomorphize/src/collector.rs @@ -165,7 +165,7 @@ //! regardless of whether it is actually needed or not. use rustc_data_structures::fx::{FxHashMap, FxHashSet}; -use rustc_data_structures::sync::{par_for_each_in, MTLock, MTLockRef}; +use rustc_data_structures::sync::{par_for_each_in, LRef, MTLock}; use rustc_hir as hir; use rustc_hir::def::DefKind; use rustc_hir::def_id::{DefId, DefIdMap, LocalDefId}; @@ -173,7 +173,7 @@ use rustc_hir::lang_items::LangItem; use rustc_middle::mir::interpret::{AllocId, ErrorHandled, GlobalAlloc, Scalar}; use rustc_middle::mir::mono::{InstantiationMode, MonoItem}; use rustc_middle::mir::visit::Visitor as MirVisitor; -use rustc_middle::mir::{self, Location}; +use rustc_middle::mir::{self, Location, MentionedItem}; use rustc_middle::query::TyCtxtAt; use rustc_middle::ty::adjustment::{CustomCoerceUnsized, PointerCoercion}; use rustc_middle::ty::layout::ValidityRequirement; @@ -214,6 +214,17 @@ pub struct UsageMap<'tcx> { type MonoItems<'tcx> = Vec>>; +/// The state that is shared across the concurrent threads that are doing collection. +struct SharedState<'tcx> { + /// Items that have been or are currently being recursively collected. + visited: MTLock>>, + /// Items that have been or are currently being recursively treated as "mentioned", i.e., their + /// consts are evaluated but nothing is added to the collection. + mentioned: MTLock>>, + /// Which items are being used where, for better errors. + usage_map: MTLock>, +} + impl<'tcx> UsageMap<'tcx> { fn new() -> UsageMap<'tcx> { UsageMap { used_map: FxHashMap::default(), user_map: FxHashMap::default() } @@ -265,13 +276,15 @@ pub fn collect_crate_mono_items( debug!("building mono item graph, beginning at roots"); - let mut visited = MTLock::new(FxHashSet::default()); - let mut usage_map = MTLock::new(UsageMap::new()); + let mut state = SharedState { + visited: MTLock::new(FxHashSet::default()), + mentioned: MTLock::new(FxHashSet::default()), + usage_map: MTLock::new(UsageMap::new()), + }; let recursion_limit = tcx.recursion_limit(); { - let visited: MTLockRef<'_, _> = &mut visited; - let usage_map: MTLockRef<'_, _> = &mut usage_map; + let state: LRef<'_, _> = &mut state; tcx.sess.time("monomorphization_collector_graph_walk", || { par_for_each_in(roots, |root| { @@ -279,16 +292,15 @@ pub fn collect_crate_mono_items( collect_items_rec( tcx, dummy_spanned(root), - visited, + state, &mut recursion_depths, recursion_limit, - usage_map, ); }); }); } - (visited.into_inner(), usage_map.into_inner()) + (state.visited.into_inner(), state.usage_map.into_inner()) } // Find all non-generic items by walking the HIR. These items serve as roots to @@ -331,21 +343,21 @@ fn collect_roots(tcx: TyCtxt<'_>, mode: MonoItemCollectionMode) -> Vec( tcx: TyCtxt<'tcx>, starting_item: Spanned>, - visited: MTLockRef<'_, FxHashSet>>, + state: LRef<'_, SharedState<'tcx>>, recursion_depths: &mut DefIdMap, recursion_limit: Limit, - usage_map: MTLockRef<'_, UsageMap<'tcx>>, ) { - if !visited.lock_mut().insert(starting_item.node) { + if !state.visited.lock_mut().insert(starting_item.node) { // We've been here already, no need to search again. return; } - let mut used_items = Vec::new(); + let mut used_items = MonoItems::new(); + let mut mentioned_items = MonoItems::new(); let recursion_depth_reset; // Post-monomorphization errors MVP @@ -373,6 +385,9 @@ fn collect_items_rec<'tcx>( // FIXME: don't rely on global state, instead bubble up errors. Note: this is very hard to do. let error_count = tcx.dcx().err_count(); + // In `mentioned_items` we collect items that were mentioned in this MIR but possibly do not + // need to be monomorphized. This is done to ensure that optimizing away function calls does not + // hide const-eval errors that those calls would otherwise have triggered. match starting_item.node { MonoItem::Static(def_id) => { let instance = Instance::mono(tcx, def_id); @@ -404,6 +419,10 @@ fn collect_items_rec<'tcx>( }), )); } + + // mentioned_items stays empty since there's no codegen for statics. statics don't get + // optimized, and if they did then the const-eval interpreter would have to worry about + // mentioned_items. } MonoItem::Fn(instance) => { // Sanity check whether this ended up being collected accidentally @@ -420,7 +439,12 @@ fn collect_items_rec<'tcx>( check_type_length_limit(tcx, instance); rustc_data_structures::stack::ensure_sufficient_stack(|| { - collect_used_items(tcx, instance, &mut used_items); + collect_used_and_mentioned_items( + tcx, + instance, + &mut used_items, + &mut mentioned_items, + ) }); } MonoItem::GlobalAsm(item_id) => { @@ -459,8 +483,10 @@ fn collect_items_rec<'tcx>( } else { span_bug!(item.span, "Mismatch between hir::Item type and MonoItem type") } + + // mention_items stays empty as nothing gets optimized here. } - } + }; // Check for PMEs and emit a diagnostic if one happened. To try to show relevant edges of the // mono item graph. @@ -474,10 +500,16 @@ fn collect_items_rec<'tcx>( formatted_item, }); } - usage_map.lock_mut().record_used(starting_item.node, &used_items); + state.usage_map.lock_mut().record_used(starting_item.node, &used_items); for used_item in used_items { - collect_items_rec(tcx, used_item, visited, recursion_depths, recursion_limit, usage_map); + collect_items_rec(tcx, used_item, state, recursion_depths, recursion_limit); + } + + // Walk over mentioned items *after* used items, so that if an item is both mentioned and used then + // the loop above has fully collected it, so this loop will skip it. + for mentioned_item in mentioned_items { + visit_mentioned_items_rec(tcx, mentioned_item, state, recursion_depths, recursion_limit); } if let Some((def_id, depth)) = recursion_depth_reset { @@ -485,6 +517,78 @@ fn collect_items_rec<'tcx>( } } +/// Visit all mentioned items reachable from the given one, and emit a note diagnostic if a +/// post-monomorphization error is encountered during a collection step. +/// +/// These items do *not* get collected, we just check that their constants all evaluated successfully. +#[instrument(skip(tcx, state, recursion_depths, recursion_limit), level = "debug")] +fn visit_mentioned_items_rec<'tcx>( + tcx: TyCtxt<'tcx>, + starting_item: Spanned>, + state: LRef<'_, SharedState<'tcx>>, + recursion_depths: &mut DefIdMap, + recursion_limit: Limit, +) { + if state.visited.lock().contains(&starting_item.node) { + // We've already done a *full* visit on this one, no need to do the "mention" visit. + return; + } + if !state.mentioned.lock_mut().insert(starting_item.node) { + // We've been here already, no need to search again. + return; + } + // There's some risk that we first do a 'mention' visit and then a full visit. But there's no + // harm in that, the mention visit will trigger all the queries and the results are cached. + + let mut mentioned_items = MonoItems::new(); + + match starting_item.node { + MonoItem::Fn(instance) => { + // Sanity check whether this ended up being collected accidentally + debug_assert!(should_codegen_locally(tcx, &instance)); + + let body = tcx.instance_mir(instance.def); + + // Need to evaluate all constants. + for const_op in &body.required_consts { + let const_ = instance.instantiate_mir_and_normalize_erasing_regions( + tcx, + ty::ParamEnv::reveal_all(), + ty::EarlyBinder::bind(const_op.const_), + ); + match const_.eval(tcx, ty::ParamEnv::reveal_all(), Some(const_op.span)) { + Ok(_) => {} + Err(ErrorHandled::TooGeneric(..)) => span_bug!( + const_op.span, + "collection encountered polymorphic constant: {:?}", + const_ + ), + Err(err @ ErrorHandled::Reported(..)) => { + err.emit_note(tcx); + } + } + } + + // And recurse into the mentioned items. + for item in &body.mentioned_items { + let item_mono = instance.instantiate_mir_and_normalize_erasing_regions( + tcx, + ty::ParamEnv::reveal_all(), + ty::EarlyBinder::bind(item.node), + ); + visit_mentioned_item(tcx, &item_mono, item.span, &mut mentioned_items); + } + } + other => { + bug!("only functions can be encountered during 'mention' collection, but got {other:?}") + } + } + + for mentioned_item in mentioned_items { + visit_mentioned_items_rec(tcx, mentioned_item, state, recursion_depths, recursion_limit); + } +} + /// Format instance name that is already known to be too long for rustc. /// Show only the first 2 types if it is longer than 32 characters to avoid blasting /// the user's terminal with thousands of lines of type-name. @@ -1493,26 +1597,55 @@ fn build_skip_move_check_fns(tcx: TyCtxt<'_>) -> Vec { } /// Scans the MIR in order to find function calls, closures, and drop-glue. -#[instrument(skip(tcx, output), level = "debug")] -fn collect_used_items<'tcx>( +/// +/// Anything that's found is added to `output`. Furthermore the "mentioned items" of the MIR are returned. +#[instrument(skip(tcx, used_items, mentioned_items), level = "debug")] +fn collect_used_and_mentioned_items<'tcx>( tcx: TyCtxt<'tcx>, instance: Instance<'tcx>, - output: &mut MonoItems<'tcx>, + used_items: &mut MonoItems<'tcx>, + mentioned_items: &mut MonoItems<'tcx>, ) { let body = tcx.instance_mir(instance.def); // Here we rely on the visitor also visiting `required_consts`, so that we evaluate them // and abort compilation if any of them errors. - MirUsedCollector { + let mut collector = MirUsedCollector { tcx, body: body, - output, + output: used_items, instance, move_size_spans: vec![], visiting_call_terminator: false, skip_move_check_fns: None, + }; + collector.visit_body(body); + + for item in &body.mentioned_items { + visit_mentioned_item(tcx, &collector.monomorphize(item.node), item.span, mentioned_items); + } +} + +fn visit_mentioned_item<'tcx>( + tcx: TyCtxt<'tcx>, + item: &MentionedItem<'tcx>, + span: Span, + output: &mut MonoItems<'tcx>, +) { + match *item { + MentionedItem::Fn(def_id, args) => { + let instance = Instance::expect_resolve(tcx, ty::ParamEnv::reveal_all(), def_id, args); + // `visit_instance_use` was written for "used" item collection but works just as well + // for "mentioned" item collection. + // We can set `is_direct_call`; that just means we'll skip a bunch of shims that anyway + // can't have their own failing constants. + visit_instance_use(tcx, instance, /*is_direct_call*/ true, span, output); + } + MentionedItem::Drop(ty) => { + let instance = Instance::resolve_drop_in_place(tcx, ty); + visit_instance_use(tcx, instance, /*is_direct_call*/ true, span, output); + } } - .visit_body(body); } #[instrument(skip(tcx, output), level = "debug")] diff --git a/tests/ui/consts/required-consts/collect-in-dead-drop.noopt.stderr b/tests/ui/consts/required-consts/collect-in-dead-drop.noopt.stderr index 0bf231d09f174..73790f7517db1 100644 --- a/tests/ui/consts/required-consts/collect-in-dead-drop.noopt.stderr +++ b/tests/ui/consts/required-consts/collect-in-dead-drop.noopt.stderr @@ -1,13 +1,13 @@ error[E0080]: evaluation of `Fail::::C` failed - --> $DIR/collect-in-dead-drop.rs:12:19 + --> $DIR/collect-in-dead-drop.rs:9:19 | LL | const C: () = panic!(); - | ^^^^^^^^ the evaluated program panicked at 'explicit panic', $DIR/collect-in-dead-drop.rs:12:19 + | ^^^^^^^^ the evaluated program panicked at 'explicit panic', $DIR/collect-in-dead-drop.rs:9:19 | = note: this error originates in the macro `$crate::panic::panic_2015` which comes from the expansion of the macro `panic` (in Nightly builds, run with -Z macro-backtrace for more info) note: erroneous constant encountered - --> $DIR/collect-in-dead-drop.rs:19:17 + --> $DIR/collect-in-dead-drop.rs:16:17 | LL | let _ = Fail::::C; | ^^^^^^^^^^^^ diff --git a/tests/ui/consts/required-consts/collect-in-dead-drop.opt.stderr b/tests/ui/consts/required-consts/collect-in-dead-drop.opt.stderr new file mode 100644 index 0000000000000..4ae13d6937f6e --- /dev/null +++ b/tests/ui/consts/required-consts/collect-in-dead-drop.opt.stderr @@ -0,0 +1,17 @@ +error[E0080]: evaluation of `Fail::::C` failed + --> $DIR/collect-in-dead-drop.rs:9:19 + | +LL | const C: () = panic!(); + | ^^^^^^^^ the evaluated program panicked at 'explicit panic', $DIR/collect-in-dead-drop.rs:9:19 + | + = note: this error originates in the macro `$crate::panic::panic_2015` which comes from the expansion of the macro `panic` (in Nightly builds, run with -Z macro-backtrace for more info) + +note: erroneous constant encountered + --> $DIR/collect-in-dead-drop.rs:16:17 + | +LL | let _ = Fail::::C; + | ^^^^^^^^^^^^ + +error: aborting due to 1 previous error + +For more information about this error, try `rustc --explain E0080`. diff --git a/tests/ui/consts/required-consts/collect-in-dead-drop.rs b/tests/ui/consts/required-consts/collect-in-dead-drop.rs index c9ffcec690317..98e9d19189597 100644 --- a/tests/ui/consts/required-consts/collect-in-dead-drop.rs +++ b/tests/ui/consts/required-consts/collect-in-dead-drop.rs @@ -1,15 +1,12 @@ //@revisions: noopt opt -//@[noopt] build-fail +//@ build-fail //@[opt] compile-flags: -O -//FIXME: `opt` revision currently does not stop with an error due to -//. -//@[opt] build-pass //! Make sure we detect erroneous constants post-monomorphization even when they are unused. This is //! crucial, people rely on it for soundness. (https://github.com/rust-lang/rust/issues/112090) struct Fail(T); impl Fail { - const C: () = panic!(); //[noopt]~ERROR evaluation of `Fail::::C` failed + const C: () = panic!(); //~ERROR evaluation of `Fail::::C` failed } // This function is not actually called, but is mentioned implicitly as destructor in dead code in a diff --git a/tests/ui/consts/required-consts/collect-in-dead-fn.noopt.stderr b/tests/ui/consts/required-consts/collect-in-dead-fn.noopt.stderr index 8bb99efe8e48d..52462076ff90b 100644 --- a/tests/ui/consts/required-consts/collect-in-dead-fn.noopt.stderr +++ b/tests/ui/consts/required-consts/collect-in-dead-fn.noopt.stderr @@ -1,19 +1,19 @@ error[E0080]: evaluation of `Fail::::C` failed - --> $DIR/collect-in-dead-fn.rs:12:19 + --> $DIR/collect-in-dead-fn.rs:9:19 | LL | const C: () = panic!(); - | ^^^^^^^^ the evaluated program panicked at 'explicit panic', $DIR/collect-in-dead-fn.rs:12:19 + | ^^^^^^^^ the evaluated program panicked at 'explicit panic', $DIR/collect-in-dead-fn.rs:9:19 | = note: this error originates in the macro `$crate::panic::panic_2015` which comes from the expansion of the macro `panic` (in Nightly builds, run with -Z macro-backtrace for more info) note: erroneous constant encountered - --> $DIR/collect-in-dead-fn.rs:22:17 + --> $DIR/collect-in-dead-fn.rs:19:17 | LL | let _ = Fail::::C; | ^^^^^^^^^^^^ note: the above error was encountered while instantiating `fn not_called::` - --> $DIR/collect-in-dead-fn.rs:29:9 + --> $DIR/collect-in-dead-fn.rs:26:9 | LL | not_called::(); | ^^^^^^^^^^^^^^^^^ diff --git a/tests/ui/consts/required-consts/collect-in-dead-fn.opt.stderr b/tests/ui/consts/required-consts/collect-in-dead-fn.opt.stderr new file mode 100644 index 0000000000000..34035abebdacb --- /dev/null +++ b/tests/ui/consts/required-consts/collect-in-dead-fn.opt.stderr @@ -0,0 +1,17 @@ +error[E0080]: evaluation of `Fail::::C` failed + --> $DIR/collect-in-dead-fn.rs:9:19 + | +LL | const C: () = panic!(); + | ^^^^^^^^ the evaluated program panicked at 'explicit panic', $DIR/collect-in-dead-fn.rs:9:19 + | + = note: this error originates in the macro `$crate::panic::panic_2015` which comes from the expansion of the macro `panic` (in Nightly builds, run with -Z macro-backtrace for more info) + +note: erroneous constant encountered + --> $DIR/collect-in-dead-fn.rs:19:17 + | +LL | let _ = Fail::::C; + | ^^^^^^^^^^^^ + +error: aborting due to 1 previous error + +For more information about this error, try `rustc --explain E0080`. diff --git a/tests/ui/consts/required-consts/collect-in-dead-fn.rs b/tests/ui/consts/required-consts/collect-in-dead-fn.rs index 9e6b151915331..754fcf7b9874d 100644 --- a/tests/ui/consts/required-consts/collect-in-dead-fn.rs +++ b/tests/ui/consts/required-consts/collect-in-dead-fn.rs @@ -1,15 +1,12 @@ //@revisions: noopt opt -//@[noopt] build-fail +//@ build-fail //@[opt] compile-flags: -O -//FIXME: `opt` revision currently does not stop with an error due to -//. -//@[opt] build-pass //! Make sure we detect erroneous constants post-monomorphization even when they are unused. This is //! crucial, people rely on it for soundness. (https://github.com/rust-lang/rust/issues/112090) struct Fail(T); impl Fail { - const C: () = panic!(); //[noopt]~ERROR evaluation of `Fail::::C` failed + const C: () = panic!(); //~ERROR evaluation of `Fail::::C` failed } // This function is not actually called, but it is mentioned in dead code in a function that is diff --git a/tests/ui/consts/required-consts/collect-in-dead-move.noopt.stderr b/tests/ui/consts/required-consts/collect-in-dead-move.noopt.stderr index 5b1df78b23265..2ab1f80e2d3b4 100644 --- a/tests/ui/consts/required-consts/collect-in-dead-move.noopt.stderr +++ b/tests/ui/consts/required-consts/collect-in-dead-move.noopt.stderr @@ -1,13 +1,13 @@ error[E0080]: evaluation of `Fail::::C` failed - --> $DIR/collect-in-dead-move.rs:12:19 + --> $DIR/collect-in-dead-move.rs:9:19 | LL | const C: () = panic!(); - | ^^^^^^^^ the evaluated program panicked at 'explicit panic', $DIR/collect-in-dead-move.rs:12:19 + | ^^^^^^^^ the evaluated program panicked at 'explicit panic', $DIR/collect-in-dead-move.rs:9:19 | = note: this error originates in the macro `$crate::panic::panic_2015` which comes from the expansion of the macro `panic` (in Nightly builds, run with -Z macro-backtrace for more info) note: erroneous constant encountered - --> $DIR/collect-in-dead-move.rs:19:17 + --> $DIR/collect-in-dead-move.rs:16:17 | LL | let _ = Fail::::C; | ^^^^^^^^^^^^ diff --git a/tests/ui/consts/required-consts/collect-in-dead-move.opt.stderr b/tests/ui/consts/required-consts/collect-in-dead-move.opt.stderr new file mode 100644 index 0000000000000..51b9b993cb2ab --- /dev/null +++ b/tests/ui/consts/required-consts/collect-in-dead-move.opt.stderr @@ -0,0 +1,17 @@ +error[E0080]: evaluation of `Fail::::C` failed + --> $DIR/collect-in-dead-move.rs:9:19 + | +LL | const C: () = panic!(); + | ^^^^^^^^ the evaluated program panicked at 'explicit panic', $DIR/collect-in-dead-move.rs:9:19 + | + = note: this error originates in the macro `$crate::panic::panic_2015` which comes from the expansion of the macro `panic` (in Nightly builds, run with -Z macro-backtrace for more info) + +note: erroneous constant encountered + --> $DIR/collect-in-dead-move.rs:16:17 + | +LL | let _ = Fail::::C; + | ^^^^^^^^^^^^ + +error: aborting due to 1 previous error + +For more information about this error, try `rustc --explain E0080`. diff --git a/tests/ui/consts/required-consts/collect-in-dead-move.rs b/tests/ui/consts/required-consts/collect-in-dead-move.rs index f3a6ba8a6577c..2fd6aea58de65 100644 --- a/tests/ui/consts/required-consts/collect-in-dead-move.rs +++ b/tests/ui/consts/required-consts/collect-in-dead-move.rs @@ -1,15 +1,12 @@ //@revisions: noopt opt -//@[noopt] build-fail +//@ build-fail //@[opt] compile-flags: -O -//FIXME: `opt` revision currently does not stop with an error due to -//. -//@[opt] build-pass //! Make sure we detect erroneous constants post-monomorphization even when they are unused. This is //! crucial, people rely on it for soundness. (https://github.com/rust-lang/rust/issues/112090) struct Fail(T); impl Fail { - const C: () = panic!(); //[noopt]~ERROR evaluation of `Fail::::C` failed + const C: () = panic!(); //~ERROR evaluation of `Fail::::C` failed } // This function is not actually called, but is mentioned implicitly as destructor in dead code in a From e4a3c8959b602b240254d3b2387824983d3ec7f7 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 16 Mar 2024 20:10:04 +0100 Subject: [PATCH 2/5] share code between used-item-visit and mentioned-item-visit logic --- .../src/mentioned_items.rs | 1 + compiler/rustc_monomorphize/src/collector.rs | 307 ++++++++++-------- .../rustc_monomorphize/src/partitioning.rs | 14 +- .../collect-in-dead-drop.opt.stderr | 3 + .../collect-in-dead-fn.opt.stderr | 6 + .../collect-in-dead-move.opt.stderr | 3 + 6 files changed, 191 insertions(+), 143 deletions(-) diff --git a/compiler/rustc_mir_transform/src/mentioned_items.rs b/compiler/rustc_mir_transform/src/mentioned_items.rs index b1b1ddbf665da..e05c925acd739 100644 --- a/compiler/rustc_mir_transform/src/mentioned_items.rs +++ b/compiler/rustc_mir_transform/src/mentioned_items.rs @@ -18,6 +18,7 @@ impl<'tcx> MirPass<'tcx> for MentionedItems { // 'mentioned' items, so if some item X gets compiled without optimizations but later called // from somewhere that has optimizations enabled, we need the `mentioned_items` of X to be // filled. + // Furthermore, even debug builds use level 1, so special-casing level 0 is just not worth it. true } diff --git a/compiler/rustc_monomorphize/src/collector.rs b/compiler/rustc_monomorphize/src/collector.rs index 2bdfa069f49e2..a41deb3b1c5aa 100644 --- a/compiler/rustc_monomorphize/src/collector.rs +++ b/compiler/rustc_monomorphize/src/collector.rs @@ -137,21 +137,41 @@ //! just linked to and no node is created; which is exactly what we want, since //! no machine code should be generated in the current crate for such an item. //! -//! Eager and Lazy Collection Mode -//! ------------------------------ -//! Mono item collection can be performed in one of two modes: +//! Eager and Lazy Collection Strategy +//! ---------------------------------- +//! Mono item collection can be performed with one of two strategies: //! -//! - Lazy mode means that items will only be instantiated when actually +//! - Lazy strategy means that items will only be instantiated when actually //! used. The goal is to produce the least amount of machine code //! possible. //! -//! - Eager mode is meant to be used in conjunction with incremental compilation +//! - Eager strategy is meant to be used in conjunction with incremental compilation //! where a stable set of mono items is more important than a minimal -//! one. Thus, eager mode will instantiate drop-glue for every drop-able type +//! one. Thus, eager strategy will instantiate drop-glue for every drop-able type //! in the crate, even if no drop call for that type exists (yet). It will //! also instantiate default implementations of trait methods, something that //! otherwise is only done on demand. //! +//! Collection-time const evaluation and "mentioned" items +//! ------------------------------------------------------ +//! +//! One important role of collection is to evaluate all constants that are used by all the items +//! which are being collected. Codegen can then rely on only encountering constants that evaluate +//! successfully, and if a constant fails to evaluate, the collector has much better context to be +//! able to show where this constant comes up. +//! +//! However, the exact set of "used" items (collected as described above), and therefore the exact +//! set of used constants, can depend on optimizations. Optimizing away dead code may optimize away +//! a function call that uses a failing constant, so an unoptimized build may fail where an +//! optimized build succeeds. This is undesirable. +//! +//! To fix this, the collector has the concept of "mentioned" items. Some time during the MIR +//! pipeline, before any optimization-level-dependent optimizations, we compute a list of all items +//! that syntactically appear in the code. These are considered "mentioned", and even if they are in +//! dead code and get optimized away (which makes them no longer "used"), they are still +//! "mentioned". For every used item, the collector ensures that all mentioned items, recursively, +//! do not use a failing constant. This is reflected via the [`CollectionMode`], which determines +//! whether we are visiting a used item or merely a mentioned item. //! //! Open Issues //! ----------- @@ -199,7 +219,7 @@ use crate::errors::{ }; #[derive(PartialEq)] -pub enum MonoItemCollectionMode { +pub enum MonoItemCollectionStrategy { Eager, Lazy, } @@ -225,6 +245,24 @@ struct SharedState<'tcx> { usage_map: MTLock>, } +/// See module-level docs on some contect for "mentioned" items. +#[derive(Copy, Clone, Debug, PartialEq)] +enum CollectionMode { + /// Collect items that are used, i.e., actually needed for codegen. + /// + /// Which items are used can depend on optimization levels, as MIR optimizations can remove + /// uses. + UsedItems, + /// Collect items that are mentioned. The goal of this mode is that it is independent of + /// optimizations: the set of "mentioned" items is computed before optimizations are run. + /// + /// The exact contents of this set are *not* a stable guarantee. (For instance, it is currently + /// computed after drop-elaboration. If we ever do some optimizations even in debug builds, we + /// might decide to run them before computing mentioned items.) The key property of this set is + /// that it is optimization-independent. + MentionedItems, +} + impl<'tcx> UsageMap<'tcx> { fn new() -> UsageMap<'tcx> { UsageMap { used_map: FxHashMap::default(), user_map: FxHashMap::default() } @@ -264,15 +302,16 @@ impl<'tcx> UsageMap<'tcx> { } } -#[instrument(skip(tcx, mode), level = "debug")] +#[instrument(skip(tcx, strategy), level = "debug")] pub fn collect_crate_mono_items( tcx: TyCtxt<'_>, - mode: MonoItemCollectionMode, + strategy: MonoItemCollectionStrategy, ) -> (FxHashSet>, UsageMap<'_>) { let _prof_timer = tcx.prof.generic_activity("monomorphization_collector"); - let roots = - tcx.sess.time("monomorphization_collector_root_collections", || collect_roots(tcx, mode)); + let roots = tcx + .sess + .time("monomorphization_collector_root_collections", || collect_roots(tcx, strategy)); debug!("building mono item graph, beginning at roots"); @@ -295,6 +334,7 @@ pub fn collect_crate_mono_items( state, &mut recursion_depths, recursion_limit, + CollectionMode::UsedItems, ); }); }); @@ -306,7 +346,7 @@ pub fn collect_crate_mono_items( // Find all non-generic items by walking the HIR. These items serve as roots to // start monomorphizing from. #[instrument(skip(tcx, mode), level = "debug")] -fn collect_roots(tcx: TyCtxt<'_>, mode: MonoItemCollectionMode) -> Vec> { +fn collect_roots(tcx: TyCtxt<'_>, mode: MonoItemCollectionStrategy) -> Vec> { debug!("collecting roots"); let mut roots = Vec::new(); @@ -315,7 +355,7 @@ fn collect_roots(tcx: TyCtxt<'_>, mode: MonoItemCollectionMode) -> Vec, mode: MonoItemCollectionMode) -> Vec( tcx: TyCtxt<'tcx>, @@ -350,10 +393,24 @@ fn collect_items_rec<'tcx>( state: LRef<'_, SharedState<'tcx>>, recursion_depths: &mut DefIdMap, recursion_limit: Limit, + mode: CollectionMode, ) { - if !state.visited.lock_mut().insert(starting_item.node) { - // We've been here already, no need to search again. - return; + if mode == CollectionMode::UsedItems { + if !state.visited.lock_mut().insert(starting_item.node) { + // We've been here already, no need to search again. + return; + } + } else { + if state.visited.lock().contains(&starting_item.node) { + // We've already done a *full* visit on this one, no need to do the "mention" visit. + return; + } + if !state.mentioned.lock_mut().insert(starting_item.node) { + // We've been here already, no need to search again. + return; + } + // There's some risk that we first do a 'mention' visit and then a full visit. But there's no + // harm in that, the mention visit will trigger all the queries and the results are cached. } let mut used_items = MonoItems::new(); @@ -390,6 +447,10 @@ fn collect_items_rec<'tcx>( // hide const-eval errors that those calls would otherwise have triggered. match starting_item.node { MonoItem::Static(def_id) => { + assert!( + mode == CollectionMode::UsedItems, + "should never encounter static when collecting mentioned items" + ); let instance = Instance::mono(tcx, def_id); // Sanity check whether this ended up being collected accidentally @@ -439,15 +500,20 @@ fn collect_items_rec<'tcx>( check_type_length_limit(tcx, instance); rustc_data_structures::stack::ensure_sufficient_stack(|| { - collect_used_and_mentioned_items( + collect_items_of_instance( tcx, instance, &mut used_items, &mut mentioned_items, + mode, ) }); } MonoItem::GlobalAsm(item_id) => { + assert!( + mode == CollectionMode::UsedItems, + "should never encounter global_asm when collecting mentioned items" + ); recursion_depth_reset = None; let item = tcx.hir().item(item_id); @@ -487,6 +553,9 @@ fn collect_items_rec<'tcx>( // mention_items stays empty as nothing gets optimized here. } }; + if mode == CollectionMode::MentionedItems { + assert!(used_items.is_empty(), "'mentioned' collection should never encounter used items"); + } // Check for PMEs and emit a diagnostic if one happened. To try to show relevant edges of the // mono item graph. @@ -500,16 +569,36 @@ fn collect_items_rec<'tcx>( formatted_item, }); } - state.usage_map.lock_mut().record_used(starting_item.node, &used_items); + // Only updating `usage_map` for used items as otherwise we may be inserting the same item + // multiple times (if it is first 'mentioned' and then later actuall used), and the usage map + // logic does not like that. + // FIXME: see if we can make the map tolerate that. + if mode == CollectionMode::UsedItems { + state.usage_map.lock_mut().record_used(starting_item.node, &used_items); + } for used_item in used_items { - collect_items_rec(tcx, used_item, state, recursion_depths, recursion_limit); + collect_items_rec( + tcx, + used_item, + state, + recursion_depths, + recursion_limit, + CollectionMode::UsedItems, + ); } // Walk over mentioned items *after* used items, so that if an item is both mentioned and used then // the loop above has fully collected it, so this loop will skip it. for mentioned_item in mentioned_items { - visit_mentioned_items_rec(tcx, mentioned_item, state, recursion_depths, recursion_limit); + collect_items_rec( + tcx, + mentioned_item, + state, + recursion_depths, + recursion_limit, + CollectionMode::MentionedItems, + ); } if let Some((def_id, depth)) = recursion_depth_reset { @@ -517,78 +606,6 @@ fn collect_items_rec<'tcx>( } } -/// Visit all mentioned items reachable from the given one, and emit a note diagnostic if a -/// post-monomorphization error is encountered during a collection step. -/// -/// These items do *not* get collected, we just check that their constants all evaluated successfully. -#[instrument(skip(tcx, state, recursion_depths, recursion_limit), level = "debug")] -fn visit_mentioned_items_rec<'tcx>( - tcx: TyCtxt<'tcx>, - starting_item: Spanned>, - state: LRef<'_, SharedState<'tcx>>, - recursion_depths: &mut DefIdMap, - recursion_limit: Limit, -) { - if state.visited.lock().contains(&starting_item.node) { - // We've already done a *full* visit on this one, no need to do the "mention" visit. - return; - } - if !state.mentioned.lock_mut().insert(starting_item.node) { - // We've been here already, no need to search again. - return; - } - // There's some risk that we first do a 'mention' visit and then a full visit. But there's no - // harm in that, the mention visit will trigger all the queries and the results are cached. - - let mut mentioned_items = MonoItems::new(); - - match starting_item.node { - MonoItem::Fn(instance) => { - // Sanity check whether this ended up being collected accidentally - debug_assert!(should_codegen_locally(tcx, &instance)); - - let body = tcx.instance_mir(instance.def); - - // Need to evaluate all constants. - for const_op in &body.required_consts { - let const_ = instance.instantiate_mir_and_normalize_erasing_regions( - tcx, - ty::ParamEnv::reveal_all(), - ty::EarlyBinder::bind(const_op.const_), - ); - match const_.eval(tcx, ty::ParamEnv::reveal_all(), Some(const_op.span)) { - Ok(_) => {} - Err(ErrorHandled::TooGeneric(..)) => span_bug!( - const_op.span, - "collection encountered polymorphic constant: {:?}", - const_ - ), - Err(err @ ErrorHandled::Reported(..)) => { - err.emit_note(tcx); - } - } - } - - // And recurse into the mentioned items. - for item in &body.mentioned_items { - let item_mono = instance.instantiate_mir_and_normalize_erasing_regions( - tcx, - ty::ParamEnv::reveal_all(), - ty::EarlyBinder::bind(item.node), - ); - visit_mentioned_item(tcx, &item_mono, item.span, &mut mentioned_items); - } - } - other => { - bug!("only functions can be encountered during 'mention' collection, but got {other:?}") - } - } - - for mentioned_item in mentioned_items { - visit_mentioned_items_rec(tcx, mentioned_item, state, recursion_depths, recursion_limit); - } -} - /// Format instance name that is already known to be too long for rustc. /// Show only the first 2 types if it is longer than 32 characters to avoid blasting /// the user's terminal with thousands of lines of type-name. @@ -700,7 +717,7 @@ fn check_type_length_limit<'tcx>(tcx: TyCtxt<'tcx>, instance: Instance<'tcx>) { struct MirUsedCollector<'a, 'tcx> { tcx: TyCtxt<'tcx>, body: &'a mir::Body<'tcx>, - output: &'a mut MonoItems<'tcx>, + used_items: &'a mut MonoItems<'tcx>, instance: Instance<'tcx>, /// Spans for move size lints already emitted. Helps avoid duplicate lints. move_size_spans: Vec, @@ -839,6 +856,31 @@ impl<'a, 'tcx> MirUsedCollector<'a, 'tcx> { ); self.move_size_spans.push(span); } + + /// Evaluates a *not yet monomorphized* constant. + fn eval_constant( + &mut self, + constant: &mir::ConstOperand<'tcx>, + ) -> Option> { + let const_ = self.monomorphize(constant.const_); + let param_env = ty::ParamEnv::reveal_all(); + // Evaluate the constant. This makes const eval failure a collection-time error (rather than + // a codegen-time error). rustc stops after collection if there was an error, so this + // ensures codegen never has to worry about failing consts. + // (codegen relies on this and ICEs will happen if this is violated.) + match const_.eval(self.tcx, param_env, Some(constant.span)) { + Ok(v) => Some(v), + Err(ErrorHandled::TooGeneric(..)) => span_bug!( + constant.span, + "collection encountered polymorphic constant: {:?}", + const_ + ), + Err(err @ ErrorHandled::Reported(..)) => { + err.emit_note(self.tcx); + return None; + } + } + } } impl<'a, 'tcx> MirVisitor<'tcx> for MirUsedCollector<'a, 'tcx> { @@ -873,7 +915,7 @@ impl<'a, 'tcx> MirVisitor<'tcx> for MirUsedCollector<'a, 'tcx> { target_ty, source_ty, span, - self.output, + self.used_items, ); } } @@ -884,7 +926,7 @@ impl<'a, 'tcx> MirVisitor<'tcx> for MirUsedCollector<'a, 'tcx> { ) => { let fn_ty = operand.ty(self.body, self.tcx); let fn_ty = self.monomorphize(fn_ty); - visit_fn_use(self.tcx, fn_ty, false, span, self.output); + visit_fn_use(self.tcx, fn_ty, false, span, self.used_items); } mir::Rvalue::Cast( mir::CastKind::PointerCoercion(PointerCoercion::ClosureFnPointer(_)), @@ -902,7 +944,7 @@ impl<'a, 'tcx> MirVisitor<'tcx> for MirUsedCollector<'a, 'tcx> { ty::ClosureKind::FnOnce, ); if should_codegen_locally(self.tcx, &instance) { - self.output.push(create_fn_mono_item(self.tcx, instance, span)); + self.used_items.push(create_fn_mono_item(self.tcx, instance, span)); } } _ => bug!(), @@ -913,7 +955,7 @@ impl<'a, 'tcx> MirVisitor<'tcx> for MirUsedCollector<'a, 'tcx> { let instance = Instance::mono(self.tcx, def_id); if should_codegen_locally(self.tcx, &instance) { trace!("collecting thread-local static {:?}", def_id); - self.output.push(respan(span, MonoItem::Static(def_id))); + self.used_items.push(respan(span, MonoItem::Static(def_id))); } } _ => { /* not interesting */ } @@ -926,26 +968,9 @@ impl<'a, 'tcx> MirVisitor<'tcx> for MirUsedCollector<'a, 'tcx> { /// to ensure that the constant evaluates successfully and walk the result. #[instrument(skip(self), level = "debug")] fn visit_constant(&mut self, constant: &mir::ConstOperand<'tcx>, location: Location) { - let const_ = self.monomorphize(constant.const_); - let param_env = ty::ParamEnv::reveal_all(); - // Evaluate the constant. This makes const eval failure a collection-time error (rather than - // a codegen-time error). rustc stops after collection if there was an error, so this - // ensures codegen never has to worry about failing consts. - // (codegen relies on this and ICEs will happen if this is violated.) - let val = match const_.eval(self.tcx, param_env, Some(constant.span)) { - Ok(v) => v, - Err(ErrorHandled::TooGeneric(..)) => span_bug!( - self.body.source_info(location).span, - "collection encountered polymorphic constant: {:?}", - const_ - ), - Err(err @ ErrorHandled::Reported(..)) => { - err.emit_note(self.tcx); - return; - } - }; - collect_const_value(self.tcx, val, self.output); - MirVisitor::visit_ty(self, const_.ty(), TyContext::Location(location)); + let Some(val) = self.eval_constant(constant) else { return }; + collect_const_value(self.tcx, val, self.used_items); + MirVisitor::visit_ty(self, constant.ty(), TyContext::Location(location)); } fn visit_terminator(&mut self, terminator: &mir::Terminator<'tcx>, location: Location) { @@ -956,7 +981,7 @@ impl<'a, 'tcx> MirVisitor<'tcx> for MirUsedCollector<'a, 'tcx> { let push_mono_lang_item = |this: &mut Self, lang_item: LangItem| { let instance = Instance::mono(tcx, tcx.require_lang_item(lang_item, Some(source))); if should_codegen_locally(tcx, &instance) { - this.output.push(create_fn_mono_item(tcx, instance, source)); + this.used_items.push(create_fn_mono_item(tcx, instance, source)); } }; @@ -965,25 +990,25 @@ impl<'a, 'tcx> MirVisitor<'tcx> for MirUsedCollector<'a, 'tcx> { let callee_ty = func.ty(self.body, tcx); let callee_ty = self.monomorphize(callee_ty); self.check_fn_args_move_size(callee_ty, args, *fn_span, location); - visit_fn_use(self.tcx, callee_ty, true, source, &mut self.output) + visit_fn_use(self.tcx, callee_ty, true, source, &mut self.used_items) } mir::TerminatorKind::Drop { ref place, .. } => { let ty = place.ty(self.body, self.tcx).ty; let ty = self.monomorphize(ty); - visit_drop_use(self.tcx, ty, true, source, self.output); + visit_drop_use(self.tcx, ty, true, source, self.used_items); } mir::TerminatorKind::InlineAsm { ref operands, .. } => { for op in operands { match *op { mir::InlineAsmOperand::SymFn { ref value } => { let fn_ty = self.monomorphize(value.const_.ty()); - visit_fn_use(self.tcx, fn_ty, false, source, self.output); + visit_fn_use(self.tcx, fn_ty, false, source, self.used_items); } mir::InlineAsmOperand::SymStatic { def_id } => { let instance = Instance::mono(self.tcx, def_id); if should_codegen_locally(self.tcx, &instance) { trace!("collecting asm sym static {:?}", def_id); - self.output.push(respan(source, MonoItem::Static(def_id))); + self.used_items.push(respan(source, MonoItem::Static(def_id))); } } _ => {} @@ -1343,7 +1368,7 @@ fn create_mono_items_for_vtable_methods<'tcx>( struct RootCollector<'a, 'tcx> { tcx: TyCtxt<'tcx>, - mode: MonoItemCollectionMode, + strategy: MonoItemCollectionStrategy, output: &'a mut MonoItems<'tcx>, entry_fn: Option<(DefId, EntryFnType)>, } @@ -1352,7 +1377,7 @@ impl<'v> RootCollector<'_, 'v> { fn process_item(&mut self, id: hir::ItemId) { match self.tcx.def_kind(id.owner_id) { DefKind::Enum | DefKind::Struct | DefKind::Union => { - if self.mode == MonoItemCollectionMode::Eager + if self.strategy == MonoItemCollectionStrategy::Eager && self.tcx.generics_of(id.owner_id).count() == 0 { debug!("RootCollector: ADT drop-glue for `{id:?}`",); @@ -1383,7 +1408,7 @@ impl<'v> RootCollector<'_, 'v> { } } DefKind::Impl { .. } => { - if self.mode == MonoItemCollectionMode::Eager { + if self.strategy == MonoItemCollectionStrategy::Eager { create_mono_items_for_default_impls(self.tcx, id, self.output); } } @@ -1402,9 +1427,9 @@ impl<'v> RootCollector<'_, 'v> { fn is_root(&self, def_id: LocalDefId) -> bool { !self.tcx.generics_of(def_id).requires_monomorphization(self.tcx) - && match self.mode { - MonoItemCollectionMode::Eager => true, - MonoItemCollectionMode::Lazy => { + && match self.strategy { + MonoItemCollectionStrategy::Eager => true, + MonoItemCollectionStrategy::Lazy => { self.entry_fn.and_then(|(id, _)| id.as_local()) == Some(def_id) || self.tcx.is_reachable_non_generic(def_id) || self @@ -1600,29 +1625,39 @@ fn build_skip_move_check_fns(tcx: TyCtxt<'_>) -> Vec { /// /// Anything that's found is added to `output`. Furthermore the "mentioned items" of the MIR are returned. #[instrument(skip(tcx, used_items, mentioned_items), level = "debug")] -fn collect_used_and_mentioned_items<'tcx>( +fn collect_items_of_instance<'tcx>( tcx: TyCtxt<'tcx>, instance: Instance<'tcx>, used_items: &mut MonoItems<'tcx>, mentioned_items: &mut MonoItems<'tcx>, + mode: CollectionMode, ) { let body = tcx.instance_mir(instance.def); - - // Here we rely on the visitor also visiting `required_consts`, so that we evaluate them - // and abort compilation if any of them errors. let mut collector = MirUsedCollector { tcx, - body: body, - output: used_items, + body, + used_items, instance, move_size_spans: vec![], visiting_call_terminator: false, skip_move_check_fns: None, }; - collector.visit_body(body); + if mode == CollectionMode::UsedItems { + // Visit everything. Here we rely on the visitor also visiting `required_consts`, so that we + // evaluate them and abort compilation if any of them errors. + collector.visit_body(body); + } else { + // We only need to evaluate all constants, but can ignore the rest of the MIR. + for const_op in &body.required_consts { + collector.eval_constant(const_op); + } + } + + // Always gather mentioned items. for item in &body.mentioned_items { - visit_mentioned_item(tcx, &collector.monomorphize(item.node), item.span, mentioned_items); + let item_mono = collector.monomorphize(item.node); + visit_mentioned_item(tcx, &item_mono, item.span, mentioned_items); } } diff --git a/compiler/rustc_monomorphize/src/partitioning.rs b/compiler/rustc_monomorphize/src/partitioning.rs index 15041b9cd4181..5a92657cb40d7 100644 --- a/compiler/rustc_monomorphize/src/partitioning.rs +++ b/compiler/rustc_monomorphize/src/partitioning.rs @@ -117,7 +117,7 @@ use rustc_session::CodegenUnits; use rustc_span::symbol::Symbol; use crate::collector::UsageMap; -use crate::collector::{self, MonoItemCollectionMode}; +use crate::collector::{self, MonoItemCollectionStrategy}; use crate::errors::{CouldntDumpMonoStats, SymbolAlreadyDefined, UnknownCguCollectionMode}; struct PartitioningCx<'a, 'tcx> { @@ -1087,30 +1087,30 @@ where } fn collect_and_partition_mono_items(tcx: TyCtxt<'_>, (): ()) -> (&DefIdSet, &[CodegenUnit<'_>]) { - let collection_mode = match tcx.sess.opts.unstable_opts.print_mono_items { + let collection_strategy = match tcx.sess.opts.unstable_opts.print_mono_items { Some(ref s) => { let mode = s.to_lowercase(); let mode = mode.trim(); if mode == "eager" { - MonoItemCollectionMode::Eager + MonoItemCollectionStrategy::Eager } else { if mode != "lazy" { tcx.dcx().emit_warn(UnknownCguCollectionMode { mode }); } - MonoItemCollectionMode::Lazy + MonoItemCollectionStrategy::Lazy } } None => { if tcx.sess.link_dead_code() { - MonoItemCollectionMode::Eager + MonoItemCollectionStrategy::Eager } else { - MonoItemCollectionMode::Lazy + MonoItemCollectionStrategy::Lazy } } }; - let (items, usage_map) = collector::collect_crate_mono_items(tcx, collection_mode); + let (items, usage_map) = collector::collect_crate_mono_items(tcx, collection_strategy); // If there was an error during collection (e.g. from one of the constants we evaluated), // then we stop here. This way codegen does not have to worry about failing constants. diff --git a/tests/ui/consts/required-consts/collect-in-dead-drop.opt.stderr b/tests/ui/consts/required-consts/collect-in-dead-drop.opt.stderr index 4ae13d6937f6e..73790f7517db1 100644 --- a/tests/ui/consts/required-consts/collect-in-dead-drop.opt.stderr +++ b/tests/ui/consts/required-consts/collect-in-dead-drop.opt.stderr @@ -12,6 +12,9 @@ note: erroneous constant encountered LL | let _ = Fail::::C; | ^^^^^^^^^^^^ +note: the above error was encountered while instantiating `fn as std::ops::Drop>::drop` + --> $SRC_DIR/core/src/ptr/mod.rs:LL:COL + error: aborting due to 1 previous error For more information about this error, try `rustc --explain E0080`. diff --git a/tests/ui/consts/required-consts/collect-in-dead-fn.opt.stderr b/tests/ui/consts/required-consts/collect-in-dead-fn.opt.stderr index 34035abebdacb..30c5c2d5c5caa 100644 --- a/tests/ui/consts/required-consts/collect-in-dead-fn.opt.stderr +++ b/tests/ui/consts/required-consts/collect-in-dead-fn.opt.stderr @@ -12,6 +12,12 @@ note: erroneous constant encountered LL | let _ = Fail::::C; | ^^^^^^^^^^^^ +note: the above error was encountered while instantiating `fn not_called::` + --> $DIR/collect-in-dead-fn.rs:26:9 + | +LL | not_called::(); + | ^^^^^^^^^^^^^^^ + error: aborting due to 1 previous error For more information about this error, try `rustc --explain E0080`. diff --git a/tests/ui/consts/required-consts/collect-in-dead-move.opt.stderr b/tests/ui/consts/required-consts/collect-in-dead-move.opt.stderr index 51b9b993cb2ab..2ab1f80e2d3b4 100644 --- a/tests/ui/consts/required-consts/collect-in-dead-move.opt.stderr +++ b/tests/ui/consts/required-consts/collect-in-dead-move.opt.stderr @@ -12,6 +12,9 @@ note: erroneous constant encountered LL | let _ = Fail::::C; | ^^^^^^^^^^^^ +note: the above error was encountered while instantiating `fn as std::ops::Drop>::drop` + --> $SRC_DIR/core/src/ptr/mod.rs:LL:COL + error: aborting due to 1 previous error For more information about this error, try `rustc --explain E0080`. From 6306c17823354801bde600e77512541107dd3292 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 17 Mar 2024 09:02:18 +0100 Subject: [PATCH 3/5] fix comments in required-consts tests --- .../required-consts/collect-in-dead-drop.noopt.stderr | 6 +++--- .../required-consts/collect-in-dead-drop.opt.stderr | 6 +++--- tests/ui/consts/required-consts/collect-in-dead-drop.rs | 3 +-- .../required-consts/collect-in-dead-fn.noopt.stderr | 8 ++++---- .../consts/required-consts/collect-in-dead-fn.opt.stderr | 8 ++++---- tests/ui/consts/required-consts/collect-in-dead-fn.rs | 3 +-- tests/ui/consts/required-consts/collect-in-dead-forget.rs | 3 +-- .../required-consts/collect-in-dead-move.noopt.stderr | 6 +++--- .../required-consts/collect-in-dead-move.opt.stderr | 6 +++--- tests/ui/consts/required-consts/collect-in-dead-move.rs | 3 +-- .../required-consts/collect-in-dead-vtable.noopt.stderr | 8 ++++---- tests/ui/consts/required-consts/collect-in-dead-vtable.rs | 3 +-- 12 files changed, 29 insertions(+), 34 deletions(-) diff --git a/tests/ui/consts/required-consts/collect-in-dead-drop.noopt.stderr b/tests/ui/consts/required-consts/collect-in-dead-drop.noopt.stderr index 73790f7517db1..796df067e0b6c 100644 --- a/tests/ui/consts/required-consts/collect-in-dead-drop.noopt.stderr +++ b/tests/ui/consts/required-consts/collect-in-dead-drop.noopt.stderr @@ -1,13 +1,13 @@ error[E0080]: evaluation of `Fail::::C` failed - --> $DIR/collect-in-dead-drop.rs:9:19 + --> $DIR/collect-in-dead-drop.rs:8:19 | LL | const C: () = panic!(); - | ^^^^^^^^ the evaluated program panicked at 'explicit panic', $DIR/collect-in-dead-drop.rs:9:19 + | ^^^^^^^^ the evaluated program panicked at 'explicit panic', $DIR/collect-in-dead-drop.rs:8:19 | = note: this error originates in the macro `$crate::panic::panic_2015` which comes from the expansion of the macro `panic` (in Nightly builds, run with -Z macro-backtrace for more info) note: erroneous constant encountered - --> $DIR/collect-in-dead-drop.rs:16:17 + --> $DIR/collect-in-dead-drop.rs:15:17 | LL | let _ = Fail::::C; | ^^^^^^^^^^^^ diff --git a/tests/ui/consts/required-consts/collect-in-dead-drop.opt.stderr b/tests/ui/consts/required-consts/collect-in-dead-drop.opt.stderr index 73790f7517db1..796df067e0b6c 100644 --- a/tests/ui/consts/required-consts/collect-in-dead-drop.opt.stderr +++ b/tests/ui/consts/required-consts/collect-in-dead-drop.opt.stderr @@ -1,13 +1,13 @@ error[E0080]: evaluation of `Fail::::C` failed - --> $DIR/collect-in-dead-drop.rs:9:19 + --> $DIR/collect-in-dead-drop.rs:8:19 | LL | const C: () = panic!(); - | ^^^^^^^^ the evaluated program panicked at 'explicit panic', $DIR/collect-in-dead-drop.rs:9:19 + | ^^^^^^^^ the evaluated program panicked at 'explicit panic', $DIR/collect-in-dead-drop.rs:8:19 | = note: this error originates in the macro `$crate::panic::panic_2015` which comes from the expansion of the macro `panic` (in Nightly builds, run with -Z macro-backtrace for more info) note: erroneous constant encountered - --> $DIR/collect-in-dead-drop.rs:16:17 + --> $DIR/collect-in-dead-drop.rs:15:17 | LL | let _ = Fail::::C; | ^^^^^^^^^^^^ diff --git a/tests/ui/consts/required-consts/collect-in-dead-drop.rs b/tests/ui/consts/required-consts/collect-in-dead-drop.rs index 98e9d19189597..ca140f7e95e45 100644 --- a/tests/ui/consts/required-consts/collect-in-dead-drop.rs +++ b/tests/ui/consts/required-consts/collect-in-dead-drop.rs @@ -1,8 +1,7 @@ //@revisions: noopt opt //@ build-fail //@[opt] compile-flags: -O -//! Make sure we detect erroneous constants post-monomorphization even when they are unused. This is -//! crucial, people rely on it for soundness. (https://github.com/rust-lang/rust/issues/112090) +//! This fails without optimizations, so it should also fail with optimizations. struct Fail(T); impl Fail { diff --git a/tests/ui/consts/required-consts/collect-in-dead-fn.noopt.stderr b/tests/ui/consts/required-consts/collect-in-dead-fn.noopt.stderr index 52462076ff90b..f0b7984d2fa35 100644 --- a/tests/ui/consts/required-consts/collect-in-dead-fn.noopt.stderr +++ b/tests/ui/consts/required-consts/collect-in-dead-fn.noopt.stderr @@ -1,19 +1,19 @@ error[E0080]: evaluation of `Fail::::C` failed - --> $DIR/collect-in-dead-fn.rs:9:19 + --> $DIR/collect-in-dead-fn.rs:8:19 | LL | const C: () = panic!(); - | ^^^^^^^^ the evaluated program panicked at 'explicit panic', $DIR/collect-in-dead-fn.rs:9:19 + | ^^^^^^^^ the evaluated program panicked at 'explicit panic', $DIR/collect-in-dead-fn.rs:8:19 | = note: this error originates in the macro `$crate::panic::panic_2015` which comes from the expansion of the macro `panic` (in Nightly builds, run with -Z macro-backtrace for more info) note: erroneous constant encountered - --> $DIR/collect-in-dead-fn.rs:19:17 + --> $DIR/collect-in-dead-fn.rs:18:17 | LL | let _ = Fail::::C; | ^^^^^^^^^^^^ note: the above error was encountered while instantiating `fn not_called::` - --> $DIR/collect-in-dead-fn.rs:26:9 + --> $DIR/collect-in-dead-fn.rs:25:9 | LL | not_called::(); | ^^^^^^^^^^^^^^^^^ diff --git a/tests/ui/consts/required-consts/collect-in-dead-fn.opt.stderr b/tests/ui/consts/required-consts/collect-in-dead-fn.opt.stderr index 30c5c2d5c5caa..25c9bb0f8c63a 100644 --- a/tests/ui/consts/required-consts/collect-in-dead-fn.opt.stderr +++ b/tests/ui/consts/required-consts/collect-in-dead-fn.opt.stderr @@ -1,19 +1,19 @@ error[E0080]: evaluation of `Fail::::C` failed - --> $DIR/collect-in-dead-fn.rs:9:19 + --> $DIR/collect-in-dead-fn.rs:8:19 | LL | const C: () = panic!(); - | ^^^^^^^^ the evaluated program panicked at 'explicit panic', $DIR/collect-in-dead-fn.rs:9:19 + | ^^^^^^^^ the evaluated program panicked at 'explicit panic', $DIR/collect-in-dead-fn.rs:8:19 | = note: this error originates in the macro `$crate::panic::panic_2015` which comes from the expansion of the macro `panic` (in Nightly builds, run with -Z macro-backtrace for more info) note: erroneous constant encountered - --> $DIR/collect-in-dead-fn.rs:19:17 + --> $DIR/collect-in-dead-fn.rs:18:17 | LL | let _ = Fail::::C; | ^^^^^^^^^^^^ note: the above error was encountered while instantiating `fn not_called::` - --> $DIR/collect-in-dead-fn.rs:26:9 + --> $DIR/collect-in-dead-fn.rs:25:9 | LL | not_called::(); | ^^^^^^^^^^^^^^^ diff --git a/tests/ui/consts/required-consts/collect-in-dead-fn.rs b/tests/ui/consts/required-consts/collect-in-dead-fn.rs index 754fcf7b9874d..f1d8c479a45fb 100644 --- a/tests/ui/consts/required-consts/collect-in-dead-fn.rs +++ b/tests/ui/consts/required-consts/collect-in-dead-fn.rs @@ -1,8 +1,7 @@ //@revisions: noopt opt //@ build-fail //@[opt] compile-flags: -O -//! Make sure we detect erroneous constants post-monomorphization even when they are unused. This is -//! crucial, people rely on it for soundness. (https://github.com/rust-lang/rust/issues/112090) +//! This fails without optimizations, so it should also fail with optimizations. struct Fail(T); impl Fail { diff --git a/tests/ui/consts/required-consts/collect-in-dead-forget.rs b/tests/ui/consts/required-consts/collect-in-dead-forget.rs index 720b7a499f781..17479ef8b37c7 100644 --- a/tests/ui/consts/required-consts/collect-in-dead-forget.rs +++ b/tests/ui/consts/required-consts/collect-in-dead-forget.rs @@ -1,8 +1,7 @@ //@revisions: noopt opt //@build-pass //@[opt] compile-flags: -O -//! Make sure we detect erroneous constants post-monomorphization even when they are unused. This is -//! crucial, people rely on it for soundness. (https://github.com/rust-lang/rust/issues/112090) +//! This passes without optimizations, so it can (and should) also pass with optimizations. struct Fail(T); impl Fail { diff --git a/tests/ui/consts/required-consts/collect-in-dead-move.noopt.stderr b/tests/ui/consts/required-consts/collect-in-dead-move.noopt.stderr index 2ab1f80e2d3b4..915949bcddf18 100644 --- a/tests/ui/consts/required-consts/collect-in-dead-move.noopt.stderr +++ b/tests/ui/consts/required-consts/collect-in-dead-move.noopt.stderr @@ -1,13 +1,13 @@ error[E0080]: evaluation of `Fail::::C` failed - --> $DIR/collect-in-dead-move.rs:9:19 + --> $DIR/collect-in-dead-move.rs:8:19 | LL | const C: () = panic!(); - | ^^^^^^^^ the evaluated program panicked at 'explicit panic', $DIR/collect-in-dead-move.rs:9:19 + | ^^^^^^^^ the evaluated program panicked at 'explicit panic', $DIR/collect-in-dead-move.rs:8:19 | = note: this error originates in the macro `$crate::panic::panic_2015` which comes from the expansion of the macro `panic` (in Nightly builds, run with -Z macro-backtrace for more info) note: erroneous constant encountered - --> $DIR/collect-in-dead-move.rs:16:17 + --> $DIR/collect-in-dead-move.rs:15:17 | LL | let _ = Fail::::C; | ^^^^^^^^^^^^ diff --git a/tests/ui/consts/required-consts/collect-in-dead-move.opt.stderr b/tests/ui/consts/required-consts/collect-in-dead-move.opt.stderr index 2ab1f80e2d3b4..915949bcddf18 100644 --- a/tests/ui/consts/required-consts/collect-in-dead-move.opt.stderr +++ b/tests/ui/consts/required-consts/collect-in-dead-move.opt.stderr @@ -1,13 +1,13 @@ error[E0080]: evaluation of `Fail::::C` failed - --> $DIR/collect-in-dead-move.rs:9:19 + --> $DIR/collect-in-dead-move.rs:8:19 | LL | const C: () = panic!(); - | ^^^^^^^^ the evaluated program panicked at 'explicit panic', $DIR/collect-in-dead-move.rs:9:19 + | ^^^^^^^^ the evaluated program panicked at 'explicit panic', $DIR/collect-in-dead-move.rs:8:19 | = note: this error originates in the macro `$crate::panic::panic_2015` which comes from the expansion of the macro `panic` (in Nightly builds, run with -Z macro-backtrace for more info) note: erroneous constant encountered - --> $DIR/collect-in-dead-move.rs:16:17 + --> $DIR/collect-in-dead-move.rs:15:17 | LL | let _ = Fail::::C; | ^^^^^^^^^^^^ diff --git a/tests/ui/consts/required-consts/collect-in-dead-move.rs b/tests/ui/consts/required-consts/collect-in-dead-move.rs index 2fd6aea58de65..d78c13b988cd4 100644 --- a/tests/ui/consts/required-consts/collect-in-dead-move.rs +++ b/tests/ui/consts/required-consts/collect-in-dead-move.rs @@ -1,8 +1,7 @@ //@revisions: noopt opt //@ build-fail //@[opt] compile-flags: -O -//! Make sure we detect erroneous constants post-monomorphization even when they are unused. This is -//! crucial, people rely on it for soundness. (https://github.com/rust-lang/rust/issues/112090) +//! This fails without optimizations, so it should also fail with optimizations. struct Fail(T); impl Fail { diff --git a/tests/ui/consts/required-consts/collect-in-dead-vtable.noopt.stderr b/tests/ui/consts/required-consts/collect-in-dead-vtable.noopt.stderr index 56b6989b441ed..2a835a4237eb8 100644 --- a/tests/ui/consts/required-consts/collect-in-dead-vtable.noopt.stderr +++ b/tests/ui/consts/required-consts/collect-in-dead-vtable.noopt.stderr @@ -1,19 +1,19 @@ error[E0080]: evaluation of `Fail::::C` failed - --> $DIR/collect-in-dead-vtable.rs:12:19 + --> $DIR/collect-in-dead-vtable.rs:11:19 | LL | const C: () = panic!(); - | ^^^^^^^^ the evaluated program panicked at 'explicit panic', $DIR/collect-in-dead-vtable.rs:12:19 + | ^^^^^^^^ the evaluated program panicked at 'explicit panic', $DIR/collect-in-dead-vtable.rs:11:19 | = note: this error originates in the macro `$crate::panic::panic_2015` which comes from the expansion of the macro `panic` (in Nightly builds, run with -Z macro-backtrace for more info) note: erroneous constant encountered - --> $DIR/collect-in-dead-vtable.rs:26:21 + --> $DIR/collect-in-dead-vtable.rs:25:21 | LL | let _ = Fail::::C; | ^^^^^^^^^^^^ note: the above error was encountered while instantiating `fn as MyTrait>::not_called` - --> $DIR/collect-in-dead-vtable.rs:35:40 + --> $DIR/collect-in-dead-vtable.rs:34:40 | LL | let gen_vtable: &dyn MyTrait = &v; // vtable "appears" here | ^^ diff --git a/tests/ui/consts/required-consts/collect-in-dead-vtable.rs b/tests/ui/consts/required-consts/collect-in-dead-vtable.rs index f21a1cc1fc2fd..3098661fbd91c 100644 --- a/tests/ui/consts/required-consts/collect-in-dead-vtable.rs +++ b/tests/ui/consts/required-consts/collect-in-dead-vtable.rs @@ -4,8 +4,7 @@ //FIXME: `opt` revision currently does not stop with an error due to //. //@[opt] build-pass -//! Make sure we detect erroneous constants post-monomorphization even when they are unused. This is -//! crucial, people rely on it for soundness. (https://github.com/rust-lang/rust/issues/112090) +//! This fails without optimizations, so it should also fail with optimizations. struct Fail(T); impl Fail { From 1230ba3d928ff46d3af57d0bc55f8687dcdfe5a4 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 17 Mar 2024 09:16:29 +0100 Subject: [PATCH 4/5] fix mentioned constants with fn ptrs that use failing constants --- compiler/rustc_monomorphize/src/collector.rs | 56 ++++++++++--------- ...ollect-in-dead-fnptr-in-const.noopt.stderr | 20 +++++++ .../collect-in-dead-fnptr-in-const.opt.stderr | 20 +++++++ .../collect-in-dead-fnptr-in-const.rs | 33 +++++++++++ .../collect-in-dead-fnptr.noopt.stderr | 23 ++++++++ .../collect-in-dead-fnptr.opt.stderr | 23 ++++++++ .../required-consts/collect-in-dead-fnptr.rs | 33 +++++++++++ 7 files changed, 181 insertions(+), 27 deletions(-) create mode 100644 tests/ui/consts/required-consts/collect-in-dead-fnptr-in-const.noopt.stderr create mode 100644 tests/ui/consts/required-consts/collect-in-dead-fnptr-in-const.opt.stderr create mode 100644 tests/ui/consts/required-consts/collect-in-dead-fnptr-in-const.rs create mode 100644 tests/ui/consts/required-consts/collect-in-dead-fnptr.noopt.stderr create mode 100644 tests/ui/consts/required-consts/collect-in-dead-fnptr.opt.stderr create mode 100644 tests/ui/consts/required-consts/collect-in-dead-fnptr.rs diff --git a/compiler/rustc_monomorphize/src/collector.rs b/compiler/rustc_monomorphize/src/collector.rs index a41deb3b1c5aa..75460c9bbdc30 100644 --- a/compiler/rustc_monomorphize/src/collector.rs +++ b/compiler/rustc_monomorphize/src/collector.rs @@ -447,38 +447,38 @@ fn collect_items_rec<'tcx>( // hide const-eval errors that those calls would otherwise have triggered. match starting_item.node { MonoItem::Static(def_id) => { - assert!( - mode == CollectionMode::UsedItems, - "should never encounter static when collecting mentioned items" - ); - let instance = Instance::mono(tcx, def_id); + recursion_depth_reset = None; - // Sanity check whether this ended up being collected accidentally - debug_assert!(should_codegen_locally(tcx, &instance)); + // Statics always get evaluted (which is possible because they can't be generic), so for + // `MentionedItems` collection there's nothing to do here. + if mode == CollectionMode::UsedItems { + let instance = Instance::mono(tcx, def_id); - let DefKind::Static { nested, .. } = tcx.def_kind(def_id) else { bug!() }; - // Nested statics have no type. - if !nested { - let ty = instance.ty(tcx, ty::ParamEnv::reveal_all()); - visit_drop_use(tcx, ty, true, starting_item.span, &mut used_items); - } + // Sanity check whether this ended up being collected accidentally + debug_assert!(should_codegen_locally(tcx, &instance)); - recursion_depth_reset = None; + let DefKind::Static { nested, .. } = tcx.def_kind(def_id) else { bug!() }; + // Nested statics have no type. + if !nested { + let ty = instance.ty(tcx, ty::ParamEnv::reveal_all()); + visit_drop_use(tcx, ty, true, starting_item.span, &mut used_items); + } - if let Ok(alloc) = tcx.eval_static_initializer(def_id) { - for &prov in alloc.inner().provenance().ptrs().values() { - collect_alloc(tcx, prov.alloc_id(), &mut used_items); + if let Ok(alloc) = tcx.eval_static_initializer(def_id) { + for &prov in alloc.inner().provenance().ptrs().values() { + collect_alloc(tcx, prov.alloc_id(), &mut used_items); + } } - } - if tcx.needs_thread_local_shim(def_id) { - used_items.push(respan( - starting_item.span, - MonoItem::Fn(Instance { - def: InstanceDef::ThreadLocalShim(def_id), - args: GenericArgs::empty(), - }), - )); + if tcx.needs_thread_local_shim(def_id) { + used_items.push(respan( + starting_item.span, + MonoItem::Fn(Instance { + def: InstanceDef::ThreadLocalShim(def_id), + args: GenericArgs::empty(), + }), + )); + } } // mentioned_items stays empty since there's no codegen for statics. statics don't get @@ -1650,7 +1650,9 @@ fn collect_items_of_instance<'tcx>( } else { // We only need to evaluate all constants, but can ignore the rest of the MIR. for const_op in &body.required_consts { - collector.eval_constant(const_op); + if let Some(val) = collector.eval_constant(const_op) { + collect_const_value(tcx, val, mentioned_items); + } } } diff --git a/tests/ui/consts/required-consts/collect-in-dead-fnptr-in-const.noopt.stderr b/tests/ui/consts/required-consts/collect-in-dead-fnptr-in-const.noopt.stderr new file mode 100644 index 0000000000000..7303b8d608a78 --- /dev/null +++ b/tests/ui/consts/required-consts/collect-in-dead-fnptr-in-const.noopt.stderr @@ -0,0 +1,20 @@ +error[E0080]: evaluation of `Late::::FAIL` failed + --> $DIR/collect-in-dead-fnptr-in-const.rs:8:22 + | +LL | const FAIL: () = panic!(); + | ^^^^^^^^ the evaluated program panicked at 'explicit panic', $DIR/collect-in-dead-fnptr-in-const.rs:8:22 + | + = note: this error originates in the macro `$crate::panic::panic_2015` which comes from the expansion of the macro `panic` (in Nightly builds, run with -Z macro-backtrace for more info) + +note: erroneous constant encountered + --> $DIR/collect-in-dead-fnptr-in-const.rs:9:28 + | +LL | const FNPTR: fn() = || Self::FAIL; + | ^^^^^^^^^^ + +note: the above error was encountered while instantiating `fn Late::::FNPTR::{closure#0}` + --> $SRC_DIR/core/src/ops/function.rs:LL:COL + +error: aborting due to 1 previous error + +For more information about this error, try `rustc --explain E0080`. diff --git a/tests/ui/consts/required-consts/collect-in-dead-fnptr-in-const.opt.stderr b/tests/ui/consts/required-consts/collect-in-dead-fnptr-in-const.opt.stderr new file mode 100644 index 0000000000000..7303b8d608a78 --- /dev/null +++ b/tests/ui/consts/required-consts/collect-in-dead-fnptr-in-const.opt.stderr @@ -0,0 +1,20 @@ +error[E0080]: evaluation of `Late::::FAIL` failed + --> $DIR/collect-in-dead-fnptr-in-const.rs:8:22 + | +LL | const FAIL: () = panic!(); + | ^^^^^^^^ the evaluated program panicked at 'explicit panic', $DIR/collect-in-dead-fnptr-in-const.rs:8:22 + | + = note: this error originates in the macro `$crate::panic::panic_2015` which comes from the expansion of the macro `panic` (in Nightly builds, run with -Z macro-backtrace for more info) + +note: erroneous constant encountered + --> $DIR/collect-in-dead-fnptr-in-const.rs:9:28 + | +LL | const FNPTR: fn() = || Self::FAIL; + | ^^^^^^^^^^ + +note: the above error was encountered while instantiating `fn Late::::FNPTR::{closure#0}` + --> $SRC_DIR/core/src/ops/function.rs:LL:COL + +error: aborting due to 1 previous error + +For more information about this error, try `rustc --explain E0080`. diff --git a/tests/ui/consts/required-consts/collect-in-dead-fnptr-in-const.rs b/tests/ui/consts/required-consts/collect-in-dead-fnptr-in-const.rs new file mode 100644 index 0000000000000..ed073e486c807 --- /dev/null +++ b/tests/ui/consts/required-consts/collect-in-dead-fnptr-in-const.rs @@ -0,0 +1,33 @@ +//@revisions: noopt opt +//@ build-fail +//@[opt] compile-flags: -O +//! This fails without optimizations, so it should also fail with optimizations. + +struct Late(T); +impl Late { + const FAIL: () = panic!(); //~ERROR evaluation of `Late::::FAIL` failed + const FNPTR: fn() = || Self::FAIL; +} + +// This function is not actually called, but it is mentioned in dead code in a function that is +// called. The function then mentions a const that evaluates to a fnptr that points to a function +// that used a const that fails to evaluate. +// This tests that when processing mentioned items, we also check the fnptrs in the final values +// of consts that we encounter. +#[inline(never)] +fn not_called() { + if false { + let _ = Late::::FNPTR; + } +} + +#[inline(never)] +fn called() { + if false { + not_called::(); + } +} + +pub fn main() { + called::(); +} diff --git a/tests/ui/consts/required-consts/collect-in-dead-fnptr.noopt.stderr b/tests/ui/consts/required-consts/collect-in-dead-fnptr.noopt.stderr new file mode 100644 index 0000000000000..2ba0b662f8a95 --- /dev/null +++ b/tests/ui/consts/required-consts/collect-in-dead-fnptr.noopt.stderr @@ -0,0 +1,23 @@ +error[E0080]: evaluation of `Fail::::C` failed + --> $DIR/collect-in-dead-fnptr.rs:8:19 + | +LL | const C: () = panic!(); + | ^^^^^^^^ the evaluated program panicked at 'explicit panic', $DIR/collect-in-dead-fnptr.rs:8:19 + | + = note: this error originates in the macro `$crate::panic::panic_2015` which comes from the expansion of the macro `panic` (in Nightly builds, run with -Z macro-backtrace for more info) + +note: erroneous constant encountered + --> $DIR/collect-in-dead-fnptr.rs:18:17 + | +LL | let _ = Fail::::C; + | ^^^^^^^^^^^^ + +note: the above error was encountered while instantiating `fn not_called::` + --> $DIR/collect-in-dead-fnptr.rs:27:28 + | +LL | let _fnptr: fn() = not_called::; + | ^^^^^^^^^^^^^^^ + +error: aborting due to 1 previous error + +For more information about this error, try `rustc --explain E0080`. diff --git a/tests/ui/consts/required-consts/collect-in-dead-fnptr.opt.stderr b/tests/ui/consts/required-consts/collect-in-dead-fnptr.opt.stderr new file mode 100644 index 0000000000000..2ba0b662f8a95 --- /dev/null +++ b/tests/ui/consts/required-consts/collect-in-dead-fnptr.opt.stderr @@ -0,0 +1,23 @@ +error[E0080]: evaluation of `Fail::::C` failed + --> $DIR/collect-in-dead-fnptr.rs:8:19 + | +LL | const C: () = panic!(); + | ^^^^^^^^ the evaluated program panicked at 'explicit panic', $DIR/collect-in-dead-fnptr.rs:8:19 + | + = note: this error originates in the macro `$crate::panic::panic_2015` which comes from the expansion of the macro `panic` (in Nightly builds, run with -Z macro-backtrace for more info) + +note: erroneous constant encountered + --> $DIR/collect-in-dead-fnptr.rs:18:17 + | +LL | let _ = Fail::::C; + | ^^^^^^^^^^^^ + +note: the above error was encountered while instantiating `fn not_called::` + --> $DIR/collect-in-dead-fnptr.rs:27:28 + | +LL | let _fnptr: fn() = not_called::; + | ^^^^^^^^^^^^^^^ + +error: aborting due to 1 previous error + +For more information about this error, try `rustc --explain E0080`. diff --git a/tests/ui/consts/required-consts/collect-in-dead-fnptr.rs b/tests/ui/consts/required-consts/collect-in-dead-fnptr.rs new file mode 100644 index 0000000000000..fd112ea31f645 --- /dev/null +++ b/tests/ui/consts/required-consts/collect-in-dead-fnptr.rs @@ -0,0 +1,33 @@ +//@revisions: noopt opt +//@ build-fail +//@[opt] compile-flags: -O +//! This fails without optimizations, so it should also fail with optimizations. + +struct Fail(T); +impl Fail { + const C: () = panic!(); //~ERROR evaluation of `Fail::::C` failed +} + +// This function is not actually called, but it is mentioned in dead code in a function that is +// called. Make sure we still find this error. +// This relies on mono-item collection checking `required_consts` in functions that syntactically +// are called in collected functions (even inside dead code). +#[inline(never)] +fn not_called() { + if false { + let _ = Fail::::C; + } +} + +#[inline(never)] +fn called() { + if false { + // We don't call the function, but turn it to a function pointer. + // Make sure it still gest added to `mentioned_items`. + let _fnptr: fn() = not_called::; + } +} + +pub fn main() { + called::(); +} From dd9cda6090c80915f98b9d1a383e6abcf1d86c65 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 17 Mar 2024 09:37:52 +0100 Subject: [PATCH 5/5] avoid processing mentioned items that are also still used --- compiler/rustc_middle/src/mir/mod.rs | 2 +- .../src/mentioned_items.rs | 3 ++- compiler/rustc_monomorphize/src/collector.rs | 27 ++++++++++++++++--- 3 files changed, 27 insertions(+), 5 deletions(-) diff --git a/compiler/rustc_middle/src/mir/mod.rs b/compiler/rustc_middle/src/mir/mod.rs index 6c9d5c670825f..c1fba223cd2f3 100644 --- a/compiler/rustc_middle/src/mir/mod.rs +++ b/compiler/rustc_middle/src/mir/mod.rs @@ -314,7 +314,7 @@ impl<'tcx> CoroutineInfo<'tcx> { } /// Some item that needs to monomorphize successfully for a MIR body to be considered well-formed. -#[derive(Copy, Clone, PartialEq, Debug, HashStable, TyEncodable, TyDecodable)] +#[derive(Copy, Clone, PartialEq, Eq, Debug, Hash, HashStable, TyEncodable, TyDecodable)] #[derive(TypeFoldable, TypeVisitable)] pub enum MentionedItem<'tcx> { Fn(DefId, GenericArgsRef<'tcx>), diff --git a/compiler/rustc_mir_transform/src/mentioned_items.rs b/compiler/rustc_mir_transform/src/mentioned_items.rs index e05c925acd739..437037aa0661d 100644 --- a/compiler/rustc_mir_transform/src/mentioned_items.rs +++ b/compiler/rustc_mir_transform/src/mentioned_items.rs @@ -33,7 +33,8 @@ impl<'tcx> MirPass<'tcx> for MentionedItems { impl<'tcx> Visitor<'tcx> for MentionedItemsVisitor<'_, 'tcx> { fn visit_constant(&mut self, constant: &ConstOperand<'tcx>, _: Location) { let const_ = constant.const_; - // This is how function items get referenced: via constants of `FnDef` type. + // This is how function items get referenced: via constants of `FnDef` type. This handles + // both functions that are called anf those that are just turned to function pointers. if let ty::FnDef(def_id, args) = const_.ty().kind() { debug!("adding to required_items: {def_id:?}"); self.mentioned_items diff --git a/compiler/rustc_monomorphize/src/collector.rs b/compiler/rustc_monomorphize/src/collector.rs index 75460c9bbdc30..78d5e1f482e4f 100644 --- a/compiler/rustc_monomorphize/src/collector.rs +++ b/compiler/rustc_monomorphize/src/collector.rs @@ -718,6 +718,8 @@ struct MirUsedCollector<'a, 'tcx> { tcx: TyCtxt<'tcx>, body: &'a mir::Body<'tcx>, used_items: &'a mut MonoItems<'tcx>, + /// See the comment in `collect_items_of_instance` for the purpose of this set. + used_mentioned_items: &'a mut FxHashSet>, instance: Instance<'tcx>, /// Spans for move size lints already emitted. Helps avoid duplicate lints. move_size_spans: Vec, @@ -988,6 +990,10 @@ impl<'a, 'tcx> MirVisitor<'tcx> for MirUsedCollector<'a, 'tcx> { match terminator.kind { mir::TerminatorKind::Call { ref func, ref args, ref fn_span, .. } => { let callee_ty = func.ty(self.body, tcx); + // *Before* monomorphizing, record that we already handled this mention. + if let ty::FnDef(def_id, args) = callee_ty.kind() { + self.used_mentioned_items.insert(MentionedItem::Fn(*def_id, args)); + } let callee_ty = self.monomorphize(callee_ty); self.check_fn_args_move_size(callee_ty, args, *fn_span, location); visit_fn_use(self.tcx, callee_ty, true, source, &mut self.used_items) @@ -1633,10 +1639,22 @@ fn collect_items_of_instance<'tcx>( mode: CollectionMode, ) { let body = tcx.instance_mir(instance.def); + // Naively, in "used" collection mode, all functions get added to *both* `used_items` and + // `mentioned_items`. Mentioned items processing will then notice that they have already been + // visited, but at that point each mentioned item has been monomorphized, added to the + // `mentioned_items` worklist, and checked in the global set of visited items. To removes that + // overhead, we have a special optimization that avoids adding items to `mentioned_items` when + // they are already added in `used_items`. We could just scan `used_items`, but that's a linear + // scan and not very efficient. Furthermore we can only do that *after* monomorphizing the + // mentioned item. So instead we collect all pre-monomorphized `MentionedItem` that were already + // added to `used_items` in a hash set, which can efficiently query in the + // `body.mentioned_items` loop below. + let mut used_mentioned_items = FxHashSet::>::default(); let mut collector = MirUsedCollector { tcx, body, used_items, + used_mentioned_items: &mut used_mentioned_items, instance, move_size_spans: vec![], visiting_call_terminator: false, @@ -1656,10 +1674,13 @@ fn collect_items_of_instance<'tcx>( } } - // Always gather mentioned items. + // 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 { - let item_mono = collector.monomorphize(item.node); - visit_mentioned_item(tcx, &item_mono, item.span, 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); + } } }