diff --git a/compiler/rustc_codegen_ssa/src/mir/retag.rs b/compiler/rustc_codegen_ssa/src/mir/retag.rs index a71a57f02c82a..6a3316fa93992 100644 --- a/compiler/rustc_codegen_ssa/src/mir/retag.rs +++ b/compiler/rustc_codegen_ssa/src/mir/retag.rs @@ -14,9 +14,10 @@ use rustc_middle::mir::{Rvalue, WithRetag}; use rustc_middle::ty::layout::{HasTypingEnv, TyAndLayout}; use rustc_middle::ty::{self, Ty}; -use crate::mir::FunctionCx; +use crate::common::IntPredicate; use crate::mir::operand::{OperandRef, OperandRefBuilder, OperandValue}; use crate::mir::place::PlaceRef; +use crate::mir::{FunctionCx, bug}; use crate::traits::{ BaseTypeCodegenMethods, BuilderMethods, ConstCodegenMethods, StaticCodegenMethods, }; @@ -35,17 +36,31 @@ enum RetagPlan { /// Indicates that one or more fields or variants of this type /// contain pointers that need to be retagged. Recurse { - field_plans: FxIndexMap>, + field_plans: Option>, variant_plans: FxIndexMap>, }, } +/// The layout of the fields that need to be retagged. +/// +/// This mirrors [`FieldsShape`] but excludes unions, +/// which are never retagged, and primitives, which fall +/// into [`RetagPlan::EmitRetag`]. +#[derive(Debug)] +enum RetagFields { + Arbitrary(FxIndexMap>), + Array(Box>), +} + impl RetagPlan { /// A helper function to move a [`RetagPlan`] into a particular field. fn for_field(self, ix: FieldIdx) -> Self { let mut field_plans = FxIndexMap::default(); field_plans.insert(ix, self); - RetagPlan::Recurse { field_plans, variant_plans: FxIndexMap::default() } + RetagPlan::Recurse { + field_plans: Some(RetagFields::Arbitrary(field_plans)), + variant_plans: FxIndexMap::default(), + } } } @@ -84,21 +99,27 @@ impl<'a, 'tcx, V> RetagPlan { layout: TyAndLayout<'tcx>, is_fn_entry: bool, ) -> Option> { - let mut field_plans = FxIndexMap::default(); - let mut variant_plans = FxIndexMap::default(); - - match &layout.fields { - FieldsShape::Union(_) | FieldsShape::Primitive => {} - _ => { + let field_plans = match &layout.fields { + FieldsShape::Union(_) | FieldsShape::Primitive => None, + FieldsShape::Arbitrary { .. } => { + let mut field_plans = FxIndexMap::default(); for ix in layout.fields.index_by_increasing_offset() { let field_layout = layout.field(bx, ix); if let Some(plan) = Self::build(bx, field_layout, is_fn_entry) { field_plans.insert(FieldIdx::from_usize(ix), plan); } } + (!field_plans.is_empty()).then_some(RetagFields::Arbitrary(field_plans)) } - } + FieldsShape::Array { count, .. } if *count > 0 => { + let field_layout = layout.field(bx, 0); + Self::build(bx, field_layout, is_fn_entry) + .map(|plan| RetagFields::Array(Box::new(plan))) + } + FieldsShape::Array { .. } => None, + }; + let mut variant_plans = FxIndexMap::default(); match &layout.variants { Variants::Single { .. } | Variants::Empty => {} Variants::Multiple { variants, .. } => { @@ -111,7 +132,7 @@ impl<'a, 'tcx, V> RetagPlan { } } - (!field_plans.is_empty() || !variant_plans.is_empty()) + (field_plans.is_some() || !variant_plans.is_empty()) .then(|| RetagPlan::Recurse { field_plans, variant_plans }) } @@ -157,8 +178,10 @@ impl<'a, 'tcx, V> RetagPlan { field_plans.insert(FieldIdx::ONE, plan); } - (!field_plans.is_empty()) - .then(|| RetagPlan::Recurse { field_plans, variant_plans: FxIndexMap::default() }) + (!field_plans.is_empty()).then(|| RetagPlan::Recurse { + field_plans: Some(RetagFields::Arbitrary(field_plans)), + variant_plans: FxIndexMap::default(), + }) } /// Determines if a pointer needs to be retagged, when it points to @@ -418,21 +441,38 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { } RetagPlan::Recurse { field_plans, variant_plans } => { let layout = curr_operand.layout; - for (ix, plan) in field_plans { - let inner_offset = layout.fields.offset(ix.as_usize()); - let field_offset = offset + inner_offset; - - let field_layout = curr_operand.layout.field(bx, ix.index()); - // Part of https://github.com/rust-lang/compiler-team/issues/838 - if curr_operand.layout.is_ssa_standalone() && !field_layout.is_ssa_standalone() - { - // FIXME: support vector types, requires insert_element as part of cg-ssa - // FIXME: Nothing should be looking at the *array* inside a `repr(simd)` type, - // as that array doesn't really exist. Perhaps this should be a `bug!`, - // with simd types handled before getting here? - } else { - let field_operand = curr_operand.extract_field(self, bx, ix.as_usize()); - self.retag_operand(bx, &plan, field_operand, builder, field_offset); + if let Some(plans) = field_plans { + let mut retag_field = |ix: FieldIdx, plan: &RetagPlan| { + let inner_offset = layout.fields.offset(ix.as_usize()); + let field_offset = offset + inner_offset; + let field_layout = curr_operand.layout.field(bx, ix.index()); + if curr_operand.layout.is_ssa_standalone() + && !field_layout.is_ssa_standalone() + { + // FIXME: Nothing should be looking at the *array* inside a `repr(simd)` type, + // as that array doesn't really exist. Perhaps this should be a `bug!`, + // with simd types handled before getting here? + } else { + let field_operand = curr_operand.extract_field(self, bx, ix.as_usize()); + self.retag_operand(bx, plan, field_operand, builder, field_offset); + } + }; + + match plans { + RetagFields::Arbitrary(field_plans) => { + for (ix, plan) in field_plans { + retag_field(*ix, plan); + } + } + RetagFields::Array(plan) => { + let FieldsShape::Array { count, .. } = layout.fields else { + bug!("Expected `FieldsShape::Array` but found {:?}", layout.fields); + }; + for idx in 0..count { + let idx = FieldIdx::from_usize(idx as usize); + retag_field(idx, plan) + } + } } } @@ -449,8 +489,8 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { self.retag_operand(bx, plan, variant_op, builder, offset); } } else { - // We create a temporary place to store the operand, because its value will differ - // depending on the variant that we have. + // We create a temporary place to store the operand, because its + // value will differ depending on the variant that we have. let scratch = PlaceRef::alloca(bx, curr_operand.layout); scratch.storage_live(bx); curr_operand.store_with_annotation(bx, scratch); @@ -487,9 +527,33 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { bx.retag_mem(place.val.llval, info); } RetagPlan::Recurse { field_plans, variant_plans } => { - for (ix, plan) in field_plans { - let field_place = place.project_field(bx, ix.as_usize()); - self.retag_place(bx, &plan, field_place); + if let Some(field_plans) = field_plans { + let mut retag_field = |ix: FieldIdx, plan: &RetagPlan| { + let field_place = place.project_field(bx, ix.as_usize()); + self.retag_place(bx, plan, field_place); + }; + match field_plans { + RetagFields::Arbitrary(fields) => { + for (ix, plan) in fields { + retag_field(*ix, plan); + } + } + RetagFields::Array(plan) => { + let FieldsShape::Array { count, .. } = place.layout.fields else { + bug!( + "Expected `FieldsShape::Array` but found {:?}", + place.layout.fields + ); + }; + // When an array of N references needs to be retagged (e.g. `[&T; N]`), + // it is more efficient to emit a loop for large values of N than + // to emit N retags in succession. + within_loop(bx, count, |bx: &mut Bx, ix: Bx::Value| { + let field_place = place.project_index(bx, ix); + self.retag_place(bx, plan, field_place); + }); + } + } } if !variant_plans.is_empty() { let operand = bx.load_operand(place); @@ -534,3 +598,45 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { bx.switch_to_block(join_block); } } + +/// Creates a simple loop, iterating the specified number of times. +/// The dynamic value of the loop counter is passed as an argument +/// to the closure. +fn within_loop<'a, 'tcx, F, Bx: BuilderMethods<'a, 'tcx>>(bx: &mut Bx, max_iter: u64, mut op: F) +where + F: FnMut(&mut Bx, Bx::Value), +{ + // Avoid emitting a no-op loop + if max_iter == 0 { + return; + } + + // Initialize the loop counter to zero. + let counter_layout = bx.layout_of(bx.tcx().types.usize); + let counter_alloca = PlaceRef::alloca(bx, counter_layout); + counter_alloca.storage_live(bx); + bx.store_to_place(bx.const_usize(0), counter_alloca.val); + + // Jump to a new block for the body of the loop. + let loop_body = bx.append_sibling_block("retag_loop"); + bx.br(loop_body); + + bx.switch_to_block(loop_body); + + // Load the value of the counter, and pass it to the closure. + let curr_count = bx.load_from_place(bx.type_isize(), counter_alloca.val); + op(bx, curr_count); + + let next_count = bx.unchecked_uadd(curr_count, bx.const_usize(1)); + bx.store_to_place(next_count, counter_alloca.val); + + // If the next count is equal to the maximum number of + // iterations, then exit the loop. + let max_count = bx.const_usize(max_iter); + let loop_exit = bx.append_sibling_block("retag_loop_exit"); + let should_exit_loop = bx.icmp(IntPredicate::IntEQ, next_count, max_count); + bx.cond_br(should_exit_loop, loop_exit, loop_body); + + bx.switch_to_block(loop_exit); + counter_alloca.storage_dead(bx); +} diff --git a/compiler/rustc_mir_dataflow/src/framework/lattice.rs b/compiler/rustc_mir_dataflow/src/framework/lattice.rs index 919e346bda747..4566d8e9732da 100644 --- a/compiler/rustc_mir_dataflow/src/framework/lattice.rs +++ b/compiler/rustc_mir_dataflow/src/framework/lattice.rs @@ -104,6 +104,17 @@ pub enum FlatSet { Top, } +impl FlatSet { + #[inline] + pub fn and_then(self, f: impl FnOnce(T) -> FlatSet) -> FlatSet { + match self { + FlatSet::Elem(elem) => f(elem), + FlatSet::Bottom => FlatSet::Bottom, + FlatSet::Top => FlatSet::Top, + } + } +} + impl JoinSemiLattice for FlatSet { fn join(&mut self, other: &Self) -> bool { let result = match (&*self, other) { diff --git a/compiler/rustc_mir_dataflow/src/value_analysis.rs b/compiler/rustc_mir_dataflow/src/value_analysis.rs index e213a295c0bd0..2acbbab220547 100644 --- a/compiler/rustc_mir_dataflow/src/value_analysis.rs +++ b/compiler/rustc_mir_dataflow/src/value_analysis.rs @@ -214,17 +214,6 @@ impl State { } } - /// Helper method for assignments to a discriminant. - pub fn assign_discr(&mut self, target: PlaceRef<'_>, result: ValueOrPlace, map: &Map<'_>) - where - V: HasTop, - { - self.flood_discr(target, map); - if let Some(target) = map.find_discr(target) { - self.insert_idx(target, result, map); - } - } - /// Retrieve the value stored for a place, or `None` if it is not tracked. fn try_get(&self, place: PlaceRef<'_>, map: &Map<'_>) -> Option { let place = map.find(place)?; diff --git a/compiler/rustc_mir_transform/src/dataflow_const_prop.rs b/compiler/rustc_mir_transform/src/dataflow_const_prop.rs index 21ea02c8ff601..e52a9a847b119 100644 --- a/compiler/rustc_mir_transform/src/dataflow_const_prop.rs +++ b/compiler/rustc_mir_transform/src/dataflow_const_prop.rs @@ -119,7 +119,51 @@ impl<'tcx> Analysis<'tcx> for ConstAnalysis<'_, 'tcx> { _location: Location, ) { if state.is_reachable() { - self.handle_statement(statement, state); + match &statement.kind { + StatementKind::Assign((place, rvalue)) => { + self.handle_assign(*place, rvalue, state); + } + StatementKind::SetDiscriminant { place, variant_index } => { + let place = (**place).as_ref(); + state.flood_discr(place, &self.map); + if let Some(target) = self.map.find_discr(place) { + let enum_ty = place.ty(self.local_decls, self.tcx).ty; + if let Some(discr) = self.eval_discriminant(enum_ty, *variant_index) { + state.insert_value_idx(target, FlatSet::Elem(discr), &self.map); + } + } + } + StatementKind::Intrinsic(intrinsic) => { + match intrinsic { + NonDivergingIntrinsic::Assume(..) => { + // Could use this, but ignoring it is sound. + } + NonDivergingIntrinsic::CopyNonOverlapping(CopyNonOverlapping { + dst: _, + src: _, + count: _, + }) => { + // This statement represents `*dst = *src`, `count` times. + } + } + } + StatementKind::StorageLive(local) | StatementKind::StorageDead(local) => { + // StorageLive leaves the local in an uninitialized state. + // StorageDead makes it UB to access the local afterwards. + state.flood_with( + Place::from(*local).as_ref(), + &self.map, + FlatSet::::BOTTOM, + ); + } + StatementKind::ConstEvalCounter + | StatementKind::Nop + | StatementKind::FakeRead(..) + | StatementKind::PlaceMention(..) + | StatementKind::Coverage(..) + | StatementKind::BackwardIncompatibleDropHint { .. } + | StatementKind::AscribeUserType(..) => {} + } } } @@ -147,7 +191,34 @@ impl<'tcx> Analysis<'tcx> for ConstAnalysis<'_, 'tcx> { _location: Location, ) { if state.is_reachable() { - self.handle_terminator(terminator, state) + match &terminator.kind { + TerminatorKind::Call { .. } | TerminatorKind::InlineAsm { .. } => { + // Effect is applied by `apply_call_return_effect`. + } + TerminatorKind::Drop { place, .. } => { + state.flood_with(place.as_ref(), &self.map, FlatSet::::BOTTOM); + } + TerminatorKind::Yield { .. } => { + // They would have an effect, but are not allowed in this phase. + bug!("encountered disallowed terminator"); + } + TerminatorKind::TailCall { .. } => { + // FIXME(explicit_tail_calls): determine if we need to do something here + // (probably not) + } + TerminatorKind::SwitchInt { .. } + | TerminatorKind::Goto { .. } + | TerminatorKind::UnwindResume + | TerminatorKind::UnwindTerminate(_) + | TerminatorKind::Return + | TerminatorKind::Unreachable + | TerminatorKind::Assert { .. } + | TerminatorKind::CoroutineDrop + | TerminatorKind::FalseEdge { .. } + | TerminatorKind::FalseUnwind { .. } => { + // These terminators have no effect on the analysis. + } + } } } @@ -158,7 +229,9 @@ impl<'tcx> Analysis<'tcx> for ConstAnalysis<'_, 'tcx> { return_places: CallReturnPlaces<'_, 'tcx>, ) { if state.is_reachable() { - self.handle_call_return(return_places, state) + return_places.for_each(|place| { + state.flood(place.as_ref(), &self.map); + }) } } } @@ -175,55 +248,15 @@ impl<'a, 'tcx> ConstAnalysis<'a, 'tcx> { } } - fn handle_statement(&self, statement: &Statement<'tcx>, state: &mut State>) { - match &statement.kind { - StatementKind::Assign((place, rvalue)) => { - self.handle_assign(*place, rvalue, state); - } - StatementKind::SetDiscriminant { place, variant_index } => { - self.handle_set_discriminant(**place, *variant_index, state); - } - StatementKind::Intrinsic(intrinsic) => { - self.handle_intrinsic(intrinsic); - } - StatementKind::StorageLive(local) | StatementKind::StorageDead(local) => { - // StorageLive leaves the local in an uninitialized state. - // StorageDead makes it UB to access the local afterwards. - state.flood_with( - Place::from(*local).as_ref(), - &self.map, - FlatSet::::BOTTOM, - ); - } - StatementKind::ConstEvalCounter - | StatementKind::Nop - | StatementKind::FakeRead(..) - | StatementKind::PlaceMention(..) - | StatementKind::Coverage(..) - | StatementKind::BackwardIncompatibleDropHint { .. } - | StatementKind::AscribeUserType(..) => {} - } - } - - fn handle_intrinsic(&self, intrinsic: &NonDivergingIntrinsic<'tcx>) { - match intrinsic { - NonDivergingIntrinsic::Assume(..) => { - // Could use this, but ignoring it is sound. - } - NonDivergingIntrinsic::CopyNonOverlapping(CopyNonOverlapping { - dst: _, - src: _, - count: _, - }) => { - // This statement represents `*dst = *src`, `count` times. - } - } - } - fn handle_operand(&self, operand: &Operand<'tcx>) -> ValueOrPlace> { match operand { Operand::RuntimeChecks(_) => ValueOrPlace::TOP, - Operand::Constant(constant) => ValueOrPlace::Value(self.handle_constant(constant)), + Operand::Constant(constant) => ValueOrPlace::Value( + constant + .const_ + .try_eval_scalar(self.tcx, self.typing_env) + .map_or(FlatSet::Top, FlatSet::Elem), + ), Operand::Copy(place) | Operand::Move(place) => { // On move, we would ideally flood the place with bottom. But with the current // framework this is not possible (similar to `InterpCx::eval_operand`). @@ -232,84 +265,19 @@ impl<'a, 'tcx> ConstAnalysis<'a, 'tcx> { } } - /// The effect of a successful function call return should not be - /// applied here, see [`Analysis::apply_primary_terminator_effect`]. - fn handle_terminator<'mir>( - &self, - terminator: &'mir Terminator<'tcx>, - state: &mut State>, - ) { - match &terminator.kind { - TerminatorKind::Call { .. } | TerminatorKind::InlineAsm { .. } => { - // Effect is applied by `handle_call_return`. - } - TerminatorKind::Drop { place, .. } => { - state.flood_with(place.as_ref(), &self.map, FlatSet::::BOTTOM); - } - TerminatorKind::Yield { .. } => { - // They would have an effect, but are not allowed in this phase. - bug!("encountered disallowed terminator"); - } - TerminatorKind::TailCall { .. } => { - // FIXME(explicit_tail_calls): determine if we need to do something here (probably - // not) - } - TerminatorKind::SwitchInt { .. } - | TerminatorKind::Goto { .. } - | TerminatorKind::UnwindResume - | TerminatorKind::UnwindTerminate(_) - | TerminatorKind::Return - | TerminatorKind::Unreachable - | TerminatorKind::Assert { .. } - | TerminatorKind::CoroutineDrop - | TerminatorKind::FalseEdge { .. } - | TerminatorKind::FalseUnwind { .. } => { - // These terminators have no effect on the analysis. - } - } - } - - fn handle_call_return( - &self, - return_places: CallReturnPlaces<'_, 'tcx>, - state: &mut State>, - ) { - return_places.for_each(|place| { - state.flood(place.as_ref(), &self.map); - }) - } - - fn handle_set_discriminant( - &self, - place: Place<'tcx>, - variant_index: VariantIdx, - state: &mut State>, - ) { - state.flood_discr(place.as_ref(), &self.map); - if self.map.find_discr(place.as_ref()).is_some() { - let enum_ty = place.ty(self.local_decls, self.tcx).ty; - if let Some(discr) = self.eval_discriminant(enum_ty, variant_index) { - state.assign_discr( - place.as_ref(), - ValueOrPlace::Value(FlatSet::Elem(discr)), - &self.map, - ); - } - } - } - fn handle_assign( &self, target: Place<'tcx>, rvalue: &Rvalue<'tcx>, state: &mut State>, ) { - match rvalue { + let result = match rvalue { Rvalue::Use(operand, _) => { state.flood(target.as_ref(), &self.map); if let Some(target) = self.map.find(target.as_ref()) { self.assign_operand(state, target, operand); } + return; } Rvalue::CopyForDeref(_) => bug!("`CopyForDeref` in runtime MIR"), Rvalue::Aggregate(kind, operands) => { @@ -355,27 +323,35 @@ impl<'a, 'tcx> ConstAnalysis<'a, 'tcx> { state.insert_value_idx(discr_idx, FlatSet::Elem(discr_val), &self.map); } } + return; } - Rvalue::BinaryOp(op, (left, right)) if op.is_overflowing() => { - // Flood everything now, so we can use `insert_value_idx` directly later. - state.flood(target.as_ref(), &self.map); + Rvalue::BinaryOp(op, (left, right)) => { + if op.is_overflowing() { + // Flood everything now, so we can use `insert_value_idx` directly later. + state.flood(target.as_ref(), &self.map); - let Some(target) = self.map.find(target.as_ref()) else { return }; + let Some(target) = self.map.find(target.as_ref()) else { return }; - let value_target = self.map.apply(target, TrackElem::Field(0_u32.into())); - let overflow_target = self.map.apply(target, TrackElem::Field(1_u32.into())); + let value_target = self.map.apply(target, TrackElem::Field(0_u32.into())); + let overflow_target = self.map.apply(target, TrackElem::Field(1_u32.into())); - if value_target.is_some() || overflow_target.is_some() { - let (val, overflow) = self.binary_op(state, *op, left, right); + if value_target.is_some() || overflow_target.is_some() { + let (val, overflow) = self.binary_op(state, *op, left, right); - if let Some(value_target) = value_target { - // We have flooded `target` earlier. - state.insert_value_idx(value_target, val, &self.map); - } - if let Some(overflow_target) = overflow_target { - // We have flooded `target` earlier. - state.insert_value_idx(overflow_target, overflow, &self.map); + if let Some(value_target) = value_target { + // We have flooded `target` earlier. + state.insert_value_idx(value_target, val, &self.map); + } + if let Some(overflow_target) = overflow_target { + // We have flooded `target` earlier. + state.insert_value_idx(overflow_target, overflow, &self.map); + } } + return; + } else { + // Overflows must be ignored here. + let (val, _overflow) = self.binary_op(state, *op, left, right); + ValueOrPlace::Value(val) } } Rvalue::Cast( @@ -395,103 +371,73 @@ impl<'a, 'tcx> ConstAnalysis<'a, 'tcx> { { state.insert_value_idx(target_len, FlatSet::Elem(len.into()), &self.map); } + return; } - _ => { - let result = self.handle_rvalue(rvalue, state); - state.assign(target.as_ref(), result, &self.map); - } - } - } - - fn handle_rvalue( - &self, - rvalue: &Rvalue<'tcx>, - state: &mut State>, - ) -> ValueOrPlace> { - let val = match rvalue { Rvalue::Cast(CastKind::IntToInt | CastKind::IntToFloat, operand, ty) => { - let Ok(layout) = self.tcx.layout_of(self.typing_env.as_query_input(*ty)) else { - return ValueOrPlace::Value(FlatSet::Top); - }; - match self.eval_operand(operand, state) { - FlatSet::Elem(op) => self - .ecx - .int_to_int_or_float(&op, layout) - .discard_err() - .map_or(FlatSet::Top, |result| self.wrap_immediate(*result)), - FlatSet::Bottom => FlatSet::Bottom, - FlatSet::Top => FlatSet::Top, - } + ValueOrPlace::Value( + if let Ok(layout) = self.tcx.layout_of(self.typing_env.as_query_input(*ty)) { + self.eval_operand(operand, state).and_then(|op| { + self.ecx + .int_to_int_or_float(&op, layout) + .discard_err() + .map_or(FlatSet::Top, |result| self.wrap_immediate(*result)) + }) + } else { + FlatSet::Top + }, + ) } Rvalue::Cast(CastKind::FloatToInt | CastKind::FloatToFloat, operand, ty) => { - let Ok(layout) = self.tcx.layout_of(self.typing_env.as_query_input(*ty)) else { - return ValueOrPlace::Value(FlatSet::Top); - }; - match self.eval_operand(operand, state) { - FlatSet::Elem(op) => self - .ecx - .float_to_float_or_int(&op, layout) - .discard_err() - .map_or(FlatSet::Top, |result| self.wrap_immediate(*result)), - FlatSet::Bottom => FlatSet::Bottom, - FlatSet::Top => FlatSet::Top, - } + ValueOrPlace::Value( + if let Ok(layout) = self.tcx.layout_of(self.typing_env.as_query_input(*ty)) { + self.eval_operand(operand, state).and_then(|op| { + self.ecx + .float_to_float_or_int(&op, layout) + .discard_err() + .map_or(FlatSet::Top, |result| self.wrap_immediate(*result)) + }) + } else { + FlatSet::Top + }, + ) } Rvalue::Cast(CastKind::Transmute | CastKind::Subtype, operand, _) => { - match self.eval_operand(operand, state) { - FlatSet::Elem(op) => self.wrap_immediate(*op), - FlatSet::Bottom => FlatSet::Bottom, - FlatSet::Top => FlatSet::Top, - } - } - Rvalue::BinaryOp(op, (left, right)) if !op.is_overflowing() => { - // Overflows must be ignored here. - // The overflowing operators are handled in `handle_assign`. - let (val, _overflow) = self.binary_op(state, *op, left, right); - val + ValueOrPlace::Value( + self.eval_operand(operand, state).and_then(|op| self.wrap_immediate(*op)), + ) } Rvalue::UnaryOp(op, operand) => { if let UnOp::PtrMetadata = op && let Some(place) = operand.place() && let Some(len) = self.map.find_len(place.as_ref()) { - return ValueOrPlace::Place(len); - } - match self.eval_operand(operand, state) { - FlatSet::Elem(value) => self - .ecx - .unary_op(*op, &value) - .discard_err() - .map_or(FlatSet::Top, |val| self.wrap_immediate(*val)), - FlatSet::Bottom => FlatSet::Bottom, - FlatSet::Top => FlatSet::Top, + ValueOrPlace::Place(len) + } else { + ValueOrPlace::Value(self.eval_operand(operand, state).and_then(|value| { + self.ecx + .unary_op(*op, &value) + .discard_err() + .map_or(FlatSet::Top, |val| self.wrap_immediate(*val)) + })) } } - Rvalue::Discriminant(place) => state.get_discr(place.as_ref(), &self.map), - Rvalue::Use(operand, _) => return self.handle_operand(operand), - Rvalue::CopyForDeref(_) => bug!("`CopyForDeref` in runtime MIR"), + Rvalue::Discriminant(place) => { + ValueOrPlace::Value(state.get_discr(place.as_ref(), &self.map)) + } Rvalue::Ref(..) | Rvalue::Reborrow(..) | Rvalue::RawPtr(..) => { // We don't track such places. - return ValueOrPlace::TOP; + ValueOrPlace::TOP } Rvalue::Repeat(..) | Rvalue::ThreadLocalRef(..) | Rvalue::Cast(..) - | Rvalue::BinaryOp(..) - | Rvalue::Aggregate(..) | Rvalue::WrapUnsafeBinder(..) => { // No modification is possible through these r-values. - return ValueOrPlace::TOP; + ValueOrPlace::TOP } }; - ValueOrPlace::Value(val) - } - - fn handle_constant(&self, constant: &ConstOperand<'tcx>) -> FlatSet { - constant - .const_ - .try_eval_scalar(self.tcx, self.typing_env) - .map_or(FlatSet::Top, FlatSet::Elem) + // For the arms that didn't return early. + state.assign(target.as_ref(), result, &self.map); } fn get_switch_int_edges<'mir>( @@ -617,7 +563,7 @@ impl<'a, 'tcx> ConstAnalysis<'a, 'tcx> { fn binary_op( &self, - state: &mut State>, + state: &State>, op: BinOp, left: &Operand<'tcx>, right: &Operand<'tcx>, @@ -647,7 +593,7 @@ impl<'a, 'tcx> ConstAnalysis<'a, 'tcx> { // Exactly one side is known, attempt some algebraic simplifications. (FlatSet::Elem(const_arg), _) | (_, FlatSet::Elem(const_arg)) => { let layout = const_arg.layout; - if !matches!(layout.backend_repr, rustc_abi::BackendRepr::Scalar(..)) { + if !layout.backend_repr.is_scalar() { return (FlatSet::Top, FlatSet::Top); } @@ -677,24 +623,18 @@ impl<'a, 'tcx> ConstAnalysis<'a, 'tcx> { fn eval_operand( &self, op: &Operand<'tcx>, - state: &mut State>, + state: &State>, ) -> FlatSet> { let value = match self.handle_operand(op) { ValueOrPlace::Value(value) => value, ValueOrPlace::Place(place) => state.get_idx(place, &self.map), }; - match value { - FlatSet::Top => FlatSet::Top, - FlatSet::Elem(scalar) => { - let ty = op.ty(self.local_decls, self.tcx); - self.tcx - .layout_of(self.typing_env.as_query_input(ty)) - .map_or(FlatSet::Top, |layout| { - FlatSet::Elem(ImmTy::from_scalar(scalar, layout)) - }) - } - FlatSet::Bottom => FlatSet::Bottom, - } + value.and_then(|scalar| { + let ty = op.ty(self.local_decls, self.tcx); + self.tcx + .layout_of(self.typing_env.as_query_input(ty)) + .map_or(FlatSet::Top, |layout| FlatSet::Elem(ImmTy::from_scalar(scalar, layout))) + }) } fn eval_discriminant(&self, enum_ty: Ty<'tcx>, variant_index: VariantIdx) -> Option { @@ -753,7 +693,7 @@ struct Patch<'tcx> { } impl<'tcx> Patch<'tcx> { - pub(crate) fn new(tcx: TyCtxt<'tcx>) -> Self { + fn new(tcx: TyCtxt<'tcx>) -> Self { Self { tcx, before_effect: FxHashMap::default(), assignments: FxHashMap::default() } } @@ -770,7 +710,7 @@ struct Collector<'a, 'tcx> { } impl<'a, 'tcx> Collector<'a, 'tcx> { - pub(crate) fn new(tcx: TyCtxt<'tcx>, body: &'a Body<'tcx>, map: &'a Map<'tcx>) -> Self { + fn new(tcx: TyCtxt<'tcx>, body: &'a Body<'tcx>, map: &'a Map<'tcx>) -> Self { Self { patch: Patch::new(tcx), local_decls: &body.local_decls, diff --git a/src/librustdoc/html/format.rs b/src/librustdoc/html/format.rs index 3f1ed079f2686..7f0c4132456ad 100644 --- a/src/librustdoc/html/format.rs +++ b/src/librustdoc/html/format.rs @@ -348,11 +348,11 @@ pub(crate) struct HrefInfo { } /// This function is to get the external macro path because they are not in the cache used in -/// `href_with_root_path`. +/// `href_with_jump_to_def_path_depth`. fn generate_macro_def_id_path( def_id: DefId, cx: &Context<'_>, - root_path: Option<&str>, + jump_to_def_path_depth: Option, ) -> Result { let tcx = cx.tcx(); let crate_name = tcx.crate_name(def_id.krate); @@ -388,16 +388,16 @@ fn generate_macro_def_id_path( let url = match cache.extern_locations[&def_id.krate] { ExternalLocation::Remote { ref url, is_absolute } => { - let mut prefix = remote_url_prefix(url, is_absolute, cx.current.len()); + let depth = jump_to_def_path_depth.unwrap_or(cx.current.len()); + let mut prefix = remote_url_prefix(url, is_absolute, depth); prefix.extend(module_path.iter().copied()); prefix.push_fmt(format_args!("{}.{last}.html", item_type.as_str())); prefix.finish() } ExternalLocation::Local => { - // `root_path` always end with a `/`. format!( - "{root_path}{path}/{item_type}.{last}.html", - root_path = root_path.unwrap_or(""), + "{prefix}{path}/{item_type}.{last}.html", + prefix = "../".repeat(jump_to_def_path_depth.unwrap_or(0)), path = fmt::from_fn(|f| module_path.iter().joined("/", f)), item_type = item_type.as_str(), ) @@ -505,7 +505,7 @@ fn generate_item_def_id_path( mut def_id: DefId, original_def_id: DefId, cx: &Context<'_>, - root_path: Option<&str>, + jump_to_def_path_depth: Option, ) -> Result { let tcx = cx.tcx(); let crate_name = tcx.crate_name(def_id.krate); @@ -578,8 +578,9 @@ fn generate_item_def_id_path( }; let module_fqp = to_module_fqp(shortty, &fqp); - let (parts, is_absolute) = url_parts(cx.cache(), def_id, module_fqp, &cx.current)?; - let mut url = make_href(root_path, shortty, parts, &fqp, is_absolute); + let (parts, is_remote) = + url_parts(cx.cache(), def_id, module_fqp, &cx.current, jump_to_def_path_depth)?; + let mut url = make_href(jump_to_def_path_depth, shortty, parts, &fqp, is_remote); if def_id != original_def_id { let kind = ItemType::from_def_id(original_def_id, tcx); @@ -624,17 +625,33 @@ fn remote_url_prefix(url: &str, is_absolute: bool, depth: usize) -> UrlPartsBuil } } +/// Returns `(url_parts, is_remote)` where `is_remote` indicates the URL points to an +/// external location and should not use relative URLs. +/// +/// This function tries to generate minimal, relative URLs. When generating an intra-doc +/// link from one item page to another, it compares the paths of both items and generates +/// exactly the right number of "../"'s to reach the deepest common parent, then fills in +/// the rest of the path. This is done by comparing `module_fqp` with `relative_to`. +/// +/// When generating a link for jump-to-def across crates, that won't work, because sources are +/// stored under `src//.rs`. When linking jump-to-def for another crate, we just +/// write enough "../"'s to reach the doc root, then generate the entire path for `module_fqp`. +/// The right number of "../"'s is stored in `jump_to_def_path_depth`. fn url_parts( cache: &Cache, def_id: DefId, module_fqp: &[Symbol], relative_to: &[Symbol], + jump_to_def_path_depth: Option, ) -> Result<(UrlPartsBuilder, bool), HrefError> { match cache.extern_locations[&def_id.krate] { ExternalLocation::Remote { ref url, is_absolute } => { - let mut builder = remote_url_prefix(url, is_absolute, relative_to.len()); + let depth = jump_to_def_path_depth.unwrap_or(relative_to.len()); + let mut builder = remote_url_prefix(url, is_absolute, depth); builder.extend(module_fqp.iter().copied()); - Ok((builder, is_absolute)) + // Remote URLs (both absolute and relative) already encode the correct path; + // no additional depth prefix should be applied. + Ok((builder, true)) } ExternalLocation::Local => Ok((href_relative_parts(module_fqp, relative_to), false)), ExternalLocation::Unknown => Err(HrefError::DocumentationNotBuilt), @@ -642,16 +659,16 @@ fn url_parts( } fn make_href( - root_path: Option<&str>, + jump_to_def_path_depth: Option, shortty: ItemType, mut url_parts: UrlPartsBuilder, fqp: &[Symbol], - is_absolute: bool, + is_remote: bool, ) -> String { - // FIXME: relative extern URLs may break when prefixed with root_path - if !is_absolute && let Some(root_path) = root_path { - let root = root_path.trim_end_matches('/'); - url_parts.push_front(root); + // Remote URLs already have the correct depth via `remote_url_prefix`; + // only local items need `jump_to_def_path_depth` to bridge from source pages to the doc tree. + if !is_remote && let Some(depth) = jump_to_def_path_depth { + url_parts.push_front(&iter::repeat_n("..", depth).join("/")); } debug!(?url_parts); match shortty { @@ -707,10 +724,12 @@ fn inherits_doc_hidden(tcx: TyCtxt<'_>, mut def_id: DefId) -> bool { } } -pub(crate) fn href_with_root_path( +/// If `jump_to_def_path_depth` is `Some`, the link comes from a source page and needs that many +/// `..` segments to reach the doc root; `None` generates a link relative to the current page. +pub(crate) fn href_with_jump_to_def_path_depth( original_did: DefId, cx: &Context<'_>, - root_path: Option<&str>, + jump_to_def_path_depth: Option, preferred_name: Option<&str>, ) -> Result { let tcx = cx.tcx(); @@ -723,7 +742,12 @@ pub(crate) fn href_with_root_path( // If this a constructor, we get the parent (either a struct or a variant) and then // generate the link for this item. DefKind::Ctor(..) => { - return href_with_root_path(tcx.parent(original_did), cx, root_path, preferred_name); + return href_with_jump_to_def_path_depth( + tcx.parent(original_did), + cx, + jump_to_def_path_depth, + preferred_name, + ); } DefKind::ExternCrate => { // Link to the crate itself, not the `extern crate` item. @@ -744,7 +768,7 @@ pub(crate) fn href_with_root_path( if !original_did.is_local() { // If we are generating an href for the "jump to def" feature, then the only case we want // to ignore is if the item is `doc(hidden)` because we can't link to it. - if root_path.is_some() { + if jump_to_def_path_depth.is_some() { if inherits_doc_hidden(tcx, original_did) { return Err(HrefError::Private); } @@ -756,7 +780,7 @@ pub(crate) fn href_with_root_path( } } - let (fqp, shortty, url_parts, is_absolute) = match cache.paths.get(&did) { + let (fqp, shortty, url_parts, is_remote) = match cache.paths.get(&did) { Some(info) => ( info.get_preferred_path(preferred_name), info.ty, @@ -772,32 +796,33 @@ pub(crate) fn href_with_root_path( // directly here whereas for intra-doc links, we have some extra computation being // performed there. // - // So if `root_path` is `Some()` and it's an associated item, we enter to enter the - // `else` branch to have the anchor generated here directly. - if (root_path.is_none() || original_did == did) + // So if `jump_to_def_path_depth` is `Some()` and it's an associated item, we enter to + // enter the `else` branch to have the anchor generated here directly. + if (jump_to_def_path_depth.is_none() || original_did == did) && let Some(&(ref fqp, shortty)) = cache.external_paths.get(&did) { let module_fqp = to_module_fqp(shortty, fqp); - let (parts, is_absolute) = url_parts(cache, did, module_fqp, relative_to)?; - (fqp.as_slice(), shortty, parts, is_absolute) + let (parts, is_remote) = + url_parts(cache, did, module_fqp, relative_to, jump_to_def_path_depth)?; + (fqp.as_slice(), shortty, parts, is_remote) } else if matches!(def_kind, DefKind::Macro(_)) { - return generate_macro_def_id_path(did, cx, root_path); + return generate_macro_def_id_path(did, cx, jump_to_def_path_depth); } else if did.is_local() { return Err(HrefError::Private); } else { - return generate_item_def_id_path(did, original_did, cx, root_path); + return generate_item_def_id_path(did, original_did, cx, jump_to_def_path_depth); } } }; Ok(HrefInfo { - url: make_href(root_path, shortty, url_parts, fqp, is_absolute), + url: make_href(jump_to_def_path_depth, shortty, url_parts, fqp, is_remote), kind: shortty, rust_path: fqp.to_vec(), }) } pub(crate) fn href(did: DefId, cx: &Context<'_>) -> Result { - href_with_root_path(did, cx, None, None) + href_with_jump_to_def_path_depth(did, cx, None, None) } pub(crate) fn href_with_path_check( @@ -805,7 +830,7 @@ pub(crate) fn href_with_path_check( cx: &Context<'_>, text: &str, ) -> Result { - href_with_root_path(did, cx, None, Some(text)) + href_with_jump_to_def_path_depth(did, cx, None, Some(text)) } /// Both paths should only be modules. diff --git a/src/librustdoc/html/highlight.rs b/src/librustdoc/html/highlight.rs index 6ef5de0b20aba..a9b8ccc3b2106 100644 --- a/src/librustdoc/html/highlight.rs +++ b/src/librustdoc/html/highlight.rs @@ -33,7 +33,7 @@ pub(crate) struct HrefContext<'a, 'tcx> { pub(crate) file_span: Span, /// This field is used to know "how far" from the top of the directory we are to link to either /// documentation pages or other source pages. - pub(crate) root_path: &'a str, + pub(crate) jump_to_def_path_depth: usize, /// This field is used to calculate precise local URLs. pub(crate) current_href: String, } @@ -1407,26 +1407,26 @@ fn generate_link_to_def( LinkFromSrc::Local(span) => { context.href_from_span_relative(*span, &href_context.current_href) } - LinkFromSrc::External(def_id) => format::href_with_root_path( + LinkFromSrc::External(def_id) => format::href_with_jump_to_def_path_depth( *def_id, context, - Some(href_context.root_path), + Some(href_context.jump_to_def_path_depth), None, ) .ok() .map(|HrefInfo { url, .. }| url), - LinkFromSrc::Primitive(prim) => format::href_with_root_path( + LinkFromSrc::Primitive(prim) => format::href_with_jump_to_def_path_depth( PrimitiveType::primitive_locations(context.tcx())[prim], context, - Some(href_context.root_path), + Some(href_context.jump_to_def_path_depth), None, ) .ok() .map(|HrefInfo { url, .. }| url), - LinkFromSrc::Doc(def_id) => format::href_with_root_path( + LinkFromSrc::Doc(def_id) => format::href_with_jump_to_def_path_depth( *def_id, context, - Some(href_context.root_path), + Some(href_context.jump_to_def_path_depth), None, ) .ok() diff --git a/src/librustdoc/html/render/mod.rs b/src/librustdoc/html/render/mod.rs index 35947921dc145..974672a050900 100644 --- a/src/librustdoc/html/render/mod.rs +++ b/src/librustdoc/html/render/mod.rs @@ -2903,7 +2903,7 @@ fn render_call_locations( contents_subset, file_span, cx, - &cx.root_path(), + cx.current.len(), &highlight::DecorationInfo(decoration_info), &sources::SourceContext::Embedded(sources::ScrapedInfo { needs_expansion, diff --git a/src/librustdoc/html/sources.rs b/src/librustdoc/html/sources.rs index ea52fe51c5358..2328f0e8b9a11 100644 --- a/src/librustdoc/html/sources.rs +++ b/src/librustdoc/html/sources.rs @@ -1,4 +1,4 @@ -use std::cell::RefCell; +use std::cell::{Cell, RefCell}; use std::ffi::OsStr; use std::path::{Component, Path, PathBuf}; use std::{fmt, fs}; @@ -193,31 +193,28 @@ impl SourceCollector<'_, '_> { let shared = &self.cx.shared; // Create the intermediate directories let cur = RefCell::new(PathBuf::new()); - let root_path = RefCell::new(PathBuf::new()); + // Tracks the extra directory depth accumulated during path traversal. + // Starts at 0; `PathBuf::pop()` on empty is a no-op, so we mirror that + // with `saturating_sub`. The base depth of 2 (for `src//`) is added after. + let extra_depth = Cell::new(0usize); clean_path( &shared.src_root, &p, |component| { cur.borrow_mut().push(component); - root_path.borrow_mut().push(".."); + extra_depth.set(extra_depth.get() + 1); }, || { cur.borrow_mut().pop(); - root_path.borrow_mut().pop(); + extra_depth.set(extra_depth.get().saturating_sub(1)); }, ); + let jump_to_def_path_depth = 2 + extra_depth.get(); + let src_fname = p.file_name().expect("source has no filename").to_os_string(); let mut fname = src_fname.clone(); - - let root_path = PathBuf::from("../../").join(root_path.into_inner()); - let mut root_path = root_path.to_string_lossy(); - if let Some(c) = root_path.as_bytes().last() - && *c != b'/' - { - root_path += "/"; - } let mut file_path = Path::new(&self.crate_name).join(&*cur.borrow()); file_path.push(&fname); fname.push(".html"); @@ -228,6 +225,7 @@ impl SourceCollector<'_, '_> { let title = format!("{} - source", src_fname.to_string_lossy()); let desc = format!("Source of the Rust file `{}`.", p.to_string_lossy()); + let root_path = "../".repeat(jump_to_def_path_depth); let page = layout::Page { title: &title, short_title: &src_fname.to_string_lossy(), @@ -251,7 +249,7 @@ impl SourceCollector<'_, '_> { contents, file_span, self.cx, - &root_path, + jump_to_def_path_depth, &highlight::DecorationInfo::default(), &source_context, ) @@ -330,7 +328,7 @@ pub(crate) fn print_src( s: &str, file_span: rustc_span::Span, cx: &Context<'_>, - root_path: &str, + jump_to_def_path_depth: usize, decoration_info: &highlight::DecorationInfo, source_context: &SourceContext<'_>, ) -> fmt::Result { @@ -358,7 +356,7 @@ pub(crate) fn print_src( Some(highlight::HrefContext { context: cx, file_span: file_span.into(), - root_path, + jump_to_def_path_depth, current_href, }), Some(decoration_info), diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index ac7cf2a42bd3b..ea131574b17e3 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -1251,7 +1251,7 @@ impl LinkCollector<'_, '_> { /// Returns `true` if a link could be generated from the given intra-doc information. /// - /// This is a very light version of `format::href_with_root_path` since we're only interested + /// This is a very light version of `format::href_with_jump_to_def_path_depth` since we're only interested /// about whether we can generate a link to an item or not. /// /// * If `original_did` is local, then we check if the item is reexported or public. diff --git a/tests/rustdoc-html/extern/extern-html-root-url-relative.rs b/tests/rustdoc-html/extern/extern-html-root-url-relative.rs index ba2b50c6bf222..dedc0819ed577 100644 --- a/tests/rustdoc-html/extern/extern-html-root-url-relative.rs +++ b/tests/rustdoc-html/extern/extern-html-root-url-relative.rs @@ -21,7 +21,11 @@ pub mod nested { pub mod intra_doc_link { } -// link-to-definition +// link-to-definition: source pages live under src// so they need +// ../../ to reach the doc root. Previously these links were incorrectly generated +// as ../../../ (one level too deep) which passed the `has` check by substring match. //@ has src/extern_html_root_url_relative/extern-html-root-url-relative.rs.html //@ has - '//a/@href' '../../core/iter/index.html' //@ has - '//a/@href' '../../core/future/index.html' +//@ !has - '//a/@href' '../../../core/iter/index.html' +//@ !has - '//a/@href' '../../../core/future/index.html'