From 077a8219b07b72d8cd0485ffba32e7147e7f7f7d Mon Sep 17 00:00:00 2001 From: Amanda Stjerna Date: Tue, 28 May 2024 12:57:26 +0200 Subject: [PATCH 01/16] Fix back-porting drop-livess from Polonius to tracing --- .../src/type_check/liveness/polonius.rs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/compiler/rustc_borrowck/src/type_check/liveness/polonius.rs b/compiler/rustc_borrowck/src/type_check/liveness/polonius.rs index ccfa9f12ef45a..808df1d66cb13 100644 --- a/compiler/rustc_borrowck/src/type_check/liveness/polonius.rs +++ b/compiler/rustc_borrowck/src/type_check/liveness/polonius.rs @@ -14,7 +14,7 @@ struct UseFactsExtractor<'me, 'tcx> { var_defined_at: &'me mut VarPointRelation, var_used_at: &'me mut VarPointRelation, location_table: &'me LocationTable, - var_dropped_at: &'me mut VarPointRelation, + var_dropped_at: &'me mut Vec<(Local, Location)>, move_data: &'me MoveData<'tcx>, path_accessed_at_base: &'me mut PathPointRelation, } @@ -37,7 +37,7 @@ impl<'tcx> UseFactsExtractor<'_, 'tcx> { fn insert_drop_use(&mut self, local: Local, location: Location) { debug!("UseFactsExtractor::insert_drop_use()"); - self.var_dropped_at.push((local, self.location_to_index(location))); + self.var_dropped_at.push((local, location)); } fn insert_path_access(&mut self, path: MovePathIndex, location: Location) { @@ -87,8 +87,12 @@ pub(super) fn populate_access_facts<'a, 'tcx>( body: &Body<'tcx>, location_table: &LocationTable, move_data: &MoveData<'tcx>, - //FIXME: this is not mutated, but expected to be modified as - // out param, bug? + // FIXME: this is an inelegant way of squirreling away a + // copy of `var_dropped_at` in the original `Location` format + // for later use in `trace::trace()`, which updates some liveness- + // internal data based on what Polonius saw. + // Ideally, that part would access the Polonius facts directly, and this + // would be regular facts gathering. dropped_at: &mut Vec<(Local, Location)>, ) { debug!("populate_access_facts()"); @@ -97,7 +101,7 @@ pub(super) fn populate_access_facts<'a, 'tcx>( let mut extractor = UseFactsExtractor { var_defined_at: &mut facts.var_defined_at, var_used_at: &mut facts.var_used_at, - var_dropped_at: &mut facts.var_dropped_at, + var_dropped_at: dropped_at, path_accessed_at_base: &mut facts.path_accessed_at_base, location_table, move_data, From e15564672e4e35a8697b717d49db1bfc6579c612 Mon Sep 17 00:00:00 2001 From: Amanda Stjerna Date: Tue, 28 May 2024 15:49:22 +0200 Subject: [PATCH 02/16] Make drop-use fact collection simpler for `polonius` This shunts all the complexity of siphoning off the drop-use facts into `LivenessResults::add_extra_drop_facts()`, which may or may not be a good approach. --- .../src/type_check/liveness/mod.rs | 11 ++---- .../src/type_check/liveness/polonius.rs | 19 +++------- .../src/type_check/liveness/trace.rs | 35 ++++++++++++++----- compiler/rustc_borrowck/src/type_check/mod.rs | 10 +----- 4 files changed, 34 insertions(+), 41 deletions(-) diff --git a/compiler/rustc_borrowck/src/type_check/liveness/mod.rs b/compiler/rustc_borrowck/src/type_check/liveness/mod.rs index 38ec9f7678ebc..8b863efad6ca2 100644 --- a/compiler/rustc_borrowck/src/type_check/liveness/mod.rs +++ b/compiler/rustc_borrowck/src/type_check/liveness/mod.rs @@ -14,7 +14,6 @@ use std::rc::Rc; use crate::{ constraints::OutlivesConstraintSet, facts::{AllFacts, AllFactsExt}, - location::LocationTable, region_infer::values::LivenessValues, universal_regions::UniversalRegions, }; @@ -39,7 +38,6 @@ pub(super) fn generate<'mir, 'tcx>( elements: &Rc, flow_inits: &mut ResultsCursor<'mir, 'tcx, MaybeInitializedPlaces<'mir, 'tcx>>, move_data: &MoveData<'tcx>, - location_table: &LocationTable, use_polonius: bool, ) { debug!("liveness::generate"); @@ -53,11 +51,9 @@ pub(super) fn generate<'mir, 'tcx>( compute_relevant_live_locals(typeck.tcx(), &free_regions, body); let facts_enabled = use_polonius || AllFacts::enabled(typeck.tcx()); - let polonius_drop_used = facts_enabled.then(|| { - let mut drop_used = Vec::new(); - polonius::populate_access_facts(typeck, body, location_table, move_data, &mut drop_used); - drop_used - }); + if facts_enabled { + polonius::populate_access_facts(typeck, body, move_data); + }; trace::trace( typeck, @@ -67,7 +63,6 @@ pub(super) fn generate<'mir, 'tcx>( move_data, relevant_live_locals, boring_locals, - polonius_drop_used, ); // Mark regions that should be live where they appear within rvalues or within a call: like diff --git a/compiler/rustc_borrowck/src/type_check/liveness/polonius.rs b/compiler/rustc_borrowck/src/type_check/liveness/polonius.rs index 808df1d66cb13..d8f03a07a63ca 100644 --- a/compiler/rustc_borrowck/src/type_check/liveness/polonius.rs +++ b/compiler/rustc_borrowck/src/type_check/liveness/polonius.rs @@ -14,7 +14,7 @@ struct UseFactsExtractor<'me, 'tcx> { var_defined_at: &'me mut VarPointRelation, var_used_at: &'me mut VarPointRelation, location_table: &'me LocationTable, - var_dropped_at: &'me mut Vec<(Local, Location)>, + var_dropped_at: &'me mut VarPointRelation, move_data: &'me MoveData<'tcx>, path_accessed_at_base: &'me mut PathPointRelation, } @@ -37,7 +37,7 @@ impl<'tcx> UseFactsExtractor<'_, 'tcx> { fn insert_drop_use(&mut self, local: Local, location: Location) { debug!("UseFactsExtractor::insert_drop_use()"); - self.var_dropped_at.push((local, location)); + self.var_dropped_at.push((local, self.location_to_index(location))); } fn insert_path_access(&mut self, path: MovePathIndex, location: Location) { @@ -85,33 +85,22 @@ impl<'a, 'tcx> Visitor<'tcx> for UseFactsExtractor<'a, 'tcx> { pub(super) fn populate_access_facts<'a, 'tcx>( typeck: &mut TypeChecker<'a, 'tcx>, body: &Body<'tcx>, - location_table: &LocationTable, move_data: &MoveData<'tcx>, - // FIXME: this is an inelegant way of squirreling away a - // copy of `var_dropped_at` in the original `Location` format - // for later use in `trace::trace()`, which updates some liveness- - // internal data based on what Polonius saw. - // Ideally, that part would access the Polonius facts directly, and this - // would be regular facts gathering. - dropped_at: &mut Vec<(Local, Location)>, ) { debug!("populate_access_facts()"); + let location_table = typeck.borrowck_context.location_table; if let Some(facts) = typeck.borrowck_context.all_facts.as_mut() { let mut extractor = UseFactsExtractor { var_defined_at: &mut facts.var_defined_at, var_used_at: &mut facts.var_used_at, - var_dropped_at: dropped_at, + var_dropped_at: &mut facts.var_dropped_at, path_accessed_at_base: &mut facts.path_accessed_at_base, location_table, move_data, }; extractor.visit_body(body); - facts.var_dropped_at.extend( - dropped_at.iter().map(|&(local, location)| (local, location_table.mid_index(location))), - ); - for (local, local_decl) in body.local_decls.iter_enumerated() { debug!( "add use_of_var_derefs_origin facts - local={:?}, type={:?}", diff --git a/compiler/rustc_borrowck/src/type_check/liveness/trace.rs b/compiler/rustc_borrowck/src/type_check/liveness/trace.rs index 6cc0e67c0f801..92b70a09a8850 100644 --- a/compiler/rustc_borrowck/src/type_check/liveness/trace.rs +++ b/compiler/rustc_borrowck/src/type_check/liveness/trace.rs @@ -16,6 +16,7 @@ use rustc_mir_dataflow::impls::MaybeInitializedPlaces; use rustc_mir_dataflow::move_paths::{HasMoveData, MoveData, MovePathIndex}; use rustc_mir_dataflow::ResultsCursor; +use crate::location::RichLocation; use crate::{ region_infer::values::{self, LiveLoans}, type_check::liveness::local_use_map::LocalUseMap, @@ -46,7 +47,6 @@ pub(super) fn trace<'mir, 'tcx>( move_data: &MoveData<'tcx>, relevant_live_locals: Vec, boring_locals: Vec, - polonius_drop_used: Option>, ) { let local_use_map = &LocalUseMap::build(&relevant_live_locals, elements, body); @@ -81,6 +81,8 @@ pub(super) fn trace<'mir, 'tcx>( borrowck_context.constraints.liveness_constraints.loans = Some(live_loans); }; + let polonius_facts_gathered = typeck.borrowck_context.all_facts.is_some(); + let cx = LivenessContext { typeck, body, @@ -93,8 +95,8 @@ pub(super) fn trace<'mir, 'tcx>( let mut results = LivenessResults::new(cx); - if let Some(drop_used) = polonius_drop_used { - results.add_extra_drop_facts(drop_used, relevant_live_locals.iter().copied().collect()) + if polonius_facts_gathered { + results.add_extra_drop_facts(relevant_live_locals.iter().copied().collect()); } results.compute_for_all_locals(relevant_live_locals); @@ -218,17 +220,32 @@ impl<'me, 'typeck, 'flow, 'tcx> LivenessResults<'me, 'typeck, 'flow, 'tcx> { /// /// Add facts for all locals with free regions, since regions may outlive /// the function body only at certain nodes in the CFG. - fn add_extra_drop_facts( - &mut self, - drop_used: Vec<(Local, Location)>, - relevant_live_locals: FxIndexSet, - ) { + fn add_extra_drop_facts(&mut self, relevant_live_locals: FxIndexSet) { + let drop_used = self + .cx + .typeck + .borrowck_context + .all_facts + .as_ref() + .map(|facts| facts.var_dropped_at.clone()) + .into_iter() + .flatten(); let locations = IntervalSet::new(self.cx.elements.num_points()); - for (local, location) in drop_used { + for (local, location_index) in drop_used { if !relevant_live_locals.contains(&local) { let local_ty = self.cx.body.local_decls[local].ty; if local_ty.has_free_regions() { + let location = match self + .cx + .typeck + .borrowck_context + .location_table + .to_location(location_index) + { + RichLocation::Start(l) => l, + RichLocation::Mid(l) => l, + }; self.cx.add_drop_live_facts_for(local, local_ty, &[location], &locations); } } diff --git a/compiler/rustc_borrowck/src/type_check/mod.rs b/compiler/rustc_borrowck/src/type_check/mod.rs index 4e46a0c62c7c7..a05fa967af03e 100644 --- a/compiler/rustc_borrowck/src/type_check/mod.rs +++ b/compiler/rustc_borrowck/src/type_check/mod.rs @@ -188,15 +188,7 @@ pub(crate) fn type_check<'mir, 'tcx>( checker.equate_inputs_and_outputs(body, universal_regions, &normalized_inputs_and_output); checker.check_signature_annotation(body); - liveness::generate( - &mut checker, - body, - elements, - flow_inits, - move_data, - location_table, - use_polonius, - ); + liveness::generate(&mut checker, body, elements, flow_inits, move_data, use_polonius); translate_outlives_facts(&mut checker); let opaque_type_values = infcx.take_opaque_types(); From 8066ebc294f110a758fb2b44a68138db10d38ce0 Mon Sep 17 00:00:00 2001 From: Amanda Stjerna Date: Tue, 28 May 2024 16:02:09 +0200 Subject: [PATCH 03/16] Move the rest of the logic into `add_extra_drop_facts()` --- .../src/type_check/liveness/trace.rs | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/compiler/rustc_borrowck/src/type_check/liveness/trace.rs b/compiler/rustc_borrowck/src/type_check/liveness/trace.rs index 92b70a09a8850..50843c602cc84 100644 --- a/compiler/rustc_borrowck/src/type_check/liveness/trace.rs +++ b/compiler/rustc_borrowck/src/type_check/liveness/trace.rs @@ -81,8 +81,6 @@ pub(super) fn trace<'mir, 'tcx>( borrowck_context.constraints.liveness_constraints.loans = Some(live_loans); }; - let polonius_facts_gathered = typeck.borrowck_context.all_facts.is_some(); - let cx = LivenessContext { typeck, body, @@ -95,9 +93,7 @@ pub(super) fn trace<'mir, 'tcx>( let mut results = LivenessResults::new(cx); - if polonius_facts_gathered { - results.add_extra_drop_facts(relevant_live_locals.iter().copied().collect()); - } + results.add_extra_drop_facts(&relevant_live_locals); results.compute_for_all_locals(relevant_live_locals); @@ -220,16 +216,17 @@ impl<'me, 'typeck, 'flow, 'tcx> LivenessResults<'me, 'typeck, 'flow, 'tcx> { /// /// Add facts for all locals with free regions, since regions may outlive /// the function body only at certain nodes in the CFG. - fn add_extra_drop_facts(&mut self, relevant_live_locals: FxIndexSet) { + fn add_extra_drop_facts(&mut self, relevant_live_locals: &[Local]) -> Option<()> { let drop_used = self .cx .typeck .borrowck_context .all_facts .as_ref() - .map(|facts| facts.var_dropped_at.clone()) - .into_iter() - .flatten(); + .map(|facts| facts.var_dropped_at.clone())?; + + let relevant_live_locals: FxIndexSet<_> = relevant_live_locals.iter().copied().collect(); + let locations = IntervalSet::new(self.cx.elements.num_points()); for (local, location_index) in drop_used { @@ -250,6 +247,7 @@ impl<'me, 'typeck, 'flow, 'tcx> LivenessResults<'me, 'typeck, 'flow, 'tcx> { } } } + Some(()) } /// Clear the value of fields that are "per local variable". From dabd05bbab9df465a580858c3e03dc1797b12deb Mon Sep 17 00:00:00 2001 From: r0cky Date: Thu, 30 May 2024 09:51:27 +0800 Subject: [PATCH 04/16] Apply x clippy --fix and x fmt --- .../src/graph/dominators/mod.rs | 2 +- .../src/graph/iterate/mod.rs | 4 ++-- .../rustc_data_structures/src/graph/scc/mod.rs | 2 +- .../rustc_data_structures/src/profiling.rs | 2 +- .../rustc_data_structures/src/sorted_map.rs | 4 ++-- .../rustc_data_structures/src/sync/lock.rs | 2 +- compiler/rustc_parse_format/src/lib.rs | 18 ++++++++---------- compiler/rustc_serialize/src/serialize.rs | 2 +- compiler/stable_mir/src/mir/pretty.rs | 4 ++-- compiler/stable_mir/src/ty.rs | 6 +++--- library/proc_macro/src/bridge/fxhash.rs | 2 +- library/proc_macro/src/bridge/rpc.rs | 6 +++--- library/test/src/cli.rs | 2 +- library/test/src/term/terminfo/parm.rs | 2 +- 14 files changed, 28 insertions(+), 30 deletions(-) diff --git a/compiler/rustc_data_structures/src/graph/dominators/mod.rs b/compiler/rustc_data_structures/src/graph/dominators/mod.rs index 30e240cf85b84..d1d2de670b82d 100644 --- a/compiler/rustc_data_structures/src/graph/dominators/mod.rs +++ b/compiler/rustc_data_structures/src/graph/dominators/mod.rs @@ -93,7 +93,7 @@ fn dominators_impl(graph: &G) -> Inner { // These are all done here rather than through one of the 'standard' // graph traversals to help make this fast. 'recurse: while let Some(frame) = stack.last_mut() { - while let Some(successor) = frame.iter.next() { + for successor in frame.iter.by_ref() { if real_to_pre_order[successor].is_none() { let pre_order_idx = pre_order_to_real.push(successor); real_to_pre_order[successor] = Some(pre_order_idx); diff --git a/compiler/rustc_data_structures/src/graph/iterate/mod.rs b/compiler/rustc_data_structures/src/graph/iterate/mod.rs index 78d05a6e1951b..6fca57d32f771 100644 --- a/compiler/rustc_data_structures/src/graph/iterate/mod.rs +++ b/compiler/rustc_data_structures/src/graph/iterate/mod.rs @@ -48,7 +48,7 @@ fn post_order_walk( let node = frame.node; visited[node] = true; - while let Some(successor) = frame.iter.next() { + for successor in frame.iter.by_ref() { if !visited[successor] { stack.push(PostOrderFrame { node: successor, iter: graph.successors(successor) }); continue 'recurse; @@ -112,7 +112,7 @@ where /// This is equivalent to just invoke `next` repeatedly until /// you get a `None` result. pub fn complete_search(&mut self) { - while let Some(_) = self.next() {} + for _ in self.by_ref() {} } /// Returns true if node has been visited thus far. diff --git a/compiler/rustc_data_structures/src/graph/scc/mod.rs b/compiler/rustc_data_structures/src/graph/scc/mod.rs index 914a6a16348c5..7f36e4ca16df0 100644 --- a/compiler/rustc_data_structures/src/graph/scc/mod.rs +++ b/compiler/rustc_data_structures/src/graph/scc/mod.rs @@ -40,7 +40,7 @@ pub struct SccData { } impl Sccs { - pub fn new(graph: &(impl DirectedGraph + Successors)) -> Self { + pub fn new(graph: &impl Successors) -> Self { SccsConstruction::construct(graph) } diff --git a/compiler/rustc_data_structures/src/profiling.rs b/compiler/rustc_data_structures/src/profiling.rs index c6d51a5d6b4f7..240f2671c3b2d 100644 --- a/compiler/rustc_data_structures/src/profiling.rs +++ b/compiler/rustc_data_structures/src/profiling.rs @@ -562,7 +562,7 @@ impl SelfProfiler { // ASLR is disabled and the heap is otherwise deterministic. let pid: u32 = process::id(); let filename = format!("{crate_name}-{pid:07}.rustc_profile"); - let path = output_directory.join(&filename); + let path = output_directory.join(filename); let profiler = Profiler::with_counter(&path, measureme::counters::Counter::by_name(counter_name)?)?; diff --git a/compiler/rustc_data_structures/src/sorted_map.rs b/compiler/rustc_data_structures/src/sorted_map.rs index 21d7c91ec4826..885f023122ab5 100644 --- a/compiler/rustc_data_structures/src/sorted_map.rs +++ b/compiler/rustc_data_structures/src/sorted_map.rs @@ -125,13 +125,13 @@ impl SortedMap { /// Iterate over the keys, sorted #[inline] - pub fn keys(&self) -> impl Iterator + ExactSizeIterator + DoubleEndedIterator { + pub fn keys(&self) -> impl ExactSizeIterator + DoubleEndedIterator { self.data.iter().map(|(k, _)| k) } /// Iterate over values, sorted by key #[inline] - pub fn values(&self) -> impl Iterator + ExactSizeIterator + DoubleEndedIterator { + pub fn values(&self) -> impl ExactSizeIterator + DoubleEndedIterator { self.data.iter().map(|(_, v)| v) } diff --git a/compiler/rustc_data_structures/src/sync/lock.rs b/compiler/rustc_data_structures/src/sync/lock.rs index 756984642c742..780be77394581 100644 --- a/compiler/rustc_data_structures/src/sync/lock.rs +++ b/compiler/rustc_data_structures/src/sync/lock.rs @@ -69,7 +69,7 @@ mod maybe_sync { match self.mode { Mode::NoSync => { let cell = unsafe { &self.lock.mode_union.no_sync }; - debug_assert_eq!(cell.get(), true); + debug_assert!(cell.get()); cell.set(false); } // SAFETY (unlock): We know that the lock is locked as this type is a proof of that. diff --git a/compiler/rustc_parse_format/src/lib.rs b/compiler/rustc_parse_format/src/lib.rs index faf6ca7846709..ef71333d1c393 100644 --- a/compiler/rustc_parse_format/src/lib.rs +++ b/compiler/rustc_parse_format/src/lib.rs @@ -286,13 +286,11 @@ impl<'a> Iterator for Parser<'a> { lbrace_byte_pos.to(InnerOffset(rbrace_byte_pos.0 + width)), ); } - } else { - if let Some(&(_, maybe)) = self.cur.peek() { - match maybe { - '?' => self.suggest_format_debug(), - '<' | '^' | '>' => self.suggest_format_align(maybe), - _ => self.suggest_positional_arg_instead_of_captured_arg(arg), - } + } else if let Some(&(_, maybe)) = self.cur.peek() { + match maybe { + '?' => self.suggest_format_debug(), + '<' | '^' | '>' => self.suggest_format_align(maybe), + _ => self.suggest_positional_arg_instead_of_captured_arg(arg), } } Some(NextArgument(Box::new(arg))) @@ -1028,7 +1026,7 @@ fn find_width_map_from_snippet( if next_c == '{' { // consume up to 6 hexanumeric chars let digits_len = - s.clone().take(6).take_while(|(_, c)| c.is_digit(16)).count(); + s.clone().take(6).take_while(|(_, c)| c.is_ascii_hexdigit()).count(); let len_utf8 = s .as_str() @@ -1047,14 +1045,14 @@ fn find_width_map_from_snippet( width += required_skips + 2; s.nth(digits_len); - } else if next_c.is_digit(16) { + } else if next_c.is_ascii_hexdigit() { width += 1; // We suggest adding `{` and `}` when appropriate, accept it here as if // it were correct let mut i = 0; // consume up to 6 hexanumeric chars while let (Some((_, c)), _) = (s.next(), i < 6) { - if c.is_digit(16) { + if c.is_ascii_hexdigit() { width += 1; } else { break; diff --git a/compiler/rustc_serialize/src/serialize.rs b/compiler/rustc_serialize/src/serialize.rs index 412f7eced433c..c84bb26735c5b 100644 --- a/compiler/rustc_serialize/src/serialize.rs +++ b/compiler/rustc_serialize/src/serialize.rs @@ -252,7 +252,7 @@ impl Encodable for () { } impl Decodable for () { - fn decode(_: &mut D) -> () {} + fn decode(_: &mut D) {} } impl Encodable for PhantomData { diff --git a/compiler/stable_mir/src/mir/pretty.rs b/compiler/stable_mir/src/mir/pretty.rs index bbca3965852a6..580dc1a2b88fc 100644 --- a/compiler/stable_mir/src/mir/pretty.rs +++ b/compiler/stable_mir/src/mir/pretty.rs @@ -156,7 +156,7 @@ fn pretty_terminator(writer: &mut W, terminator: &TerminatorKind) -> i fn pretty_terminator_head(writer: &mut W, terminator: &TerminatorKind) -> io::Result<()> { use self::TerminatorKind::*; - const INDENT: &'static str = " "; + const INDENT: &str = " "; match terminator { Goto { .. } => write!(writer, "{INDENT}goto"), SwitchInt { discr, .. } => { @@ -315,7 +315,7 @@ fn pretty_operand(operand: &Operand) -> String { } fn pretty_const(literal: &Const) -> String { - with(|cx| cx.const_pretty(&literal)) + with(|cx| cx.const_pretty(literal)) } fn pretty_rvalue(writer: &mut W, rval: &Rvalue) -> io::Result<()> { diff --git a/compiler/stable_mir/src/ty.rs b/compiler/stable_mir/src/ty.rs index d62054eff6092..50bf0a5d74ef3 100644 --- a/compiler/stable_mir/src/ty.rs +++ b/compiler/stable_mir/src/ty.rs @@ -526,7 +526,7 @@ pub enum IntTy { impl IntTy { pub fn num_bytes(self) -> usize { match self { - IntTy::Isize => crate::target::MachineInfo::target_pointer_width().bytes().into(), + IntTy::Isize => crate::target::MachineInfo::target_pointer_width().bytes(), IntTy::I8 => 1, IntTy::I16 => 2, IntTy::I32 => 4, @@ -549,7 +549,7 @@ pub enum UintTy { impl UintTy { pub fn num_bytes(self) -> usize { match self { - UintTy::Usize => crate::target::MachineInfo::target_pointer_width().bytes().into(), + UintTy::Usize => crate::target::MachineInfo::target_pointer_width().bytes(), UintTy::U8 => 1, UintTy::U16 => 2, UintTy::U32 => 4, @@ -1185,7 +1185,7 @@ impl Allocation { match self.read_int()? { 0 => Ok(false), 1 => Ok(true), - val @ _ => Err(error!("Unexpected value for bool: `{val}`")), + val => Err(error!("Unexpected value for bool: `{val}`")), } } diff --git a/library/proc_macro/src/bridge/fxhash.rs b/library/proc_macro/src/bridge/fxhash.rs index f4e9054419721..f9f74c63fc4f6 100644 --- a/library/proc_macro/src/bridge/fxhash.rs +++ b/library/proc_macro/src/bridge/fxhash.rs @@ -69,7 +69,7 @@ impl Hasher for FxHasher { hash.add_to_hash(u16::from_ne_bytes(bytes[..2].try_into().unwrap()) as usize); bytes = &bytes[2..]; } - if (size_of::() > 1) && bytes.len() >= 1 { + if (size_of::() > 1) && !bytes.is_empty() { hash.add_to_hash(bytes[0] as usize); } self.hash = hash.hash; diff --git a/library/proc_macro/src/bridge/rpc.rs b/library/proc_macro/src/bridge/rpc.rs index 6d75a5a627c82..202a8e04543b2 100644 --- a/library/proc_macro/src/bridge/rpc.rs +++ b/library/proc_macro/src/bridge/rpc.rs @@ -264,9 +264,9 @@ impl From> for PanicMessage { } } -impl Into> for PanicMessage { - fn into(self) -> Box { - match self { +impl From for Box { + fn from(val: PanicMessage) -> Self { + match val { PanicMessage::StaticStr(s) => Box::new(s), PanicMessage::String(s) => Box::new(s), PanicMessage::Unknown => { diff --git a/library/test/src/cli.rs b/library/test/src/cli.rs index 6ac3b3eaa797b..b7d24405b775e 100644 --- a/library/test/src/cli.rs +++ b/library/test/src/cli.rs @@ -200,7 +200,7 @@ Test Attributes: pub fn parse_opts(args: &[String]) -> Option { // Parse matches. let opts = optgroups(); - let binary = args.get(0).map(|c| &**c).unwrap_or("..."); + let binary = args.first().map(|c| &**c).unwrap_or("..."); let args = args.get(1..).unwrap_or(args); let matches = match opts.parse(args) { Ok(m) => m, diff --git a/library/test/src/term/terminfo/parm.rs b/library/test/src/term/terminfo/parm.rs index 2815f6cfc77fe..c5b4ef01893c2 100644 --- a/library/test/src/term/terminfo/parm.rs +++ b/library/test/src/term/terminfo/parm.rs @@ -524,7 +524,7 @@ fn format(val: Param, op: FormatOp, flags: Flags) -> Result, String> { } else { let mut s_ = Vec::with_capacity(flags.width); s_.extend(repeat(b' ').take(n)); - s_.extend(s.into_iter()); + s_.extend(s); s = s_; } } From fa563c138433583ebbd64c963666f6c9ddb53739 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dorian=20P=C3=A9ron?= Date: Fri, 26 Apr 2024 13:01:06 +0000 Subject: [PATCH 05/16] coverage: Add CLI support for `-Zcoverage-options=condition` --- compiler/rustc_session/src/config.rs | 18 +++++++++++++++++- compiler/rustc_session/src/options.rs | 3 ++- compiler/rustc_session/src/session.rs | 5 +++++ .../src/compiler-flags/coverage-options.md | 7 +++++-- .../coverage-options.bad.stderr | 2 +- .../ui/instrument-coverage/coverage-options.rs | 5 ++++- 6 files changed, 34 insertions(+), 6 deletions(-) diff --git a/compiler/rustc_session/src/config.rs b/compiler/rustc_session/src/config.rs index 0287085ad60ef..a622f1b577df4 100644 --- a/compiler/rustc_session/src/config.rs +++ b/compiler/rustc_session/src/config.rs @@ -159,7 +159,23 @@ pub enum CoverageLevel { Block, /// Also instrument branch points (includes block coverage). Branch, - /// Instrument for MC/DC. Mostly a superset of branch coverage, but might + /// Same as branch coverage, but also adds branch instrumentation for + /// certain boolean expressions that are not directly used for branching. + /// + /// For example, in the following code, `b` does not directly participate + /// in a branch, but condition coverage will instrument it as its own + /// artificial branch: + /// ``` + /// # let (a, b) = (false, true); + /// let x = a && b; + /// // ^ last operand + /// ``` + /// + /// This level is mainly intended to be a stepping-stone towards full MC/DC + /// instrumentation, so it might be removed in the future when MC/DC is + /// sufficiently complete, or if it is making MC/DC changes difficult. + Condition, + /// Instrument for MC/DC. Mostly a superset of condition coverage, but might /// differ in some corner cases. Mcdc, } diff --git a/compiler/rustc_session/src/options.rs b/compiler/rustc_session/src/options.rs index cca9f1eb9978f..fd4a3a9e6cebd 100644 --- a/compiler/rustc_session/src/options.rs +++ b/compiler/rustc_session/src/options.rs @@ -395,7 +395,7 @@ mod desc { pub const parse_optimization_fuel: &str = "crate=integer"; pub const parse_dump_mono_stats: &str = "`markdown` (default) or `json`"; pub const parse_instrument_coverage: &str = parse_bool; - pub const parse_coverage_options: &str = "`block` | `branch` | `mcdc`"; + pub const parse_coverage_options: &str = "`block` | `branch` | `condition` | `mcdc`"; pub const parse_instrument_xray: &str = "either a boolean (`yes`, `no`, `on`, `off`, etc), or a comma separated list of settings: `always` or `never` (mutually exclusive), `ignore-loops`, `instruction-threshold=N`, `skip-entry`, `skip-exit`"; pub const parse_unpretty: &str = "`string` or `string=string`"; pub const parse_treat_err_as_bug: &str = "either no value or a non-negative number"; @@ -961,6 +961,7 @@ mod parse { match option { "block" => slot.level = CoverageLevel::Block, "branch" => slot.level = CoverageLevel::Branch, + "condition" => slot.level = CoverageLevel::Condition, "mcdc" => slot.level = CoverageLevel::Mcdc, _ => return false, } diff --git a/compiler/rustc_session/src/session.rs b/compiler/rustc_session/src/session.rs index a5cf136db6a62..87bbfcf07c846 100644 --- a/compiler/rustc_session/src/session.rs +++ b/compiler/rustc_session/src/session.rs @@ -353,6 +353,11 @@ impl Session { && self.opts.unstable_opts.coverage_options.level >= CoverageLevel::Branch } + pub fn instrument_coverage_condition(&self) -> bool { + self.instrument_coverage() + && self.opts.unstable_opts.coverage_options.level >= CoverageLevel::Condition + } + pub fn instrument_coverage_mcdc(&self) -> bool { self.instrument_coverage() && self.opts.unstable_opts.coverage_options.level >= CoverageLevel::Mcdc diff --git a/src/doc/unstable-book/src/compiler-flags/coverage-options.md b/src/doc/unstable-book/src/compiler-flags/coverage-options.md index 2127883355025..87a9a00b3c0f7 100644 --- a/src/doc/unstable-book/src/compiler-flags/coverage-options.md +++ b/src/doc/unstable-book/src/compiler-flags/coverage-options.md @@ -5,13 +5,16 @@ This option controls details of the coverage instrumentation performed by Multiple options can be passed, separated by commas. Valid options are: -- `block`, `branch`, `mcdc`: +- `block`, `branch`, `condition`, `mcdc`: Sets the level of coverage instrumentation. Setting the level will override any previously-specified level. - `block` (default): Blocks in the control-flow graph will be instrumented for coverage. - `branch`: In addition to block coverage, also enables branch coverage instrumentation. + - `condition`: + In addition to branch coverage, also instruments some boolean expressions + as branches, even if they are not directly used as branch conditions. - `mcdc`: - In addition to block and branch coverage, also enables MC/DC instrumentation. + In addition to condition coverage, also enables MC/DC instrumentation. (Branch coverage instrumentation may differ in some cases.) diff --git a/tests/ui/instrument-coverage/coverage-options.bad.stderr b/tests/ui/instrument-coverage/coverage-options.bad.stderr index 9e2c19e90d596..4a272cf97fb00 100644 --- a/tests/ui/instrument-coverage/coverage-options.bad.stderr +++ b/tests/ui/instrument-coverage/coverage-options.bad.stderr @@ -1,2 +1,2 @@ -error: incorrect value `bad` for unstable option `coverage-options` - `block` | `branch` | `mcdc` was expected +error: incorrect value `bad` for unstable option `coverage-options` - `block` | `branch` | `condition` | `mcdc` was expected diff --git a/tests/ui/instrument-coverage/coverage-options.rs b/tests/ui/instrument-coverage/coverage-options.rs index 2a80ce4ab2e91..8f523a5fd11a0 100644 --- a/tests/ui/instrument-coverage/coverage-options.rs +++ b/tests/ui/instrument-coverage/coverage-options.rs @@ -1,5 +1,5 @@ //@ needs-profiler-support -//@ revisions: block branch mcdc bad +//@ revisions: block branch condition mcdc bad //@ compile-flags -Cinstrument-coverage //@ [block] check-pass @@ -8,6 +8,9 @@ //@ [branch] check-pass //@ [branch] compile-flags: -Zcoverage-options=branch +//@ [condition] check-pass +//@ [condition] compile-flags: -Zcoverage-options=condition + //@ [mcdc] check-pass //@ [mcdc] compile-flags: -Zcoverage-options=mcdc From 20174e6638eb0d5ac0195706ce6cc7185949a407 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dorian=20P=C3=A9ron?= Date: Mon, 29 Apr 2024 15:58:14 +0000 Subject: [PATCH 06/16] coverage: Add a test for `-Zcoverage-options=condition` --- tests/coverage/condition/conditions.cov-map | 117 +++++++++++++++++++ tests/coverage/condition/conditions.coverage | 90 ++++++++++++++ tests/coverage/condition/conditions.rs | 67 +++++++++++ 3 files changed, 274 insertions(+) create mode 100644 tests/coverage/condition/conditions.cov-map create mode 100644 tests/coverage/condition/conditions.coverage create mode 100644 tests/coverage/condition/conditions.rs diff --git a/tests/coverage/condition/conditions.cov-map b/tests/coverage/condition/conditions.cov-map new file mode 100644 index 0000000000000..6c81f7bda9854 --- /dev/null +++ b/tests/coverage/condition/conditions.cov-map @@ -0,0 +1,117 @@ +Function name: conditions::assign_3_and_or +Raw bytes (60): 0x[01, 01, 06, 0d, 13, 09, 16, 01, 05, 01, 05, 09, 16, 01, 05, 08, 01, 1c, 01, 00, 2f, 03, 01, 09, 00, 0a, 01, 00, 0d, 00, 0e, 20, 05, 16, 00, 0d, 00, 0e, 05, 00, 12, 00, 13, 20, 0d, 09, 00, 12, 00, 13, 13, 00, 17, 00, 18, 03, 01, 05, 01, 02] +Number of files: 1 +- file 0 => global file 1 +Number of expressions: 6 +- expression 0 operands: lhs = Counter(3), rhs = Expression(4, Add) +- expression 1 operands: lhs = Counter(2), rhs = Expression(5, Sub) +- expression 2 operands: lhs = Counter(0), rhs = Counter(1) +- expression 3 operands: lhs = Counter(0), rhs = Counter(1) +- expression 4 operands: lhs = Counter(2), rhs = Expression(5, Sub) +- expression 5 operands: lhs = Counter(0), rhs = Counter(1) +Number of file 0 mappings: 8 +- Code(Counter(0)) at (prev + 28, 1) to (start + 0, 47) +- Code(Expression(0, Add)) at (prev + 1, 9) to (start + 0, 10) + = (c3 + (c2 + (c0 - c1))) +- Code(Counter(0)) at (prev + 0, 13) to (start + 0, 14) +- Branch { true: Counter(1), false: Expression(5, Sub) } at (prev + 0, 13) to (start + 0, 14) + true = c1 + false = (c0 - c1) +- Code(Counter(1)) at (prev + 0, 18) to (start + 0, 19) +- Branch { true: Counter(3), false: Counter(2) } at (prev + 0, 18) to (start + 0, 19) + true = c3 + false = c2 +- Code(Expression(4, Add)) at (prev + 0, 23) to (start + 0, 24) + = (c2 + (c0 - c1)) +- Code(Expression(0, Add)) at (prev + 1, 5) to (start + 1, 2) + = (c3 + (c2 + (c0 - c1))) + +Function name: conditions::assign_3_or_and +Raw bytes (56): 0x[01, 01, 04, 05, 07, 09, 0d, 01, 05, 01, 05, 08, 01, 17, 01, 00, 2f, 03, 01, 09, 00, 0a, 01, 00, 0d, 00, 0e, 20, 05, 0e, 00, 0d, 00, 0e, 0e, 00, 12, 00, 13, 20, 09, 0d, 00, 12, 00, 13, 09, 00, 17, 00, 18, 03, 01, 05, 01, 02] +Number of files: 1 +- file 0 => global file 1 +Number of expressions: 4 +- expression 0 operands: lhs = Counter(1), rhs = Expression(1, Add) +- expression 1 operands: lhs = Counter(2), rhs = Counter(3) +- expression 2 operands: lhs = Counter(0), rhs = Counter(1) +- expression 3 operands: lhs = Counter(0), rhs = Counter(1) +Number of file 0 mappings: 8 +- Code(Counter(0)) at (prev + 23, 1) to (start + 0, 47) +- Code(Expression(0, Add)) at (prev + 1, 9) to (start + 0, 10) + = (c1 + (c2 + c3)) +- Code(Counter(0)) at (prev + 0, 13) to (start + 0, 14) +- Branch { true: Counter(1), false: Expression(3, Sub) } at (prev + 0, 13) to (start + 0, 14) + true = c1 + false = (c0 - c1) +- Code(Expression(3, Sub)) at (prev + 0, 18) to (start + 0, 19) + = (c0 - c1) +- Branch { true: Counter(2), false: Counter(3) } at (prev + 0, 18) to (start + 0, 19) + true = c2 + false = c3 +- Code(Counter(2)) at (prev + 0, 23) to (start + 0, 24) +- Code(Expression(0, Add)) at (prev + 1, 5) to (start + 1, 2) + = (c1 + (c2 + c3)) + +Function name: conditions::assign_and +Raw bytes (38): 0x[01, 01, 01, 01, 05, 06, 01, 0d, 01, 00, 21, 01, 01, 09, 00, 0a, 01, 00, 0d, 00, 0e, 20, 05, 02, 00, 0d, 00, 0e, 05, 00, 12, 00, 13, 01, 01, 05, 01, 02] +Number of files: 1 +- file 0 => global file 1 +Number of expressions: 1 +- expression 0 operands: lhs = Counter(0), rhs = Counter(1) +Number of file 0 mappings: 6 +- Code(Counter(0)) at (prev + 13, 1) to (start + 0, 33) +- Code(Counter(0)) at (prev + 1, 9) to (start + 0, 10) +- Code(Counter(0)) at (prev + 0, 13) to (start + 0, 14) +- Branch { true: Counter(1), false: Expression(0, Sub) } at (prev + 0, 13) to (start + 0, 14) + true = c1 + false = (c0 - c1) +- Code(Counter(1)) at (prev + 0, 18) to (start + 0, 19) +- Code(Counter(0)) at (prev + 1, 5) to (start + 1, 2) + +Function name: conditions::assign_or +Raw bytes (38): 0x[01, 01, 01, 01, 05, 06, 01, 12, 01, 00, 20, 01, 01, 09, 00, 0a, 01, 00, 0d, 00, 0e, 20, 05, 02, 00, 0d, 00, 0e, 02, 00, 12, 00, 13, 01, 01, 05, 01, 02] +Number of files: 1 +- file 0 => global file 1 +Number of expressions: 1 +- expression 0 operands: lhs = Counter(0), rhs = Counter(1) +Number of file 0 mappings: 6 +- Code(Counter(0)) at (prev + 18, 1) to (start + 0, 32) +- Code(Counter(0)) at (prev + 1, 9) to (start + 0, 10) +- Code(Counter(0)) at (prev + 0, 13) to (start + 0, 14) +- Branch { true: Counter(1), false: Expression(0, Sub) } at (prev + 0, 13) to (start + 0, 14) + true = c1 + false = (c0 - c1) +- Code(Expression(0, Sub)) at (prev + 0, 18) to (start + 0, 19) + = (c0 - c1) +- Code(Counter(0)) at (prev + 1, 5) to (start + 1, 2) + +Function name: conditions::foo +Raw bytes (9): 0x[01, 01, 00, 01, 01, 21, 01, 02, 02] +Number of files: 1 +- file 0 => global file 1 +Number of expressions: 0 +Number of file 0 mappings: 1 +- Code(Counter(0)) at (prev + 33, 1) to (start + 2, 2) + +Function name: conditions::func_call +Raw bytes (28): 0x[01, 01, 01, 01, 05, 04, 01, 25, 01, 01, 0a, 20, 05, 02, 01, 09, 00, 0a, 05, 00, 0e, 00, 0f, 01, 01, 01, 00, 02] +Number of files: 1 +- file 0 => global file 1 +Number of expressions: 1 +- expression 0 operands: lhs = Counter(0), rhs = Counter(1) +Number of file 0 mappings: 4 +- Code(Counter(0)) at (prev + 37, 1) to (start + 1, 10) +- Branch { true: Counter(1), false: Expression(0, Sub) } at (prev + 1, 9) to (start + 0, 10) + true = c1 + false = (c0 - c1) +- Code(Counter(1)) at (prev + 0, 14) to (start + 0, 15) +- Code(Counter(0)) at (prev + 1, 1) to (start + 0, 2) + +Function name: conditions::simple_assign +Raw bytes (9): 0x[01, 01, 00, 01, 01, 08, 01, 03, 02] +Number of files: 1 +- file 0 => global file 1 +Number of expressions: 0 +Number of file 0 mappings: 1 +- Code(Counter(0)) at (prev + 8, 1) to (start + 3, 2) + diff --git a/tests/coverage/condition/conditions.coverage b/tests/coverage/condition/conditions.coverage new file mode 100644 index 0000000000000..a71ab8e5d8943 --- /dev/null +++ b/tests/coverage/condition/conditions.coverage @@ -0,0 +1,90 @@ + LL| |#![feature(coverage_attribute)] + LL| |//@ edition: 2021 + LL| |//@ compile-flags: -Zcoverage-options=condition + LL| |//@ llvm-cov-flags: --show-branches=count + LL| | + LL| |use core::hint::black_box; + LL| | + LL| 2|fn simple_assign(a: bool) { + LL| 2| let x = a; + LL| 2| black_box(x); + LL| 2|} + LL| | + LL| 3|fn assign_and(a: bool, b: bool) { + LL| 3| let x = a && b; + ^2 + ------------------ + | Branch (LL:13): [True: 2, False: 1] + ------------------ + LL| 3| black_box(x); + LL| 3|} + LL| | + LL| 3|fn assign_or(a: bool, b: bool) { + LL| 3| let x = a || b; + ^1 + ------------------ + | Branch (LL:13): [True: 2, False: 1] + ------------------ + LL| 3| black_box(x); + LL| 3|} + LL| | + LL| 4|fn assign_3_or_and(a: bool, b: bool, c: bool) { + LL| 4| let x = a || b && c; + ^2 ^1 + ------------------ + | Branch (LL:13): [True: 2, False: 2] + | Branch (LL:18): [True: 1, False: 1] + ------------------ + LL| 4| black_box(x); + LL| 4|} + LL| | + LL| 4|fn assign_3_and_or(a: bool, b: bool, c: bool) { + LL| 4| let x = a && b || c; + ^2 ^3 + ------------------ + | Branch (LL:13): [True: 2, False: 2] + | Branch (LL:18): [True: 1, False: 1] + ------------------ + LL| 4| black_box(x); + LL| 4|} + LL| | + LL| 3|fn foo(a: bool) -> bool { + LL| 3| black_box(a) + LL| 3|} + LL| | + LL| 3|fn func_call(a: bool, b: bool) { + LL| 3| foo(a && b); + ^2 + ------------------ + | Branch (LL:9): [True: 2, False: 1] + ------------------ + LL| 3|} + LL| | + LL| |#[coverage(off)] + LL| |fn main() { + LL| | simple_assign(true); + LL| | simple_assign(false); + LL| | + LL| | assign_and(true, false); + LL| | assign_and(true, true); + LL| | assign_and(false, false); + LL| | + LL| | assign_or(true, false); + LL| | assign_or(true, true); + LL| | assign_or(false, false); + LL| | + LL| | assign_3_or_and(true, false, false); + LL| | assign_3_or_and(true, true, false); + LL| | assign_3_or_and(false, false, true); + LL| | assign_3_or_and(false, true, true); + LL| | + LL| | assign_3_and_or(true, false, false); + LL| | assign_3_and_or(true, true, false); + LL| | assign_3_and_or(false, false, true); + LL| | assign_3_and_or(false, true, true); + LL| | + LL| | func_call(true, false); + LL| | func_call(true, true); + LL| | func_call(false, false); + LL| |} + diff --git a/tests/coverage/condition/conditions.rs b/tests/coverage/condition/conditions.rs new file mode 100644 index 0000000000000..3d658dc93e0c7 --- /dev/null +++ b/tests/coverage/condition/conditions.rs @@ -0,0 +1,67 @@ +#![feature(coverage_attribute)] +//@ edition: 2021 +//@ compile-flags: -Zcoverage-options=condition +//@ llvm-cov-flags: --show-branches=count + +use core::hint::black_box; + +fn simple_assign(a: bool) { + let x = a; + black_box(x); +} + +fn assign_and(a: bool, b: bool) { + let x = a && b; + black_box(x); +} + +fn assign_or(a: bool, b: bool) { + let x = a || b; + black_box(x); +} + +fn assign_3_or_and(a: bool, b: bool, c: bool) { + let x = a || b && c; + black_box(x); +} + +fn assign_3_and_or(a: bool, b: bool, c: bool) { + let x = a && b || c; + black_box(x); +} + +fn foo(a: bool) -> bool { + black_box(a) +} + +fn func_call(a: bool, b: bool) { + foo(a && b); +} + +#[coverage(off)] +fn main() { + simple_assign(true); + simple_assign(false); + + assign_and(true, false); + assign_and(true, true); + assign_and(false, false); + + assign_or(true, false); + assign_or(true, true); + assign_or(false, false); + + assign_3_or_and(true, false, false); + assign_3_or_and(true, true, false); + assign_3_or_and(false, false, true); + assign_3_or_and(false, true, true); + + assign_3_and_or(true, false, false); + assign_3_and_or(true, true, false); + assign_3_and_or(false, false, true); + assign_3_and_or(false, true, true); + + func_call(true, false); + func_call(true, true); + func_call(false, false); +} From 35a8746832a4d9103f317c5cac56cfa1e0316d8c Mon Sep 17 00:00:00 2001 From: Zalathar Date: Wed, 1 May 2024 17:33:33 +1000 Subject: [PATCH 07/16] coverage: Instrument the RHS value of lazy logical operators When a lazy logical operator (`&&` or `||`) occurs outside of an `if` condition, it normally doesn't have any associated control-flow branch, so we don't have an existing way to track whether it was true or false. This patch adds special code to handle this case, by inserting extra MIR blocks in a diamond shape after evaluating the RHS. This gives us a place to insert the appropriate marker statements, which can then be given their own counters. --- .../rustc_mir_build/src/build/coverageinfo.rs | 57 ++++++++ .../rustc_mir_build/src/build/expr/into.rs | 8 +- tests/coverage/condition/conditions.cov-map | 125 +++++++++++------- tests/coverage/condition/conditions.coverage | 5 + 4 files changed, 148 insertions(+), 47 deletions(-) diff --git a/compiler/rustc_mir_build/src/build/coverageinfo.rs b/compiler/rustc_mir_build/src/build/coverageinfo.rs index ee9eeb62990d2..855dcbbcb346e 100644 --- a/compiler/rustc_mir_build/src/build/coverageinfo.rs +++ b/compiler/rustc_mir_build/src/build/coverageinfo.rs @@ -157,6 +157,63 @@ impl BranchInfoBuilder { } impl<'tcx> Builder<'_, 'tcx> { + /// If condition coverage is enabled, inject extra blocks and marker statements + /// that will let us track the value of the condition in `place`. + pub(crate) fn visit_coverage_standalone_condition( + &mut self, + mut expr_id: ExprId, // Expression giving the span of the condition + place: mir::Place<'tcx>, // Already holds the boolean condition value + block: &mut BasicBlock, + ) { + // Bail out if condition coverage is not enabled for this function. + let Some(branch_info) = self.coverage_branch_info.as_mut() else { return }; + if !self.tcx.sess.instrument_coverage_condition() { + return; + }; + + // Remove any wrappers, so that we can inspect the real underlying expression. + while let ExprKind::Use { source: inner } | ExprKind::Scope { value: inner, .. } = + self.thir[expr_id].kind + { + expr_id = inner; + } + // If the expression is a lazy logical op, it will naturally get branch + // coverage as part of its normal lowering, so we can disregard it here. + if let ExprKind::LogicalOp { .. } = self.thir[expr_id].kind { + return; + } + + let source_info = SourceInfo { span: self.thir[expr_id].span, scope: self.source_scope }; + + // Using the boolean value that has already been stored in `place`, set up + // control flow in the shape of a diamond, so that we can place separate + // marker statements in the true and false blocks. The coverage MIR pass + // will use those markers to inject coverage counters as appropriate. + // + // block + // / \ + // true_block false_block + // (marker) (marker) + // \ / + // join_block + + let true_block = self.cfg.start_new_block(); + let false_block = self.cfg.start_new_block(); + self.cfg.terminate( + *block, + source_info, + mir::TerminatorKind::if_(mir::Operand::Copy(place), true_block, false_block), + ); + + branch_info.add_two_way_branch(&mut self.cfg, source_info, true_block, false_block); + + let join_block = self.cfg.start_new_block(); + self.cfg.goto(true_block, source_info, join_block); + self.cfg.goto(false_block, source_info, join_block); + // Any subsequent codegen in the caller should use the new join block. + *block = join_block; + } + /// If branch coverage is enabled, inject marker statements into `then_block` /// and `else_block`, and record their IDs in the table of branch spans. pub(crate) fn visit_coverage_branch_condition( diff --git a/compiler/rustc_mir_build/src/build/expr/into.rs b/compiler/rustc_mir_build/src/build/expr/into.rs index 9349133d2dbc1..c08a3b6691afc 100644 --- a/compiler/rustc_mir_build/src/build/expr/into.rs +++ b/compiler/rustc_mir_build/src/build/expr/into.rs @@ -183,9 +183,13 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { const_: Const::from_bool(this.tcx, constant), }, ); - let rhs = unpack!(this.expr_into_dest(destination, continuation, rhs)); + let mut rhs_block = unpack!(this.expr_into_dest(destination, continuation, rhs)); + // Instrument the lowered RHS's value for condition coverage. + // (Does nothing if condition coverage is not enabled.) + this.visit_coverage_standalone_condition(rhs, destination, &mut rhs_block); + let target = this.cfg.start_new_block(); - this.cfg.goto(rhs, source_info, target); + this.cfg.goto(rhs_block, source_info, target); this.cfg.goto(short_circuit, source_info, target); target.unit() } diff --git a/tests/coverage/condition/conditions.cov-map b/tests/coverage/condition/conditions.cov-map index 6c81f7bda9854..74726e269fc67 100644 --- a/tests/coverage/condition/conditions.cov-map +++ b/tests/coverage/condition/conditions.cov-map @@ -1,89 +1,118 @@ Function name: conditions::assign_3_and_or -Raw bytes (60): 0x[01, 01, 06, 0d, 13, 09, 16, 01, 05, 01, 05, 09, 16, 01, 05, 08, 01, 1c, 01, 00, 2f, 03, 01, 09, 00, 0a, 01, 00, 0d, 00, 0e, 20, 05, 16, 00, 0d, 00, 0e, 05, 00, 12, 00, 13, 20, 0d, 09, 00, 12, 00, 13, 13, 00, 17, 00, 18, 03, 01, 05, 01, 02] +Raw bytes (69): 0x[01, 01, 07, 07, 11, 09, 0d, 01, 05, 05, 09, 16, 1a, 05, 09, 01, 05, 09, 01, 1c, 01, 00, 2f, 03, 01, 09, 00, 0a, 01, 00, 0d, 00, 0e, 20, 05, 1a, 00, 0d, 00, 0e, 05, 00, 12, 00, 13, 20, 09, 16, 00, 12, 00, 13, 13, 00, 17, 00, 18, 20, 0d, 11, 00, 17, 00, 18, 03, 01, 05, 01, 02] Number of files: 1 - file 0 => global file 1 -Number of expressions: 6 -- expression 0 operands: lhs = Counter(3), rhs = Expression(4, Add) -- expression 1 operands: lhs = Counter(2), rhs = Expression(5, Sub) +Number of expressions: 7 +- expression 0 operands: lhs = Expression(1, Add), rhs = Counter(4) +- expression 1 operands: lhs = Counter(2), rhs = Counter(3) - expression 2 operands: lhs = Counter(0), rhs = Counter(1) -- expression 3 operands: lhs = Counter(0), rhs = Counter(1) -- expression 4 operands: lhs = Counter(2), rhs = Expression(5, Sub) -- expression 5 operands: lhs = Counter(0), rhs = Counter(1) -Number of file 0 mappings: 8 +- expression 3 operands: lhs = Counter(1), rhs = Counter(2) +- expression 4 operands: lhs = Expression(5, Sub), rhs = Expression(6, Sub) +- expression 5 operands: lhs = Counter(1), rhs = Counter(2) +- expression 6 operands: lhs = Counter(0), rhs = Counter(1) +Number of file 0 mappings: 9 - Code(Counter(0)) at (prev + 28, 1) to (start + 0, 47) - Code(Expression(0, Add)) at (prev + 1, 9) to (start + 0, 10) - = (c3 + (c2 + (c0 - c1))) + = ((c2 + c3) + c4) - Code(Counter(0)) at (prev + 0, 13) to (start + 0, 14) -- Branch { true: Counter(1), false: Expression(5, Sub) } at (prev + 0, 13) to (start + 0, 14) +- Branch { true: Counter(1), false: Expression(6, Sub) } at (prev + 0, 13) to (start + 0, 14) true = c1 false = (c0 - c1) - Code(Counter(1)) at (prev + 0, 18) to (start + 0, 19) -- Branch { true: Counter(3), false: Counter(2) } at (prev + 0, 18) to (start + 0, 19) - true = c3 - false = c2 +- Branch { true: Counter(2), false: Expression(5, Sub) } at (prev + 0, 18) to (start + 0, 19) + true = c2 + false = (c1 - c2) - Code(Expression(4, Add)) at (prev + 0, 23) to (start + 0, 24) - = (c2 + (c0 - c1)) + = ((c1 - c2) + (c0 - c1)) +- Branch { true: Counter(3), false: Counter(4) } at (prev + 0, 23) to (start + 0, 24) + true = c3 + false = c4 - Code(Expression(0, Add)) at (prev + 1, 5) to (start + 1, 2) - = (c3 + (c2 + (c0 - c1))) + = ((c2 + c3) + c4) Function name: conditions::assign_3_or_and -Raw bytes (56): 0x[01, 01, 04, 05, 07, 09, 0d, 01, 05, 01, 05, 08, 01, 17, 01, 00, 2f, 03, 01, 09, 00, 0a, 01, 00, 0d, 00, 0e, 20, 05, 0e, 00, 0d, 00, 0e, 0e, 00, 12, 00, 13, 20, 09, 0d, 00, 12, 00, 13, 09, 00, 17, 00, 18, 03, 01, 05, 01, 02] +Raw bytes (73): 0x[01, 01, 09, 05, 07, 0b, 11, 09, 0d, 01, 05, 01, 05, 22, 11, 01, 05, 22, 11, 01, 05, 09, 01, 17, 01, 00, 2f, 03, 01, 09, 00, 0a, 01, 00, 0d, 00, 0e, 20, 05, 22, 00, 0d, 00, 0e, 22, 00, 12, 00, 13, 20, 1e, 11, 00, 12, 00, 13, 1e, 00, 17, 00, 18, 20, 09, 0d, 00, 17, 00, 18, 03, 01, 05, 01, 02] Number of files: 1 - file 0 => global file 1 -Number of expressions: 4 +Number of expressions: 9 - expression 0 operands: lhs = Counter(1), rhs = Expression(1, Add) -- expression 1 operands: lhs = Counter(2), rhs = Counter(3) -- expression 2 operands: lhs = Counter(0), rhs = Counter(1) +- expression 1 operands: lhs = Expression(2, Add), rhs = Counter(4) +- expression 2 operands: lhs = Counter(2), rhs = Counter(3) - expression 3 operands: lhs = Counter(0), rhs = Counter(1) -Number of file 0 mappings: 8 +- expression 4 operands: lhs = Counter(0), rhs = Counter(1) +- expression 5 operands: lhs = Expression(8, Sub), rhs = Counter(4) +- expression 6 operands: lhs = Counter(0), rhs = Counter(1) +- expression 7 operands: lhs = Expression(8, Sub), rhs = Counter(4) +- expression 8 operands: lhs = Counter(0), rhs = Counter(1) +Number of file 0 mappings: 9 - Code(Counter(0)) at (prev + 23, 1) to (start + 0, 47) - Code(Expression(0, Add)) at (prev + 1, 9) to (start + 0, 10) - = (c1 + (c2 + c3)) + = (c1 + ((c2 + c3) + c4)) - Code(Counter(0)) at (prev + 0, 13) to (start + 0, 14) -- Branch { true: Counter(1), false: Expression(3, Sub) } at (prev + 0, 13) to (start + 0, 14) +- Branch { true: Counter(1), false: Expression(8, Sub) } at (prev + 0, 13) to (start + 0, 14) true = c1 false = (c0 - c1) -- Code(Expression(3, Sub)) at (prev + 0, 18) to (start + 0, 19) +- Code(Expression(8, Sub)) at (prev + 0, 18) to (start + 0, 19) = (c0 - c1) -- Branch { true: Counter(2), false: Counter(3) } at (prev + 0, 18) to (start + 0, 19) +- Branch { true: Expression(7, Sub), false: Counter(4) } at (prev + 0, 18) to (start + 0, 19) + true = ((c0 - c1) - c4) + false = c4 +- Code(Expression(7, Sub)) at (prev + 0, 23) to (start + 0, 24) + = ((c0 - c1) - c4) +- Branch { true: Counter(2), false: Counter(3) } at (prev + 0, 23) to (start + 0, 24) true = c2 false = c3 -- Code(Counter(2)) at (prev + 0, 23) to (start + 0, 24) - Code(Expression(0, Add)) at (prev + 1, 5) to (start + 1, 2) - = (c1 + (c2 + c3)) + = (c1 + ((c2 + c3) + c4)) Function name: conditions::assign_and -Raw bytes (38): 0x[01, 01, 01, 01, 05, 06, 01, 0d, 01, 00, 21, 01, 01, 09, 00, 0a, 01, 00, 0d, 00, 0e, 20, 05, 02, 00, 0d, 00, 0e, 05, 00, 12, 00, 13, 01, 01, 05, 01, 02] +Raw bytes (51): 0x[01, 01, 04, 07, 0e, 09, 0d, 01, 05, 01, 05, 07, 01, 0d, 01, 00, 21, 03, 01, 09, 00, 0a, 01, 00, 0d, 00, 0e, 20, 05, 0e, 00, 0d, 00, 0e, 05, 00, 12, 00, 13, 20, 09, 0d, 00, 12, 00, 13, 03, 01, 05, 01, 02] Number of files: 1 - file 0 => global file 1 -Number of expressions: 1 -- expression 0 operands: lhs = Counter(0), rhs = Counter(1) -Number of file 0 mappings: 6 +Number of expressions: 4 +- expression 0 operands: lhs = Expression(1, Add), rhs = Expression(3, Sub) +- expression 1 operands: lhs = Counter(2), rhs = Counter(3) +- expression 2 operands: lhs = Counter(0), rhs = Counter(1) +- expression 3 operands: lhs = Counter(0), rhs = Counter(1) +Number of file 0 mappings: 7 - Code(Counter(0)) at (prev + 13, 1) to (start + 0, 33) -- Code(Counter(0)) at (prev + 1, 9) to (start + 0, 10) +- Code(Expression(0, Add)) at (prev + 1, 9) to (start + 0, 10) + = ((c2 + c3) + (c0 - c1)) - Code(Counter(0)) at (prev + 0, 13) to (start + 0, 14) -- Branch { true: Counter(1), false: Expression(0, Sub) } at (prev + 0, 13) to (start + 0, 14) +- Branch { true: Counter(1), false: Expression(3, Sub) } at (prev + 0, 13) to (start + 0, 14) true = c1 false = (c0 - c1) - Code(Counter(1)) at (prev + 0, 18) to (start + 0, 19) -- Code(Counter(0)) at (prev + 1, 5) to (start + 1, 2) +- Branch { true: Counter(2), false: Counter(3) } at (prev + 0, 18) to (start + 0, 19) + true = c2 + false = c3 +- Code(Expression(0, Add)) at (prev + 1, 5) to (start + 1, 2) + = ((c2 + c3) + (c0 - c1)) Function name: conditions::assign_or -Raw bytes (38): 0x[01, 01, 01, 01, 05, 06, 01, 12, 01, 00, 20, 01, 01, 09, 00, 0a, 01, 00, 0d, 00, 0e, 20, 05, 02, 00, 0d, 00, 0e, 02, 00, 12, 00, 13, 01, 01, 05, 01, 02] +Raw bytes (51): 0x[01, 01, 04, 07, 0d, 05, 09, 01, 05, 01, 05, 07, 01, 12, 01, 00, 20, 03, 01, 09, 00, 0a, 01, 00, 0d, 00, 0e, 20, 05, 0e, 00, 0d, 00, 0e, 0e, 00, 12, 00, 13, 20, 09, 0d, 00, 12, 00, 13, 03, 01, 05, 01, 02] Number of files: 1 - file 0 => global file 1 -Number of expressions: 1 -- expression 0 operands: lhs = Counter(0), rhs = Counter(1) -Number of file 0 mappings: 6 +Number of expressions: 4 +- expression 0 operands: lhs = Expression(1, Add), rhs = Counter(3) +- expression 1 operands: lhs = Counter(1), rhs = Counter(2) +- expression 2 operands: lhs = Counter(0), rhs = Counter(1) +- expression 3 operands: lhs = Counter(0), rhs = Counter(1) +Number of file 0 mappings: 7 - Code(Counter(0)) at (prev + 18, 1) to (start + 0, 32) -- Code(Counter(0)) at (prev + 1, 9) to (start + 0, 10) +- Code(Expression(0, Add)) at (prev + 1, 9) to (start + 0, 10) + = ((c1 + c2) + c3) - Code(Counter(0)) at (prev + 0, 13) to (start + 0, 14) -- Branch { true: Counter(1), false: Expression(0, Sub) } at (prev + 0, 13) to (start + 0, 14) +- Branch { true: Counter(1), false: Expression(3, Sub) } at (prev + 0, 13) to (start + 0, 14) true = c1 false = (c0 - c1) -- Code(Expression(0, Sub)) at (prev + 0, 18) to (start + 0, 19) +- Code(Expression(3, Sub)) at (prev + 0, 18) to (start + 0, 19) = (c0 - c1) -- Code(Counter(0)) at (prev + 1, 5) to (start + 1, 2) +- Branch { true: Counter(2), false: Counter(3) } at (prev + 0, 18) to (start + 0, 19) + true = c2 + false = c3 +- Code(Expression(0, Add)) at (prev + 1, 5) to (start + 1, 2) + = ((c1 + c2) + c3) Function name: conditions::foo Raw bytes (9): 0x[01, 01, 00, 01, 01, 21, 01, 02, 02] @@ -94,18 +123,24 @@ Number of file 0 mappings: 1 - Code(Counter(0)) at (prev + 33, 1) to (start + 2, 2) Function name: conditions::func_call -Raw bytes (28): 0x[01, 01, 01, 01, 05, 04, 01, 25, 01, 01, 0a, 20, 05, 02, 01, 09, 00, 0a, 05, 00, 0e, 00, 0f, 01, 01, 01, 00, 02] +Raw bytes (39): 0x[01, 01, 03, 01, 05, 0b, 02, 09, 0d, 05, 01, 25, 01, 01, 0a, 20, 05, 02, 01, 09, 00, 0a, 05, 00, 0e, 00, 0f, 20, 09, 0d, 00, 0e, 00, 0f, 07, 01, 01, 00, 02] Number of files: 1 - file 0 => global file 1 -Number of expressions: 1 +Number of expressions: 3 - expression 0 operands: lhs = Counter(0), rhs = Counter(1) -Number of file 0 mappings: 4 +- expression 1 operands: lhs = Expression(2, Add), rhs = Expression(0, Sub) +- expression 2 operands: lhs = Counter(2), rhs = Counter(3) +Number of file 0 mappings: 5 - Code(Counter(0)) at (prev + 37, 1) to (start + 1, 10) - Branch { true: Counter(1), false: Expression(0, Sub) } at (prev + 1, 9) to (start + 0, 10) true = c1 false = (c0 - c1) - Code(Counter(1)) at (prev + 0, 14) to (start + 0, 15) -- Code(Counter(0)) at (prev + 1, 1) to (start + 0, 2) +- Branch { true: Counter(2), false: Counter(3) } at (prev + 0, 14) to (start + 0, 15) + true = c2 + false = c3 +- Code(Expression(1, Add)) at (prev + 1, 1) to (start + 0, 2) + = ((c2 + c3) + (c0 - c1)) Function name: conditions::simple_assign Raw bytes (9): 0x[01, 01, 00, 01, 01, 08, 01, 03, 02] diff --git a/tests/coverage/condition/conditions.coverage b/tests/coverage/condition/conditions.coverage index a71ab8e5d8943..3215b391d622e 100644 --- a/tests/coverage/condition/conditions.coverage +++ b/tests/coverage/condition/conditions.coverage @@ -15,6 +15,7 @@ ^2 ------------------ | Branch (LL:13): [True: 2, False: 1] + | Branch (LL:18): [True: 1, False: 1] ------------------ LL| 3| black_box(x); LL| 3|} @@ -24,6 +25,7 @@ ^1 ------------------ | Branch (LL:13): [True: 2, False: 1] + | Branch (LL:18): [True: 0, False: 1] ------------------ LL| 3| black_box(x); LL| 3|} @@ -34,6 +36,7 @@ ------------------ | Branch (LL:13): [True: 2, False: 2] | Branch (LL:18): [True: 1, False: 1] + | Branch (LL:23): [True: 1, False: 0] ------------------ LL| 4| black_box(x); LL| 4|} @@ -44,6 +47,7 @@ ------------------ | Branch (LL:13): [True: 2, False: 2] | Branch (LL:18): [True: 1, False: 1] + | Branch (LL:23): [True: 2, False: 1] ------------------ LL| 4| black_box(x); LL| 4|} @@ -57,6 +61,7 @@ ^2 ------------------ | Branch (LL:9): [True: 2, False: 1] + | Branch (LL:14): [True: 1, False: 1] ------------------ LL| 3|} LL| | From 4b96e44ebbf42dc8b6f3e5ac4894739adc17d826 Mon Sep 17 00:00:00 2001 From: Scott McMurray Date: Thu, 30 May 2024 22:05:30 -0700 Subject: [PATCH 08/16] Also InstSimplify `&raw*` We do this for `&*` and `&mut*` already; might as well do it for raw pointers too. --- .../rustc_mir_transform/src/instsimplify.rs | 2 +- .../ref_of_deref.pointers.InstSimplify.diff | 58 +++++++++++++++++++ .../ref_of_deref.references.InstSimplify.diff | 58 +++++++++++++++++++ tests/mir-opt/instsimplify/ref_of_deref.rs | 40 +++++++++++++ 4 files changed, 157 insertions(+), 1 deletion(-) create mode 100644 tests/mir-opt/instsimplify/ref_of_deref.pointers.InstSimplify.diff create mode 100644 tests/mir-opt/instsimplify/ref_of_deref.references.InstSimplify.diff create mode 100644 tests/mir-opt/instsimplify/ref_of_deref.rs diff --git a/compiler/rustc_mir_transform/src/instsimplify.rs b/compiler/rustc_mir_transform/src/instsimplify.rs index a54332b6f2571..40db3e38fd32b 100644 --- a/compiler/rustc_mir_transform/src/instsimplify.rs +++ b/compiler/rustc_mir_transform/src/instsimplify.rs @@ -123,7 +123,7 @@ impl<'tcx> InstSimplifyContext<'tcx, '_> { /// Transform `&(*a)` ==> `a`. fn simplify_ref_deref(&self, source_info: &SourceInfo, rvalue: &mut Rvalue<'tcx>) { - if let Rvalue::Ref(_, _, place) = rvalue { + if let Rvalue::Ref(_, _, place) | Rvalue::AddressOf(_, place) = rvalue { if let Some((base, ProjectionElem::Deref)) = place.as_ref().last_projection() { if rvalue.ty(self.local_decls, self.tcx) != base.ty(self.local_decls, self.tcx).ty { return; diff --git a/tests/mir-opt/instsimplify/ref_of_deref.pointers.InstSimplify.diff b/tests/mir-opt/instsimplify/ref_of_deref.pointers.InstSimplify.diff new file mode 100644 index 0000000000000..52b3d1e1d40b3 --- /dev/null +++ b/tests/mir-opt/instsimplify/ref_of_deref.pointers.InstSimplify.diff @@ -0,0 +1,58 @@ +- // MIR for `pointers` before InstSimplify ++ // MIR for `pointers` after InstSimplify + + fn pointers(_1: *const [i32], _2: *mut i32) -> () { + debug const_ptr => _1; + debug mut_ptr => _2; + let mut _0: (); + let _3: &[i32]; + scope 1 { + debug _a => _3; + let _4: &i32; + scope 2 { + debug _b => _4; + let _5: &mut i32; + scope 3 { + debug _c => _5; + let _6: *const [i32]; + scope 4 { + debug _d => _6; + let _7: *const i32; + scope 5 { + debug _e => _7; + let _8: *mut i32; + scope 6 { + debug _f => _8; + } + } + } + } + } + } + + bb0: { + StorageLive(_3); + _3 = &(*_1); + StorageLive(_4); + _4 = &(*_2); + StorageLive(_5); + _5 = &mut (*_2); + StorageLive(_6); +- _6 = &raw const (*_1); ++ _6 = _1; + StorageLive(_7); + _7 = &raw const (*_2); + StorageLive(_8); +- _8 = &raw mut (*_2); ++ _8 = _2; + _0 = const (); + StorageDead(_8); + StorageDead(_7); + StorageDead(_6); + StorageDead(_5); + StorageDead(_4); + StorageDead(_3); + return; + } + } + diff --git a/tests/mir-opt/instsimplify/ref_of_deref.references.InstSimplify.diff b/tests/mir-opt/instsimplify/ref_of_deref.references.InstSimplify.diff new file mode 100644 index 0000000000000..ca0828a225a01 --- /dev/null +++ b/tests/mir-opt/instsimplify/ref_of_deref.references.InstSimplify.diff @@ -0,0 +1,58 @@ +- // MIR for `references` before InstSimplify ++ // MIR for `references` after InstSimplify + + fn references(_1: &i32, _2: &mut [i32]) -> () { + debug const_ref => _1; + debug mut_ref => _2; + let mut _0: (); + let _3: &i32; + scope 1 { + debug _a => _3; + let _4: &[i32]; + scope 2 { + debug _b => _4; + let _5: &mut [i32]; + scope 3 { + debug _c => _5; + let _6: *const i32; + scope 4 { + debug _d => _6; + let _7: *const [i32]; + scope 5 { + debug _e => _7; + let _8: *mut [i32]; + scope 6 { + debug _f => _8; + } + } + } + } + } + } + + bb0: { + StorageLive(_3); +- _3 = &(*_1); ++ _3 = _1; + StorageLive(_4); + _4 = &(*_2); + StorageLive(_5); +- _5 = &mut (*_2); ++ _5 = _2; + StorageLive(_6); + _6 = &raw const (*_1); + StorageLive(_7); + _7 = &raw const (*_2); + StorageLive(_8); + _8 = &raw mut (*_2); + _0 = const (); + StorageDead(_8); + StorageDead(_7); + StorageDead(_6); + StorageDead(_5); + StorageDead(_4); + StorageDead(_3); + return; + } + } + diff --git a/tests/mir-opt/instsimplify/ref_of_deref.rs b/tests/mir-opt/instsimplify/ref_of_deref.rs new file mode 100644 index 0000000000000..37e164bc17f18 --- /dev/null +++ b/tests/mir-opt/instsimplify/ref_of_deref.rs @@ -0,0 +1,40 @@ +//@ test-mir-pass: InstSimplify +#![crate_type = "lib"] +#![feature(raw_ref_op)] + +// For each of these, only 2 of the 6 should simplify, +// as the others have the wrong types. + +// EMIT_MIR ref_of_deref.references.InstSimplify.diff +// CHECK-LABEL: references +pub fn references(const_ref: &i32, mut_ref: &mut [i32]) { + // CHECK: _3 = _1; + let _a = &*const_ref; + // CHECK: _4 = &(*_2); + let _b = &*mut_ref; + // CHECK: _5 = _2; + let _c = &mut *mut_ref; + // CHECK: _6 = &raw const (*_1); + let _d = &raw const *const_ref; + // CHECK: _7 = &raw const (*_2); + let _e = &raw const *mut_ref; + // CHECK: _8 = &raw mut (*_2); + let _f = &raw mut *mut_ref; +} + +// EMIT_MIR ref_of_deref.pointers.InstSimplify.diff +// CHECK-LABEL: pointers +pub unsafe fn pointers(const_ptr: *const [i32], mut_ptr: *mut i32) { + // CHECK: _3 = &(*_1); + let _a = &*const_ptr; + // CHECK: _4 = &(*_2); + let _b = &*mut_ptr; + // CHECK: _5 = &mut (*_2); + let _c = &mut *mut_ptr; + // CHECK: _6 = _1; + let _d = &raw const *const_ptr; + // CHECK: _7 = &raw const (*_2); + let _e = &raw const *mut_ptr; + // CHECK: _8 = _2; + let _f = &raw mut *mut_ptr; +} From be572a8010cd3223896afb09d84dab95e3e570fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=AE=B8=E6=9D=B0=E5=8F=8B=20Jieyou=20Xu=20=28Joe=29?= Date: Thu, 30 May 2024 00:50:39 +0000 Subject: [PATCH 09/16] run-make-support: add `#[must_use]` and drop bombs for command wrappers Especially for command wrappers like `Rustc`, it's very easy to build up a command invocation but forget to actually execute it, e.g. by using `run()`. This commit adds "drop bombs" to command wrappers, which are armed on command wrapper construction, and only defused if the command is executed (through `run`, `run_fail` or `run_fail_assert_exit_code`). If the test writer forgets to execute the command, the drop bomb will "explode" and panic with an error message. This is so that tests don't silently pass with constructed-but-not-executed command wrappers. We don't add `#[must_use]` for command wrapper helper methods because they return `&mut Self` and can be annoying e.g. if a helper method is conditionally called, such as ``` if condition { cmd.arg("-Cprefer-dynamic"); // <- unused_must_use fires } cmd.run(); // <- even though cmd is eventually executed ``` We also add `#[must_use]` attributes to functions in the support library where suitable to help catch unintentionally discarded values. --- src/tools/run-make-support/src/cc.rs | 7 ++- src/tools/run-make-support/src/clang.rs | 13 ++++- src/tools/run-make-support/src/diff/mod.rs | 10 +++- .../run-make-support/src/drop_bomb/mod.rs | 37 ++++++++++++ .../run-make-support/src/drop_bomb/tests.rs | 15 +++++ src/tools/run-make-support/src/lib.rs | 57 +++++++++++++++---- .../run-make-support/src/llvm_readobj.rs | 5 +- src/tools/run-make-support/src/rustc.rs | 28 ++++----- src/tools/run-make-support/src/rustdoc.rs | 29 ++++------ 9 files changed, 149 insertions(+), 52 deletions(-) create mode 100644 src/tools/run-make-support/src/drop_bomb/mod.rs create mode 100644 src/tools/run-make-support/src/drop_bomb/tests.rs diff --git a/src/tools/run-make-support/src/cc.rs b/src/tools/run-make-support/src/cc.rs index 799c36b104999..f0551f054ef91 100644 --- a/src/tools/run-make-support/src/cc.rs +++ b/src/tools/run-make-support/src/cc.rs @@ -2,6 +2,7 @@ use std::env; use std::path::Path; use std::process::Command; +use crate::drop_bomb::DropBomb; use crate::{bin_name, cygpath_windows, handle_failed_output, is_msvc, is_windows, tmp_dir, uname}; /// Construct a new platform-specific C compiler invocation. @@ -14,9 +15,11 @@ pub fn cc() -> Cc { /// A platform-specific C compiler invocation builder. The specific C compiler used is /// passed down from compiletest. +#[must_use] #[derive(Debug)] pub struct Cc { cmd: Command, + drop_bomb: DropBomb, } crate::impl_common_helpers!(Cc); @@ -36,7 +39,7 @@ impl Cc { cmd.arg(flag); } - Self { cmd } + Self { cmd, drop_bomb: DropBomb::arm("cc invocation must be executed") } } /// Specify path of the input file. @@ -87,6 +90,7 @@ impl Cc { } /// `EXTRACFLAGS` +#[must_use] pub fn extra_c_flags() -> Vec<&'static str> { // Adapted from tools.mk (trimmed): // @@ -145,6 +149,7 @@ pub fn extra_c_flags() -> Vec<&'static str> { } /// `EXTRACXXFLAGS` +#[must_use] pub fn extra_cxx_flags() -> Vec<&'static str> { // Adapted from tools.mk (trimmed): // diff --git a/src/tools/run-make-support/src/clang.rs b/src/tools/run-make-support/src/clang.rs index 6ccce67b250db..53dd8a3d6aaec 100644 --- a/src/tools/run-make-support/src/clang.rs +++ b/src/tools/run-make-support/src/clang.rs @@ -2,6 +2,7 @@ use std::env; use std::path::Path; use std::process::Command; +use crate::drop_bomb::DropBomb; use crate::{bin_name, handle_failed_output, tmp_dir}; /// Construct a new `clang` invocation. `clang` is not always available for all targets. @@ -10,23 +11,27 @@ pub fn clang() -> Clang { } /// A `clang` invocation builder. +#[must_use] #[derive(Debug)] pub struct Clang { cmd: Command, + drop_bomb: DropBomb, } crate::impl_common_helpers!(Clang); impl Clang { /// Construct a new `clang` invocation. `clang` is not always available for all targets. + #[must_use] pub fn new() -> Self { let clang = env::var("CLANG").expect("`CLANG` not specified, but this is required to find `clang`"); let cmd = Command::new(clang); - Self { cmd } + Self { cmd, drop_bomb: DropBomb::arm("clang invocation must be executed") } } /// Provide an input file. + #[must_use] pub fn input>(&mut self, path: P) -> &mut Self { self.cmd.arg(path.as_ref()); self @@ -34,6 +39,7 @@ impl Clang { /// Specify the name of the executable. The executable will be placed under `$TMPDIR`, and the /// extension will be determined by [`bin_name`]. + #[must_use] pub fn out_exe(&mut self, name: &str) -> &mut Self { self.cmd.arg("-o"); self.cmd.arg(tmp_dir().join(bin_name(name))); @@ -41,6 +47,7 @@ impl Clang { } /// Specify which target triple clang should target. + #[must_use] pub fn target(&mut self, target_triple: &str) -> &mut Self { self.cmd.arg("-target"); self.cmd.arg(target_triple); @@ -48,24 +55,28 @@ impl Clang { } /// Pass `-nostdlib` to disable linking the C standard library. + #[must_use] pub fn no_stdlib(&mut self) -> &mut Self { self.cmd.arg("-nostdlib"); self } /// Specify architecture. + #[must_use] pub fn arch(&mut self, arch: &str) -> &mut Self { self.cmd.arg(format!("-march={arch}")); self } /// Specify LTO settings. + #[must_use] pub fn lto(&mut self, lto: &str) -> &mut Self { self.cmd.arg(format!("-flto={lto}")); self } /// Specify which ld to use. + #[must_use] pub fn use_ld(&mut self, ld: &str) -> &mut Self { self.cmd.arg(format!("-fuse-ld={ld}")); self diff --git a/src/tools/run-make-support/src/diff/mod.rs b/src/tools/run-make-support/src/diff/mod.rs index d864ddf4eb175..05b93b573c18d 100644 --- a/src/tools/run-make-support/src/diff/mod.rs +++ b/src/tools/run-make-support/src/diff/mod.rs @@ -2,6 +2,8 @@ use regex::Regex; use similar::TextDiff; use std::path::Path; +use crate::drop_bomb::DropBomb; + #[cfg(test)] mod tests; @@ -9,6 +11,7 @@ pub fn diff() -> Diff { Diff::new() } +#[must_use] #[derive(Debug)] pub struct Diff { expected: Option, @@ -16,10 +19,12 @@ pub struct Diff { actual: Option, actual_name: Option, normalizers: Vec<(String, String)>, + drop_bomb: DropBomb, } impl Diff { /// Construct a bare `diff` invocation. + #[must_use] pub fn new() -> Self { Self { expected: None, @@ -27,6 +32,7 @@ impl Diff { actual: None, actual_name: None, normalizers: Vec::new(), + drop_bomb: DropBomb::arm("diff invocation must be executed"), } } @@ -79,9 +85,9 @@ impl Diff { self } - /// Executes the diff process, prints any differences to the standard error. #[track_caller] - pub fn run(&self) { + pub fn run(&mut self) { + self.drop_bomb.defuse(); let expected = self.expected.as_ref().expect("expected text not set"); let mut actual = self.actual.as_ref().expect("actual text not set").to_string(); let expected_name = self.expected_name.as_ref().unwrap(); diff --git a/src/tools/run-make-support/src/drop_bomb/mod.rs b/src/tools/run-make-support/src/drop_bomb/mod.rs new file mode 100644 index 0000000000000..57dba70f2631f --- /dev/null +++ b/src/tools/run-make-support/src/drop_bomb/mod.rs @@ -0,0 +1,37 @@ +//! This module implements "drop bombs" intended for use by command wrappers to ensure that the +//! constructed commands are *eventually* executed. This is exactly like `rustc_errors::Diag` +//! where we force every `Diag` to be consumed or we emit a bug, but we panic instead. +//! +//! This is inspired by . + +use std::borrow::Cow; + +#[cfg(test)] +mod tests; + +#[derive(Debug)] +pub(crate) struct DropBomb { + msg: Cow<'static, str>, + defused: bool, +} + +impl DropBomb { + /// Arm a [`DropBomb`]. If the value is dropped without being [`defused`][Self::defused], then + /// it will panic. + pub(crate) fn arm>>(message: S) -> DropBomb { + DropBomb { msg: message.into(), defused: false } + } + + /// Defuse the [`DropBomb`]. This will prevent the drop bomb from panicking when dropped. + pub(crate) fn defuse(&mut self) { + self.defused = true; + } +} + +impl Drop for DropBomb { + fn drop(&mut self) { + if !self.defused && !std::thread::panicking() { + panic!("{}", self.msg) + } + } +} diff --git a/src/tools/run-make-support/src/drop_bomb/tests.rs b/src/tools/run-make-support/src/drop_bomb/tests.rs new file mode 100644 index 0000000000000..4a488c0f67080 --- /dev/null +++ b/src/tools/run-make-support/src/drop_bomb/tests.rs @@ -0,0 +1,15 @@ +use super::DropBomb; + +#[test] +#[should_panic] +fn test_arm() { + let bomb = DropBomb::arm("hi :3"); + drop(bomb); // <- armed bomb should explode when not defused +} + +#[test] +fn test_defuse() { + let mut bomb = DropBomb::arm("hi :3"); + bomb.defuse(); + drop(bomb); // <- defused bomb should not explode +} diff --git a/src/tools/run-make-support/src/lib.rs b/src/tools/run-make-support/src/lib.rs index 0cf64db6ac9c0..0ef7b7ae6a551 100644 --- a/src/tools/run-make-support/src/lib.rs +++ b/src/tools/run-make-support/src/lib.rs @@ -6,6 +6,7 @@ pub mod cc; pub mod clang; pub mod diff; +mod drop_bomb; pub mod llvm_readobj; pub mod run; pub mod rustc; @@ -31,52 +32,62 @@ pub use rustc::{aux_build, rustc, Rustc}; pub use rustdoc::{bare_rustdoc, rustdoc, Rustdoc}; /// Path of `TMPDIR` (a temporary build directory, not under `/tmp`). +#[must_use] pub fn tmp_dir() -> PathBuf { env::var_os("TMPDIR").unwrap().into() } /// `TARGET` +#[must_use] pub fn target() -> String { env::var("TARGET").unwrap() } /// Check if target is windows-like. +#[must_use] pub fn is_windows() -> bool { target().contains("windows") } /// Check if target uses msvc. +#[must_use] pub fn is_msvc() -> bool { target().contains("msvc") } /// Check if target uses macOS. +#[must_use] pub fn is_darwin() -> bool { target().contains("darwin") } /// Construct a path to a static library under `$TMPDIR` given the library name. This will return a /// path with `$TMPDIR` joined with platform-and-compiler-specific library name. +#[must_use] pub fn static_lib(name: &str) -> PathBuf { tmp_dir().join(static_lib_name(name)) } +#[must_use] pub fn python_command() -> Command { let python_path = std::env::var("PYTHON").expect("PYTHON environment variable does not exist"); Command::new(python_path) } +#[must_use] pub fn htmldocck() -> Command { let mut python = python_command(); python.arg(source_path().join("src/etc/htmldocck.py")); python } +#[must_use] pub fn source_path() -> PathBuf { std::env::var("S").expect("S variable does not exist").into() } /// Construct the static library name based on the platform. +#[must_use] pub fn static_lib_name(name: &str) -> String { // See tools.mk (irrelevant lines omitted): // @@ -102,11 +113,13 @@ pub fn static_lib_name(name: &str) -> String { /// Construct a path to a dynamic library under `$TMPDIR` given the library name. This will return a /// path with `$TMPDIR` joined with platform-and-compiler-specific library name. +#[must_use] pub fn dynamic_lib(name: &str) -> PathBuf { tmp_dir().join(dynamic_lib_name(name)) } /// Construct the dynamic library name based on the platform. +#[must_use] pub fn dynamic_lib_name(name: &str) -> String { // See tools.mk (irrelevant lines omitted): // @@ -134,6 +147,7 @@ pub fn dynamic_lib_name(name: &str) -> String { /// Construct a path to a rust library (rlib) under `$TMPDIR` given the library name. This will return a /// path with `$TMPDIR` joined with the library name. +#[must_use] pub fn rust_lib(name: &str) -> PathBuf { tmp_dir().join(rust_lib_name(name)) } @@ -145,12 +159,14 @@ pub fn rust_lib_name(name: &str) -> String { } /// Construct the binary name based on platform. +#[must_use] pub fn bin_name(name: &str) -> String { if is_windows() { format!("{name}.exe") } else { name.to_string() } } /// Use `cygpath -w` on a path to get a Windows path string back. This assumes that `cygpath` is /// available on the platform! +#[must_use] #[track_caller] pub fn cygpath_windows>(path: P) -> String { let caller_location = std::panic::Location::caller(); @@ -169,6 +185,7 @@ pub fn cygpath_windows>(path: P) -> String { } /// Run `uname`. This assumes that `uname` is available on the platform! +#[must_use] #[track_caller] pub fn uname() -> String { let caller_location = std::panic::Location::caller(); @@ -285,19 +302,21 @@ pub fn assert_not_contains(haystack: &str, needle: &str) { } } +// FIXME(jieyouxu): convert this macro to become a command wrapper trait. /// Implement common helpers for command wrappers. This assumes that the command wrapper is a struct -/// containing a `cmd: Command` field and a `output` function. The provided helpers are: +/// containing a `cmd: Command` field, a `command_output()` function, and a `drop_bomb` field. The +/// provided helpers can be grouped into a few categories: /// /// 1. Generic argument acceptors: `arg` and `args` (delegated to [`Command`]). These are intended /// to be *fallback* argument acceptors, when specific helpers don't make sense. Prefer to add /// new specific helper methods over relying on these generic argument providers. /// 2. Environment manipulation methods: `env`, `env_remove` and `env_clear`: these delegate to /// methods of the same name on [`Command`]. -/// 3. Output and execution: `output`, `run` and `run_fail` are provided. `output` waits for the -/// command to finish running and returns the process's [`Output`]. `run` and `run_fail` are -/// higher-level convenience methods which waits for the command to finish running and assert -/// that the command successfully ran or failed as expected. Prefer `run` and `run_fail` when -/// possible. +/// 3. Output and execution: `run`, `run_fail` and `run_fail_assert_exit_code` are provided. `run*` +/// are higher-level convenience methods which waits for the command to finish running and assert +/// that the command successfully ran or failed as expected. +/// +/// `command_output()` should almost never be used in test code. /// /// Example usage: /// @@ -378,9 +397,16 @@ macro_rules! impl_common_helpers { self } + /// Set the path where the command will be run. + pub fn current_dir>(&mut self, path: P) -> &mut Self { + self.cmd.current_dir(path); + self + } + /// Run the constructed command and assert that it is successfully run. #[track_caller] pub fn run(&mut self) -> ::std::process::Output { + self.drop_bomb.defuse(); let caller_location = ::std::panic::Location::caller(); let caller_line_number = caller_location.line(); @@ -394,6 +420,7 @@ macro_rules! impl_common_helpers { /// Run the constructed command and assert that it does not successfully run. #[track_caller] pub fn run_fail(&mut self) -> ::std::process::Output { + self.drop_bomb.defuse(); let caller_location = ::std::panic::Location::caller(); let caller_line_number = caller_location.line(); @@ -404,10 +431,20 @@ macro_rules! impl_common_helpers { output } - /// Set the path where the command will be run. - pub fn current_dir>(&mut self, path: P) -> &mut Self { - self.cmd.current_dir(path); - self + /// Run the constructed command and assert that it does not successfully run and + /// exits with the expected exit code. + // FIXME(jieyouxu): we should probably *always* assert the expected exit code? + #[track_caller] + pub fn run_fail_assert_exit_code(&mut self, code: i32) -> ::std::process::Output { + self.drop_bomb.defuse(); + let caller_location = ::std::panic::Location::caller(); + let caller_line_number = caller_location.line(); + + let output = self.command_output(); + if output.status.code().unwrap() != code { + handle_failed_output(&self.cmd, output, caller_line_number); + } + output } } }; diff --git a/src/tools/run-make-support/src/llvm_readobj.rs b/src/tools/run-make-support/src/llvm_readobj.rs index f114aacfa3fc7..9460fa12d2240 100644 --- a/src/tools/run-make-support/src/llvm_readobj.rs +++ b/src/tools/run-make-support/src/llvm_readobj.rs @@ -2,6 +2,7 @@ use std::env; use std::path::{Path, PathBuf}; use std::process::Command; +use crate::drop_bomb::DropBomb; use crate::handle_failed_output; /// Construct a new `llvm-readobj` invocation. This assumes that `llvm-readobj` is available @@ -11,9 +12,11 @@ pub fn llvm_readobj() -> LlvmReadobj { } /// A `llvm-readobj` invocation builder. +#[must_use] #[derive(Debug)] pub struct LlvmReadobj { cmd: Command, + drop_bomb: DropBomb, } crate::impl_common_helpers!(LlvmReadobj); @@ -27,7 +30,7 @@ impl LlvmReadobj { let llvm_bin_dir = PathBuf::from(llvm_bin_dir); let llvm_readobj = llvm_bin_dir.join("llvm-readobj"); let cmd = Command::new(llvm_readobj); - Self { cmd } + Self { cmd, drop_bomb: DropBomb::arm("llvm-readobj invocation must be executed") } } /// Provide an input file. diff --git a/src/tools/run-make-support/src/rustc.rs b/src/tools/run-make-support/src/rustc.rs index 8b0252b8f04c4..840ff1aef7afe 100644 --- a/src/tools/run-make-support/src/rustc.rs +++ b/src/tools/run-make-support/src/rustc.rs @@ -4,6 +4,7 @@ use std::io::Write; use std::path::Path; use std::process::{Command, Output, Stdio}; +use crate::drop_bomb::DropBomb; use crate::{handle_failed_output, set_host_rpath, tmp_dir}; /// Construct a new `rustc` invocation. @@ -17,20 +18,23 @@ pub fn aux_build() -> Rustc { } /// A `rustc` invocation builder. +#[must_use] #[derive(Debug)] pub struct Rustc { cmd: Command, stdin: Option>, + drop_bomb: DropBomb, } crate::impl_common_helpers!(Rustc); -fn setup_common() -> Command { +fn setup_common() -> (Command, DropBomb) { let rustc = env::var("RUSTC").unwrap(); let mut cmd = Command::new(rustc); set_host_rpath(&mut cmd); cmd.arg("--out-dir").arg(tmp_dir()).arg("-L").arg(tmp_dir()); - cmd + let drop_bomb = DropBomb::arm("rustc invocation must be executed"); + (cmd, drop_bomb) } impl Rustc { @@ -38,15 +42,15 @@ impl Rustc { /// Construct a new `rustc` invocation. pub fn new() -> Self { - let cmd = setup_common(); - Self { cmd, stdin: None } + let (cmd, drop_bomb) = setup_common(); + Self { cmd, stdin: None, drop_bomb } } /// Construct a new `rustc` invocation with `aux_build` preset (setting `--crate-type=lib`). pub fn new_aux_build() -> Self { - let mut cmd = setup_common(); + let (mut cmd, drop_bomb) = setup_common(); cmd.arg("--crate-type=lib"); - Self { cmd, stdin: None } + Self { cmd, stdin: None, drop_bomb } } // Argument provider methods @@ -230,16 +234,4 @@ impl Rustc { self.cmd.output().expect("failed to get output of finished process") } } - - #[track_caller] - pub fn run_fail_assert_exit_code(&mut self, code: i32) -> Output { - let caller_location = std::panic::Location::caller(); - let caller_line_number = caller_location.line(); - - let output = self.command_output(); - if output.status.code().unwrap() != code { - handle_failed_output(&self.cmd, output, caller_line_number); - } - output - } } diff --git a/src/tools/run-make-support/src/rustdoc.rs b/src/tools/run-make-support/src/rustdoc.rs index 61d7448a6bfe7..69925287b3e6e 100644 --- a/src/tools/run-make-support/src/rustdoc.rs +++ b/src/tools/run-make-support/src/rustdoc.rs @@ -4,6 +4,7 @@ use std::io::Write; use std::path::Path; use std::process::{Command, Output, Stdio}; +use crate::drop_bomb::DropBomb; use crate::{handle_failed_output, set_host_rpath}; /// Construct a plain `rustdoc` invocation with no flags set. @@ -16,34 +17,36 @@ pub fn rustdoc() -> Rustdoc { Rustdoc::new() } +#[must_use] #[derive(Debug)] pub struct Rustdoc { cmd: Command, stdin: Option>, + drop_bomb: DropBomb, } crate::impl_common_helpers!(Rustdoc); -fn setup_common() -> Command { +fn setup_common() -> (Command, DropBomb) { let rustdoc = env::var("RUSTDOC").unwrap(); let mut cmd = Command::new(rustdoc); set_host_rpath(&mut cmd); - cmd + (cmd, DropBomb::arm("rustdoc invocation must be executed")) } impl Rustdoc { /// Construct a bare `rustdoc` invocation. pub fn bare() -> Self { - let cmd = setup_common(); - Self { cmd, stdin: None } + let (cmd, drop_bomb) = setup_common(); + Self { cmd, stdin: None, drop_bomb } } /// Construct a `rustdoc` invocation with `-L $(TARGET_RPATH_DIR)` set. pub fn new() -> Self { - let mut cmd = setup_common(); + let (mut cmd, drop_bomb) = setup_common(); let target_rpath_dir = env::var_os("TARGET_RPATH_DIR").unwrap(); cmd.arg(format!("-L{}", target_rpath_dir.to_string_lossy())); - Self { cmd, stdin: None } + Self { cmd, stdin: None, drop_bomb } } /// Specify where an external library is located. @@ -96,7 +99,7 @@ impl Rustdoc { /// Get the [`Output`] of the finished process. #[track_caller] - pub fn command_output(&mut self) -> ::std::process::Output { + pub fn command_output(&mut self) -> Output { // let's make sure we piped all the input and outputs self.cmd.stdin(Stdio::piped()); self.cmd.stdout(Stdio::piped()); @@ -157,16 +160,4 @@ impl Rustdoc { self.cmd.arg(format); self } - - #[track_caller] - pub fn run_fail_assert_exit_code(&mut self, code: i32) -> Output { - let caller_location = std::panic::Location::caller(); - let caller_line_number = caller_location.line(); - - let output = self.command_output(); - if output.status.code().unwrap() != code { - handle_failed_output(&self.cmd, output, caller_line_number); - } - output - } } From 288c2deeeaadaf2d6c9c2af96feca711181146fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=AE=B8=E6=9D=B0=E5=8F=8B=20Jieyou=20Xu=20=28Joe=29?= Date: Thu, 30 May 2024 00:59:32 +0000 Subject: [PATCH 10/16] compiletest: compile rmake.rs with -Dunused_must_use --- src/tools/compiletest/src/runtest.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/tools/compiletest/src/runtest.rs b/src/tools/compiletest/src/runtest.rs index 4ea12a0f9e480..f1bc6d0982570 100644 --- a/src/tools/compiletest/src/runtest.rs +++ b/src/tools/compiletest/src/runtest.rs @@ -3507,6 +3507,10 @@ impl<'test> TestCx<'test> { .env_remove("MFLAGS") .env_remove("CARGO_MAKEFLAGS"); + // In test code we want to be very pedantic about values being silently discarded that are + // annotated with `#[must_use]`. + cmd.arg("-Dunused_must_use"); + if std::env::var_os("COMPILETEST_FORCE_STAGE0").is_some() { let mut stage0_sysroot = build_root.clone(); stage0_sysroot.push("stage0-sysroot"); From a77a5f440ca92d3400290273aa3774354d483b68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=AE=B8=E6=9D=B0=E5=8F=8B=20Jieyou=20Xu=20=28Joe=29?= Date: Thu, 30 May 2024 03:14:55 +0000 Subject: [PATCH 11/16] run-make-support: extract rust_lib_name helper --- src/tools/run-make-support/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/tools/run-make-support/src/lib.rs b/src/tools/run-make-support/src/lib.rs index 0ef7b7ae6a551..50776b2d1ea8c 100644 --- a/src/tools/run-make-support/src/lib.rs +++ b/src/tools/run-make-support/src/lib.rs @@ -145,8 +145,8 @@ pub fn dynamic_lib_name(name: &str) -> String { } } -/// Construct a path to a rust library (rlib) under `$TMPDIR` given the library name. This will return a -/// path with `$TMPDIR` joined with the library name. +/// Construct a path to a rust library (rlib) under `$TMPDIR` given the library name. This will +/// return a path with `$TMPDIR` joined with the library name. #[must_use] pub fn rust_lib(name: &str) -> PathBuf { tmp_dir().join(rust_lib_name(name)) From 7867fb75a377fb7105fc0679e50a05add842ba25 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=AE=B8=E6=9D=B0=E5=8F=8B=20Jieyou=20Xu=20=28Joe=29?= Date: Thu, 30 May 2024 02:40:27 +0000 Subject: [PATCH 12/16] tests/run-make: fix unused_must_use issues and other test failures --- tests/run-make/CURRENT_RUSTC_VERSION/rmake.rs | 3 +-- .../c-link-to-rust-staticlib/rmake.rs | 2 +- .../run-make/doctests-keep-binaries/rmake.rs | 4 ++-- tests/run-make/doctests-runtool/rmake.rs | 4 ++-- tests/run-make/reset-codegen-1/rmake.rs | 5 +++-- tests/run-make/rustdoc-error-lines/rmake.rs | 5 ++--- .../rustdoc-scrape-examples-macros/rmake.rs | 19 ++++++++----------- tests/run-make/rustdoc-shared-flags/rmake.rs | 4 ++-- .../same-lib-two-locations-no-panic/rmake.rs | 12 ++++++------ 9 files changed, 27 insertions(+), 31 deletions(-) diff --git a/tests/run-make/CURRENT_RUSTC_VERSION/rmake.rs b/tests/run-make/CURRENT_RUSTC_VERSION/rmake.rs index 1bdb634757120..f30f59d818690 100644 --- a/tests/run-make/CURRENT_RUSTC_VERSION/rmake.rs +++ b/tests/run-make/CURRENT_RUSTC_VERSION/rmake.rs @@ -13,8 +13,7 @@ fn main() { let mut stable_path = PathBuf::from(env!("TMPDIR")); stable_path.push("libstable.rmeta"); - let output = - rustc().input("main.rs").emit("metadata").extern_("stable", &stable_path).command_output(); + let output = rustc().input("main.rs").emit("metadata").extern_("stable", &stable_path).run(); let stderr = String::from_utf8_lossy(&output.stderr); let version = include_str!(concat!(env!("S"), "/src/version")); diff --git a/tests/run-make/c-link-to-rust-staticlib/rmake.rs b/tests/run-make/c-link-to-rust-staticlib/rmake.rs index 63d5eb78c6987..2f21c7356dfb8 100644 --- a/tests/run-make/c-link-to-rust-staticlib/rmake.rs +++ b/tests/run-make/c-link-to-rust-staticlib/rmake.rs @@ -10,6 +10,6 @@ fn main() { rustc().input("foo.rs").run(); cc().input("bar.c").input(static_lib("foo")).out_exe("bar").args(&extra_c_flags()).run(); run("bar"); - fs::remove_file(static_lib("foo")); + fs::remove_file(static_lib("foo")).unwrap(); run("bar"); } diff --git a/tests/run-make/doctests-keep-binaries/rmake.rs b/tests/run-make/doctests-keep-binaries/rmake.rs index 0613ef4839b14..137abc5ce672f 100644 --- a/tests/run-make/doctests-keep-binaries/rmake.rs +++ b/tests/run-make/doctests-keep-binaries/rmake.rs @@ -10,7 +10,7 @@ fn setup_test_env(callback: F) { create_dir(&out_dir).expect("failed to create doctests folder"); rustc().input("t.rs").crate_type("rlib").run(); callback(&out_dir, &tmp_dir().join("libt.rlib")); - remove_dir_all(out_dir); + remove_dir_all(out_dir).unwrap(); } fn check_generated_binaries() { @@ -60,6 +60,6 @@ fn main() { .extern_("t", "libt.rlib") .run(); - remove_dir_all(run_dir_path); + remove_dir_all(run_dir_path).unwrap(); }); } diff --git a/tests/run-make/doctests-runtool/rmake.rs b/tests/run-make/doctests-runtool/rmake.rs index 6cc7c6bbdafd2..1985f1f2e368f 100644 --- a/tests/run-make/doctests-runtool/rmake.rs +++ b/tests/run-make/doctests-runtool/rmake.rs @@ -33,6 +33,6 @@ fn main() { .current_dir(tmp_dir()) .run(); - remove_dir_all(run_dir); - remove_dir_all(run_tool); + remove_dir_all(run_dir).unwrap(); + remove_dir_all(run_tool).unwrap(); } diff --git a/tests/run-make/reset-codegen-1/rmake.rs b/tests/run-make/reset-codegen-1/rmake.rs index 4b91ba7df90bd..973636cec5bf7 100644 --- a/tests/run-make/reset-codegen-1/rmake.rs +++ b/tests/run-make/reset-codegen-1/rmake.rs @@ -31,8 +31,9 @@ fn main() { ("multi-output", Some("asm,obj")), ]; for (output_file, emit) in flags { - fs::remove_file(output_file).unwrap_or_default(); + fs::remove_dir_all(tmp_dir()).unwrap_or_default(); + fs::create_dir_all(tmp_dir()).unwrap(); compile(output_file, emit); - fs::remove_file(output_file); + fs::remove_dir_all(tmp_dir()).unwrap(); } } diff --git a/tests/run-make/rustdoc-error-lines/rmake.rs b/tests/run-make/rustdoc-error-lines/rmake.rs index 31536c78dd460..bba3aa7f96128 100644 --- a/tests/run-make/rustdoc-error-lines/rmake.rs +++ b/tests/run-make/rustdoc-error-lines/rmake.rs @@ -4,9 +4,8 @@ use run_make_support::rustdoc; fn main() { - let output = - String::from_utf8(rustdoc().input("input.rs").arg("--test").command_output().stdout) - .unwrap(); + let output = rustdoc().input("input.rs").arg("--test").run_fail(); + let output = String::from_utf8(output.stdout).unwrap(); let should_contain = &[ "input.rs - foo (line 5)", diff --git a/tests/run-make/rustdoc-scrape-examples-macros/rmake.rs b/tests/run-make/rustdoc-scrape-examples-macros/rmake.rs index 81b7defafc6c0..0fbdbc426b52d 100644 --- a/tests/run-make/rustdoc-scrape-examples-macros/rmake.rs +++ b/tests/run-make/rustdoc-scrape-examples-macros/rmake.rs @@ -9,17 +9,14 @@ fn main() { let proc_crate_name = "foobar_macro"; let crate_name = "foobar"; - let dylib_name = String::from_utf8( - rustc() - .crate_name(proc_crate_name) - .crate_type("dylib") - .arg("--print") - .arg("file-names") - .arg("-") - .command_output() - .stdout, - ) - .unwrap(); + let output = rustc() + .crate_name(proc_crate_name) + .crate_type("dylib") + .arg("--print") + .arg("file-names") + .arg("-") + .run(); + let dylib_name = String::from_utf8(output.stdout).unwrap(); rustc() .input("src/proc.rs") diff --git a/tests/run-make/rustdoc-shared-flags/rmake.rs b/tests/run-make/rustdoc-shared-flags/rmake.rs index 2db613f781764..105ca0b6dec59 100644 --- a/tests/run-make/rustdoc-shared-flags/rmake.rs +++ b/tests/run-make/rustdoc-shared-flags/rmake.rs @@ -1,8 +1,8 @@ use run_make_support::{rustc, rustdoc, Diff}; fn compare_outputs(args: &[&str]) { - let rustc_output = String::from_utf8(rustc().args(args).command_output().stdout).unwrap(); - let rustdoc_output = String::from_utf8(rustdoc().args(args).command_output().stdout).unwrap(); + let rustc_output = String::from_utf8(rustc().args(args).run().stdout).unwrap(); + let rustdoc_output = String::from_utf8(rustdoc().args(args).run().stdout).unwrap(); Diff::new().expected_text("rustc", rustc_output).actual_text("rustdoc", rustdoc_output).run(); } diff --git a/tests/run-make/same-lib-two-locations-no-panic/rmake.rs b/tests/run-make/same-lib-two-locations-no-panic/rmake.rs index 2900c3c8b749c..fb75300e90fa7 100644 --- a/tests/run-make/same-lib-two-locations-no-panic/rmake.rs +++ b/tests/run-make/same-lib-two-locations-no-panic/rmake.rs @@ -7,22 +7,22 @@ //@ ignore-cross-compile -use run_make_support::{dynamic_lib, rust_lib, rustc, tmp_dir}; +use run_make_support::{dynamic_lib, dynamic_lib_name, rust_lib, rust_lib_name, rustc, tmp_dir}; use std::fs; fn main() { let tmp_dir_other = tmp_dir().join("other"); - fs::create_dir(&tmp_dir_other); + fs::create_dir(&tmp_dir_other).unwrap(); rustc().input("foo.rs").crate_type("dylib").arg("-Cprefer-dynamic").run(); - fs::rename(dynamic_lib("foo"), &tmp_dir_other); + fs::rename(dynamic_lib("foo"), tmp_dir_other.join(dynamic_lib_name("foo"))).unwrap(); rustc().input("foo.rs").crate_type("dylib").arg("-Cprefer-dynamic").run(); rustc().input("bar.rs").library_search_path(&tmp_dir_other).run(); - fs::remove_dir_all(tmp_dir()); + fs::remove_dir_all(tmp_dir()).unwrap(); - fs::create_dir_all(&tmp_dir_other); + fs::create_dir_all(&tmp_dir_other).unwrap(); rustc().input("foo.rs").crate_type("rlib").run(); - fs::rename(rust_lib("foo"), &tmp_dir_other); + fs::rename(rust_lib("foo"), tmp_dir_other.join(rust_lib_name("foo"))).unwrap(); rustc().input("foo.rs").crate_type("rlib").run(); rustc().input("bar.rs").library_search_path(tmp_dir_other).run(); } From 9abfebdf1e3eb821d160673d0fbd1d58b310b4e6 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Thu, 30 May 2024 16:06:14 +1000 Subject: [PATCH 13/16] Add an alternate `--demangle` mode to coverage-dump The coverage-dump tool already needs `rustc_demangle` for its own purposes, so the amount of extra code needed for a demangle mode is very small. --- src/tools/coverage-dump/README.md | 5 +++++ src/tools/coverage-dump/src/main.rs | 19 +++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/src/tools/coverage-dump/README.md b/src/tools/coverage-dump/README.md index e2625d5adf27e..49d8e14c7bcc2 100644 --- a/src/tools/coverage-dump/README.md +++ b/src/tools/coverage-dump/README.md @@ -6,3 +6,8 @@ The output format is mostly arbitrary, so it's OK to change the output as long as any affected tests are also re-blessed. However, the output should be consistent across different executions on different platforms, so avoid printing any information that is platform-specific or non-deterministic. + +## Demangle mode + +When run as `coverage-dump --demangle`, this tool instead functions as a +command-line demangler that can be invoked by `llvm-cov`. diff --git a/src/tools/coverage-dump/src/main.rs b/src/tools/coverage-dump/src/main.rs index 93fed1799e041..b21e3e292f2b4 100644 --- a/src/tools/coverage-dump/src/main.rs +++ b/src/tools/coverage-dump/src/main.rs @@ -7,6 +7,13 @@ fn main() -> anyhow::Result<()> { let args = std::env::args().collect::>(); + // The coverage-dump tool already needs `rustc_demangle` in order to read + // coverage metadata, so it's very easy to also have a separate mode that + // turns it into a command-line demangler for use by coverage-run tests. + if &args[1..] == &["--demangle"] { + return demangle(); + } + let llvm_ir_path = args.get(1).context("LLVM IR file not specified")?; let llvm_ir = std::fs::read_to_string(llvm_ir_path).context("couldn't read LLVM IR file")?; @@ -15,3 +22,15 @@ fn main() -> anyhow::Result<()> { Ok(()) } + +fn demangle() -> anyhow::Result<()> { + use std::fmt::Write as _; + + let stdin = std::io::read_to_string(std::io::stdin())?; + let mut output = String::with_capacity(stdin.len()); + for line in stdin.lines() { + writeln!(output, "{:#}", rustc_demangle::demangle(line))?; + } + print!("{output}"); + Ok(()) +} From 10ffc228a851489b1678d830517541bd00d4da49 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Thu, 30 May 2024 16:21:42 +1000 Subject: [PATCH 14/16] Use `coverage-dump --demangle` as the demangler for coverage-run tests This avoids the need to build `rust-demangler` when running coverage tests, since we typically need to build `coverage-dump` anyway. --- src/bootstrap/src/core/build_steps/test.rs | 13 +------------ src/tools/compiletest/src/runtest/coverage.rs | 17 ++++++++++++----- 2 files changed, 13 insertions(+), 17 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/test.rs b/src/bootstrap/src/core/build_steps/test.rs index 2158868636242..76302a7900dee 100644 --- a/src/bootstrap/src/core/build_steps/test.rs +++ b/src/bootstrap/src/core/build_steps/test.rs @@ -1781,7 +1781,7 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the .arg(builder.ensure(tool::JsonDocLint { compiler: json_compiler, target })); } - if mode == "coverage-map" { + if matches!(mode, "coverage-map" | "coverage-run") { let coverage_dump = builder.ensure(tool::CoverageDump { compiler: compiler.with_stage(0), target: compiler.host, @@ -1789,17 +1789,6 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the cmd.arg("--coverage-dump-path").arg(coverage_dump); } - if mode == "coverage-run" { - // The demangler doesn't need the current compiler, so we can avoid - // unnecessary rebuilds by using the bootstrap compiler instead. - let rust_demangler = builder.ensure(tool::RustDemangler { - compiler: compiler.with_stage(0), - target: compiler.host, - extra_features: Vec::new(), - }); - cmd.arg("--rust-demangler-path").arg(rust_demangler); - } - cmd.arg("--src-base").arg(builder.src.join("tests").join(suite)); cmd.arg("--build-base").arg(testdir(builder, compiler.host).join(suite)); diff --git a/src/tools/compiletest/src/runtest/coverage.rs b/src/tools/compiletest/src/runtest/coverage.rs index dad3fb3013333..8bd7c7e808d3f 100644 --- a/src/tools/compiletest/src/runtest/coverage.rs +++ b/src/tools/compiletest/src/runtest/coverage.rs @@ -10,10 +10,15 @@ use crate::common::{UI_COVERAGE, UI_COVERAGE_MAP}; use crate::runtest::{static_regex, Emit, ProcRes, TestCx, WillExecute}; impl<'test> TestCx<'test> { + fn coverage_dump_path(&self) -> &Path { + self.config + .coverage_dump_path + .as_deref() + .unwrap_or_else(|| self.fatal("missing --coverage-dump")) + } + pub(crate) fn run_coverage_map_test(&self) { - let Some(coverage_dump_path) = &self.config.coverage_dump_path else { - self.fatal("missing --coverage-dump"); - }; + let coverage_dump_path = self.coverage_dump_path(); let (proc_res, llvm_ir_path) = self.compile_test_and_save_ir(); if !proc_res.status.success() { @@ -102,8 +107,10 @@ impl<'test> TestCx<'test> { let proc_res = self.run_llvm_tool("llvm-cov", |cmd| { cmd.args(["show", "--format=text", "--show-line-counts-or-regions"]); - cmd.arg("--Xdemangler"); - cmd.arg(self.config.rust_demangler_path.as_ref().unwrap()); + // Specify the demangler binary and its arguments. + let coverage_dump_path = self.coverage_dump_path(); + cmd.arg("--Xdemangler").arg(coverage_dump_path); + cmd.arg("--Xdemangler").arg("--demangle"); cmd.arg("--instr-profile"); cmd.arg(&profdata_path); From feb8f3cc5d0728db5038de151f02bac540dc713a Mon Sep 17 00:00:00 2001 From: Zalathar Date: Thu, 30 May 2024 17:15:36 +1000 Subject: [PATCH 15/16] Use `Builder::tool_exe` to build the coverage-dump tool This appears to be the canonical way to build a tool with the stage 0 compiler. --- src/bootstrap/src/core/build_steps/test.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/test.rs b/src/bootstrap/src/core/build_steps/test.rs index 76302a7900dee..29b3d1669b4bf 100644 --- a/src/bootstrap/src/core/build_steps/test.rs +++ b/src/bootstrap/src/core/build_steps/test.rs @@ -1782,10 +1782,7 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the } if matches!(mode, "coverage-map" | "coverage-run") { - let coverage_dump = builder.ensure(tool::CoverageDump { - compiler: compiler.with_stage(0), - target: compiler.host, - }); + let coverage_dump = builder.tool_exe(Tool::CoverageDump); cmd.arg("--coverage-dump-path").arg(coverage_dump); } From 54b6849e06cc4847625cdf6a7312eb017786867f Mon Sep 17 00:00:00 2001 From: Zalathar Date: Thu, 30 May 2024 17:22:02 +1000 Subject: [PATCH 16/16] Remove unused rust-demangler support from compiletest --- src/tools/compiletest/src/common.rs | 3 --- src/tools/compiletest/src/lib.rs | 3 --- src/tools/compiletest/src/runtest.rs | 4 ---- 3 files changed, 10 deletions(-) diff --git a/src/tools/compiletest/src/common.rs b/src/tools/compiletest/src/common.rs index 7ff45edd4b26b..b0047770564c4 100644 --- a/src/tools/compiletest/src/common.rs +++ b/src/tools/compiletest/src/common.rs @@ -187,9 +187,6 @@ pub struct Config { /// The rustdoc executable. pub rustdoc_path: Option, - /// The rust-demangler executable. - pub rust_demangler_path: Option, - /// The coverage-dump executable. pub coverage_dump_path: Option, diff --git a/src/tools/compiletest/src/lib.rs b/src/tools/compiletest/src/lib.rs index 99bde107f3a47..62e71e9b59ddb 100644 --- a/src/tools/compiletest/src/lib.rs +++ b/src/tools/compiletest/src/lib.rs @@ -46,7 +46,6 @@ pub fn parse_config(args: Vec) -> Config { .reqopt("", "run-lib-path", "path to target shared libraries", "PATH") .reqopt("", "rustc-path", "path to rustc to use for compiling", "PATH") .optopt("", "rustdoc-path", "path to rustdoc to use for compiling", "PATH") - .optopt("", "rust-demangler-path", "path to rust-demangler to use in tests", "PATH") .optopt("", "coverage-dump-path", "path to coverage-dump to use in tests", "PATH") .reqopt("", "python", "path to python to use for doc tests", "PATH") .optopt("", "jsondocck-path", "path to jsondocck to use for doc tests", "PATH") @@ -232,7 +231,6 @@ pub fn parse_config(args: Vec) -> Config { run_lib_path: make_absolute(opt_path(matches, "run-lib-path")), rustc_path: opt_path(matches, "rustc-path"), rustdoc_path: matches.opt_str("rustdoc-path").map(PathBuf::from), - rust_demangler_path: matches.opt_str("rust-demangler-path").map(PathBuf::from), coverage_dump_path: matches.opt_str("coverage-dump-path").map(PathBuf::from), python: matches.opt_str("python").unwrap(), jsondocck_path: matches.opt_str("jsondocck-path"), @@ -337,7 +335,6 @@ pub fn log_config(config: &Config) { logv(c, format!("run_lib_path: {:?}", config.run_lib_path)); logv(c, format!("rustc_path: {:?}", config.rustc_path.display())); logv(c, format!("rustdoc_path: {:?}", config.rustdoc_path)); - logv(c, format!("rust_demangler_path: {:?}", config.rust_demangler_path)); logv(c, format!("src_base: {:?}", config.src_base.display())); logv(c, format!("build_base: {:?}", config.build_base.display())); logv(c, format!("stage_id: {}", config.stage_id)); diff --git a/src/tools/compiletest/src/runtest.rs b/src/tools/compiletest/src/runtest.rs index 4ea12a0f9e480..79e158992d477 100644 --- a/src/tools/compiletest/src/runtest.rs +++ b/src/tools/compiletest/src/runtest.rs @@ -3561,10 +3561,6 @@ impl<'test> TestCx<'test> { cmd.env("RUSTDOC", cwd.join(rustdoc)); } - if let Some(ref rust_demangler) = self.config.rust_demangler_path { - cmd.env("RUST_DEMANGLER", cwd.join(rust_demangler)); - } - if let Some(ref node) = self.config.nodejs { cmd.env("NODE", node); }