From 0f06b07552e80f3438cb85d30ec407a2cd8995e8 Mon Sep 17 00:00:00 2001 From: Konrad Borowski Date: Fri, 12 Aug 2022 15:02:34 +0000 Subject: [PATCH 01/11] Implement UnwindSafe and RefUnwindSafe for Backtrace Backtrace doesn't have visible mutable state. --- library/std/src/backtrace.rs | 3 ++- library/std/src/backtrace/tests.rs | 7 +++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/library/std/src/backtrace.rs b/library/std/src/backtrace.rs index f0e199fac737c..e7110aebdea16 100644 --- a/library/std/src/backtrace.rs +++ b/library/std/src/backtrace.rs @@ -92,6 +92,7 @@ use crate::backtrace_rs::{self, BytesOrWideString}; use crate::env; use crate::ffi::c_void; use crate::fmt; +use crate::panic::UnwindSafe; use crate::sync::atomic::{AtomicUsize, Ordering::Relaxed}; use crate::sync::LazyLock; use crate::sys_common::backtrace::{lock, output_filename}; @@ -427,7 +428,7 @@ impl fmt::Display for Backtrace { } } -type LazyResolve = impl (FnOnce() -> Capture) + Send + Sync; +type LazyResolve = impl (FnOnce() -> Capture) + Send + Sync + UnwindSafe; fn lazy_resolve(mut capture: Capture) -> LazyResolve { move || { diff --git a/library/std/src/backtrace/tests.rs b/library/std/src/backtrace/tests.rs index ef419806dccbd..73543a3af548f 100644 --- a/library/std/src/backtrace/tests.rs +++ b/library/std/src/backtrace/tests.rs @@ -1,4 +1,5 @@ use super::*; +use crate::panic::{RefUnwindSafe, UnwindSafe}; fn generate_fake_frames() -> Vec { vec![ @@ -91,3 +92,9 @@ fn test_frames() { assert!(iter.all(|(f, e)| format!("{f:#?}") == *e)); } + +#[test] +fn backtrace_unwind_safe() { + fn assert_unwind_safe() {} + assert_unwind_safe::(); +} From b169ee7c1a0a72576df06d7b01699703f6ed65a8 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 31 Jul 2023 17:29:30 +0200 Subject: [PATCH 02/11] fix alignment handling for Repeat expressions --- .../rustc_const_eval/src/interpret/step.rs | 8 +++---- .../pass/align_repeat_into_packed_field.rs | 22 +++++++++++++++++++ ...> align_repeat_into_well_aligned_array.rs} | 2 ++ 3 files changed, 28 insertions(+), 4 deletions(-) create mode 100644 src/tools/miri/tests/pass/align_repeat_into_packed_field.rs rename src/tools/miri/tests/pass/{issues/issue-miri-1925.rs => align_repeat_into_well_aligned_array.rs} (79%) diff --git a/compiler/rustc_const_eval/src/interpret/step.rs b/compiler/rustc_const_eval/src/interpret/step.rs index 91341ddacd107..ead172f04a3f9 100644 --- a/compiler/rustc_const_eval/src/interpret/step.rs +++ b/compiler/rustc_const_eval/src/interpret/step.rs @@ -208,13 +208,13 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { let rest_ptr = first_ptr.offset(elem_size, self)?; // For the alignment of `rest_ptr`, we crucially do *not* use `first.align` as // that place might be more aligned than its type mandates (a `u8` array could - // be 4-aligned if it sits at the right spot in a struct). Instead we use - // `first.layout.align`, i.e., the alignment given by the type. + // be 4-aligned if it sits at the right spot in a struct). We have to also factor + // in element size. self.mem_copy_repeatedly( first_ptr, - first.align, + dest.align, rest_ptr, - first.layout.align.abi, + dest.align.restrict_for_offset(elem_size), elem_size, length - 1, /*nonoverlapping:*/ true, diff --git a/src/tools/miri/tests/pass/align_repeat_into_packed_field.rs b/src/tools/miri/tests/pass/align_repeat_into_packed_field.rs new file mode 100644 index 0000000000000..3affb20420509 --- /dev/null +++ b/src/tools/miri/tests/pass/align_repeat_into_packed_field.rs @@ -0,0 +1,22 @@ +#![feature(custom_mir, core_intrinsics)] +use std::intrinsics::mir::*; + +#[repr(packed)] +struct S { field: [u32; 2] } + +#[custom_mir(dialect = "runtime", phase = "optimized")] +fn test() { mir! { + let s: S; + { + // Store a repeat expression directly into a field of a packed struct. + s.field = [0; 2]; + Return() + } +} } + +fn main() { + // Run this a bunch of time to make sure it doesn't pass by chance. + for _ in 0..20 { + test(); + } +} diff --git a/src/tools/miri/tests/pass/issues/issue-miri-1925.rs b/src/tools/miri/tests/pass/align_repeat_into_well_aligned_array.rs similarity index 79% rename from src/tools/miri/tests/pass/issues/issue-miri-1925.rs rename to src/tools/miri/tests/pass/align_repeat_into_well_aligned_array.rs index 8655681349194..735251039f772 100644 --- a/src/tools/miri/tests/pass/issues/issue-miri-1925.rs +++ b/src/tools/miri/tests/pass/align_repeat_into_well_aligned_array.rs @@ -4,6 +4,8 @@ use std::mem::size_of; fn main() { let mut a = Params::new(); + // The array itself here happens to be quite well-aligned, but not all its elements have that + // large alignment and we better make sure that is still accepted by Miri. a.key_block = [0; BLOCKBYTES]; } From b84942a0fe11c06a94cfe46ee787084e80c51fb9 Mon Sep 17 00:00:00 2001 From: Taras Tsugrii Date: Mon, 31 Jul 2023 12:40:11 -0700 Subject: [PATCH 03/11] [rustc_data_structures][perf] Simplify base_n::push_str. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This minor change removes the need to reverse resulting digits. Since reverse is O(|digit_num|) but bounded by 128, it's unlikely to be a noticeable in practice. At the same time, this code is also a 1 line shorter, so combined with tiny perf win, why not? I ran https://gist.github.com/ttsugriy/ed14860ef597ab315d4129d5f8adb191 on M1 macbook air and got a small improvement ``` Running benches/base_n_benchmark.rs (target/release/deps/base_n_benchmark-825fe5895b5c2693) push_str/old time: [14.180 µs 14.313 µs 14.462 µs] Performance has improved. Found 5 outliers among 100 measurements (5.00%) 4 (4.00%) high mild 1 (1.00%) high severe push_str/new time: [13.741 µs 13.839 µs 13.973 µs] Performance has improved. Found 8 outliers among 100 measurements (8.00%) 3 (3.00%) high mild 5 (5.00%) high severe ``` --- compiler/rustc_data_structures/src/base_n.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/compiler/rustc_data_structures/src/base_n.rs b/compiler/rustc_data_structures/src/base_n.rs index 4567759c004da..587043507064c 100644 --- a/compiler/rustc_data_structures/src/base_n.rs +++ b/compiler/rustc_data_structures/src/base_n.rs @@ -16,22 +16,21 @@ const BASE_64: &[u8; MAX_BASE] = pub fn push_str(mut n: u128, base: usize, output: &mut String) { debug_assert!(base >= 2 && base <= MAX_BASE); let mut s = [0u8; 128]; - let mut index = 0; + let mut index = s.len(); let base = base as u128; loop { + index -= 1; s[index] = BASE_64[(n % base) as usize]; - index += 1; n /= base; if n == 0 { break; } } - s[0..index].reverse(); - output.push_str(str::from_utf8(&s[0..index]).unwrap()); + output.push_str(str::from_utf8(&s[index..]).unwrap()); } #[inline] From 5a808d40f41c9d021361c4a9c4f53e099af40dc5 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Tue, 4 Jul 2023 12:53:54 +1000 Subject: [PATCH 04/11] Add some line comments to enum `CoverageKind` The actual motivation here is to prevent `rustfmt` from suddenly reformatting these enum variants onto a single line, when they become slightly shorter in the future. But there's no harm in adding some helpful documentation at the same time. --- compiler/rustc_middle/src/mir/coverage.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/compiler/rustc_middle/src/mir/coverage.rs b/compiler/rustc_middle/src/mir/coverage.rs index db24dae11304f..037d9d07b3cbd 100644 --- a/compiler/rustc_middle/src/mir/coverage.rs +++ b/compiler/rustc_middle/src/mir/coverage.rs @@ -99,9 +99,13 @@ impl From for ExpressionOperandId { pub enum CoverageKind { Counter { function_source_hash: u64, + /// ID of this counter within its enclosing function. + /// Expressions in the same function can refer to it as an operand. id: CounterValueReference, }, Expression { + /// ID of this coverage-counter expression within its enclosing function. + /// Other expressions in the same function can refer to it as an operand. id: InjectedExpressionId, lhs: ExpressionOperandId, op: Op, From 1a014d42f45de1b829ca7916d8f639fda6e0770a Mon Sep 17 00:00:00 2001 From: Zalathar Date: Mon, 26 Jun 2023 22:28:48 +1000 Subject: [PATCH 05/11] Replace `ExpressionOperandId` with enum `Operand` Because the three kinds of operand are now distinguished explicitly, we no longer need fiddly code to disambiguate counter IDs and expression IDs based on the total number of counters/expressions in a function. This does increase the size of operands from 4 bytes to 8 bytes, but that shouldn't be a big deal since they are mostly stored inside boxed structures, and the current coverage code is not particularly size-optimized anyway. --- .../src/coverageinfo/map_data.rs | 43 +++++------ .../src/coverageinfo/mod.rs | 6 +- compiler/rustc_middle/src/mir/coverage.rs | 74 +++++++------------ .../rustc_middle/src/ty/structural_impls.rs | 1 - .../src/coverage/counters.rs | 26 +++---- .../rustc_mir_transform/src/coverage/debug.rs | 27 ++++--- .../rustc_mir_transform/src/coverage/graph.rs | 11 +-- .../rustc_mir_transform/src/coverage/mod.rs | 2 +- .../rustc_mir_transform/src/coverage/query.rs | 45 ++++------- ...age_graphviz.main.InstrumentCoverage.0.dot | 2 +- ...ment_coverage.main.InstrumentCoverage.diff | 6 +- 11 files changed, 96 insertions(+), 147 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs index 06844afd6b870..0d4d8ca61e939 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs @@ -3,17 +3,17 @@ pub use super::ffi::*; use rustc_index::{IndexSlice, IndexVec}; use rustc_middle::bug; use rustc_middle::mir::coverage::{ - CodeRegion, CounterValueReference, ExpressionOperandId, InjectedExpressionId, - InjectedExpressionIndex, MappedExpressionIndex, Op, + CodeRegion, CounterValueReference, InjectedExpressionId, InjectedExpressionIndex, + MappedExpressionIndex, Op, Operand, }; use rustc_middle::ty::Instance; use rustc_middle::ty::TyCtxt; #[derive(Clone, Debug, PartialEq)] pub struct Expression { - lhs: ExpressionOperandId, + lhs: Operand, op: Op, - rhs: ExpressionOperandId, + rhs: Operand, region: Option, } @@ -105,16 +105,16 @@ impl<'tcx> FunctionCoverage<'tcx> { pub fn add_counter_expression( &mut self, expression_id: InjectedExpressionId, - lhs: ExpressionOperandId, + lhs: Operand, op: Op, - rhs: ExpressionOperandId, + rhs: Operand, region: Option, ) { debug!( "add_counter_expression({:?}, lhs={:?}, op={:?}, rhs={:?} at {:?}", expression_id, lhs, op, rhs, region ); - let expression_index = self.expression_index(u32::from(expression_id)); + let expression_index = self.expression_index(expression_id); debug_assert!( expression_index.as_usize() < self.expressions.len(), "expression_index {} is out of range for expressions.len() = {} @@ -186,10 +186,7 @@ impl<'tcx> FunctionCoverage<'tcx> { // This closure converts any `Expression` operand (`lhs` or `rhs` of the `Op::Add` or // `Op::Subtract` operation) into its native `llvm::coverage::Counter::CounterKind` type - // and value. Operand ID value `0` maps to `CounterKind::Zero`; values in the known range - // of injected LLVM counters map to `CounterKind::CounterValueReference` (and the value - // matches the injected counter index); and any other value is converted into a - // `CounterKind::Expression` with the expression's `new_index`. + // and value. // // Expressions will be returned from this function in a sequential vector (array) of // `CounterExpression`, so the expression IDs must be mapped from their original, @@ -206,17 +203,13 @@ impl<'tcx> FunctionCoverage<'tcx> { // `expression_index`s lower than the referencing `Expression`. Therefore, it is // reasonable to look up the new index of an expression operand while the `new_indexes` // vector is only complete up to the current `ExpressionIndex`. - let id_to_counter = |new_indexes: &IndexSlice< - InjectedExpressionIndex, - Option, - >, - id: ExpressionOperandId| { - if id == ExpressionOperandId::ZERO { - Some(Counter::zero()) - } else if id.index() < self.counters.len() { + type NewIndexes = IndexSlice>; + let id_to_counter = |new_indexes: &NewIndexes, operand: Operand| match operand { + Operand::Zero => Some(Counter::zero()), + Operand::Counter(id) => { debug_assert!( id.index() > 0, - "ExpressionOperandId indexes for counters are 1-based, but this id={}", + "Operand::Counter indexes for counters are 1-based, but this id={}", id.index() ); // Note: Some codegen-injected Counters may be only referenced by `Expression`s, @@ -224,8 +217,9 @@ impl<'tcx> FunctionCoverage<'tcx> { let index = CounterValueReference::from(id.index()); // Note, the conversion to LLVM `Counter` adjusts the index to be zero-based. Some(Counter::counter_value_reference(index)) - } else { - let index = self.expression_index(u32::from(id)); + } + Operand::Expression(id) => { + let index = self.expression_index(id); self.expressions .get(index) .expect("expression id is out of range") @@ -341,8 +335,9 @@ impl<'tcx> FunctionCoverage<'tcx> { self.unreachable_regions.iter().map(|region| (Counter::zero(), region)) } - fn expression_index(&self, id_descending_from_max: u32) -> InjectedExpressionIndex { - debug_assert!(id_descending_from_max >= self.counters.len() as u32); + fn expression_index(&self, id: InjectedExpressionId) -> InjectedExpressionIndex { + debug_assert!(id.as_usize() >= self.counters.len()); + let id_descending_from_max = id.as_u32(); InjectedExpressionIndex::from(u32::MAX - id_descending_from_max) } } diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs index 60cbb3d3d9d6b..d6a32497f7ff6 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs @@ -17,7 +17,7 @@ use rustc_hir::def_id::DefId; use rustc_llvm::RustString; use rustc_middle::bug; use rustc_middle::mir::coverage::{ - CodeRegion, CounterValueReference, CoverageKind, ExpressionOperandId, InjectedExpressionId, Op, + CodeRegion, CounterValueReference, CoverageKind, InjectedExpressionId, Op, Operand, }; use rustc_middle::mir::Coverage; use rustc_middle::ty; @@ -203,9 +203,9 @@ impl<'tcx> Builder<'_, '_, 'tcx> { &mut self, instance: Instance<'tcx>, id: InjectedExpressionId, - lhs: ExpressionOperandId, + lhs: Operand, op: Op, - rhs: ExpressionOperandId, + rhs: Operand, region: Option, ) -> bool { if let Some(coverage_context) = self.coverage_context() { diff --git a/compiler/rustc_middle/src/mir/coverage.rs b/compiler/rustc_middle/src/mir/coverage.rs index 037d9d07b3cbd..7894e564b02fe 100644 --- a/compiler/rustc_middle/src/mir/coverage.rs +++ b/compiler/rustc_middle/src/mir/coverage.rs @@ -5,32 +5,6 @@ use rustc_span::Symbol; use std::fmt::{self, Debug, Formatter}; -rustc_index::newtype_index! { - /// An ExpressionOperandId value is assigned directly from either a - /// CounterValueReference.as_u32() (which ascend from 1) or an ExpressionOperandId.as_u32() - /// (which _*descend*_ from u32::MAX). Id value `0` (zero) represents a virtual counter with a - /// constant value of `0`. - #[derive(HashStable)] - #[max = 0xFFFF_FFFF] - #[debug_format = "ExpressionOperandId({})"] - pub struct ExpressionOperandId { - } -} - -impl ExpressionOperandId { - /// An expression operand for a "zero counter", as described in the following references: - /// - /// * - /// * - /// * - /// - /// This operand can be used to count two or more separate code regions with a single counter, - /// if they run sequentially with no branches, by injecting the `Counter` in a `BasicBlock` for - /// one of the code regions, and inserting `CounterExpression`s ("add ZERO to the counter") in - /// the coverage map for the other code regions. - pub const ZERO: Self = Self::from_u32(0); -} - rustc_index::newtype_index! { #[derive(HashStable)] #[max = 0xFFFF_FFFF] @@ -39,7 +13,7 @@ rustc_index::newtype_index! { } impl CounterValueReference { - /// Counters start at 1 to reserve 0 for ExpressionOperandId::ZERO. + /// Counters start at 1 for historical reasons. pub const START: Self = Self::from_u32(1); /// Returns explicitly-requested zero-based version of the counter id, used @@ -52,8 +26,6 @@ impl CounterValueReference { } rustc_index::newtype_index! { - /// InjectedExpressionId.as_u32() converts to ExpressionOperandId.as_u32() - /// /// Values descend from u32::MAX. #[derive(HashStable)] #[max = 0xFFFF_FFFF] @@ -62,8 +34,6 @@ rustc_index::newtype_index! { } rustc_index::newtype_index! { - /// InjectedExpressionIndex.as_u32() translates to u32::MAX - ExpressionOperandId.as_u32() - /// /// Values ascend from 0. #[derive(HashStable)] #[max = 0xFFFF_FFFF] @@ -81,17 +51,25 @@ rustc_index::newtype_index! { pub struct MappedExpressionIndex {} } -impl From for ExpressionOperandId { - #[inline] - fn from(v: CounterValueReference) -> ExpressionOperandId { - ExpressionOperandId::from(v.as_u32()) - } +/// Operand of a coverage-counter expression. +/// +/// Operands can be a constant zero value, an actual coverage counter, or another +/// expression. Counter/expression operands are referred to by ID. +#[derive(Copy, Clone, PartialEq, Eq)] +#[derive(TyEncodable, TyDecodable, Hash, HashStable, TypeFoldable, TypeVisitable)] +pub enum Operand { + Zero, + Counter(CounterValueReference), + Expression(InjectedExpressionId), } -impl From for ExpressionOperandId { - #[inline] - fn from(v: InjectedExpressionId) -> ExpressionOperandId { - ExpressionOperandId::from(v.as_u32()) +impl Debug for Operand { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + match self { + Self::Zero => write!(f, "Zero"), + Self::Counter(id) => f.debug_tuple("Counter").field(&id.as_u32()).finish(), + Self::Expression(id) => f.debug_tuple("Expression").field(&id.as_u32()).finish(), + } } } @@ -107,19 +85,19 @@ pub enum CoverageKind { /// ID of this coverage-counter expression within its enclosing function. /// Other expressions in the same function can refer to it as an operand. id: InjectedExpressionId, - lhs: ExpressionOperandId, + lhs: Operand, op: Op, - rhs: ExpressionOperandId, + rhs: Operand, }, Unreachable, } impl CoverageKind { - pub fn as_operand_id(&self) -> ExpressionOperandId { + pub fn as_operand(&self) -> Operand { use CoverageKind::*; match *self { - Counter { id, .. } => ExpressionOperandId::from(id), - Expression { id, .. } => ExpressionOperandId::from(id), + Counter { id, .. } => Operand::Counter(id), + Expression { id, .. } => Operand::Expression(id), Unreachable => bug!("Unreachable coverage cannot be part of an expression"), } } @@ -136,14 +114,14 @@ impl Debug for CoverageKind { Counter { id, .. } => write!(fmt, "Counter({:?})", id.index()), Expression { id, lhs, op, rhs } => write!( fmt, - "Expression({:?}) = {} {} {}", + "Expression({:?}) = {:?} {} {:?}", id.index(), - lhs.index(), + lhs, match op { Op::Add => "+", Op::Subtract => "-", }, - rhs.index(), + rhs, ), Unreachable => write!(fmt, "Unreachable"), } diff --git a/compiler/rustc_middle/src/ty/structural_impls.rs b/compiler/rustc_middle/src/ty/structural_impls.rs index 2a6044cad376d..10cff0e2f500e 100644 --- a/compiler/rustc_middle/src/ty/structural_impls.rs +++ b/compiler/rustc_middle/src/ty/structural_impls.rs @@ -470,7 +470,6 @@ TrivialTypeTraversalAndLiftImpls! { ::rustc_hir::Unsafety, ::rustc_target::asm::InlineAsmRegOrRegClass, ::rustc_target::spec::abi::Abi, - crate::mir::coverage::ExpressionOperandId, crate::mir::coverage::CounterValueReference, crate::mir::coverage::InjectedExpressionId, crate::mir::coverage::InjectedExpressionIndex, diff --git a/compiler/rustc_mir_transform/src/coverage/counters.rs b/compiler/rustc_mir_transform/src/coverage/counters.rs index e9c5f856d35fd..9d168404264aa 100644 --- a/compiler/rustc_mir_transform/src/coverage/counters.rs +++ b/compiler/rustc_mir_transform/src/coverage/counters.rs @@ -65,9 +65,9 @@ impl CoverageCounters { fn make_expression( &mut self, - lhs: ExpressionOperandId, + lhs: Operand, op: Op, - rhs: ExpressionOperandId, + rhs: Operand, debug_block_label_fn: F, ) -> CoverageKind where @@ -81,13 +81,13 @@ impl CoverageCounters { expression } - pub fn make_identity_counter(&mut self, counter_operand: ExpressionOperandId) -> CoverageKind { + pub fn make_identity_counter(&mut self, counter_operand: Operand) -> CoverageKind { let some_debug_block_label = if self.debug_counters.is_enabled() { self.debug_counters.some_block_label(counter_operand).cloned() } else { None }; - self.make_expression(counter_operand, Op::Add, ExpressionOperandId::ZERO, || { + self.make_expression(counter_operand, Op::Add, Operand::Zero, || { some_debug_block_label.clone() }) } @@ -199,7 +199,7 @@ impl<'a> BcbCounters<'a> { &mut self, traversal: &mut TraverseCoverageGraphWithLoops, branching_bcb: BasicCoverageBlock, - branching_counter_operand: ExpressionOperandId, + branching_counter_operand: Operand, collect_intermediate_expressions: &mut Vec, ) -> Result<(), Error> { let branches = self.bcb_branches(branching_bcb); @@ -261,7 +261,7 @@ impl<'a> BcbCounters<'a> { " [new intermediate expression: {}]", self.format_counter(&intermediate_expression) ); - let intermediate_expression_operand = intermediate_expression.as_operand_id(); + let intermediate_expression_operand = intermediate_expression.as_operand(); collect_intermediate_expressions.push(intermediate_expression); some_sumup_counter_operand.replace(intermediate_expression_operand); } @@ -298,7 +298,7 @@ impl<'a> BcbCounters<'a> { &mut self, bcb: BasicCoverageBlock, collect_intermediate_expressions: &mut Vec, - ) -> Result { + ) -> Result { self.recursive_get_or_make_counter_operand(bcb, collect_intermediate_expressions, 1) } @@ -307,7 +307,7 @@ impl<'a> BcbCounters<'a> { bcb: BasicCoverageBlock, collect_intermediate_expressions: &mut Vec, debug_indent_level: usize, - ) -> Result { + ) -> Result { // If the BCB already has a counter, return it. if let Some(counter_kind) = self.basic_coverage_blocks[bcb].counter() { debug!( @@ -316,7 +316,7 @@ impl<'a> BcbCounters<'a> { bcb, self.format_counter(counter_kind), ); - return Ok(counter_kind.as_operand_id()); + return Ok(counter_kind.as_operand()); } // A BCB with only one incoming edge gets a simple `Counter` (via `make_counter()`). @@ -383,7 +383,7 @@ impl<'a> BcbCounters<'a> { NESTED_INDENT.repeat(debug_indent_level), self.format_counter(&intermediate_expression) ); - let intermediate_expression_operand = intermediate_expression.as_operand_id(); + let intermediate_expression_operand = intermediate_expression.as_operand(); collect_intermediate_expressions.push(intermediate_expression); some_sumup_edge_counter_operand.replace(intermediate_expression_operand); } @@ -408,7 +408,7 @@ impl<'a> BcbCounters<'a> { from_bcb: BasicCoverageBlock, to_bcb: BasicCoverageBlock, collect_intermediate_expressions: &mut Vec, - ) -> Result { + ) -> Result { self.recursive_get_or_make_edge_counter_operand( from_bcb, to_bcb, @@ -423,7 +423,7 @@ impl<'a> BcbCounters<'a> { to_bcb: BasicCoverageBlock, collect_intermediate_expressions: &mut Vec, debug_indent_level: usize, - ) -> Result { + ) -> Result { // If the source BCB has only one successor (assumed to be the given target), an edge // counter is unnecessary. Just get or make a counter for the source BCB. let successors = self.bcb_successors(from_bcb).iter(); @@ -444,7 +444,7 @@ impl<'a> BcbCounters<'a> { to_bcb, self.format_counter(counter_kind) ); - return Ok(counter_kind.as_operand_id()); + return Ok(counter_kind.as_operand()); } // Make a new counter to count this edge. diff --git a/compiler/rustc_mir_transform/src/coverage/debug.rs b/compiler/rustc_mir_transform/src/coverage/debug.rs index c9914eb9f8207..0e1065a40e704 100644 --- a/compiler/rustc_mir_transform/src/coverage/debug.rs +++ b/compiler/rustc_mir_transform/src/coverage/debug.rs @@ -246,7 +246,7 @@ impl Default for ExpressionFormat { } } -/// If enabled, this struct maintains a map from `CoverageKind` IDs (as `ExpressionOperandId`) to +/// If enabled, this struct maintains a map from `CoverageKind` IDs (as `Operand`) to /// the `CoverageKind` data and optional label (normally, the counter's associated /// `BasicCoverageBlock` format string, if any). /// @@ -258,7 +258,7 @@ impl Default for ExpressionFormat { /// `DebugCounters` supports a recursive rendering of `Expression` counters, so they can be /// presented as nested expressions such as `(bcb3 - (bcb0 + bcb1))`. pub(super) struct DebugCounters { - some_counters: Option>, + some_counters: Option>, } impl DebugCounters { @@ -277,14 +277,14 @@ impl DebugCounters { pub fn add_counter(&mut self, counter_kind: &CoverageKind, some_block_label: Option) { if let Some(counters) = &mut self.some_counters { - let id = counter_kind.as_operand_id(); + let id = counter_kind.as_operand(); counters .try_insert(id, DebugCounter::new(counter_kind.clone(), some_block_label)) .expect("attempt to add the same counter_kind to DebugCounters more than once"); } } - pub fn some_block_label(&self, operand: ExpressionOperandId) -> Option<&String> { + pub fn some_block_label(&self, operand: Operand) -> Option<&String> { self.some_counters.as_ref().and_then(|counters| { counters.get(&operand).and_then(|debug_counter| debug_counter.some_block_label.as_ref()) }) @@ -323,24 +323,24 @@ impl DebugCounters { } } - let id = counter_kind.as_operand_id(); + let id = counter_kind.as_operand(); if self.some_counters.is_some() && (counter_format.block || !counter_format.id) { let counters = self.some_counters.as_ref().unwrap(); if let Some(DebugCounter { some_block_label: Some(block_label), .. }) = counters.get(&id) { return if counter_format.id { - format!("{}#{}", block_label, id.index()) + format!("{}#{:?}", block_label, id) } else { block_label.to_string() }; } } - format!("#{}", id.index()) + format!("#{:?}", id) } - fn format_operand(&self, operand: ExpressionOperandId) -> String { - if operand.index() == 0 { + fn format_operand(&self, operand: Operand) -> String { + if matches!(operand, Operand::Zero) { return String::from("0"); } if let Some(counters) = &self.some_counters { @@ -358,7 +358,7 @@ impl DebugCounters { return self.format_counter_kind(counter_kind); } } - format!("#{}", operand.index()) + format!("#{:?}", operand) } } @@ -485,8 +485,7 @@ impl GraphvizData { /// _not_ used are retained in the `unused_expressions` Vec, to be included in debug output (logs /// and/or a `CoverageGraph` graphviz output). pub(super) struct UsedExpressions { - some_used_expression_operands: - Option>>, + some_used_expression_operands: Option>>, some_unused_expressions: Option, BasicCoverageBlock)>>, } @@ -517,7 +516,7 @@ impl UsedExpressions { pub fn expression_is_used(&self, expression: &CoverageKind) -> bool { if let Some(used_expression_operands) = self.some_used_expression_operands.as_ref() { - used_expression_operands.contains_key(&expression.as_operand_id()) + used_expression_operands.contains_key(&expression.as_operand()) } else { false } @@ -530,7 +529,7 @@ impl UsedExpressions { target_bcb: BasicCoverageBlock, ) { if let Some(used_expression_operands) = self.some_used_expression_operands.as_ref() { - if !used_expression_operands.contains_key(&expression.as_operand_id()) { + if !used_expression_operands.contains_key(&expression.as_operand()) { self.some_unused_expressions.as_mut().unwrap().push(( expression.clone(), edge_from_bcb, diff --git a/compiler/rustc_mir_transform/src/coverage/graph.rs b/compiler/rustc_mir_transform/src/coverage/graph.rs index 5d843f4ade095..f94dad4c8da67 100644 --- a/compiler/rustc_mir_transform/src/coverage/graph.rs +++ b/compiler/rustc_mir_transform/src/coverage/graph.rs @@ -345,10 +345,7 @@ impl BasicCoverageBlockData { &mir_body[self.last_bb()].terminator() } - pub fn set_counter( - &mut self, - counter_kind: CoverageKind, - ) -> Result { + pub fn set_counter(&mut self, counter_kind: CoverageKind) -> Result { debug_assert!( // If the BCB has an edge counter (to be injected into a new `BasicBlock`), it can also // have an expression (to be injected into an existing `BasicBlock` represented by this @@ -356,7 +353,7 @@ impl BasicCoverageBlockData { self.edge_from_bcbs.is_none() || counter_kind.is_expression(), "attempt to add a `Counter` to a BCB target with existing incoming edge counters" ); - let operand = counter_kind.as_operand_id(); + let operand = counter_kind.as_operand(); if let Some(replaced) = self.counter_kind.replace(counter_kind) { Error::from_string(format!( "attempt to set a BasicCoverageBlock coverage counter more than once; \ @@ -381,7 +378,7 @@ impl BasicCoverageBlockData { &mut self, from_bcb: BasicCoverageBlock, counter_kind: CoverageKind, - ) -> Result { + ) -> Result { if level_enabled!(tracing::Level::DEBUG) { // If the BCB has an edge counter (to be injected into a new `BasicBlock`), it can also // have an expression (to be injected into an existing `BasicBlock` represented by this @@ -393,7 +390,7 @@ impl BasicCoverageBlockData { )); } } - let operand = counter_kind.as_operand_id(); + let operand = counter_kind.as_operand(); if let Some(replaced) = self.edge_from_bcbs.get_or_insert_default().insert(from_bcb, counter_kind) { diff --git a/compiler/rustc_mir_transform/src/coverage/mod.rs b/compiler/rustc_mir_transform/src/coverage/mod.rs index 076e714d70370..f713613d313aa 100644 --- a/compiler/rustc_mir_transform/src/coverage/mod.rs +++ b/compiler/rustc_mir_transform/src/coverage/mod.rs @@ -304,7 +304,7 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> { let counter_kind = if let Some(&counter_operand) = bcb_counters[bcb].as_ref() { self.coverage_counters.make_identity_counter(counter_operand) } else if let Some(counter_kind) = self.bcb_data_mut(bcb).take_counter() { - bcb_counters[bcb] = Some(counter_kind.as_operand_id()); + bcb_counters[bcb] = Some(counter_kind.as_operand()); debug_used_expressions.add_expression_operands(&counter_kind); counter_kind } else { diff --git a/compiler/rustc_mir_transform/src/coverage/query.rs b/compiler/rustc_mir_transform/src/coverage/query.rs index 74b4b4a07c550..1261cf2765ca9 100644 --- a/compiler/rustc_mir_transform/src/coverage/query.rs +++ b/compiler/rustc_mir_transform/src/coverage/query.rs @@ -47,39 +47,24 @@ impl CoverageVisitor { /// final computed number of counters should be the number of all `CoverageKind::Counter` /// statements in the MIR *plus one* for the implicit `ZERO` counter. #[inline(always)] - fn update_num_counters(&mut self, counter_id: u32) { + fn update_num_counters(&mut self, counter_id: CounterValueReference) { + let counter_id = counter_id.as_u32(); self.info.num_counters = std::cmp::max(self.info.num_counters, counter_id + 1); } /// Computes an expression index for each expression ID, and updates `num_expressions` to the /// maximum encountered index plus 1. #[inline(always)] - fn update_num_expressions(&mut self, expression_id: u32) { - let expression_index = u32::MAX - expression_id; + fn update_num_expressions(&mut self, expression_id: InjectedExpressionId) { + let expression_index = u32::MAX - expression_id.as_u32(); self.info.num_expressions = std::cmp::max(self.info.num_expressions, expression_index + 1); } - fn update_from_expression_operand(&mut self, operand_id: u32) { - if operand_id >= self.info.num_counters { - let operand_as_expression_index = u32::MAX - operand_id; - if operand_as_expression_index >= self.info.num_expressions { - // The operand ID is outside the known range of counter IDs and also outside the - // known range of expression IDs. In either case, the result of a missing operand - // (if and when used in an expression) will be zero, so from a computation - // perspective, it doesn't matter whether it is interpreted as a counter or an - // expression. - // - // However, the `num_counters` and `num_expressions` query results are used to - // allocate arrays when generating the coverage map (during codegen), so choose - // the type that grows either `num_counters` or `num_expressions` the least. - if operand_id - self.info.num_counters - < operand_as_expression_index - self.info.num_expressions - { - self.update_num_counters(operand_id) - } else { - self.update_num_expressions(operand_id) - } - } + fn update_from_expression_operand(&mut self, operand: Operand) { + match operand { + Operand::Counter(id) => self.update_num_counters(id), + Operand::Expression(id) => self.update_num_expressions(id), + Operand::Zero => {} } } @@ -100,19 +85,15 @@ impl CoverageVisitor { if self.add_missing_operands { match coverage.kind { CoverageKind::Expression { lhs, rhs, .. } => { - self.update_from_expression_operand(u32::from(lhs)); - self.update_from_expression_operand(u32::from(rhs)); + self.update_from_expression_operand(lhs); + self.update_from_expression_operand(rhs); } _ => {} } } else { match coverage.kind { - CoverageKind::Counter { id, .. } => { - self.update_num_counters(u32::from(id)); - } - CoverageKind::Expression { id, .. } => { - self.update_num_expressions(u32::from(id)); - } + CoverageKind::Counter { id, .. } => self.update_num_counters(id), + CoverageKind::Expression { id, .. } => self.update_num_expressions(id), _ => {} } } diff --git a/tests/mir-opt/coverage_graphviz.main.InstrumentCoverage.0.dot b/tests/mir-opt/coverage_graphviz.main.InstrumentCoverage.0.dot index c4d389b2d7648..5d19b863114e8 100644 --- a/tests/mir-opt/coverage_graphviz.main.InstrumentCoverage.0.dot +++ b/tests/mir-opt/coverage_graphviz.main.InstrumentCoverage.0.dot @@ -3,7 +3,7 @@ digraph Cov_0_3 { node [fontname="Courier, monospace"]; edge [fontname="Courier, monospace"]; bcb3__Cov_0_3 [shape="none", label=<
bcb3
Counter(bcb3) at 13:10-13:10
13:10-13:10: @5[0]: Coverage::Counter(2) for $DIR/coverage_graphviz.rs:13:10 - 13:11
bb5: Goto
>]; - bcb2__Cov_0_3 [shape="none", label=<
bcb2
Expression(bcb1:(bcb0 + bcb3) - bcb3) at 12:13-12:18
12:13-12:18: @4[0]: Coverage::Expression(4294967293) = 4294967294 + 0 for $DIR/coverage_graphviz.rs:15:1 - 15:2
Expression(bcb2:(bcb1:(bcb0 + bcb3) - bcb3) + 0) at 15:2-15:2
15:2-15:2: @4.Return: return
bb4: Return
>]; + bcb2__Cov_0_3 [shape="none", label=<
bcb2
Expression(bcb1:(bcb0 + bcb3) - bcb3) at 12:13-12:18
12:13-12:18: @4[0]: Coverage::Expression(4294967293) = Expression(4294967294) + Zero for $DIR/coverage_graphviz.rs:15:1 - 15:2
Expression(bcb2:(bcb1:(bcb0 + bcb3) - bcb3) + 0) at 15:2-15:2
15:2-15:2: @4.Return: return
bb4: Return
>]; bcb1__Cov_0_3 [shape="none", label=<
bcb1
Expression(bcb0 + bcb3) at 10:5-11:17
11:12-11:17: @2.Call: _2 = bar() -> [return: bb3, unwind: bb6]
bb1: FalseUnwind
bb2: Call
bb3: SwitchInt
>]; bcb0__Cov_0_3 [shape="none", label=<
bcb0
Counter(bcb0) at 9:1-9:11
bb0: Goto
>]; bcb3__Cov_0_3 -> bcb1__Cov_0_3 [label=<>]; diff --git a/tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff b/tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff index 7ec9011a526c5..5bd271456c6bd 100644 --- a/tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff +++ b/tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff @@ -13,7 +13,7 @@ } bb1: { -+ Coverage::Expression(4294967295) = 1 + 2 for /the/src/instrument_coverage.rs:12:5 - 13:17; ++ Coverage::Expression(4294967295) = Counter(1) + Counter(2) for /the/src/instrument_coverage.rs:12:5 - 13:17; falseUnwind -> [real: bb2, unwind: bb6]; } @@ -27,8 +27,8 @@ } bb4: { -+ Coverage::Expression(4294967293) = 4294967294 + 0 for /the/src/instrument_coverage.rs:17:1 - 17:2; -+ Coverage::Expression(4294967294) = 4294967295 - 2 for /the/src/instrument_coverage.rs:14:13 - 14:18; ++ Coverage::Expression(4294967293) = Expression(4294967294) + Zero for /the/src/instrument_coverage.rs:17:1 - 17:2; ++ Coverage::Expression(4294967294) = Expression(4294967295) - Counter(2) for /the/src/instrument_coverage.rs:14:13 - 14:18; _0 = const (); StorageDead(_2); return; From f103db894fdcf94822d57cf28e30bc498c042631 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Thu, 29 Jun 2023 12:14:04 +1000 Subject: [PATCH 06/11] Make coverage expression IDs count up from 0, not down from `u32::MAX` Operand types are now tracked explicitly, so there is no need for expression IDs to avoid counter IDs by descending from `u32::MAX`. Instead they can just count up from 0, and can be used directly as indices when necessary. --- .../src/coverageinfo/map_data.rs | 49 ++++++------------- .../src/coverageinfo/mod.rs | 4 +- compiler/rustc_middle/src/mir/coverage.rs | 26 +++++----- .../rustc_middle/src/ty/structural_impls.rs | 3 +- .../src/coverage/counters.rs | 19 +++---- .../rustc_mir_transform/src/coverage/debug.rs | 2 +- .../rustc_mir_transform/src/coverage/query.rs | 9 ++-- ...age_graphviz.main.InstrumentCoverage.0.dot | 2 +- ...ment_coverage.main.InstrumentCoverage.diff | 6 +-- 9 files changed, 49 insertions(+), 71 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs index 0d4d8ca61e939..4fada3293670a 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs @@ -3,8 +3,7 @@ pub use super::ffi::*; use rustc_index::{IndexSlice, IndexVec}; use rustc_middle::bug; use rustc_middle::mir::coverage::{ - CodeRegion, CounterValueReference, InjectedExpressionId, InjectedExpressionIndex, - MappedExpressionIndex, Op, Operand, + CodeRegion, CounterValueReference, ExpressionId, MappedExpressionIndex, Op, Operand, }; use rustc_middle::ty::Instance; use rustc_middle::ty::TyCtxt; @@ -19,8 +18,7 @@ pub struct Expression { /// Collects all of the coverage regions associated with (a) injected counters, (b) counter /// expressions (additions or subtraction), and (c) unreachable regions (always counted as zero), -/// for a given Function. Counters and counter expressions have non-overlapping `id`s because they -/// can both be operands in an expression. This struct also stores the `function_source_hash`, +/// for a given Function. This struct also stores the `function_source_hash`, /// computed during instrumentation, and forwarded with counters. /// /// Note, it may be important to understand LLVM's definitions of `unreachable` regions versus "gap @@ -35,7 +33,7 @@ pub struct FunctionCoverage<'tcx> { source_hash: u64, is_used: bool, counters: IndexVec>, - expressions: IndexVec>, + expressions: IndexVec>, unreachable_regions: Vec, } @@ -89,22 +87,11 @@ impl<'tcx> FunctionCoverage<'tcx> { } /// Both counters and "counter expressions" (or simply, "expressions") can be operands in other - /// expressions. Expression IDs start from `u32::MAX` and go down, so the range of expression - /// IDs will not overlap with the range of counter IDs. Counters and expressions can be added in - /// any order, and expressions can still be assigned contiguous (though descending) IDs, without - /// knowing what the last counter ID will be. - /// - /// When storing the expression data in the `expressions` vector in the `FunctionCoverage` - /// struct, its vector index is computed, from the given expression ID, by subtracting from - /// `u32::MAX`. - /// - /// Since the expression operands (`lhs` and `rhs`) can reference either counters or - /// expressions, an operand that references an expression also uses its original ID, descending - /// from `u32::MAX`. Theses operands are translated only during code generation, after all - /// counters and expressions have been added. + /// expressions. These are tracked as separate variants of `Operand`, so there is no ambiguity + /// between operands that are counter IDs and operands that are expression IDs. pub fn add_counter_expression( &mut self, - expression_id: InjectedExpressionId, + expression_id: ExpressionId, lhs: Operand, op: Op, rhs: Operand, @@ -114,16 +101,15 @@ impl<'tcx> FunctionCoverage<'tcx> { "add_counter_expression({:?}, lhs={:?}, op={:?}, rhs={:?} at {:?}", expression_id, lhs, op, rhs, region ); - let expression_index = self.expression_index(expression_id); debug_assert!( - expression_index.as_usize() < self.expressions.len(), - "expression_index {} is out of range for expressions.len() = {} + expression_id.as_usize() < self.expressions.len(), + "expression_id {} is out of range for expressions.len() = {} for {:?}", - expression_index.as_usize(), + expression_id.as_usize(), self.expressions.len(), self, ); - if let Some(previous_expression) = self.expressions[expression_index].replace(Expression { + if let Some(previous_expression) = self.expressions[expression_id].replace(Expression { lhs, op, rhs, @@ -190,7 +176,7 @@ impl<'tcx> FunctionCoverage<'tcx> { // // Expressions will be returned from this function in a sequential vector (array) of // `CounterExpression`, so the expression IDs must be mapped from their original, - // potentially sparse set of indexes, originally in reverse order from `u32::MAX`. + // potentially sparse set of indexes. // // An `Expression` as an operand will have already been encountered as an `Expression` with // operands, so its new_index will already have been generated (as a 1-up index value). @@ -203,7 +189,7 @@ impl<'tcx> FunctionCoverage<'tcx> { // `expression_index`s lower than the referencing `Expression`. Therefore, it is // reasonable to look up the new index of an expression operand while the `new_indexes` // vector is only complete up to the current `ExpressionIndex`. - type NewIndexes = IndexSlice>; + type NewIndexes = IndexSlice>; let id_to_counter = |new_indexes: &NewIndexes, operand: Operand| match operand { Operand::Zero => Some(Counter::zero()), Operand::Counter(id) => { @@ -219,15 +205,14 @@ impl<'tcx> FunctionCoverage<'tcx> { Some(Counter::counter_value_reference(index)) } Operand::Expression(id) => { - let index = self.expression_index(id); self.expressions - .get(index) + .get(id) .expect("expression id is out of range") .as_ref() // If an expression was optimized out, assume it would have produced a count // of zero. This ensures that expressions dependent on optimized-out // expressions are still valid. - .map_or(Some(Counter::zero()), |_| new_indexes[index].map(Counter::expression)) + .map_or(Some(Counter::zero()), |_| new_indexes[id].map(Counter::expression)) } }; @@ -334,10 +319,4 @@ impl<'tcx> FunctionCoverage<'tcx> { fn unreachable_regions(&self) -> impl Iterator { self.unreachable_regions.iter().map(|region| (Counter::zero(), region)) } - - fn expression_index(&self, id: InjectedExpressionId) -> InjectedExpressionIndex { - debug_assert!(id.as_usize() >= self.counters.len()); - let id_descending_from_max = id.as_u32(); - InjectedExpressionIndex::from(u32::MAX - id_descending_from_max) - } } diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs index d6a32497f7ff6..5e0c5df2bcb96 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs @@ -17,7 +17,7 @@ use rustc_hir::def_id::DefId; use rustc_llvm::RustString; use rustc_middle::bug; use rustc_middle::mir::coverage::{ - CodeRegion, CounterValueReference, CoverageKind, InjectedExpressionId, Op, Operand, + CodeRegion, CounterValueReference, CoverageKind, ExpressionId, Op, Operand, }; use rustc_middle::mir::Coverage; use rustc_middle::ty; @@ -202,7 +202,7 @@ impl<'tcx> Builder<'_, '_, 'tcx> { fn add_coverage_counter_expression( &mut self, instance: Instance<'tcx>, - id: InjectedExpressionId, + id: ExpressionId, lhs: Operand, op: Op, rhs: Operand, diff --git a/compiler/rustc_middle/src/mir/coverage.rs b/compiler/rustc_middle/src/mir/coverage.rs index 7894e564b02fe..bcec46d9149a0 100644 --- a/compiler/rustc_middle/src/mir/coverage.rs +++ b/compiler/rustc_middle/src/mir/coverage.rs @@ -26,19 +26,23 @@ impl CounterValueReference { } rustc_index::newtype_index! { - /// Values descend from u32::MAX. + /// ID of a coverage-counter expression. Values ascend from 0. + /// + /// Note that LLVM handles expression IDs as `uint32_t`, so there is no need + /// to use a larger representation on the Rust side. #[derive(HashStable)] #[max = 0xFFFF_FFFF] - #[debug_format = "InjectedExpressionId({})"] - pub struct InjectedExpressionId {} + #[debug_format = "ExpressionId({})"] + pub struct ExpressionId {} } -rustc_index::newtype_index! { - /// Values ascend from 0. - #[derive(HashStable)] - #[max = 0xFFFF_FFFF] - #[debug_format = "InjectedExpressionIndex({})"] - pub struct InjectedExpressionIndex {} +impl ExpressionId { + pub const START: Self = Self::from_u32(0); + + #[inline(always)] + pub fn next_id(self) -> Self { + Self::from_u32(self.as_u32() + 1) + } } rustc_index::newtype_index! { @@ -60,7 +64,7 @@ rustc_index::newtype_index! { pub enum Operand { Zero, Counter(CounterValueReference), - Expression(InjectedExpressionId), + Expression(ExpressionId), } impl Debug for Operand { @@ -84,7 +88,7 @@ pub enum CoverageKind { Expression { /// ID of this coverage-counter expression within its enclosing function. /// Other expressions in the same function can refer to it as an operand. - id: InjectedExpressionId, + id: ExpressionId, lhs: Operand, op: Op, rhs: Operand, diff --git a/compiler/rustc_middle/src/ty/structural_impls.rs b/compiler/rustc_middle/src/ty/structural_impls.rs index 10cff0e2f500e..15cf4c4678880 100644 --- a/compiler/rustc_middle/src/ty/structural_impls.rs +++ b/compiler/rustc_middle/src/ty/structural_impls.rs @@ -471,8 +471,7 @@ TrivialTypeTraversalAndLiftImpls! { ::rustc_target::asm::InlineAsmRegOrRegClass, ::rustc_target::spec::abi::Abi, crate::mir::coverage::CounterValueReference, - crate::mir::coverage::InjectedExpressionId, - crate::mir::coverage::InjectedExpressionIndex, + crate::mir::coverage::ExpressionId, crate::mir::coverage::MappedExpressionIndex, crate::mir::Local, crate::mir::Promoted, diff --git a/compiler/rustc_mir_transform/src/coverage/counters.rs b/compiler/rustc_mir_transform/src/coverage/counters.rs index 9d168404264aa..cd2539a5e6639 100644 --- a/compiler/rustc_mir_transform/src/coverage/counters.rs +++ b/compiler/rustc_mir_transform/src/coverage/counters.rs @@ -17,7 +17,7 @@ use rustc_middle::mir::coverage::*; pub(super) struct CoverageCounters { function_source_hash: u64, next_counter_id: u32, - num_expressions: u32, + next_expression_id: ExpressionId, pub debug_counters: DebugCounters, } @@ -26,7 +26,7 @@ impl CoverageCounters { Self { function_source_hash, next_counter_id: CounterValueReference::START.as_u32(), - num_expressions: 0, + next_expression_id: ExpressionId::START, debug_counters: DebugCounters::new(), } } @@ -94,20 +94,17 @@ impl CoverageCounters { /// Counter IDs start from one and go up. fn next_counter(&mut self) -> CounterValueReference { - assert!(self.next_counter_id < u32::MAX - self.num_expressions); let next = self.next_counter_id; self.next_counter_id += 1; CounterValueReference::from(next) } - /// Expression IDs start from u32::MAX and go down because an Expression can reference - /// (add or subtract counts) of both Counter regions and Expression regions. The counter - /// expression operand IDs must be unique across both types. - fn next_expression(&mut self) -> InjectedExpressionId { - assert!(self.next_counter_id < u32::MAX - self.num_expressions); - let next = u32::MAX - self.num_expressions; - self.num_expressions += 1; - InjectedExpressionId::from(next) + /// Expression IDs start from 0 and go up. + /// (Counter IDs and Expression IDs are distinguished by the `Operand` enum.) + fn next_expression(&mut self) -> ExpressionId { + let next = self.next_expression_id; + self.next_expression_id = next.next_id(); + next } } diff --git a/compiler/rustc_mir_transform/src/coverage/debug.rs b/compiler/rustc_mir_transform/src/coverage/debug.rs index 0e1065a40e704..26f9cfd0b86c2 100644 --- a/compiler/rustc_mir_transform/src/coverage/debug.rs +++ b/compiler/rustc_mir_transform/src/coverage/debug.rs @@ -485,7 +485,7 @@ impl GraphvizData { /// _not_ used are retained in the `unused_expressions` Vec, to be included in debug output (logs /// and/or a `CoverageGraph` graphviz output). pub(super) struct UsedExpressions { - some_used_expression_operands: Option>>, + some_used_expression_operands: Option>>, some_unused_expressions: Option, BasicCoverageBlock)>>, } diff --git a/compiler/rustc_mir_transform/src/coverage/query.rs b/compiler/rustc_mir_transform/src/coverage/query.rs index 1261cf2765ca9..c1a896dbd349e 100644 --- a/compiler/rustc_mir_transform/src/coverage/query.rs +++ b/compiler/rustc_mir_transform/src/coverage/query.rs @@ -52,12 +52,11 @@ impl CoverageVisitor { self.info.num_counters = std::cmp::max(self.info.num_counters, counter_id + 1); } - /// Computes an expression index for each expression ID, and updates `num_expressions` to the - /// maximum encountered index plus 1. + /// Updates `num_expressions` to the maximum encountered expression ID plus 1. #[inline(always)] - fn update_num_expressions(&mut self, expression_id: InjectedExpressionId) { - let expression_index = u32::MAX - expression_id.as_u32(); - self.info.num_expressions = std::cmp::max(self.info.num_expressions, expression_index + 1); + fn update_num_expressions(&mut self, expression_id: ExpressionId) { + let expression_id = expression_id.as_u32(); + self.info.num_expressions = std::cmp::max(self.info.num_expressions, expression_id + 1); } fn update_from_expression_operand(&mut self, operand: Operand) { diff --git a/tests/mir-opt/coverage_graphviz.main.InstrumentCoverage.0.dot b/tests/mir-opt/coverage_graphviz.main.InstrumentCoverage.0.dot index 5d19b863114e8..affd0fb18d573 100644 --- a/tests/mir-opt/coverage_graphviz.main.InstrumentCoverage.0.dot +++ b/tests/mir-opt/coverage_graphviz.main.InstrumentCoverage.0.dot @@ -3,7 +3,7 @@ digraph Cov_0_3 { node [fontname="Courier, monospace"]; edge [fontname="Courier, monospace"]; bcb3__Cov_0_3 [shape="none", label=<
bcb3
Counter(bcb3) at 13:10-13:10
13:10-13:10: @5[0]: Coverage::Counter(2) for $DIR/coverage_graphviz.rs:13:10 - 13:11
bb5: Goto
>]; - bcb2__Cov_0_3 [shape="none", label=<
bcb2
Expression(bcb1:(bcb0 + bcb3) - bcb3) at 12:13-12:18
12:13-12:18: @4[0]: Coverage::Expression(4294967293) = Expression(4294967294) + Zero for $DIR/coverage_graphviz.rs:15:1 - 15:2
Expression(bcb2:(bcb1:(bcb0 + bcb3) - bcb3) + 0) at 15:2-15:2
15:2-15:2: @4.Return: return
bb4: Return
>]; + bcb2__Cov_0_3 [shape="none", label=<
bcb2
Expression(bcb1:(bcb0 + bcb3) - bcb3) at 12:13-12:18
12:13-12:18: @4[0]: Coverage::Expression(2) = Expression(1) + Zero for $DIR/coverage_graphviz.rs:15:1 - 15:2
Expression(bcb2:(bcb1:(bcb0 + bcb3) - bcb3) + 0) at 15:2-15:2
15:2-15:2: @4.Return: return
bb4: Return
>]; bcb1__Cov_0_3 [shape="none", label=<
bcb1
Expression(bcb0 + bcb3) at 10:5-11:17
11:12-11:17: @2.Call: _2 = bar() -> [return: bb3, unwind: bb6]
bb1: FalseUnwind
bb2: Call
bb3: SwitchInt
>]; bcb0__Cov_0_3 [shape="none", label=<
bcb0
Counter(bcb0) at 9:1-9:11
bb0: Goto
>]; bcb3__Cov_0_3 -> bcb1__Cov_0_3 [label=<>]; diff --git a/tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff b/tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff index 5bd271456c6bd..e87ed5268e069 100644 --- a/tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff +++ b/tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff @@ -13,7 +13,7 @@ } bb1: { -+ Coverage::Expression(4294967295) = Counter(1) + Counter(2) for /the/src/instrument_coverage.rs:12:5 - 13:17; ++ Coverage::Expression(0) = Counter(1) + Counter(2) for /the/src/instrument_coverage.rs:12:5 - 13:17; falseUnwind -> [real: bb2, unwind: bb6]; } @@ -27,8 +27,8 @@ } bb4: { -+ Coverage::Expression(4294967293) = Expression(4294967294) + Zero for /the/src/instrument_coverage.rs:17:1 - 17:2; -+ Coverage::Expression(4294967294) = Expression(4294967295) - Counter(2) for /the/src/instrument_coverage.rs:14:13 - 14:18; ++ Coverage::Expression(2) = Expression(1) + Zero for /the/src/instrument_coverage.rs:17:1 - 17:2; ++ Coverage::Expression(1) = Expression(0) - Counter(2) for /the/src/instrument_coverage.rs:14:13 - 14:18; _0 = const (); StorageDead(_2); return; From 3920e07f0bd97d9815a037eaeea197266424cd56 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Thu, 29 Jun 2023 12:36:19 +1000 Subject: [PATCH 07/11] Make coverage counter IDs count up from 0, not 1 Operand types are now tracked explicitly, so there is no need to reserve ID 0 for the special always-zero counter. As part of the renumbering, this change fixes an off-by-one error in the way counters were counted by the `coverageinfo` query. As a result, functions should now have exactly the number of counters they actually need, instead of always having an extra counter that is never used. --- .../src/coverageinfo/ffi.rs | 10 +++---- .../src/coverageinfo/map_data.rs | 19 +++----------- .../src/coverageinfo/mod.rs | 10 +++---- compiler/rustc_middle/src/mir/coverage.rs | 26 +++++++++---------- .../rustc_middle/src/ty/structural_impls.rs | 2 +- .../src/coverage/counters.rs | 10 +++---- .../rustc_mir_transform/src/coverage/query.rs | 9 +++---- .../rustc_mir_transform/src/coverage/tests.rs | 4 +-- ...rage_graphviz.bar.InstrumentCoverage.0.dot | 2 +- ...age_graphviz.main.InstrumentCoverage.0.dot | 2 +- ...ument_coverage.bar.InstrumentCoverage.diff | 2 +- ...ment_coverage.main.InstrumentCoverage.diff | 8 +++--- 12 files changed, 43 insertions(+), 61 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs index 1791ce4b31559..6ca61aa43d7b7 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs @@ -1,4 +1,4 @@ -use rustc_middle::mir::coverage::{CounterValueReference, MappedExpressionIndex}; +use rustc_middle::mir::coverage::{CounterId, MappedExpressionIndex}; /// Must match the layout of `LLVMRustCounterKind`. #[derive(Copy, Clone, Debug)] @@ -36,11 +36,9 @@ impl Counter { Self { kind: CounterKind::Zero, id: 0 } } - /// Constructs a new `Counter` of kind `CounterValueReference`, and converts - /// the given 1-based counter_id to the required 0-based equivalent for - /// the `Counter` encoding. - pub fn counter_value_reference(counter_id: CounterValueReference) -> Self { - Self { kind: CounterKind::CounterValueReference, id: counter_id.zero_based_index() } + /// Constructs a new `Counter` of kind `CounterValueReference`. + pub fn counter_value_reference(counter_id: CounterId) -> Self { + Self { kind: CounterKind::CounterValueReference, id: counter_id.as_u32() } } /// Constructs a new `Counter` of kind `Expression`. diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs index 4fada3293670a..7e981af0a5310 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs @@ -3,7 +3,7 @@ pub use super::ffi::*; use rustc_index::{IndexSlice, IndexVec}; use rustc_middle::bug; use rustc_middle::mir::coverage::{ - CodeRegion, CounterValueReference, ExpressionId, MappedExpressionIndex, Op, Operand, + CodeRegion, CounterId, ExpressionId, MappedExpressionIndex, Op, Operand, }; use rustc_middle::ty::Instance; use rustc_middle::ty::TyCtxt; @@ -32,7 +32,7 @@ pub struct FunctionCoverage<'tcx> { instance: Instance<'tcx>, source_hash: u64, is_used: bool, - counters: IndexVec>, + counters: IndexVec>, expressions: IndexVec>, unreachable_regions: Vec, } @@ -80,7 +80,7 @@ impl<'tcx> FunctionCoverage<'tcx> { } /// Adds a code region to be counted by an injected counter intrinsic. - pub fn add_counter(&mut self, id: CounterValueReference, region: CodeRegion) { + pub fn add_counter(&mut self, id: CounterId, region: CodeRegion) { if let Some(previous_region) = self.counters[id].replace(region.clone()) { assert_eq!(previous_region, region, "add_counter: code region for id changed"); } @@ -192,18 +192,7 @@ impl<'tcx> FunctionCoverage<'tcx> { type NewIndexes = IndexSlice>; let id_to_counter = |new_indexes: &NewIndexes, operand: Operand| match operand { Operand::Zero => Some(Counter::zero()), - Operand::Counter(id) => { - debug_assert!( - id.index() > 0, - "Operand::Counter indexes for counters are 1-based, but this id={}", - id.index() - ); - // Note: Some codegen-injected Counters may be only referenced by `Expression`s, - // and may not have their own `CodeRegion`s, - let index = CounterValueReference::from(id.index()); - // Note, the conversion to LLVM `Counter` adjusts the index to be zero-based. - Some(Counter::counter_value_reference(index)) - } + Operand::Counter(id) => Some(Counter::counter_value_reference(id)), Operand::Expression(id) => { self.expressions .get(id) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs index 5e0c5df2bcb96..c1017ab8d7cae 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs @@ -16,9 +16,7 @@ use rustc_hir as hir; use rustc_hir::def_id::DefId; use rustc_llvm::RustString; use rustc_middle::bug; -use rustc_middle::mir::coverage::{ - CodeRegion, CounterValueReference, CoverageKind, ExpressionId, Op, Operand, -}; +use rustc_middle::mir::coverage::{CodeRegion, CounterId, CoverageKind, ExpressionId, Op, Operand}; use rustc_middle::mir::Coverage; use rustc_middle::ty; use rustc_middle::ty::layout::{FnAbiOf, HasTyCtxt}; @@ -33,7 +31,7 @@ mod ffi; pub(crate) mod map_data; pub mod mapgen; -const UNUSED_FUNCTION_COUNTER_ID: CounterValueReference = CounterValueReference::START; +const UNUSED_FUNCTION_COUNTER_ID: CounterId = CounterId::START; const VAR_ALIGN_BYTES: usize = 8; @@ -125,7 +123,7 @@ impl<'tcx> CoverageInfoBuilderMethods<'tcx> for Builder<'_, '_, 'tcx> { let fn_name = bx.get_pgo_func_name_var(instance); let hash = bx.const_u64(function_source_hash); let num_counters = bx.const_u32(coverageinfo.num_counters); - let index = bx.const_u32(id.zero_based_index()); + let index = bx.const_u32(id.as_u32()); debug!( "codegen intrinsic instrprof.increment(fn_name={:?}, hash={:?}, num_counters={:?}, index={:?})", fn_name, hash, num_counters, index, @@ -178,7 +176,7 @@ impl<'tcx> Builder<'_, '_, 'tcx> { fn add_coverage_counter( &mut self, instance: Instance<'tcx>, - id: CounterValueReference, + id: CounterId, region: CodeRegion, ) -> bool { if let Some(coverage_context) = self.coverage_context() { diff --git a/compiler/rustc_middle/src/mir/coverage.rs b/compiler/rustc_middle/src/mir/coverage.rs index bcec46d9149a0..d7d6e3a0086cd 100644 --- a/compiler/rustc_middle/src/mir/coverage.rs +++ b/compiler/rustc_middle/src/mir/coverage.rs @@ -6,22 +6,22 @@ use rustc_span::Symbol; use std::fmt::{self, Debug, Formatter}; rustc_index::newtype_index! { + /// ID of a coverage counter. Values ascend from 0. + /// + /// Note that LLVM handles counter IDs as `uint32_t`, so there is no need + /// to use a larger representation on the Rust side. #[derive(HashStable)] #[max = 0xFFFF_FFFF] - #[debug_format = "CounterValueReference({})"] - pub struct CounterValueReference {} + #[debug_format = "CounterId({})"] + pub struct CounterId {} } -impl CounterValueReference { - /// Counters start at 1 for historical reasons. - pub const START: Self = Self::from_u32(1); +impl CounterId { + pub const START: Self = Self::from_u32(0); - /// Returns explicitly-requested zero-based version of the counter id, used - /// during codegen. LLVM expects zero-based indexes. - pub fn zero_based_index(self) -> u32 { - let one_based_index = self.as_u32(); - debug_assert!(one_based_index > 0); - one_based_index - 1 + #[inline(always)] + pub fn next_id(self) -> Self { + Self::from_u32(self.as_u32() + 1) } } @@ -63,7 +63,7 @@ rustc_index::newtype_index! { #[derive(TyEncodable, TyDecodable, Hash, HashStable, TypeFoldable, TypeVisitable)] pub enum Operand { Zero, - Counter(CounterValueReference), + Counter(CounterId), Expression(ExpressionId), } @@ -83,7 +83,7 @@ pub enum CoverageKind { function_source_hash: u64, /// ID of this counter within its enclosing function. /// Expressions in the same function can refer to it as an operand. - id: CounterValueReference, + id: CounterId, }, Expression { /// ID of this coverage-counter expression within its enclosing function. diff --git a/compiler/rustc_middle/src/ty/structural_impls.rs b/compiler/rustc_middle/src/ty/structural_impls.rs index 15cf4c4678880..c6b6f0e89905c 100644 --- a/compiler/rustc_middle/src/ty/structural_impls.rs +++ b/compiler/rustc_middle/src/ty/structural_impls.rs @@ -470,7 +470,7 @@ TrivialTypeTraversalAndLiftImpls! { ::rustc_hir::Unsafety, ::rustc_target::asm::InlineAsmRegOrRegClass, ::rustc_target::spec::abi::Abi, - crate::mir::coverage::CounterValueReference, + crate::mir::coverage::CounterId, crate::mir::coverage::ExpressionId, crate::mir::coverage::MappedExpressionIndex, crate::mir::Local, diff --git a/compiler/rustc_mir_transform/src/coverage/counters.rs b/compiler/rustc_mir_transform/src/coverage/counters.rs index cd2539a5e6639..97bdb878ab17a 100644 --- a/compiler/rustc_mir_transform/src/coverage/counters.rs +++ b/compiler/rustc_mir_transform/src/coverage/counters.rs @@ -16,7 +16,7 @@ use rustc_middle::mir::coverage::*; /// `Coverage` statements. pub(super) struct CoverageCounters { function_source_hash: u64, - next_counter_id: u32, + next_counter_id: CounterId, next_expression_id: ExpressionId, pub debug_counters: DebugCounters, } @@ -25,7 +25,7 @@ impl CoverageCounters { pub fn new(function_source_hash: u64) -> Self { Self { function_source_hash, - next_counter_id: CounterValueReference::START.as_u32(), + next_counter_id: CounterId::START, next_expression_id: ExpressionId::START, debug_counters: DebugCounters::new(), } @@ -93,10 +93,10 @@ impl CoverageCounters { } /// Counter IDs start from one and go up. - fn next_counter(&mut self) -> CounterValueReference { + fn next_counter(&mut self) -> CounterId { let next = self.next_counter_id; - self.next_counter_id += 1; - CounterValueReference::from(next) + self.next_counter_id = next.next_id(); + next } /// Expression IDs start from 0 and go up. diff --git a/compiler/rustc_mir_transform/src/coverage/query.rs b/compiler/rustc_mir_transform/src/coverage/query.rs index c1a896dbd349e..aa205655f9dab 100644 --- a/compiler/rustc_mir_transform/src/coverage/query.rs +++ b/compiler/rustc_mir_transform/src/coverage/query.rs @@ -43,11 +43,9 @@ struct CoverageVisitor { } impl CoverageVisitor { - /// Updates `num_counters` to the maximum encountered zero-based counter_id plus 1. Note the - /// final computed number of counters should be the number of all `CoverageKind::Counter` - /// statements in the MIR *plus one* for the implicit `ZERO` counter. + /// Updates `num_counters` to the maximum encountered counter ID plus 1. #[inline(always)] - fn update_num_counters(&mut self, counter_id: CounterValueReference) { + fn update_num_counters(&mut self, counter_id: CounterId) { let counter_id = counter_id.as_u32(); self.info.num_counters = std::cmp::max(self.info.num_counters, counter_id + 1); } @@ -103,8 +101,7 @@ fn coverageinfo<'tcx>(tcx: TyCtxt<'tcx>, instance_def: ty::InstanceDef<'tcx>) -> let mir_body = tcx.instance_mir(instance_def); let mut coverage_visitor = CoverageVisitor { - // num_counters always has at least the `ZERO` counter. - info: CoverageInfo { num_counters: 1, num_expressions: 0 }, + info: CoverageInfo { num_counters: 0, num_expressions: 0 }, add_missing_operands: false, }; diff --git a/compiler/rustc_mir_transform/src/coverage/tests.rs b/compiler/rustc_mir_transform/src/coverage/tests.rs index 25891d3ca0f88..248a192f8f571 100644 --- a/compiler/rustc_mir_transform/src/coverage/tests.rs +++ b/compiler/rustc_mir_transform/src/coverage/tests.rs @@ -683,7 +683,7 @@ fn test_make_bcb_counters() { let_bcb!(1); assert_eq!( - 1, // coincidentally, bcb1 has a `Counter` with id = 1 + 0, // bcb1 has a `Counter` with id = 0 match basic_coverage_blocks[bcb1].counter().expect("should have a counter") { CoverageKind::Counter { id, .. } => id, _ => panic!("expected a Counter"), @@ -693,7 +693,7 @@ fn test_make_bcb_counters() { let_bcb!(2); assert_eq!( - 2, // coincidentally, bcb2 has a `Counter` with id = 2 + 1, // bcb2 has a `Counter` with id = 1 match basic_coverage_blocks[bcb2].counter().expect("should have a counter") { CoverageKind::Counter { id, .. } => id, _ => panic!("expected a Counter"), diff --git a/tests/mir-opt/coverage_graphviz.bar.InstrumentCoverage.0.dot b/tests/mir-opt/coverage_graphviz.bar.InstrumentCoverage.0.dot index 03df5c9504be2..3b90aaeae4236 100644 --- a/tests/mir-opt/coverage_graphviz.bar.InstrumentCoverage.0.dot +++ b/tests/mir-opt/coverage_graphviz.bar.InstrumentCoverage.0.dot @@ -2,5 +2,5 @@ digraph Cov_0_4 { graph [fontname="Courier, monospace"]; node [fontname="Courier, monospace"]; edge [fontname="Courier, monospace"]; - bcb0__Cov_0_4 [shape="none", label=<
bcb0
Counter(bcb0) at 18:1-20:2
19:5-19:9: @0[0]: Coverage::Counter(1) for $DIR/coverage_graphviz.rs:18:1 - 20:2
20:2-20:2: @0.Return: return
bb0: Return
>]; + bcb0__Cov_0_4 [shape="none", label=<
bcb0
Counter(bcb0) at 18:1-20:2
19:5-19:9: @0[0]: Coverage::Counter(0) for $DIR/coverage_graphviz.rs:18:1 - 20:2
20:2-20:2: @0.Return: return
bb0: Return
>]; } diff --git a/tests/mir-opt/coverage_graphviz.main.InstrumentCoverage.0.dot b/tests/mir-opt/coverage_graphviz.main.InstrumentCoverage.0.dot index affd0fb18d573..19c220e2e1d8e 100644 --- a/tests/mir-opt/coverage_graphviz.main.InstrumentCoverage.0.dot +++ b/tests/mir-opt/coverage_graphviz.main.InstrumentCoverage.0.dot @@ -2,7 +2,7 @@ digraph Cov_0_3 { graph [fontname="Courier, monospace"]; node [fontname="Courier, monospace"]; edge [fontname="Courier, monospace"]; - bcb3__Cov_0_3 [shape="none", label=<
bcb3
Counter(bcb3) at 13:10-13:10
13:10-13:10: @5[0]: Coverage::Counter(2) for $DIR/coverage_graphviz.rs:13:10 - 13:11
bb5: Goto
>]; + bcb3__Cov_0_3 [shape="none", label=<
bcb3
Counter(bcb3) at 13:10-13:10
13:10-13:10: @5[0]: Coverage::Counter(1) for $DIR/coverage_graphviz.rs:13:10 - 13:11
bb5: Goto
>]; bcb2__Cov_0_3 [shape="none", label=<
bcb2
Expression(bcb1:(bcb0 + bcb3) - bcb3) at 12:13-12:18
12:13-12:18: @4[0]: Coverage::Expression(2) = Expression(1) + Zero for $DIR/coverage_graphviz.rs:15:1 - 15:2
Expression(bcb2:(bcb1:(bcb0 + bcb3) - bcb3) + 0) at 15:2-15:2
15:2-15:2: @4.Return: return
bb4: Return
>]; bcb1__Cov_0_3 [shape="none", label=<
bcb1
Expression(bcb0 + bcb3) at 10:5-11:17
11:12-11:17: @2.Call: _2 = bar() -> [return: bb3, unwind: bb6]
bb1: FalseUnwind
bb2: Call
bb3: SwitchInt
>]; bcb0__Cov_0_3 [shape="none", label=<
bcb0
Counter(bcb0) at 9:1-9:11
bb0: Goto
>]; diff --git a/tests/mir-opt/instrument_coverage.bar.InstrumentCoverage.diff b/tests/mir-opt/instrument_coverage.bar.InstrumentCoverage.diff index 0aece766bd5d0..afcfde09c02f1 100644 --- a/tests/mir-opt/instrument_coverage.bar.InstrumentCoverage.diff +++ b/tests/mir-opt/instrument_coverage.bar.InstrumentCoverage.diff @@ -5,7 +5,7 @@ let mut _0: bool; bb0: { -+ Coverage::Counter(1) for /the/src/instrument_coverage.rs:20:1 - 22:2; ++ Coverage::Counter(0) for /the/src/instrument_coverage.rs:20:1 - 22:2; _0 = const true; return; } diff --git a/tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff b/tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff index e87ed5268e069..e17c6ddc56e55 100644 --- a/tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff +++ b/tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff @@ -8,12 +8,12 @@ let mut _3: !; bb0: { -+ Coverage::Counter(1) for /the/src/instrument_coverage.rs:11:1 - 11:11; ++ Coverage::Counter(0) for /the/src/instrument_coverage.rs:11:1 - 11:11; goto -> bb1; } bb1: { -+ Coverage::Expression(0) = Counter(1) + Counter(2) for /the/src/instrument_coverage.rs:12:5 - 13:17; ++ Coverage::Expression(0) = Counter(0) + Counter(1) for /the/src/instrument_coverage.rs:12:5 - 13:17; falseUnwind -> [real: bb2, unwind: bb6]; } @@ -28,14 +28,14 @@ bb4: { + Coverage::Expression(2) = Expression(1) + Zero for /the/src/instrument_coverage.rs:17:1 - 17:2; -+ Coverage::Expression(1) = Expression(0) - Counter(2) for /the/src/instrument_coverage.rs:14:13 - 14:18; ++ Coverage::Expression(1) = Expression(0) - Counter(1) for /the/src/instrument_coverage.rs:14:13 - 14:18; _0 = const (); StorageDead(_2); return; } bb5: { -+ Coverage::Counter(2) for /the/src/instrument_coverage.rs:15:10 - 15:11; ++ Coverage::Counter(1) for /the/src/instrument_coverage.rs:15:10 - 15:11; _1 = const (); StorageDead(_2); goto -> bb1; From 3eb5733ed4799dd636432f5080dd62cf2d401ab7 Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Mon, 31 Jul 2023 09:50:44 +0000 Subject: [PATCH 08/11] Always use parking_lot's RwLock, even without parallel compiler --- .../rustc_data_structures/src/sync/vec.rs | 28 +++++-------------- 1 file changed, 7 insertions(+), 21 deletions(-) diff --git a/compiler/rustc_data_structures/src/sync/vec.rs b/compiler/rustc_data_structures/src/sync/vec.rs index e36dded9e5e5e..314496ce9f095 100644 --- a/compiler/rustc_data_structures/src/sync/vec.rs +++ b/compiler/rustc_data_structures/src/sync/vec.rs @@ -43,37 +43,23 @@ impl AppendOnlyIndexVec { #[derive(Default)] pub struct AppendOnlyVec { - #[cfg(not(parallel_compiler))] - vec: elsa::vec::FrozenVec, - #[cfg(parallel_compiler)] - vec: elsa::sync::LockFreeFrozenVec, + vec: parking_lot::RwLock>, } impl AppendOnlyVec { pub fn new() -> Self { - Self { - #[cfg(not(parallel_compiler))] - vec: elsa::vec::FrozenVec::new(), - #[cfg(parallel_compiler)] - vec: elsa::sync::LockFreeFrozenVec::new(), - } + Self { vec: Default::default() } } pub fn push(&self, val: T) -> usize { - #[cfg(not(parallel_compiler))] - let i = self.vec.len(); - #[cfg(not(parallel_compiler))] - self.vec.push(val); - #[cfg(parallel_compiler)] - let i = self.vec.push(val); - i + let mut v = self.vec.write(); + let n = v.len(); + v.push(val); + n } pub fn get(&self, i: usize) -> Option { - #[cfg(not(parallel_compiler))] - return self.vec.get_copy(i); - #[cfg(parallel_compiler)] - return self.vec.get(i); + self.vec.read().get(i).copied() } pub fn iter_enumerated(&self) -> impl Iterator + '_ { From ad0729e9d21127ba15aa3d927c057b4442156631 Mon Sep 17 00:00:00 2001 From: Urgau Date: Mon, 31 Jul 2023 14:09:18 +0200 Subject: [PATCH 09/11] Improve diagnostic for wrong borrow on binary operations --- compiler/rustc_hir_typeck/src/op.rs | 167 ++++++++++++++---- tests/ui/binop/borrow-suggestion-109352-2.rs | 27 +++ .../binop/borrow-suggestion-109352-2.stderr | 64 +++++++ tests/ui/binop/borrow-suggestion-109352.fixed | 27 +++ tests/ui/binop/borrow-suggestion-109352.rs | 27 +++ .../ui/binop/borrow-suggestion-109352.stderr | 45 +++++ 6 files changed, 326 insertions(+), 31 deletions(-) create mode 100644 tests/ui/binop/borrow-suggestion-109352-2.rs create mode 100644 tests/ui/binop/borrow-suggestion-109352-2.stderr create mode 100644 tests/ui/binop/borrow-suggestion-109352.fixed create mode 100644 tests/ui/binop/borrow-suggestion-109352.rs create mode 100644 tests/ui/binop/borrow-suggestion-109352.stderr diff --git a/compiler/rustc_hir_typeck/src/op.rs b/compiler/rustc_hir_typeck/src/op.rs index 1eae258c1b25a..a283cd1abf5dd 100644 --- a/compiler/rustc_hir_typeck/src/op.rs +++ b/compiler/rustc_hir_typeck/src/op.rs @@ -4,7 +4,7 @@ use super::method::MethodCallee; use super::{has_expected_num_generic_args, FnCtxt}; use crate::Expectation; use rustc_ast as ast; -use rustc_errors::{self, struct_span_err, Applicability, Diagnostic}; +use rustc_errors::{self, struct_span_err, Applicability, Diagnostic, DiagnosticBuilder}; use rustc_hir as hir; use rustc_infer::infer::type_variable::{TypeVariableOrigin, TypeVariableOriginKind}; use rustc_infer::traits::ObligationCauseCode; @@ -380,33 +380,93 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { } }; - let mut suggest_deref_binop = |lhs_deref_ty: Ty<'tcx>| { - if self - .lookup_op_method( - lhs_deref_ty, - Some((rhs_expr, rhs_ty)), - Op::Binary(op, is_assign), - expected, - ) - .is_ok() - { - let msg = format!( - "`{}{}` can be used on `{}` if you dereference the left-hand side", - op.node.as_str(), - match is_assign { - IsAssign::Yes => "=", - IsAssign::No => "", - }, - lhs_deref_ty, - ); - err.span_suggestion_verbose( - lhs_expr.span.shrink_to_lo(), - msg, - "*", - rustc_errors::Applicability::MachineApplicable, - ); - } - }; + let suggest_deref_binop = + |err: &mut DiagnosticBuilder<'_, _>, lhs_deref_ty: Ty<'tcx>| { + if self + .lookup_op_method( + lhs_deref_ty, + Some((rhs_expr, rhs_ty)), + Op::Binary(op, is_assign), + expected, + ) + .is_ok() + { + let msg = format!( + "`{}{}` can be used on `{}` if you dereference the left-hand side", + op.node.as_str(), + match is_assign { + IsAssign::Yes => "=", + IsAssign::No => "", + }, + lhs_deref_ty, + ); + err.span_suggestion_verbose( + lhs_expr.span.shrink_to_lo(), + msg, + "*", + rustc_errors::Applicability::MachineApplicable, + ); + } + }; + + let suggest_different_borrow = + |err: &mut DiagnosticBuilder<'_, _>, + lhs_adjusted_ty, + lhs_new_mutbl: Option, + rhs_adjusted_ty, + rhs_new_mutbl: Option| { + if self + .lookup_op_method( + lhs_adjusted_ty, + Some((rhs_expr, rhs_adjusted_ty)), + Op::Binary(op, is_assign), + expected, + ) + .is_ok() + { + let op_str = op.node.as_str(); + err.note(format!("an implementation for `{lhs_adjusted_ty} {op_str} {rhs_adjusted_ty}` exists")); + + if let Some(lhs_new_mutbl) = lhs_new_mutbl + && let Some(rhs_new_mutbl) = rhs_new_mutbl + && lhs_new_mutbl.is_not() + && rhs_new_mutbl.is_not() { + err.multipart_suggestion_verbose( + "consider reborrowing both sides", + vec![ + (lhs_expr.span.shrink_to_lo(), "&*".to_string()), + (rhs_expr.span.shrink_to_lo(), "&*".to_string()) + ], + rustc_errors::Applicability::MachineApplicable, + ); + } else { + let mut suggest_new_borrow = |new_mutbl: ast::Mutability, sp: Span| { + // Can reborrow (&mut -> &) + if new_mutbl.is_not() { + err.span_suggestion_verbose( + sp.shrink_to_lo(), + "consider reborrowing this side", + "&*", + rustc_errors::Applicability::MachineApplicable, + ); + // Works on &mut but have & + } else { + err.span_help( + sp, + "consider making this expression a mutable borrow", + ); + } + }; + + if let Some(lhs_new_mutbl) = lhs_new_mutbl { + suggest_new_borrow(lhs_new_mutbl, lhs_expr.span); + } + if let Some(rhs_new_mutbl) = rhs_new_mutbl { + suggest_new_borrow(rhs_new_mutbl, rhs_expr.span); + } + } + } + }; let is_compatible_after_call = |lhs_ty, rhs_ty| { self.lookup_op_method( @@ -429,15 +489,60 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { } else if is_assign == IsAssign::Yes && let Some(lhs_deref_ty) = self.deref_once_mutably_for_diagnostic(lhs_ty) { - suggest_deref_binop(lhs_deref_ty); + suggest_deref_binop(&mut err, lhs_deref_ty); } else if is_assign == IsAssign::No - && let Ref(_, lhs_deref_ty, _) = lhs_ty.kind() + && let Ref(region, lhs_deref_ty, mutbl) = lhs_ty.kind() { if self.type_is_copy_modulo_regions( self.param_env, *lhs_deref_ty, ) { - suggest_deref_binop(*lhs_deref_ty); + suggest_deref_binop(&mut err, *lhs_deref_ty); + } else { + let lhs_inv_mutbl = mutbl.invert(); + let lhs_inv_mutbl_ty = Ty::new_ref( + self.tcx, + *region, + ty::TypeAndMut { + ty: *lhs_deref_ty, + mutbl: lhs_inv_mutbl, + }, + ); + + suggest_different_borrow( + &mut err, + lhs_inv_mutbl_ty, + Some(lhs_inv_mutbl), + rhs_ty, + None, + ); + + if let Ref(region, rhs_deref_ty, mutbl) = rhs_ty.kind() { + let rhs_inv_mutbl = mutbl.invert(); + let rhs_inv_mutbl_ty = Ty::new_ref( + self.tcx, + *region, + ty::TypeAndMut { + ty: *rhs_deref_ty, + mutbl: rhs_inv_mutbl, + }, + ); + + suggest_different_borrow( + &mut err, + lhs_ty, + None, + rhs_inv_mutbl_ty, + Some(rhs_inv_mutbl), + ); + suggest_different_borrow( + &mut err, + lhs_inv_mutbl_ty, + Some(lhs_inv_mutbl), + rhs_inv_mutbl_ty, + Some(rhs_inv_mutbl), + ); + } } } else if self.suggest_fn_call(&mut err, lhs_expr, lhs_ty, |lhs_ty| { is_compatible_after_call(lhs_ty, rhs_ty) diff --git a/tests/ui/binop/borrow-suggestion-109352-2.rs b/tests/ui/binop/borrow-suggestion-109352-2.rs new file mode 100644 index 0000000000000..43dab95296211 --- /dev/null +++ b/tests/ui/binop/borrow-suggestion-109352-2.rs @@ -0,0 +1,27 @@ +struct Bar; + +impl std::ops::Mul for &mut Bar { + type Output = Bar; + + fn mul(self, _rhs: Self) -> Self::Output { + unimplemented!() + } +} + +fn main() { + let ref_mut_bar: &mut Bar = &mut Bar; + let ref_bar: &Bar = &Bar; + let owned_bar: Bar = Bar; + + let _ = ref_mut_bar * ref_mut_bar; + + // FIXME: we should be able to suggest borrowing both side + let _ = owned_bar * owned_bar; + //~^ ERROR cannot multiply + let _ = ref_bar * ref_bar; + //~^ ERROR cannot multiply + let _ = ref_bar * ref_mut_bar; + //~^ ERROR cannot multiply + let _ = ref_mut_bar * ref_bar; + //~^ ERROR mismatched types +} diff --git a/tests/ui/binop/borrow-suggestion-109352-2.stderr b/tests/ui/binop/borrow-suggestion-109352-2.stderr new file mode 100644 index 0000000000000..18ed08d73dd0d --- /dev/null +++ b/tests/ui/binop/borrow-suggestion-109352-2.stderr @@ -0,0 +1,64 @@ +error[E0369]: cannot multiply `Bar` by `Bar` + --> $DIR/borrow-suggestion-109352-2.rs:19:23 + | +LL | let _ = owned_bar * owned_bar; + | --------- ^ --------- Bar + | | + | Bar + | +note: an implementation of `Mul` might be missing for `Bar` + --> $DIR/borrow-suggestion-109352-2.rs:1:1 + | +LL | struct Bar; + | ^^^^^^^^^^ must implement `Mul` +note: the trait `Mul` must be implemented + --> $SRC_DIR/core/src/ops/arith.rs:LL:COL + +error[E0369]: cannot multiply `&Bar` by `&Bar` + --> $DIR/borrow-suggestion-109352-2.rs:21:21 + | +LL | let _ = ref_bar * ref_bar; + | ------- ^ ------- &Bar + | | + | &Bar + | + = note: an implementation for `&mut Bar * &mut Bar` exists +help: consider making this expression a mutable borrow + --> $DIR/borrow-suggestion-109352-2.rs:21:13 + | +LL | let _ = ref_bar * ref_bar; + | ^^^^^^^ +help: consider making this expression a mutable borrow + --> $DIR/borrow-suggestion-109352-2.rs:21:23 + | +LL | let _ = ref_bar * ref_bar; + | ^^^^^^^ + +error[E0369]: cannot multiply `&Bar` by `&mut Bar` + --> $DIR/borrow-suggestion-109352-2.rs:23:21 + | +LL | let _ = ref_bar * ref_mut_bar; + | ------- ^ ----------- &mut Bar + | | + | &Bar + | + = note: an implementation for `&mut Bar * &mut Bar` exists +help: consider making this expression a mutable borrow + --> $DIR/borrow-suggestion-109352-2.rs:23:13 + | +LL | let _ = ref_bar * ref_mut_bar; + | ^^^^^^^ + +error[E0308]: mismatched types + --> $DIR/borrow-suggestion-109352-2.rs:25:27 + | +LL | let _ = ref_mut_bar * ref_bar; + | ^^^^^^^ types differ in mutability + | + = note: expected mutable reference `&mut Bar` + found reference `&Bar` + +error: aborting due to 4 previous errors + +Some errors have detailed explanations: E0308, E0369. +For more information about an error, try `rustc --explain E0308`. diff --git a/tests/ui/binop/borrow-suggestion-109352.fixed b/tests/ui/binop/borrow-suggestion-109352.fixed new file mode 100644 index 0000000000000..3374a9d78b2de --- /dev/null +++ b/tests/ui/binop/borrow-suggestion-109352.fixed @@ -0,0 +1,27 @@ +// run-rustfix + +struct Foo; + +impl std::ops::Mul for &Foo { + type Output = Foo; + + fn mul(self, _rhs: Self) -> Self::Output { + unimplemented!() + } +} + +fn main() { + let ref_mut_foo: &mut Foo = &mut Foo; + let ref_foo: &Foo = &Foo; + let owned_foo: Foo = Foo; + + let _ = ref_foo * ref_foo; + let _ = ref_foo * ref_mut_foo; + + let _ = &*ref_mut_foo * ref_foo; + //~^ ERROR cannot multiply + let _ = &*ref_mut_foo * &*ref_mut_foo; + //~^ ERROR cannot multiply + let _ = &*ref_mut_foo * &owned_foo; + //~^ ERROR cannot multiply +} diff --git a/tests/ui/binop/borrow-suggestion-109352.rs b/tests/ui/binop/borrow-suggestion-109352.rs new file mode 100644 index 0000000000000..4e8510e0de532 --- /dev/null +++ b/tests/ui/binop/borrow-suggestion-109352.rs @@ -0,0 +1,27 @@ +// run-rustfix + +struct Foo; + +impl std::ops::Mul for &Foo { + type Output = Foo; + + fn mul(self, _rhs: Self) -> Self::Output { + unimplemented!() + } +} + +fn main() { + let ref_mut_foo: &mut Foo = &mut Foo; + let ref_foo: &Foo = &Foo; + let owned_foo: Foo = Foo; + + let _ = ref_foo * ref_foo; + let _ = ref_foo * ref_mut_foo; + + let _ = ref_mut_foo * ref_foo; + //~^ ERROR cannot multiply + let _ = ref_mut_foo * ref_mut_foo; + //~^ ERROR cannot multiply + let _ = ref_mut_foo * &owned_foo; + //~^ ERROR cannot multiply +} diff --git a/tests/ui/binop/borrow-suggestion-109352.stderr b/tests/ui/binop/borrow-suggestion-109352.stderr new file mode 100644 index 0000000000000..71e44f54b1706 --- /dev/null +++ b/tests/ui/binop/borrow-suggestion-109352.stderr @@ -0,0 +1,45 @@ +error[E0369]: cannot multiply `&mut Foo` by `&Foo` + --> $DIR/borrow-suggestion-109352.rs:21:25 + | +LL | let _ = ref_mut_foo * ref_foo; + | ----------- ^ ------- &Foo + | | + | &mut Foo + | + = note: an implementation for `&Foo * &Foo` exists +help: consider reborrowing this side + | +LL | let _ = &*ref_mut_foo * ref_foo; + | ++ + +error[E0369]: cannot multiply `&mut Foo` by `&mut Foo` + --> $DIR/borrow-suggestion-109352.rs:23:25 + | +LL | let _ = ref_mut_foo * ref_mut_foo; + | ----------- ^ ----------- &mut Foo + | | + | &mut Foo + | + = note: an implementation for `&Foo * &Foo` exists +help: consider reborrowing both sides + | +LL | let _ = &*ref_mut_foo * &*ref_mut_foo; + | ++ ++ + +error[E0369]: cannot multiply `&mut Foo` by `&Foo` + --> $DIR/borrow-suggestion-109352.rs:25:25 + | +LL | let _ = ref_mut_foo * &owned_foo; + | ----------- ^ ---------- &Foo + | | + | &mut Foo + | + = note: an implementation for `&Foo * &Foo` exists +help: consider reborrowing this side + | +LL | let _ = &*ref_mut_foo * &owned_foo; + | ++ + +error: aborting due to 3 previous errors + +For more information about this error, try `rustc --explain E0369`. From 206bfc47ea8d159b52de8cc09f40452b064ab2f8 Mon Sep 17 00:00:00 2001 From: ouz-a Date: Tue, 1 Aug 2023 12:57:13 +0300 Subject: [PATCH 10/11] Cover statements for stable_mir --- compiler/rustc_smir/src/rustc_smir/mod.rs | 190 ++++++++++++++++-- .../rustc_smir/src/stable_mir/mir/body.rs | 91 ++++++++- 2 files changed, 267 insertions(+), 14 deletions(-) diff --git a/compiler/rustc_smir/src/rustc_smir/mod.rs b/compiler/rustc_smir/src/rustc_smir/mod.rs index 365ef36824802..845b120e202ed 100644 --- a/compiler/rustc_smir/src/rustc_smir/mod.rs +++ b/compiler/rustc_smir/src/rustc_smir/mod.rs @@ -8,11 +8,13 @@ //! For now, we are developing everything inside `rustc`, thus, we keep this module private. use crate::rustc_internal::{self, opaque}; +use crate::stable_mir::mir::{CopyNonOverlapping, UserTypeProjection}; use crate::stable_mir::ty::{FloatTy, IntTy, Movability, RigidTy, TyKind, UintTy}; use crate::stable_mir::{self, Context}; use rustc_hir as hir; -use rustc_middle::mir; -use rustc_middle::ty::{self, Ty, TyCtxt}; +use rustc_middle::mir::coverage::CodeRegion; +use rustc_middle::mir::{self}; +use rustc_middle::ty::{self, Ty, TyCtxt, Variance}; use rustc_span::def_id::{CrateNum, DefId, LOCAL_CRATE}; use rustc_target::abi::FieldIdx; use tracing::debug; @@ -110,17 +112,43 @@ impl<'tcx> Stable<'tcx> for mir::Statement<'tcx> { Assign(assign) => { stable_mir::mir::Statement::Assign(assign.0.stable(tables), assign.1.stable(tables)) } - FakeRead(_) => todo!(), - SetDiscriminant { .. } => todo!(), - Deinit(_) => todo!(), - StorageLive(_) => todo!(), - StorageDead(_) => todo!(), - Retag(_, _) => todo!(), - PlaceMention(_) => todo!(), - AscribeUserType(_, _) => todo!(), - Coverage(_) => todo!(), - Intrinsic(_) => todo!(), - ConstEvalCounter => todo!(), + FakeRead(fake_read_place) => stable_mir::mir::Statement::FakeRead( + fake_read_place.0.stable(tables), + fake_read_place.1.stable(tables), + ), + SetDiscriminant { place: plc, variant_index: idx } => { + stable_mir::mir::Statement::SetDiscriminant { + place: plc.as_ref().stable(tables), + variant_index: idx.stable(tables), + } + } + Deinit(place) => stable_mir::mir::Statement::Deinit(place.stable(tables)), + StorageLive(place) => stable_mir::mir::Statement::StorageLive(place.stable(tables)), + StorageDead(place) => stable_mir::mir::Statement::StorageDead(place.stable(tables)), + Retag(retag, place) => { + stable_mir::mir::Statement::Retag(retag.stable(tables), place.stable(tables)) + } + PlaceMention(place) => stable_mir::mir::Statement::PlaceMention(place.stable(tables)), + AscribeUserType(place_projection, variance) => { + stable_mir::mir::Statement::AscribeUserType( + ( + place_projection.as_ref().0.stable(tables), + UserTypeProjection { + base: place_projection.as_ref().1.base.stable(tables), + projection: format!("{:?}", place_projection.as_ref().1.projs), + }, + ), + variance.stable(tables), + ) + } + Coverage(coverage) => stable_mir::mir::Statement::Coverage(stable_mir::mir::Coverage { + kind: coverage.kind.stable(tables), + code_region: coverage.code_region.as_ref().map(|reg| reg.stable(tables)), + }), + Intrinsic(intrinstic) => { + stable_mir::mir::Statement::Intrinsic(intrinstic.stable(tables)) + } + ConstEvalCounter => stable_mir::mir::Statement::ConstEvalCounter, Nop => stable_mir::mir::Statement::Nop, } } @@ -364,6 +392,24 @@ impl<'tcx> Stable<'tcx> for rustc_hir::Unsafety { } } +impl<'tcx> Stable<'tcx> for mir::FakeReadCause { + type T = stable_mir::mir::FakeReadCause; + fn stable(&self, _: &mut Tables<'tcx>) -> Self::T { + use mir::FakeReadCause::*; + match self { + ForMatchGuard => stable_mir::mir::FakeReadCause::ForMatchGuard, + ForMatchedPlace(local_def_id) => stable_mir::mir::FakeReadCause::ForMatchedPlace( + local_def_id.map(|id| id.to_def_id().index.index()), + ), + ForGuardBinding => stable_mir::mir::FakeReadCause::ForGuardBinding, + ForLet(local_def_id) => stable_mir::mir::FakeReadCause::ForLet( + local_def_id.map(|id| id.to_def_id().index.index()), + ), + ForIndex => stable_mir::mir::FakeReadCause::ForIndex, + } + } +} + impl<'tcx> Stable<'tcx> for FieldIdx { type T = usize; fn stable(&self, _: &mut Tables<'tcx>) -> Self::T { @@ -393,6 +439,104 @@ impl<'tcx> Stable<'tcx> for mir::Place<'tcx> { } } +impl<'tcx> Stable<'tcx> for mir::coverage::CoverageKind { + type T = stable_mir::mir::CoverageKind; + fn stable(&self, tables: &mut Tables<'tcx>) -> Self::T { + use rustc_middle::mir::coverage::CoverageKind; + match self { + CoverageKind::Counter { function_source_hash, id } => { + stable_mir::mir::CoverageKind::Counter { + function_source_hash: *function_source_hash as usize, + id: id.as_usize(), + } + } + CoverageKind::Expression { id, lhs, op, rhs } => { + stable_mir::mir::CoverageKind::Expression { + id: id.as_usize(), + lhs: lhs.as_usize(), + op: op.stable(tables), + rhs: rhs.as_usize(), + } + } + CoverageKind::Unreachable => stable_mir::mir::CoverageKind::Unreachable, + } + } +} + +impl<'tcx> Stable<'tcx> for mir::coverage::Op { + type T = stable_mir::mir::Op; + + fn stable(&self, _: &mut Tables<'tcx>) -> Self::T { + use rustc_middle::mir::coverage::Op::*; + match self { + Subtract => stable_mir::mir::Op::Subtract, + Add => stable_mir::mir::Op::Add, + } + } +} + +impl<'tcx> Stable<'tcx> for mir::Local { + type T = usize; + fn stable(&self, _: &mut Tables<'tcx>) -> Self::T { + self.as_usize() + } +} + +impl<'tcx> Stable<'tcx> for rustc_target::abi::VariantIdx { + type T = usize; + fn stable(&self, _: &mut Tables<'tcx>) -> Self::T { + self.as_usize() + } +} + +impl<'tcx> Stable<'tcx> for Variance { + type T = stable_mir::mir::Variance; + fn stable(&self, _: &mut Tables<'tcx>) -> Self::T { + match self { + Variance::Bivariant => stable_mir::mir::Variance::Bivariant, + Variance::Contravariant => stable_mir::mir::Variance::Contravariant, + Variance::Covariant => stable_mir::mir::Variance::Covariant, + Variance::Invariant => stable_mir::mir::Variance::Invariant, + } + } +} + +impl<'tcx> Stable<'tcx> for mir::RetagKind { + type T = stable_mir::mir::RetagKind; + fn stable(&self, _: &mut Tables<'tcx>) -> Self::T { + use rustc_middle::mir::RetagKind; + match self { + RetagKind::FnEntry => stable_mir::mir::RetagKind::FnEntry, + RetagKind::TwoPhase => stable_mir::mir::RetagKind::TwoPhase, + RetagKind::Raw => stable_mir::mir::RetagKind::Raw, + RetagKind::Default => stable_mir::mir::RetagKind::Default, + } + } +} + +impl<'tcx> Stable<'tcx> for rustc_middle::ty::UserTypeAnnotationIndex { + type T = usize; + fn stable(&self, _: &mut Tables<'tcx>) -> Self::T { + self.as_usize() + } +} + +impl<'tcx> Stable<'tcx> for CodeRegion { + type T = stable_mir::mir::CodeRegion; + + fn stable(&self, _: &mut Tables<'tcx>) -> Self::T { + match self { + _ => stable_mir::mir::CodeRegion { + file_name: self.file_name.as_u32() as usize, + start_line: self.start_line as usize, + start_col: self.start_col as usize, + end_line: self.end_line as usize, + end_col: self.end_col as usize, + }, + } + } +} + impl<'tcx> Stable<'tcx> for mir::UnwindAction { type T = stable_mir::mir::UnwindAction; fn stable(&self, _: &mut Tables<'tcx>) -> Self::T { @@ -406,6 +550,26 @@ impl<'tcx> Stable<'tcx> for mir::UnwindAction { } } +impl<'tcx> Stable<'tcx> for mir::NonDivergingIntrinsic<'tcx> { + type T = stable_mir::mir::NonDivergingIntrinsic; + + fn stable(&self, tables: &mut Tables<'tcx>) -> Self::T { + use rustc_middle::mir::NonDivergingIntrinsic; + match self { + NonDivergingIntrinsic::Assume(op) => { + stable_mir::mir::NonDivergingIntrinsic::Assume(op.stable(tables)) + } + NonDivergingIntrinsic::CopyNonOverlapping(copy_non_overlapping) => { + stable_mir::mir::NonDivergingIntrinsic::CopyNonOverlapping(CopyNonOverlapping { + src: copy_non_overlapping.src.stable(tables), + dst: copy_non_overlapping.dst.stable(tables), + count: copy_non_overlapping.count.stable(tables), + }) + } + } + } +} + impl<'tcx> Stable<'tcx> for mir::AssertMessage<'tcx> { type T = stable_mir::mir::AssertMessage; fn stable(&self, tables: &mut Tables<'tcx>) -> Self::T { diff --git a/compiler/rustc_smir/src/stable_mir/mir/body.rs b/compiler/rustc_smir/src/stable_mir/mir/body.rs index e08359067dfc3..90f46e5d95be2 100644 --- a/compiler/rustc_smir/src/stable_mir/mir/body.rs +++ b/compiler/rustc_smir/src/stable_mir/mir/body.rs @@ -133,9 +133,90 @@ pub enum AsyncGeneratorKind { Fn, } +// FIXME: We are using `usize` for now but we should be using DefIds and find what +// what those are refering to and then use appropirate ty_defs for them (i.e AdtDef) +/// The FakeReadCause describes the type of pattern why a FakeRead statement exists. +#[derive(Clone, Debug)] +pub enum FakeReadCause { + ForMatchGuard, + ForMatchedPlace(Option), + ForGuardBinding, + ForLet(Option), + ForIndex, +} + +/// Describes what kind of retag is to be performed +#[derive(Clone, Debug)] +pub enum RetagKind { + FnEntry, + TwoPhase, + Raw, + Default, +} + +#[derive(Clone, Debug)] +pub enum Variance { + Covariant, + Invariant, + Contravariant, + Bivariant, +} + +#[derive(Clone, Debug)] +pub enum Op { + Subtract, + Add, +} + +#[derive(Clone, Debug)] +pub enum CoverageKind { + Counter { function_source_hash: usize, id: usize }, + Expression { id: usize, lhs: usize, op: Op, rhs: usize }, + Unreachable, +} + +#[derive(Clone, Copy, Debug)] +pub struct CodeRegion { + pub file_name: usize, + pub start_line: usize, + pub start_col: usize, + pub end_line: usize, + pub end_col: usize, +} + +#[derive(Clone, Debug)] +pub struct Coverage { + pub kind: CoverageKind, + pub code_region: Option, +} + +#[derive(Clone, Debug)] +pub struct CopyNonOverlapping { + pub src: Operand, + pub dst: Operand, + pub count: Operand, +} + +#[derive(Clone, Debug)] +pub enum NonDivergingIntrinsic { + Assume(Operand), + CopyNonOverlapping(CopyNonOverlapping), +} + #[derive(Clone, Debug)] pub enum Statement { Assign(Place, Rvalue), + FakeRead(FakeReadCause, Place), + SetDiscriminant { place: Place, variant_index: VariantIdx }, + Deinit(Place), + StorageLive(Local), + StorageDead(Local), + Retag(RetagKind, Place), + PlaceMention(Place), + AscribeUserType((Place, UserTypeProjection), Variance), + Coverage(Coverage), + Intrinsic(NonDivergingIntrinsic), + ConstEvalCounter, Nop, } @@ -271,10 +352,18 @@ pub enum Operand { #[derive(Clone, Debug)] pub struct Place { - pub local: usize, + pub local: Local, + pub projection: String, +} + +#[derive(Clone, Debug)] +pub struct UserTypeProjection { + pub base: UserTypeAnnotationIndex, pub projection: String, } +type Local = usize; + type FieldIdx = usize; /// The source-order index of a variant in a type. From 2ff62fdfcc4635568da7f47a59e0c49793a0436d Mon Sep 17 00:00:00 2001 From: ouz-a Date: Tue, 1 Aug 2023 17:48:20 +0300 Subject: [PATCH 11/11] clean up, use opaque types --- compiler/rustc_smir/src/rustc_smir/mod.rs | 61 +++++++++---------- .../rustc_smir/src/stable_mir/mir/body.rs | 34 +++++++---- 2 files changed, 53 insertions(+), 42 deletions(-) diff --git a/compiler/rustc_smir/src/rustc_smir/mod.rs b/compiler/rustc_smir/src/rustc_smir/mod.rs index 845b120e202ed..c4bdec0ee28b1 100644 --- a/compiler/rustc_smir/src/rustc_smir/mod.rs +++ b/compiler/rustc_smir/src/rustc_smir/mod.rs @@ -8,7 +8,7 @@ //! For now, we are developing everything inside `rustc`, thus, we keep this module private. use crate::rustc_internal::{self, opaque}; -use crate::stable_mir::mir::{CopyNonOverlapping, UserTypeProjection}; +use crate::stable_mir::mir::{CopyNonOverlapping, UserTypeProjection, VariantIdx}; use crate::stable_mir::ty::{FloatTy, IntTy, Movability, RigidTy, TyKind, UintTy}; use crate::stable_mir::{self, Context}; use rustc_hir as hir; @@ -130,16 +130,11 @@ impl<'tcx> Stable<'tcx> for mir::Statement<'tcx> { } PlaceMention(place) => stable_mir::mir::Statement::PlaceMention(place.stable(tables)), AscribeUserType(place_projection, variance) => { - stable_mir::mir::Statement::AscribeUserType( - ( - place_projection.as_ref().0.stable(tables), - UserTypeProjection { - base: place_projection.as_ref().1.base.stable(tables), - projection: format!("{:?}", place_projection.as_ref().1.projs), - }, - ), - variance.stable(tables), - ) + stable_mir::mir::Statement::AscribeUserType { + place: place_projection.as_ref().0.stable(tables), + projections: place_projection.as_ref().1.stable(tables), + variance: variance.stable(tables), + } } Coverage(coverage) => stable_mir::mir::Statement::Coverage(stable_mir::mir::Coverage { kind: coverage.kind.stable(tables), @@ -398,13 +393,11 @@ impl<'tcx> Stable<'tcx> for mir::FakeReadCause { use mir::FakeReadCause::*; match self { ForMatchGuard => stable_mir::mir::FakeReadCause::ForMatchGuard, - ForMatchedPlace(local_def_id) => stable_mir::mir::FakeReadCause::ForMatchedPlace( - local_def_id.map(|id| id.to_def_id().index.index()), - ), + ForMatchedPlace(local_def_id) => { + stable_mir::mir::FakeReadCause::ForMatchedPlace(opaque(local_def_id)) + } ForGuardBinding => stable_mir::mir::FakeReadCause::ForGuardBinding, - ForLet(local_def_id) => stable_mir::mir::FakeReadCause::ForLet( - local_def_id.map(|id| id.to_def_id().index.index()), - ), + ForLet(local_def_id) => stable_mir::mir::FakeReadCause::ForLet(opaque(local_def_id)), ForIndex => stable_mir::mir::FakeReadCause::ForIndex, } } @@ -447,15 +440,15 @@ impl<'tcx> Stable<'tcx> for mir::coverage::CoverageKind { CoverageKind::Counter { function_source_hash, id } => { stable_mir::mir::CoverageKind::Counter { function_source_hash: *function_source_hash as usize, - id: id.as_usize(), + id: opaque(id), } } CoverageKind::Expression { id, lhs, op, rhs } => { stable_mir::mir::CoverageKind::Expression { - id: id.as_usize(), - lhs: lhs.as_usize(), + id: opaque(id), + lhs: opaque(lhs), op: op.stable(tables), - rhs: rhs.as_usize(), + rhs: opaque(rhs), } } CoverageKind::Unreachable => stable_mir::mir::CoverageKind::Unreachable, @@ -463,6 +456,14 @@ impl<'tcx> Stable<'tcx> for mir::coverage::CoverageKind { } } +impl<'tcx> Stable<'tcx> for mir::UserTypeProjection { + type T = stable_mir::mir::UserTypeProjection; + + fn stable(&self, _: &mut Tables<'tcx>) -> Self::T { + UserTypeProjection { base: self.base.as_usize(), projection: format!("{:?}", self.projs) } + } +} + impl<'tcx> Stable<'tcx> for mir::coverage::Op { type T = stable_mir::mir::Op; @@ -476,14 +477,14 @@ impl<'tcx> Stable<'tcx> for mir::coverage::Op { } impl<'tcx> Stable<'tcx> for mir::Local { - type T = usize; + type T = stable_mir::mir::Local; fn stable(&self, _: &mut Tables<'tcx>) -> Self::T { self.as_usize() } } impl<'tcx> Stable<'tcx> for rustc_target::abi::VariantIdx { - type T = usize; + type T = VariantIdx; fn stable(&self, _: &mut Tables<'tcx>) -> Self::T { self.as_usize() } @@ -525,14 +526,12 @@ impl<'tcx> Stable<'tcx> for CodeRegion { type T = stable_mir::mir::CodeRegion; fn stable(&self, _: &mut Tables<'tcx>) -> Self::T { - match self { - _ => stable_mir::mir::CodeRegion { - file_name: self.file_name.as_u32() as usize, - start_line: self.start_line as usize, - start_col: self.start_col as usize, - end_line: self.end_line as usize, - end_col: self.end_col as usize, - }, + stable_mir::mir::CodeRegion { + file_name: self.file_name.as_str().to_string(), + start_line: self.start_line as usize, + start_col: self.start_col as usize, + end_line: self.end_line as usize, + end_col: self.end_col as usize, } } } diff --git a/compiler/rustc_smir/src/stable_mir/mir/body.rs b/compiler/rustc_smir/src/stable_mir/mir/body.rs index 90f46e5d95be2..c16bd6cbd70e2 100644 --- a/compiler/rustc_smir/src/stable_mir/mir/body.rs +++ b/compiler/rustc_smir/src/stable_mir/mir/body.rs @@ -1,3 +1,4 @@ +use crate::rustc_internal::Opaque; use crate::stable_mir::ty::{ AdtDef, ClosureDef, Const, GeneratorDef, GenericArgs, Movability, Region, }; @@ -133,15 +134,18 @@ pub enum AsyncGeneratorKind { Fn, } -// FIXME: We are using `usize` for now but we should be using DefIds and find what -// what those are refering to and then use appropirate ty_defs for them (i.e AdtDef) +pub(crate) type LocalDefId = Opaque; +pub(crate) type CounterValueReference = Opaque; +pub(crate) type InjectedExpressionId = Opaque; +pub(crate) type ExpressionOperandId = Opaque; + /// The FakeReadCause describes the type of pattern why a FakeRead statement exists. #[derive(Clone, Debug)] pub enum FakeReadCause { ForMatchGuard, - ForMatchedPlace(Option), + ForMatchedPlace(LocalDefId), ForGuardBinding, - ForLet(Option), + ForLet(LocalDefId), ForIndex, } @@ -170,14 +174,22 @@ pub enum Op { #[derive(Clone, Debug)] pub enum CoverageKind { - Counter { function_source_hash: usize, id: usize }, - Expression { id: usize, lhs: usize, op: Op, rhs: usize }, + Counter { + function_source_hash: usize, + id: CounterValueReference, + }, + Expression { + id: InjectedExpressionId, + lhs: ExpressionOperandId, + op: Op, + rhs: ExpressionOperandId, + }, Unreachable, } -#[derive(Clone, Copy, Debug)] +#[derive(Clone, Debug)] pub struct CodeRegion { - pub file_name: usize, + pub file_name: String, pub start_line: usize, pub start_col: usize, pub end_line: usize, @@ -213,7 +225,7 @@ pub enum Statement { StorageDead(Local), Retag(RetagKind, Place), PlaceMention(Place), - AscribeUserType((Place, UserTypeProjection), Variance), + AscribeUserType { place: Place, projections: UserTypeProjection, variance: Variance }, Coverage(Coverage), Intrinsic(NonDivergingIntrinsic), ConstEvalCounter, @@ -362,12 +374,12 @@ pub struct UserTypeProjection { pub projection: String, } -type Local = usize; +pub type Local = usize; type FieldIdx = usize; /// The source-order index of a variant in a type. -type VariantIdx = usize; +pub type VariantIdx = usize; type UserTypeAnnotationIndex = usize;