From 8c7c84b4e8923779ff56a518e4cd39c1c600c7d0 Mon Sep 17 00:00:00 2001 From: Rich Kadel Date: Thu, 18 Jun 2020 13:29:43 -0700 Subject: [PATCH 1/8] code coverage foundation for hash and num_counters Replaced dummy values for hash and num_counters with computed values, and refactored InstrumentCoverage pass to simplify injecting more counters per function in upcoming versions. Improved usage documentation and error messaging. --- src/librustc_codegen_llvm/intrinsic.rs | 37 ++-- src/librustc_codegen_ssa/mir/block.rs | 2 +- src/librustc_codegen_ssa/mir/mod.rs | 4 +- src/librustc_codegen_ssa/traits/intrinsic.rs | 6 +- src/librustc_metadata/locator.rs | 2 + src/librustc_middle/ich/hcx.rs | 33 ++- src/librustc_middle/mir/mod.rs | 23 +- src/librustc_middle/ty/context.rs | 7 + .../transform/instrument_coverage.rs | 196 +++++++++++++----- src/librustc_session/options.rs | 5 +- src/librustc_span/symbol.rs | 1 + 11 files changed, 230 insertions(+), 86 deletions(-) diff --git a/src/librustc_codegen_llvm/intrinsic.rs b/src/librustc_codegen_llvm/intrinsic.rs index 95465939070a0..8c0ccde6b16bb 100644 --- a/src/librustc_codegen_llvm/intrinsic.rs +++ b/src/librustc_codegen_llvm/intrinsic.rs @@ -16,6 +16,7 @@ use rustc_codegen_ssa::common::TypeKind; use rustc_codegen_ssa::glue; use rustc_codegen_ssa::mir::operand::{OperandRef, OperandValue}; use rustc_codegen_ssa::mir::place::PlaceRef; +use rustc_codegen_ssa::mir::FunctionCx; use rustc_codegen_ssa::traits::*; use rustc_codegen_ssa::MemFlags; use rustc_hir as hir; @@ -23,7 +24,6 @@ use rustc_middle::ty::layout::{FnAbiExt, HasTyCtxt}; use rustc_middle::ty::{self, Ty}; use rustc_middle::{bug, span_bug}; use rustc_span::Span; -use rustc_span::Symbol; use rustc_target::abi::{self, HasDataLayout, LayoutOf, Primitive}; use rustc_target::spec::PanicStrategy; @@ -82,14 +82,14 @@ fn get_simple_intrinsic(cx: &CodegenCx<'ll, '_>, name: &str) -> Option<&'ll Valu } impl IntrinsicCallMethods<'tcx> for Builder<'a, 'll, 'tcx> { - fn codegen_intrinsic_call( + fn codegen_intrinsic_call<'b, Bx: BuilderMethods<'b, 'tcx>>( &mut self, + fx: &FunctionCx<'b, 'tcx, Bx>, instance: ty::Instance<'tcx>, fn_abi: &FnAbi<'tcx, Ty<'tcx>>, args: &[OperandRef<'tcx, &'ll Value>], llresult: &'ll Value, span: Span, - caller_instance: ty::Instance<'tcx>, ) { let tcx = self.tcx; let callee_ty = instance.monomorphic_ty(tcx); @@ -141,26 +141,17 @@ impl IntrinsicCallMethods<'tcx> for Builder<'a, 'll, 'tcx> { self.call(llfn, &[], None) } "count_code_region" => { - if let ty::InstanceDef::Item(fn_def_id) = caller_instance.def { - let caller_fn_path = tcx.def_path_str(fn_def_id); - debug!( - "count_code_region to llvm.instrprof.increment(fn_name={})", - caller_fn_path - ); - - // FIXME(richkadel): (1) Replace raw function name with mangled function name; - // (2) Replace hardcoded `1234` in `hash` with a computed hash (as discussed in) - // the MCP (compiler-team/issues/278); and replace the hardcoded `1` for - // `num_counters` with the actual number of counters per function (when the - // changes are made to inject more than one counter per function). - let (fn_name, _len_val) = self.const_str(Symbol::intern(&caller_fn_path)); - let index = args[0].immediate(); - let hash = self.const_u64(1234); - let num_counters = self.const_u32(1); - self.instrprof_increment(fn_name, hash, num_counters, index) - } else { - bug!("intrinsic count_code_region: no src.instance"); - } + let coverage_data = fx.mir.coverage_data.as_ref().unwrap(); + let mangled_fn = tcx.symbol_name(fx.instance); + let (mangled_fn_name, _len_val) = self.const_str(mangled_fn.name); + let hash = self.const_u64(coverage_data.hash); + let index = args[0].immediate(); + let num_counters = self.const_u32(coverage_data.num_counters as u32); + debug!( + "count_code_region to LLVM intrinsic instrprof.increment(fn_name={}, hash={:?}, num_counters={:?}, index={:?})", + mangled_fn.name, hash, index, num_counters + ); + self.instrprof_increment(mangled_fn_name, hash, num_counters, index) } "va_start" => self.va_start(args[0].immediate()), "va_end" => self.va_end(args[0].immediate()), diff --git a/src/librustc_codegen_ssa/mir/block.rs b/src/librustc_codegen_ssa/mir/block.rs index d56c816811b3c..945c3d4843471 100644 --- a/src/librustc_codegen_ssa/mir/block.rs +++ b/src/librustc_codegen_ssa/mir/block.rs @@ -688,12 +688,12 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { .collect(); bx.codegen_intrinsic_call( + self, *instance.as_ref().unwrap(), &fn_abi, &args, dest, terminator.source_info.span, - self.instance, ); if let ReturnDest::IndirectOperand(dst, _) = ret_dest { diff --git a/src/librustc_codegen_ssa/mir/mod.rs b/src/librustc_codegen_ssa/mir/mod.rs index 00b4bf96afa59..fd2e779f681be 100644 --- a/src/librustc_codegen_ssa/mir/mod.rs +++ b/src/librustc_codegen_ssa/mir/mod.rs @@ -21,9 +21,9 @@ use self::operand::{OperandRef, OperandValue}; /// Master context for codegenning from MIR. pub struct FunctionCx<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> { - instance: Instance<'tcx>, + pub instance: Instance<'tcx>, - mir: &'tcx mir::Body<'tcx>, + pub mir: &'tcx mir::Body<'tcx>, debug_context: Option>, diff --git a/src/librustc_codegen_ssa/traits/intrinsic.rs b/src/librustc_codegen_ssa/traits/intrinsic.rs index f62019498511c..0036eadf4db81 100644 --- a/src/librustc_codegen_ssa/traits/intrinsic.rs +++ b/src/librustc_codegen_ssa/traits/intrinsic.rs @@ -1,5 +1,7 @@ use super::BackendTypes; use crate::mir::operand::OperandRef; +use crate::mir::FunctionCx; +use crate::traits::BuilderMethods; use rustc_middle::ty::{self, Ty}; use rustc_span::Span; use rustc_target::abi::call::FnAbi; @@ -8,14 +10,14 @@ pub trait IntrinsicCallMethods<'tcx>: BackendTypes { /// Remember to add all intrinsics here, in librustc_typeck/check/mod.rs, /// and in libcore/intrinsics.rs; if you need access to any llvm intrinsics, /// add them to librustc_codegen_llvm/context.rs - fn codegen_intrinsic_call( + fn codegen_intrinsic_call<'a, Bx: BuilderMethods<'a, 'tcx>>( &mut self, + fx: &FunctionCx<'a, 'tcx, Bx>, instance: ty::Instance<'tcx>, fn_abi: &FnAbi<'tcx, Ty<'tcx>>, args: &[OperandRef<'tcx, Self::Value>], llresult: Self::Value, span: Span, - caller_instance: ty::Instance<'tcx>, ); fn abort(&mut self); diff --git a/src/librustc_metadata/locator.rs b/src/librustc_metadata/locator.rs index 5a4862d452183..662794f0aa11f 100644 --- a/src/librustc_metadata/locator.rs +++ b/src/librustc_metadata/locator.rs @@ -488,6 +488,8 @@ impl<'a> CrateLocator<'a> { && self.triple != TargetTriple::from_triple(config::host_triple()) { err.note(&format!("the `{}` target may not be installed", self.triple)); + } else if self.crate_name == sym::profiler_builtins { + err.note(&"the compiler may have been built without `profiler = true`"); } err.span_label(self.span, "can't find crate"); err diff --git a/src/librustc_middle/ich/hcx.rs b/src/librustc_middle/ich/hcx.rs index 69b4adb3a0e1d..f5b0b73c49de1 100644 --- a/src/librustc_middle/ich/hcx.rs +++ b/src/librustc_middle/ich/hcx.rs @@ -67,13 +67,15 @@ impl<'a> StableHashingContext<'a> { /// Don't use it for anything else or you'll run the risk of /// leaking data out of the tracking system. #[inline] - pub fn new( + fn new_with_or_without_spans( sess: &'a Session, krate: &'a hir::Crate<'a>, definitions: &'a Definitions, cstore: &'a dyn CrateStore, + always_ignore_spans: bool, ) -> Self { - let hash_spans_initial = !sess.opts.debugging_opts.incremental_ignore_spans; + let hash_spans_initial = + !always_ignore_spans && !sess.opts.debugging_opts.incremental_ignore_spans; StableHashingContext { sess, @@ -88,6 +90,33 @@ impl<'a> StableHashingContext<'a> { } } + #[inline] + pub fn new( + sess: &'a Session, + krate: &'a hir::Crate<'a>, + definitions: &'a Definitions, + cstore: &'a dyn CrateStore, + ) -> Self { + Self::new_with_or_without_spans( + sess, + krate, + definitions, + cstore, + /*always_ignore_spans=*/ false, + ) + } + + #[inline] + pub fn ignore_spans( + sess: &'a Session, + krate: &'a hir::Crate<'a>, + definitions: &'a Definitions, + cstore: &'a dyn CrateStore, + ) -> Self { + let always_ignore_spans = true; + Self::new_with_or_without_spans(sess, krate, definitions, cstore, always_ignore_spans) + } + #[inline] pub fn sess(&self) -> &'a Session { self.sess diff --git a/src/librustc_middle/mir/mod.rs b/src/librustc_middle/mir/mod.rs index 3381b95c2a38e..a89a5ef3f8218 100644 --- a/src/librustc_middle/mir/mod.rs +++ b/src/librustc_middle/mir/mod.rs @@ -88,6 +88,19 @@ impl MirPhase { } } +/// Coverage data computed by the `InstrumentCoverage` MIR pass, when compiling with +/// `-Zinstrument_coverage`. +#[derive(Clone, RustcEncodable, RustcDecodable, Debug, HashStable, TypeFoldable)] +pub struct CoverageData { + /// A hash value that can be used by the consumer of the coverage profile data to detect + /// changes to the instrumented source of the associated MIR body (typically, for an + /// individual function). + pub hash: u64, + + /// The total number of coverage region counters added to this MIR Body. + pub num_counters: usize, +} + /// The lowered representation of a single function. #[derive(Clone, RustcEncodable, RustcDecodable, Debug, HashStable, TypeFoldable)] pub struct Body<'tcx> { @@ -164,13 +177,17 @@ pub struct Body<'tcx> { /// The user may be writing e.g. `&[(SOME_CELL, 42)][i].1` and this would get promoted, because /// we'd statically know that no thing with interior mutability will ever be available to the /// user without some serious unsafe code. Now this means that our promoted is actually - /// `&[(SOME_CELL, 42)]` and the MIR using it will do the `&promoted[i].1` projection because the - /// index may be a runtime value. Such a promoted value is illegal because it has reachable + /// `&[(SOME_CELL, 42)]` and the MIR using it will do the `&promoted[i].1` projection because + /// the index may be a runtime value. Such a promoted value is illegal because it has reachable /// interior mutability. This flag just makes this situation very obvious where the previous /// implementation without the flag hid this situation silently. /// FIXME(oli-obk): rewrite the promoted during promotion to eliminate the cell components. pub ignore_interior_mut_in_const_validation: bool, + /// If compiling with `-Zinstrument_coverage`, the `InstrumentCoverage` pass stores summary + /// information associated with the MIR, used in code generation of the coverage counters. + pub coverage_data: Option, + predecessor_cache: PredecessorCache, } @@ -211,6 +228,7 @@ impl<'tcx> Body<'tcx> { required_consts: Vec::new(), ignore_interior_mut_in_const_validation: false, control_flow_destroyed, + coverage_data: None, predecessor_cache: PredecessorCache::new(), } } @@ -238,6 +256,7 @@ impl<'tcx> Body<'tcx> { generator_kind: None, var_debug_info: Vec::new(), ignore_interior_mut_in_const_validation: false, + coverage_data: None, predecessor_cache: PredecessorCache::new(), } } diff --git a/src/librustc_middle/ty/context.rs b/src/librustc_middle/ty/context.rs index 62d6de2d71e6d..0696cae4810ec 100644 --- a/src/librustc_middle/ty/context.rs +++ b/src/librustc_middle/ty/context.rs @@ -1284,6 +1284,13 @@ impl<'tcx> TyCtxt<'tcx> { StableHashingContext::new(self.sess, krate, self.definitions, &*self.cstore) } + #[inline(always)] + pub fn create_no_span_stable_hashing_context(self) -> StableHashingContext<'tcx> { + let krate = self.gcx.untracked_crate; + + StableHashingContext::ignore_spans(self.sess, krate, self.definitions, &*self.cstore) + } + // This method makes sure that we have a DepNode and a Fingerprint for // every upstream crate. It needs to be called once right after the tcx is // created. diff --git a/src/librustc_mir/transform/instrument_coverage.rs b/src/librustc_mir/transform/instrument_coverage.rs index c36614938e10f..793ccbb081bed 100644 --- a/src/librustc_mir/transform/instrument_coverage.rs +++ b/src/librustc_mir/transform/instrument_coverage.rs @@ -1,8 +1,15 @@ use crate::transform::{MirPass, MirSource}; use crate::util::patch::MirPatch; +use rustc_data_structures::fingerprint::Fingerprint; +use rustc_data_structures::stable_hasher::{HashStable, StableHasher}; use rustc_hir::lang_items; +use rustc_hir::*; +use rustc_middle::ich::StableHashingContext; use rustc_middle::mir::interpret::Scalar; -use rustc_middle::mir::*; +use rustc_middle::mir::{ + self, BasicBlock, BasicBlockData, CoverageData, Operand, Place, SourceInfo, StatementKind, + Terminator, TerminatorKind, START_BLOCK, +}; use rustc_middle::ty; use rustc_middle::ty::TyCtxt; use rustc_span::def_id::DefId; @@ -12,64 +19,104 @@ use rustc_span::Span; /// the intrinsic llvm.instrprof.increment. pub struct InstrumentCoverage; +struct Instrumentor<'tcx> { + tcx: TyCtxt<'tcx>, + num_counters: usize, +} + impl<'tcx> MirPass<'tcx> for InstrumentCoverage { - fn run_pass(&self, tcx: TyCtxt<'tcx>, src: MirSource<'tcx>, body: &mut Body<'tcx>) { + fn run_pass(&self, tcx: TyCtxt<'tcx>, src: MirSource<'tcx>, mir_body: &mut mir::Body<'tcx>) { if tcx.sess.opts.debugging_opts.instrument_coverage { - debug!("instrumenting {:?}", src.def_id()); - instrument_coverage(tcx, body); + // If the InstrumentCoverage pass is called on promoted MIRs, skip them. + // See: https://github.com/rust-lang/rust/pull/73011#discussion_r438317601 + if src.promoted.is_none() { + assert!(mir_body.coverage_data.is_none()); + + let hash = hash_mir_source(tcx, &src); + + debug!( + "instrumenting {:?}, hash: {}, span: {}", + src.def_id(), + hash, + tcx.sess.source_map().span_to_string(mir_body.span) + ); + + let num_counters = Instrumentor::new(tcx).inject_counters(mir_body); + + mir_body.coverage_data = Some(CoverageData { hash, num_counters }); + } } } } -// The first counter (start of the function) is index zero. -const INIT_FUNCTION_COUNTER: u32 = 0; - -/// Injects calls to placeholder function `count_code_region()`. -// FIXME(richkadel): As a first step, counters are only injected at the top of each function. -// The complete solution will inject counters at each conditional code branch. -pub fn instrument_coverage<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) { - let span = body.span.shrink_to_lo(); - - let count_code_region_fn = function_handle( - tcx, - tcx.require_lang_item(lang_items::CountCodeRegionFnLangItem, None), - span, - ); - let counter_index = Operand::const_from_scalar( - tcx, - tcx.types.u32, - Scalar::from_u32(INIT_FUNCTION_COUNTER), - span, - ); - - let mut patch = MirPatch::new(body); - - let new_block = patch.new_block(placeholder_block(SourceInfo::outermost(body.span))); - let next_block = START_BLOCK; - - let temp = patch.new_temp(tcx.mk_unit(), body.span); - patch.patch_terminator( - new_block, - TerminatorKind::Call { - func: count_code_region_fn, - args: vec![counter_index], - // new_block will swapped with the next_block, after applying patch - destination: Some((Place::from(temp), new_block)), - cleanup: None, - from_hir_call: false, - fn_span: span, - }, - ); +impl<'tcx> Instrumentor<'tcx> { + fn new(tcx: TyCtxt<'tcx>) -> Self { + Self { tcx, num_counters: 0 } + } + + fn next_counter(&mut self) -> u32 { + let next = self.num_counters as u32; + self.num_counters += 1; + next + } + + fn inject_counters(&mut self, mir_body: &mut mir::Body<'tcx>) -> usize { + // FIXME(richkadel): As a first step, counters are only injected at the top of each + // function. The complete solution will inject counters at each conditional code branch. + let top_of_function = START_BLOCK; + let entire_function = mir_body.span; + + self.inject_counter(mir_body, top_of_function, entire_function); + + self.num_counters + } + + fn inject_counter( + &mut self, + mir_body: &mut mir::Body<'tcx>, + next_block: BasicBlock, + code_region: Span, + ) { + let injection_point = code_region.shrink_to_lo(); - patch.add_statement(new_block.start_location(), StatementKind::StorageLive(temp)); - patch.add_statement(next_block.start_location(), StatementKind::StorageDead(temp)); + let count_code_region_fn = function_handle( + self.tcx, + self.tcx.require_lang_item(lang_items::CountCodeRegionFnLangItem, None), + injection_point, + ); + let counter_index = Operand::const_from_scalar( + self.tcx, + self.tcx.types.u32, + Scalar::from_u32(self.next_counter()), + injection_point, + ); - patch.apply(body); + let mut patch = MirPatch::new(mir_body); - // To insert the `new_block` in front of the first block in the counted branch (for example, - // the START_BLOCK, at the top of the function), just swap the indexes, leaving the rest of the - // graph unchanged. - body.basic_blocks_mut().swap(next_block, new_block); + let temp = patch.new_temp(self.tcx.mk_unit(), code_region); + let new_block = patch.new_block(placeholder_block(code_region)); + patch.patch_terminator( + new_block, + TerminatorKind::Call { + func: count_code_region_fn, + args: vec![counter_index], + // new_block will swapped with the next_block, after applying patch + destination: Some((Place::from(temp), new_block)), + cleanup: None, + from_hir_call: false, + fn_span: injection_point, + }, + ); + + patch.add_statement(new_block.start_location(), StatementKind::StorageLive(temp)); + patch.add_statement(next_block.start_location(), StatementKind::StorageDead(temp)); + + patch.apply(mir_body); + + // To insert the `new_block` in front of the first block in the counted branch (the + // `next_block`), just swap the indexes, leaving the rest of the graph unchanged. + mir_body.basic_blocks_mut().swap(next_block, new_block); + } } fn function_handle<'tcx>(tcx: TyCtxt<'tcx>, fn_def_id: DefId, span: Span) -> Operand<'tcx> { @@ -79,14 +126,59 @@ fn function_handle<'tcx>(tcx: TyCtxt<'tcx>, fn_def_id: DefId, span: Span) -> Ope Operand::function_handle(tcx, fn_def_id, substs, span) } -fn placeholder_block<'tcx>(source_info: SourceInfo) -> BasicBlockData<'tcx> { +fn placeholder_block(span: Span) -> BasicBlockData<'tcx> { BasicBlockData { statements: vec![], terminator: Some(Terminator { - source_info, + source_info: SourceInfo::outermost(span), // this gets overwritten by the counter Call kind: TerminatorKind::Unreachable, }), is_cleanup: false, } } + +fn hash_mir_source<'tcx>(tcx: TyCtxt<'tcx>, src: &MirSource<'tcx>) -> u64 { + let fn_body_id = match tcx.hir().get_if_local(src.def_id()) { + Some(node) => match associated_body(node) { + Some(body_id) => body_id, + _ => bug!("instrumented MirSource does not include a function body: {:?}", node), + }, + None => bug!("instrumented MirSource is not local: {:?}", src), + }; + let hir_body = tcx.hir().body(fn_body_id); + let mut hcx = tcx.create_no_span_stable_hashing_context(); + hash(&mut hcx, &hir_body.value).to_smaller_hash() +} + +fn hash( + hcx: &mut StableHashingContext<'tcx>, + node: &impl HashStable>, +) -> Fingerprint { + let mut stable_hasher = StableHasher::new(); + node.hash_stable(hcx, &mut stable_hasher); + stable_hasher.finish() +} + +fn associated_body<'hir>(node: Node<'hir>) -> Option { + match node { + Node::Item(Item { + kind: ItemKind::Const(_, body) | ItemKind::Static(.., body) | ItemKind::Fn(.., body), + .. + }) + | Node::TraitItem(TraitItem { + kind: + TraitItemKind::Const(_, Some(body)) | TraitItemKind::Fn(_, TraitFn::Provided(body)), + .. + }) + | Node::ImplItem(ImplItem { + kind: ImplItemKind::Const(_, body) | ImplItemKind::Fn(_, body), + .. + }) + | Node::Expr(Expr { kind: ExprKind::Closure(.., body, _, _), .. }) => Some(*body), + + Node::AnonConst(constant) => Some(constant.body), + + _ => None, + } +} diff --git a/src/librustc_session/options.rs b/src/librustc_session/options.rs index 2d231359057fd..c2d056ad26d0b 100644 --- a/src/librustc_session/options.rs +++ b/src/librustc_session/options.rs @@ -877,8 +877,9 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options, (such as entering an empty infinite loop) by inserting llvm.sideeffect \ (default: no)"), instrument_coverage: bool = (false, parse_bool, [TRACKED], - "instrument the generated code with LLVM code region counters to \ - (in the future) generate coverage reports (experimental; default: no)"), + "instrument the generated code with LLVM code region counters to (in the \ + future) generate coverage reports (default: no; note, the compiler build \ + config must include `profiler = true`)"), instrument_mcount: bool = (false, parse_bool, [TRACKED], "insert function instrument code for mcount-based tracing (default: no)"), keep_hygiene_data: bool = (false, parse_bool, [UNTRACKED], diff --git a/src/librustc_span/symbol.rs b/src/librustc_span/symbol.rs index 970a26325926c..6efe9b20d0d8f 100644 --- a/src/librustc_span/symbol.rs +++ b/src/librustc_span/symbol.rs @@ -587,6 +587,7 @@ symbols! { proc_macro_mod, proc_macro_non_items, proc_macro_path_invoc, + profiler_builtins, profiler_runtime, ptr_offset_from, pub_restricted, From 933fe805777e46163c52371d81390ba721a37252 Mon Sep 17 00:00:00 2001 From: Rich Kadel Date: Sun, 21 Jun 2020 23:48:39 -0700 Subject: [PATCH 2/8] num_counters to u32, after implementing TypeFoldable --- src/librustc_codegen_llvm/intrinsic.rs | 2 +- src/librustc_middle/mir/mod.rs | 2 +- src/librustc_middle/ty/structural_impls.rs | 1 + src/librustc_mir/transform/instrument_coverage.rs | 6 +++--- 4 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/librustc_codegen_llvm/intrinsic.rs b/src/librustc_codegen_llvm/intrinsic.rs index 8c0ccde6b16bb..c6e7820a60ed4 100644 --- a/src/librustc_codegen_llvm/intrinsic.rs +++ b/src/librustc_codegen_llvm/intrinsic.rs @@ -146,7 +146,7 @@ impl IntrinsicCallMethods<'tcx> for Builder<'a, 'll, 'tcx> { let (mangled_fn_name, _len_val) = self.const_str(mangled_fn.name); let hash = self.const_u64(coverage_data.hash); let index = args[0].immediate(); - let num_counters = self.const_u32(coverage_data.num_counters as u32); + let num_counters = self.const_u32(coverage_data.num_counters); debug!( "count_code_region to LLVM intrinsic instrprof.increment(fn_name={}, hash={:?}, num_counters={:?}, index={:?})", mangled_fn.name, hash, index, num_counters diff --git a/src/librustc_middle/mir/mod.rs b/src/librustc_middle/mir/mod.rs index a89a5ef3f8218..e88329db992f5 100644 --- a/src/librustc_middle/mir/mod.rs +++ b/src/librustc_middle/mir/mod.rs @@ -98,7 +98,7 @@ pub struct CoverageData { pub hash: u64, /// The total number of coverage region counters added to this MIR Body. - pub num_counters: usize, + pub num_counters: u32, } /// The lowered representation of a single function. diff --git a/src/librustc_middle/ty/structural_impls.rs b/src/librustc_middle/ty/structural_impls.rs index f04d31601ea5b..463e2c57d46c9 100644 --- a/src/librustc_middle/ty/structural_impls.rs +++ b/src/librustc_middle/ty/structural_impls.rs @@ -262,6 +262,7 @@ CloneTypeFoldableAndLiftImpls! { bool, usize, ::rustc_target::abi::VariantIdx, + u32, u64, String, crate::middle::region::Scope, diff --git a/src/librustc_mir/transform/instrument_coverage.rs b/src/librustc_mir/transform/instrument_coverage.rs index 793ccbb081bed..a24d0acf4212c 100644 --- a/src/librustc_mir/transform/instrument_coverage.rs +++ b/src/librustc_mir/transform/instrument_coverage.rs @@ -21,7 +21,7 @@ pub struct InstrumentCoverage; struct Instrumentor<'tcx> { tcx: TyCtxt<'tcx>, - num_counters: usize, + num_counters: u32, } impl<'tcx> MirPass<'tcx> for InstrumentCoverage { @@ -55,12 +55,12 @@ impl<'tcx> Instrumentor<'tcx> { } fn next_counter(&mut self) -> u32 { - let next = self.num_counters as u32; + let next = self.num_counters; self.num_counters += 1; next } - fn inject_counters(&mut self, mir_body: &mut mir::Body<'tcx>) -> usize { + fn inject_counters(&mut self, mir_body: &mut mir::Body<'tcx>) -> u32 { // FIXME(richkadel): As a first step, counters are only injected at the top of each // function. The complete solution will inject counters at each conditional code branch. let top_of_function = START_BLOCK; From f4a79385cf28b263894be9ebd2e541532ae82898 Mon Sep 17 00:00:00 2001 From: Rich Kadel Date: Mon, 22 Jun 2020 14:11:55 -0700 Subject: [PATCH 3/8] implemented query for coverage data This commit adds a query that allows the CoverageData to be pulled from a call on tcx, avoiding the need to change the `codegen_intrinsic_call()` signature (no need to pass in the FunctionCx or any additional arguments. The commit does not change where/when the CoverageData is computed. It's still done in the `pass`, and saved in the MIR `Body`. See discussion (in progress) here: https://github.com/rust-lang/rust/pull/73488#discussion_r443825646 --- src/librustc_codegen_llvm/intrinsic.rs | 12 +++++++----- src/librustc_codegen_ssa/mir/block.rs | 2 +- src/librustc_codegen_ssa/mir/mod.rs | 4 ++-- src/librustc_codegen_ssa/traits/intrinsic.rs | 6 ++---- src/librustc_middle/query/mod.rs | 6 ++++++ src/librustc_mir/transform/mod.rs | 8 +++++++- 6 files changed, 25 insertions(+), 13 deletions(-) diff --git a/src/librustc_codegen_llvm/intrinsic.rs b/src/librustc_codegen_llvm/intrinsic.rs index c6e7820a60ed4..f1104ca3a98b3 100644 --- a/src/librustc_codegen_llvm/intrinsic.rs +++ b/src/librustc_codegen_llvm/intrinsic.rs @@ -16,7 +16,6 @@ use rustc_codegen_ssa::common::TypeKind; use rustc_codegen_ssa::glue; use rustc_codegen_ssa::mir::operand::{OperandRef, OperandValue}; use rustc_codegen_ssa::mir::place::PlaceRef; -use rustc_codegen_ssa::mir::FunctionCx; use rustc_codegen_ssa::traits::*; use rustc_codegen_ssa::MemFlags; use rustc_hir as hir; @@ -82,14 +81,14 @@ fn get_simple_intrinsic(cx: &CodegenCx<'ll, '_>, name: &str) -> Option<&'ll Valu } impl IntrinsicCallMethods<'tcx> for Builder<'a, 'll, 'tcx> { - fn codegen_intrinsic_call<'b, Bx: BuilderMethods<'b, 'tcx>>( + fn codegen_intrinsic_call( &mut self, - fx: &FunctionCx<'b, 'tcx, Bx>, instance: ty::Instance<'tcx>, fn_abi: &FnAbi<'tcx, Ty<'tcx>>, args: &[OperandRef<'tcx, &'ll Value>], llresult: &'ll Value, span: Span, + caller_instance: ty::Instance<'tcx>, ) { let tcx = self.tcx; let callee_ty = instance.monomorphic_ty(tcx); @@ -141,8 +140,11 @@ impl IntrinsicCallMethods<'tcx> for Builder<'a, 'll, 'tcx> { self.call(llfn, &[], None) } "count_code_region" => { - let coverage_data = fx.mir.coverage_data.as_ref().unwrap(); - let mangled_fn = tcx.symbol_name(fx.instance); + let coverage_data = tcx + .coverage_data(caller_instance.def_id()) + .as_ref() + .expect("LLVM intrinsic count_code_region call has associated coverage_data"); + let mangled_fn = tcx.symbol_name(caller_instance); let (mangled_fn_name, _len_val) = self.const_str(mangled_fn.name); let hash = self.const_u64(coverage_data.hash); let index = args[0].immediate(); diff --git a/src/librustc_codegen_ssa/mir/block.rs b/src/librustc_codegen_ssa/mir/block.rs index 945c3d4843471..d56c816811b3c 100644 --- a/src/librustc_codegen_ssa/mir/block.rs +++ b/src/librustc_codegen_ssa/mir/block.rs @@ -688,12 +688,12 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { .collect(); bx.codegen_intrinsic_call( - self, *instance.as_ref().unwrap(), &fn_abi, &args, dest, terminator.source_info.span, + self.instance, ); if let ReturnDest::IndirectOperand(dst, _) = ret_dest { diff --git a/src/librustc_codegen_ssa/mir/mod.rs b/src/librustc_codegen_ssa/mir/mod.rs index fd2e779f681be..00b4bf96afa59 100644 --- a/src/librustc_codegen_ssa/mir/mod.rs +++ b/src/librustc_codegen_ssa/mir/mod.rs @@ -21,9 +21,9 @@ use self::operand::{OperandRef, OperandValue}; /// Master context for codegenning from MIR. pub struct FunctionCx<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> { - pub instance: Instance<'tcx>, + instance: Instance<'tcx>, - pub mir: &'tcx mir::Body<'tcx>, + mir: &'tcx mir::Body<'tcx>, debug_context: Option>, diff --git a/src/librustc_codegen_ssa/traits/intrinsic.rs b/src/librustc_codegen_ssa/traits/intrinsic.rs index 0036eadf4db81..f62019498511c 100644 --- a/src/librustc_codegen_ssa/traits/intrinsic.rs +++ b/src/librustc_codegen_ssa/traits/intrinsic.rs @@ -1,7 +1,5 @@ use super::BackendTypes; use crate::mir::operand::OperandRef; -use crate::mir::FunctionCx; -use crate::traits::BuilderMethods; use rustc_middle::ty::{self, Ty}; use rustc_span::Span; use rustc_target::abi::call::FnAbi; @@ -10,14 +8,14 @@ pub trait IntrinsicCallMethods<'tcx>: BackendTypes { /// Remember to add all intrinsics here, in librustc_typeck/check/mod.rs, /// and in libcore/intrinsics.rs; if you need access to any llvm intrinsics, /// add them to librustc_codegen_llvm/context.rs - fn codegen_intrinsic_call<'a, Bx: BuilderMethods<'a, 'tcx>>( + fn codegen_intrinsic_call( &mut self, - fx: &FunctionCx<'a, 'tcx, Bx>, instance: ty::Instance<'tcx>, fn_abi: &FnAbi<'tcx, Ty<'tcx>>, args: &[OperandRef<'tcx, Self::Value>], llresult: Self::Value, span: Span, + caller_instance: ty::Instance<'tcx>, ); fn abort(&mut self); diff --git a/src/librustc_middle/query/mod.rs b/src/librustc_middle/query/mod.rs index 3b6d54a1bc1ee..1092e6c306411 100644 --- a/src/librustc_middle/query/mod.rs +++ b/src/librustc_middle/query/mod.rs @@ -214,6 +214,12 @@ rustc_queries! { cache_on_disk_if { key.is_local() } } + query coverage_data(key: DefId) -> Option { + desc { |tcx| "retrieving coverage data, if computed from MIR for `{}`", tcx.def_path_str(key) } + storage(ArenaCacheSelector<'tcx>) + cache_on_disk_if { key.is_local() } + } + query promoted_mir(key: DefId) -> IndexVec> { desc { |tcx| "optimizing promoted MIR for `{}`", tcx.def_path_str(key) } storage(ArenaCacheSelector<'tcx>) diff --git a/src/librustc_mir/transform/mod.rs b/src/librustc_mir/transform/mod.rs index 846ed1f86d8d6..a2fdc7efd184d 100644 --- a/src/librustc_mir/transform/mod.rs +++ b/src/librustc_mir/transform/mod.rs @@ -6,7 +6,7 @@ use rustc_hir::def_id::{CrateNum, DefId, LocalDefId, LOCAL_CRATE}; use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor}; use rustc_index::vec::IndexVec; use rustc_middle::mir::visit::Visitor as _; -use rustc_middle::mir::{traversal, Body, ConstQualifs, MirPhase, Promoted}; +use rustc_middle::mir::{traversal, Body, ConstQualifs, CoverageData, MirPhase, Promoted}; use rustc_middle::ty::query::Providers; use rustc_middle::ty::steal::Steal; use rustc_middle::ty::{InstanceDef, TyCtxt, TypeFoldable}; @@ -53,6 +53,7 @@ pub(crate) fn provide(providers: &mut Providers<'_>) { mir_drops_elaborated_and_const_checked, optimized_mir, is_mir_available, + coverage_data, promoted_mir, ..*providers }; @@ -422,6 +423,11 @@ fn run_optimization_passes<'tcx>( ); } +fn coverage_data(tcx: TyCtxt<'_>, def_id: DefId) -> Option { + let body = tcx.optimized_mir(def_id); + body.coverage_data.clone() +} + fn optimized_mir(tcx: TyCtxt<'_>, def_id: DefId) -> Body<'_> { if tcx.is_constructor(def_id) { // There's no reason to run all of the MIR passes on constructors when From 994d9d03272363d57a655ebacbf58f0069b3b177 Mon Sep 17 00:00:00 2001 From: Rich Kadel Date: Mon, 22 Jun 2020 15:54:28 -0700 Subject: [PATCH 4/8] Address remaining feedback items --- src/librustc_metadata/locator.rs | 2 +- src/librustc_middle/hir/map/mod.rs | 2 +- src/librustc_middle/query/mod.rs | 2 +- .../transform/instrument_coverage.rs | 27 ++----------------- 4 files changed, 5 insertions(+), 28 deletions(-) diff --git a/src/librustc_metadata/locator.rs b/src/librustc_metadata/locator.rs index 662794f0aa11f..1bdac1039b55a 100644 --- a/src/librustc_metadata/locator.rs +++ b/src/librustc_metadata/locator.rs @@ -489,7 +489,7 @@ impl<'a> CrateLocator<'a> { { err.note(&format!("the `{}` target may not be installed", self.triple)); } else if self.crate_name == sym::profiler_builtins { - err.note(&"the compiler may have been built without `profiler = true`"); + err.note(&"the compiler may have been built without the profiler runtime"); } err.span_label(self.span, "can't find crate"); err diff --git a/src/librustc_middle/hir/map/mod.rs b/src/librustc_middle/hir/map/mod.rs index e3e0856ffc52e..d60d24aa9aed5 100644 --- a/src/librustc_middle/hir/map/mod.rs +++ b/src/librustc_middle/hir/map/mod.rs @@ -56,7 +56,7 @@ fn fn_sig<'hir>(node: Node<'hir>) -> Option<&'hir FnSig<'hir>> { } } -fn associated_body<'hir>(node: Node<'hir>) -> Option { +pub fn associated_body<'hir>(node: Node<'hir>) -> Option { match node { Node::Item(Item { kind: ItemKind::Const(_, body) | ItemKind::Static(.., body) | ItemKind::Fn(.., body), diff --git a/src/librustc_middle/query/mod.rs b/src/librustc_middle/query/mod.rs index 1092e6c306411..993b48afb7a9a 100644 --- a/src/librustc_middle/query/mod.rs +++ b/src/librustc_middle/query/mod.rs @@ -215,7 +215,7 @@ rustc_queries! { } query coverage_data(key: DefId) -> Option { - desc { |tcx| "retrieving coverage data, if computed from MIR for `{}`", tcx.def_path_str(key) } + desc { |tcx| "retrieving coverage data from MIR for `{}`", tcx.def_path_str(key) } storage(ArenaCacheSelector<'tcx>) cache_on_disk_if { key.is_local() } } diff --git a/src/librustc_mir/transform/instrument_coverage.rs b/src/librustc_mir/transform/instrument_coverage.rs index a24d0acf4212c..94aa26b3081e5 100644 --- a/src/librustc_mir/transform/instrument_coverage.rs +++ b/src/librustc_mir/transform/instrument_coverage.rs @@ -3,7 +3,7 @@ use crate::util::patch::MirPatch; use rustc_data_structures::fingerprint::Fingerprint; use rustc_data_structures::stable_hasher::{HashStable, StableHasher}; use rustc_hir::lang_items; -use rustc_hir::*; +use rustc_middle::hir; use rustc_middle::ich::StableHashingContext; use rustc_middle::mir::interpret::Scalar; use rustc_middle::mir::{ @@ -140,7 +140,7 @@ fn placeholder_block(span: Span) -> BasicBlockData<'tcx> { fn hash_mir_source<'tcx>(tcx: TyCtxt<'tcx>, src: &MirSource<'tcx>) -> u64 { let fn_body_id = match tcx.hir().get_if_local(src.def_id()) { - Some(node) => match associated_body(node) { + Some(node) => match hir::map::associated_body(node) { Some(body_id) => body_id, _ => bug!("instrumented MirSource does not include a function body: {:?}", node), }, @@ -159,26 +159,3 @@ fn hash( node.hash_stable(hcx, &mut stable_hasher); stable_hasher.finish() } - -fn associated_body<'hir>(node: Node<'hir>) -> Option { - match node { - Node::Item(Item { - kind: ItemKind::Const(_, body) | ItemKind::Static(.., body) | ItemKind::Fn(.., body), - .. - }) - | Node::TraitItem(TraitItem { - kind: - TraitItemKind::Const(_, Some(body)) | TraitItemKind::Fn(_, TraitFn::Provided(body)), - .. - }) - | Node::ImplItem(ImplItem { - kind: ImplItemKind::Const(_, body) | ImplItemKind::Fn(_, body), - .. - }) - | Node::Expr(Expr { kind: ExprKind::Closure(.., body, _, _), .. }) => Some(*body), - - Node::AnonConst(constant) => Some(constant.body), - - _ => None, - } -} From 08ec4cbb9e0672118946c85f410b50ea4001b1dd Mon Sep 17 00:00:00 2001 From: Rich Kadel Date: Mon, 22 Jun 2020 19:21:56 -0700 Subject: [PATCH 5/8] moves coverage data computation from pass to query --- src/librustc_codegen_llvm/intrinsic.rs | 9 +-- src/librustc_middle/mir/mod.rs | 34 +++++------ src/librustc_middle/query/mod.rs | 2 +- .../transform/instrument_coverage.rs | 58 +++++++++++-------- src/librustc_mir/transform/mod.rs | 9 +-- 5 files changed, 56 insertions(+), 56 deletions(-) diff --git a/src/librustc_codegen_llvm/intrinsic.rs b/src/librustc_codegen_llvm/intrinsic.rs index f1104ca3a98b3..b9193a85b1e48 100644 --- a/src/librustc_codegen_llvm/intrinsic.rs +++ b/src/librustc_codegen_llvm/intrinsic.rs @@ -140,18 +140,15 @@ impl IntrinsicCallMethods<'tcx> for Builder<'a, 'll, 'tcx> { self.call(llfn, &[], None) } "count_code_region" => { - let coverage_data = tcx - .coverage_data(caller_instance.def_id()) - .as_ref() - .expect("LLVM intrinsic count_code_region call has associated coverage_data"); + let coverage_data = tcx.coverage_data(caller_instance.def_id()); let mangled_fn = tcx.symbol_name(caller_instance); let (mangled_fn_name, _len_val) = self.const_str(mangled_fn.name); let hash = self.const_u64(coverage_data.hash); - let index = args[0].immediate(); let num_counters = self.const_u32(coverage_data.num_counters); + let index = args[0].immediate(); debug!( "count_code_region to LLVM intrinsic instrprof.increment(fn_name={}, hash={:?}, num_counters={:?}, index={:?})", - mangled_fn.name, hash, index, num_counters + mangled_fn.name, hash, num_counters, index ); self.instrprof_increment(mangled_fn_name, hash, num_counters, index) } diff --git a/src/librustc_middle/mir/mod.rs b/src/librustc_middle/mir/mod.rs index e88329db992f5..854fda095b65b 100644 --- a/src/librustc_middle/mir/mod.rs +++ b/src/librustc_middle/mir/mod.rs @@ -88,19 +88,6 @@ impl MirPhase { } } -/// Coverage data computed by the `InstrumentCoverage` MIR pass, when compiling with -/// `-Zinstrument_coverage`. -#[derive(Clone, RustcEncodable, RustcDecodable, Debug, HashStable, TypeFoldable)] -pub struct CoverageData { - /// A hash value that can be used by the consumer of the coverage profile data to detect - /// changes to the instrumented source of the associated MIR body (typically, for an - /// individual function). - pub hash: u64, - - /// The total number of coverage region counters added to this MIR Body. - pub num_counters: u32, -} - /// The lowered representation of a single function. #[derive(Clone, RustcEncodable, RustcDecodable, Debug, HashStable, TypeFoldable)] pub struct Body<'tcx> { @@ -184,10 +171,6 @@ pub struct Body<'tcx> { /// FIXME(oli-obk): rewrite the promoted during promotion to eliminate the cell components. pub ignore_interior_mut_in_const_validation: bool, - /// If compiling with `-Zinstrument_coverage`, the `InstrumentCoverage` pass stores summary - /// information associated with the MIR, used in code generation of the coverage counters. - pub coverage_data: Option, - predecessor_cache: PredecessorCache, } @@ -228,7 +211,6 @@ impl<'tcx> Body<'tcx> { required_consts: Vec::new(), ignore_interior_mut_in_const_validation: false, control_flow_destroyed, - coverage_data: None, predecessor_cache: PredecessorCache::new(), } } @@ -256,7 +238,6 @@ impl<'tcx> Body<'tcx> { generator_kind: None, var_debug_info: Vec::new(), ignore_interior_mut_in_const_validation: false, - coverage_data: None, predecessor_cache: PredecessorCache::new(), } } @@ -2938,3 +2919,18 @@ impl Location { } } } + +/// Coverage data associated with each function (MIR) instrumented with coverage counters, when +/// compiled with `-Zinstrument_coverage`. The query `tcx.coverage_data(DefId)` computes these +/// values on demand (during code generation). This query is only valid after executing the MIR pass +/// `InstrumentCoverage`. +#[derive(Clone, RustcEncodable, RustcDecodable, Debug, HashStable)] +pub struct CoverageData { + /// A hash value that can be used by the consumer of the coverage profile data to detect + /// changes to the instrumented source of the associated MIR body (typically, for an + /// individual function). + pub hash: u64, + + /// The total number of coverage region counters added to the MIR `Body`. + pub num_counters: u32, +} diff --git a/src/librustc_middle/query/mod.rs b/src/librustc_middle/query/mod.rs index 993b48afb7a9a..4815f2617b69b 100644 --- a/src/librustc_middle/query/mod.rs +++ b/src/librustc_middle/query/mod.rs @@ -214,7 +214,7 @@ rustc_queries! { cache_on_disk_if { key.is_local() } } - query coverage_data(key: DefId) -> Option { + query coverage_data(key: DefId) -> mir::CoverageData { desc { |tcx| "retrieving coverage data from MIR for `{}`", tcx.def_path_str(key) } storage(ArenaCacheSelector<'tcx>) cache_on_disk_if { key.is_local() } diff --git a/src/librustc_mir/transform/instrument_coverage.rs b/src/librustc_mir/transform/instrument_coverage.rs index 94aa26b3081e5..8d263c3089c47 100644 --- a/src/librustc_mir/transform/instrument_coverage.rs +++ b/src/librustc_mir/transform/instrument_coverage.rs @@ -7,10 +7,12 @@ use rustc_middle::hir; use rustc_middle::ich::StableHashingContext; use rustc_middle::mir::interpret::Scalar; use rustc_middle::mir::{ - self, BasicBlock, BasicBlockData, CoverageData, Operand, Place, SourceInfo, StatementKind, - Terminator, TerminatorKind, START_BLOCK, + self, traversal, BasicBlock, BasicBlockData, CoverageData, Operand, Place, SourceInfo, + StatementKind, Terminator, TerminatorKind, START_BLOCK, }; use rustc_middle::ty; +use rustc_middle::ty::query::Providers; +use rustc_middle::ty::FnDef; use rustc_middle::ty::TyCtxt; use rustc_span::def_id::DefId; use rustc_span::Span; @@ -19,6 +21,31 @@ use rustc_span::Span; /// the intrinsic llvm.instrprof.increment. pub struct InstrumentCoverage; +/// The `query` provider for `CoverageData`, requested by `codegen_intrinsic_call()` when +/// constructing the arguments for `llvm.instrprof.increment`. +pub(crate) fn provide(providers: &mut Providers<'_>) { + providers.coverage_data = |tcx, def_id| { + let body = tcx.optimized_mir(def_id); + let count_code_region_fn = + tcx.require_lang_item(lang_items::CountCodeRegionFnLangItem, None); + let mut num_counters: u32 = 0; + for (_, data) in traversal::preorder(body) { + if let Some(terminator) = &data.terminator { + if let TerminatorKind::Call { func: Operand::Constant(func), .. } = &terminator.kind + { + if let FnDef(called_fn_def_id, _) = func.literal.ty.kind { + if called_fn_def_id == count_code_region_fn { + num_counters += 1; + } + } + } + } + } + let hash = if num_counters > 0 { hash_mir_source(tcx, def_id) } else { 0 }; + CoverageData { num_counters, hash } + }; +} + struct Instrumentor<'tcx> { tcx: TyCtxt<'tcx>, num_counters: u32, @@ -30,20 +57,12 @@ impl<'tcx> MirPass<'tcx> for InstrumentCoverage { // If the InstrumentCoverage pass is called on promoted MIRs, skip them. // See: https://github.com/rust-lang/rust/pull/73011#discussion_r438317601 if src.promoted.is_none() { - assert!(mir_body.coverage_data.is_none()); - - let hash = hash_mir_source(tcx, &src); - debug!( - "instrumenting {:?}, hash: {}, span: {}", + "instrumenting {:?}, span: {}", src.def_id(), - hash, tcx.sess.source_map().span_to_string(mir_body.span) ); - - let num_counters = Instrumentor::new(tcx).inject_counters(mir_body); - - mir_body.coverage_data = Some(CoverageData { hash, num_counters }); + Instrumentor::new(tcx).inject_counters(mir_body); } } } @@ -60,15 +79,13 @@ impl<'tcx> Instrumentor<'tcx> { next } - fn inject_counters(&mut self, mir_body: &mut mir::Body<'tcx>) -> u32 { + fn inject_counters(&mut self, mir_body: &mut mir::Body<'tcx>) { // FIXME(richkadel): As a first step, counters are only injected at the top of each // function. The complete solution will inject counters at each conditional code branch. let top_of_function = START_BLOCK; let entire_function = mir_body.span; self.inject_counter(mir_body, top_of_function, entire_function); - - self.num_counters } fn inject_counter( @@ -138,14 +155,9 @@ fn placeholder_block(span: Span) -> BasicBlockData<'tcx> { } } -fn hash_mir_source<'tcx>(tcx: TyCtxt<'tcx>, src: &MirSource<'tcx>) -> u64 { - let fn_body_id = match tcx.hir().get_if_local(src.def_id()) { - Some(node) => match hir::map::associated_body(node) { - Some(body_id) => body_id, - _ => bug!("instrumented MirSource does not include a function body: {:?}", node), - }, - None => bug!("instrumented MirSource is not local: {:?}", src), - }; +fn hash_mir_source<'tcx>(tcx: TyCtxt<'tcx>, def_id: DefId) -> u64 { + let hir_node = tcx.hir().get_if_local(def_id).expect("DefId is local"); + let fn_body_id = hir::map::associated_body(hir_node).expect("HIR node is a function with body"); let hir_body = tcx.hir().body(fn_body_id); let mut hcx = tcx.create_no_span_stable_hashing_context(); hash(&mut hcx, &hir_body.value).to_smaller_hash() diff --git a/src/librustc_mir/transform/mod.rs b/src/librustc_mir/transform/mod.rs index a2fdc7efd184d..8ca240d2c7da7 100644 --- a/src/librustc_mir/transform/mod.rs +++ b/src/librustc_mir/transform/mod.rs @@ -6,7 +6,7 @@ use rustc_hir::def_id::{CrateNum, DefId, LocalDefId, LOCAL_CRATE}; use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor}; use rustc_index::vec::IndexVec; use rustc_middle::mir::visit::Visitor as _; -use rustc_middle::mir::{traversal, Body, ConstQualifs, CoverageData, MirPhase, Promoted}; +use rustc_middle::mir::{traversal, Body, ConstQualifs, MirPhase, Promoted}; use rustc_middle::ty::query::Providers; use rustc_middle::ty::steal::Steal; use rustc_middle::ty::{InstanceDef, TyCtxt, TypeFoldable}; @@ -53,10 +53,10 @@ pub(crate) fn provide(providers: &mut Providers<'_>) { mir_drops_elaborated_and_const_checked, optimized_mir, is_mir_available, - coverage_data, promoted_mir, ..*providers }; + instrument_coverage::provide(providers); } fn is_mir_available(tcx: TyCtxt<'_>, def_id: DefId) -> bool { @@ -423,11 +423,6 @@ fn run_optimization_passes<'tcx>( ); } -fn coverage_data(tcx: TyCtxt<'_>, def_id: DefId) -> Option { - let body = tcx.optimized_mir(def_id); - body.coverage_data.clone() -} - fn optimized_mir(tcx: TyCtxt<'_>, def_id: DefId) -> Body<'_> { if tcx.is_constructor(def_id) { // There's no reason to run all of the MIR passes on constructors when From 3d0192e7c80b4aaa53f7609c1a73118bc91d30aa Mon Sep 17 00:00:00 2001 From: Rich Kadel Date: Mon, 22 Jun 2020 19:27:48 -0700 Subject: [PATCH 6/8] PR no longer requires u32 impl TypeFoldable --- src/librustc_middle/ty/structural_impls.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/librustc_middle/ty/structural_impls.rs b/src/librustc_middle/ty/structural_impls.rs index 463e2c57d46c9..f04d31601ea5b 100644 --- a/src/librustc_middle/ty/structural_impls.rs +++ b/src/librustc_middle/ty/structural_impls.rs @@ -262,7 +262,6 @@ CloneTypeFoldableAndLiftImpls! { bool, usize, ::rustc_target::abi::VariantIdx, - u32, u64, String, crate::middle::region::Scope, From a04514026824f9342ab93d9b608e3ec5dab53dad Mon Sep 17 00:00:00 2001 From: Rich Kadel Date: Mon, 22 Jun 2020 19:30:52 -0700 Subject: [PATCH 7/8] using "mir_body" (vs "body") in InstrumentCoverage The mod uses both MIR bodies and HIR bodies, so I'm trying to maintain consistency with these names. --- src/librustc_mir/transform/instrument_coverage.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/librustc_mir/transform/instrument_coverage.rs b/src/librustc_mir/transform/instrument_coverage.rs index 8d263c3089c47..27aaf47bbf2ae 100644 --- a/src/librustc_mir/transform/instrument_coverage.rs +++ b/src/librustc_mir/transform/instrument_coverage.rs @@ -25,11 +25,11 @@ pub struct InstrumentCoverage; /// constructing the arguments for `llvm.instrprof.increment`. pub(crate) fn provide(providers: &mut Providers<'_>) { providers.coverage_data = |tcx, def_id| { - let body = tcx.optimized_mir(def_id); + let mir_body = tcx.optimized_mir(def_id); let count_code_region_fn = tcx.require_lang_item(lang_items::CountCodeRegionFnLangItem, None); let mut num_counters: u32 = 0; - for (_, data) in traversal::preorder(body) { + for (_, data) in traversal::preorder(mir_body) { if let Some(terminator) = &data.terminator { if let TerminatorKind::Call { func: Operand::Constant(func), .. } = &terminator.kind { From 977ce57d915914139c4aa643e63f368913e5f437 Mon Sep 17 00:00:00 2001 From: Rich Kadel Date: Mon, 22 Jun 2020 23:31:41 -0700 Subject: [PATCH 8/8] Updated query for num_counters to compute from max index Also added FIXME comments to note the possible need to accommodate counter increment calls in source-based functions that differ from the function context of the caller instance (e.g., inline functions). --- src/librustc_codegen_llvm/intrinsic.rs | 3 ++ .../transform/instrument_coverage.rs | 28 ++++++++++++++++--- 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/src/librustc_codegen_llvm/intrinsic.rs b/src/librustc_codegen_llvm/intrinsic.rs index b9193a85b1e48..dfe97b1ee2e9d 100644 --- a/src/librustc_codegen_llvm/intrinsic.rs +++ b/src/librustc_codegen_llvm/intrinsic.rs @@ -140,6 +140,9 @@ impl IntrinsicCallMethods<'tcx> for Builder<'a, 'll, 'tcx> { self.call(llfn, &[], None) } "count_code_region" => { + // FIXME(richkadel): The current implementation assumes the MIR for the given + // caller_instance represents a single function. Validate and/or correct if inlining + // and/or monomorphization invalidates these assumptions. let coverage_data = tcx.coverage_data(caller_instance.def_id()); let mangled_fn = tcx.symbol_name(caller_instance); let (mangled_fn_name, _len_val) = self.const_str(mangled_fn.name); diff --git a/src/librustc_mir/transform/instrument_coverage.rs b/src/librustc_mir/transform/instrument_coverage.rs index 27aaf47bbf2ae..06b648ab5a908 100644 --- a/src/librustc_mir/transform/instrument_coverage.rs +++ b/src/librustc_mir/transform/instrument_coverage.rs @@ -5,15 +5,15 @@ use rustc_data_structures::stable_hasher::{HashStable, StableHasher}; use rustc_hir::lang_items; use rustc_middle::hir; use rustc_middle::ich::StableHashingContext; -use rustc_middle::mir::interpret::Scalar; +use rustc_middle::mir::interpret::{ConstValue, Scalar}; use rustc_middle::mir::{ self, traversal, BasicBlock, BasicBlockData, CoverageData, Operand, Place, SourceInfo, StatementKind, Terminator, TerminatorKind, START_BLOCK, }; use rustc_middle::ty; use rustc_middle::ty::query::Providers; -use rustc_middle::ty::FnDef; use rustc_middle::ty::TyCtxt; +use rustc_middle::ty::{ConstKind, FnDef}; use rustc_span::def_id::DefId; use rustc_span::Span; @@ -26,16 +26,36 @@ pub struct InstrumentCoverage; pub(crate) fn provide(providers: &mut Providers<'_>) { providers.coverage_data = |tcx, def_id| { let mir_body = tcx.optimized_mir(def_id); + // FIXME(richkadel): The current implementation assumes the MIR for the given DefId + // represents a single function. Validate and/or correct if inlining and/or monomorphization + // invalidates these assumptions. let count_code_region_fn = tcx.require_lang_item(lang_items::CountCodeRegionFnLangItem, None); let mut num_counters: u32 = 0; + // The `num_counters` argument to `llvm.instrprof.increment` is the number of injected + // counters, with each counter having an index from `0..num_counters-1`. MIR optimization + // may split and duplicate some BasicBlock sequences. Simply counting the calls may not + // not work; but computing the num_counters by adding `1` to the highest index (for a given + // instrumented function) is valid. for (_, data) in traversal::preorder(mir_body) { if let Some(terminator) = &data.terminator { - if let TerminatorKind::Call { func: Operand::Constant(func), .. } = &terminator.kind + if let TerminatorKind::Call { func: Operand::Constant(func), args, .. } = + &terminator.kind { if let FnDef(called_fn_def_id, _) = func.literal.ty.kind { if called_fn_def_id == count_code_region_fn { - num_counters += 1; + if let Operand::Constant(constant) = + args.get(0).expect("count_code_region has at least one arg") + { + if let ConstKind::Value(ConstValue::Scalar(value)) = + constant.literal.val + { + let index = value + .to_u32() + .expect("count_code_region index at arg0 is u32"); + num_counters = std::cmp::max(num_counters, index + 1); + } + } } } }