From bcf755562a1e7c2ed4f6a292b3c82afc9b5b5fe6 Mon Sep 17 00:00:00 2001 From: Rich Kadel Date: Mon, 15 Mar 2021 16:32:45 -0700 Subject: coverage bug fixes and optimization support Adjusted LLVM codegen for code compiled with `-Zinstrument-coverage` to address multiple, somewhat related issues. Fixed a significant flaw in prior coverage solution: Every counter generated a new counter variable, but there should have only been one counter variable per function. This appears to have bloated .profraw files significantly. (For a small program, it increased the size by about 40%. I have not tested large programs, but there is anecdotal evidence that profraw files were way too large. This is a good fix, regardless, but hopefully it also addresses related issues. Fixes: #82144 Invalid LLVM coverage data produced when compiled with -C opt-level=1 Existing tests now work up to at least `opt-level=3`. This required a detailed analysis of the LLVM IR, comparisons with Clang C++ LLVM IR when compiled with coverage, and a lot of trial and error with codegen adjustments. The biggest hurdle was figuring out how to continue to support coverage results for unused functions and generics. Rust's coverage results have three advantages over Clang's coverage results: 1. Rust's coverage map does not include any overlapping code regions, making coverage counting unambiguous. 2. Rust generates coverage results (showing zero counts) for all unused functions, including generics. (Clang does not generate coverage for uninstantiated template functions.) 3. Rust's unused functions produce minimal stubbed functions in LLVM IR, sufficient for including in the coverage results; while Clang must generate the complete LLVM IR for each unused function, even though it will never be called. This PR removes the previous hack of attempting to inject coverage into some other existing function instance, and generates dedicated instances for each unused function. This change, and a few other adjustments (similar to what is required for `-C link-dead-code`, but with lower impact), makes it possible to support LLVM optimizations. Fixes: #79651 Coverage report: "Unexecuted instantiation:..." for a generic function from multiple crates Fixed by removing the aforementioned hack. Some "Unexecuted instantiation" notices are unavoidable, as explained in the `used_crate.rs` test, but `-Zinstrument-coverage` has new options to back off support for either unused generics, or all unused functions, which avoids the notice, at the cost of less coverage of unused functions. Fixes: #82875 Invalid LLVM coverage data produced with crate brotli_decompressor Fixed by disabling the LLVM function attribute that forces inlining, if `-Z instrument-coverage` is enabled. This attribute is applied to Rust functions with `#[inline(always)], and in some cases, the forced inlining breaks coverage instrumentation and reports. --- compiler/rustc_codegen_ssa/src/back/link.rs | 2 +- .../rustc_codegen_ssa/src/back/symbol_export.rs | 4 +-- compiler/rustc_codegen_ssa/src/back/write.rs | 2 +- compiler/rustc_codegen_ssa/src/coverageinfo/map.rs | 25 +++++++++++++--- compiler/rustc_codegen_ssa/src/mir/coverageinfo.rs | 2 +- .../rustc_codegen_ssa/src/traits/coverageinfo.rs | 33 ++++++++++++++++++++-- compiler/rustc_codegen_ssa/src/traits/mod.rs | 4 +-- 7 files changed, 57 insertions(+), 15 deletions(-) (limited to 'compiler/rustc_codegen_ssa/src') diff --git a/compiler/rustc_codegen_ssa/src/back/link.rs b/compiler/rustc_codegen_ssa/src/back/link.rs index b11821b7db0..4d6bc838130 100644 --- a/compiler/rustc_codegen_ssa/src/back/link.rs +++ b/compiler/rustc_codegen_ssa/src/back/link.rs @@ -1746,7 +1746,7 @@ fn linker_with_args<'a, B: ArchiveBuilder<'a>>( ); // OBJECT-FILES-NO, AUDIT-ORDER - if sess.opts.cg.profile_generate.enabled() || sess.opts.debugging_opts.instrument_coverage { + if sess.opts.cg.profile_generate.enabled() || sess.instrument_coverage() { cmd.pgo_gen(); } diff --git a/compiler/rustc_codegen_ssa/src/back/symbol_export.rs b/compiler/rustc_codegen_ssa/src/back/symbol_export.rs index 9a6f8cde1b2..abad8281d3a 100644 --- a/compiler/rustc_codegen_ssa/src/back/symbol_export.rs +++ b/compiler/rustc_codegen_ssa/src/back/symbol_export.rs @@ -188,9 +188,7 @@ fn exported_symbols_provider_local( } } - if tcx.sess.opts.debugging_opts.instrument_coverage - || tcx.sess.opts.cg.profile_generate.enabled() - { + if tcx.sess.instrument_coverage() || tcx.sess.opts.cg.profile_generate.enabled() { // These are weak symbols that point to the profile version and the // profile name, which need to be treated as exported so LTO doesn't nix // them. diff --git a/compiler/rustc_codegen_ssa/src/back/write.rs b/compiler/rustc_codegen_ssa/src/back/write.rs index 490b3d33112..7a8d8fb1304 100644 --- a/compiler/rustc_codegen_ssa/src/back/write.rs +++ b/compiler/rustc_codegen_ssa/src/back/write.rs @@ -176,7 +176,7 @@ impl ModuleConfig { // The rustc option `-Zinstrument_coverage` injects intrinsic calls to // `llvm.instrprof.increment()`, which requires the LLVM `instrprof` pass. - if sess.opts.debugging_opts.instrument_coverage { + if sess.instrument_coverage() { passes.push("instrprof".to_owned()); } passes diff --git a/compiler/rustc_codegen_ssa/src/coverageinfo/map.rs b/compiler/rustc_codegen_ssa/src/coverageinfo/map.rs index af6482fdbc2..7a17bced1c0 100644 --- a/compiler/rustc_codegen_ssa/src/coverageinfo/map.rs +++ b/compiler/rustc_codegen_ssa/src/coverageinfo/map.rs @@ -31,27 +31,44 @@ pub struct Expression { pub struct FunctionCoverage<'tcx> { instance: Instance<'tcx>, source_hash: u64, + is_used: bool, counters: IndexVec>, expressions: IndexVec>, unreachable_regions: Vec, } impl<'tcx> FunctionCoverage<'tcx> { + /// Creates a new set of coverage data for a used (called) function. pub fn new(tcx: TyCtxt<'tcx>, instance: Instance<'tcx>) -> Self { + Self::create(tcx, instance, true) + } + + /// Creates a new set of coverage data for an unused (never called) function. + pub fn unused(tcx: TyCtxt<'tcx>, instance: Instance<'tcx>) -> Self { + Self::create(tcx, instance, false) + } + + fn create(tcx: TyCtxt<'tcx>, instance: Instance<'tcx>, is_used: bool) -> Self { let coverageinfo = tcx.coverageinfo(instance.def_id()); debug!( - "FunctionCoverage::new(instance={:?}) has coverageinfo={:?}", - instance, coverageinfo + "FunctionCoverage::new(instance={:?}) has coverageinfo={:?}. is_used={}", + instance, coverageinfo, is_used ); Self { instance, source_hash: 0, // will be set with the first `add_counter()` + is_used, counters: IndexVec::from_elem_n(None, coverageinfo.num_counters as usize), expressions: IndexVec::from_elem_n(None, coverageinfo.num_expressions as usize), unreachable_regions: Vec::new(), } } + /// Returns true for a used (called) function, and false for an unused function. + pub fn is_used(&self) -> bool { + self.is_used + } + /// Sets the function source hash value. If called multiple times for the same function, all /// calls should have the same hash value. pub fn set_function_source_hash(&mut self, source_hash: u64) { @@ -128,8 +145,8 @@ impl<'tcx> FunctionCoverage<'tcx> { &'a self, ) -> (Vec, impl Iterator) { assert!( - self.source_hash != 0, - "No counters provided the source_hash for function: {:?}", + self.source_hash != 0 || !self.is_used, + "No counters provided the source_hash for used function: {:?}", self.instance ); diff --git a/compiler/rustc_codegen_ssa/src/mir/coverageinfo.rs b/compiler/rustc_codegen_ssa/src/mir/coverageinfo.rs index 5ab1baafb57..e2f75b2e337 100644 --- a/compiler/rustc_codegen_ssa/src/mir/coverageinfo.rs +++ b/compiler/rustc_codegen_ssa/src/mir/coverageinfo.rs @@ -33,7 +33,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { let coverageinfo = bx.tcx().coverageinfo(instance.def_id()); - let fn_name = bx.create_pgo_func_name_var(instance); + 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(u32::from(id)); diff --git a/compiler/rustc_codegen_ssa/src/traits/coverageinfo.rs b/compiler/rustc_codegen_ssa/src/traits/coverageinfo.rs index 95bddfb4b41..c7157528396 100644 --- a/compiler/rustc_codegen_ssa/src/traits/coverageinfo.rs +++ b/compiler/rustc_codegen_ssa/src/traits/coverageinfo.rs @@ -1,14 +1,41 @@ use super::BackendTypes; +use rustc_hir::def_id::DefId; use rustc_middle::mir::coverage::*; use rustc_middle::ty::Instance; -pub trait CoverageInfoMethods: BackendTypes { +pub trait CoverageInfoMethods<'tcx>: BackendTypes { fn coverageinfo_finalize(&self); -} -pub trait CoverageInfoBuilderMethods<'tcx>: BackendTypes { + /// Functions with MIR-based coverage are normally codegenned _only_ if + /// called. LLVM coverage tools typically expect every function to be + /// defined (even if unused), with at least one call to LLVM intrinsic + /// `instrprof.increment`. + /// + /// Codegen a small function that will never be called, with one counter + /// that will never be incremented. + /// + /// For used/called functions, the coverageinfo was already added to the + /// `function_coverage_map` (keyed by function `Instance`) during codegen. + /// But in this case, since the unused function was _not_ previously + /// codegenned, collect the coverage `CodeRegion`s from the MIR and add + /// them. The first `CodeRegion` is used to add a single counter, with the + /// same counter ID used in the injected `instrprof.increment` intrinsic + /// call. Since the function is never called, all other `CodeRegion`s can be + /// added as `unreachable_region`s. + fn define_unused_fn(&self, def_id: DefId); + + /// For LLVM codegen, returns a function-specific `Value` for a global + /// string, to hold the function name passed to LLVM intrinsic + /// `instrprof.increment()`. The `Value` is only created once per instance. + /// Multiple invocations with the same instance return the same `Value`. + fn get_pgo_func_name_var(&self, instance: Instance<'tcx>) -> Self::Value; + + /// Creates a new PGO function name variable. This should only be called + /// to fill in the unused function names array. fn create_pgo_func_name_var(&self, instance: Instance<'tcx>) -> Self::Value; +} +pub trait CoverageInfoBuilderMethods<'tcx>: BackendTypes { /// Returns true if the function source hash was added to the coverage map (even if it had /// already been added, for this instance). Returns false *only* if `-Z instrument-coverage` is /// not enabled (a coverage map is not being generated). diff --git a/compiler/rustc_codegen_ssa/src/traits/mod.rs b/compiler/rustc_codegen_ssa/src/traits/mod.rs index 8ada6c10479..be2e0ea230f 100644 --- a/compiler/rustc_codegen_ssa/src/traits/mod.rs +++ b/compiler/rustc_codegen_ssa/src/traits/mod.rs @@ -58,7 +58,7 @@ pub trait CodegenMethods<'tcx>: + MiscMethods<'tcx> + ConstMethods<'tcx> + StaticMethods - + CoverageInfoMethods + + CoverageInfoMethods<'tcx> + DebugInfoMethods<'tcx> + AsmMethods + PreDefineMethods<'tcx> @@ -74,7 +74,7 @@ impl<'tcx, T> CodegenMethods<'tcx> for T where + MiscMethods<'tcx> + ConstMethods<'tcx> + StaticMethods - + CoverageInfoMethods + + CoverageInfoMethods<'tcx> + DebugInfoMethods<'tcx> + AsmMethods + PreDefineMethods<'tcx> -- cgit 1.4.1-3-g733a5 From 5a484a1aed9265f44980a748ebca41037b2eb923 Mon Sep 17 00:00:00 2001 From: Rich Kadel Date: Fri, 19 Mar 2021 20:46:15 -0700 Subject: gave unused_fn WeakAnyLinkage; moved create_pgo_func_name_var The sample json5format tests produce coverage results again (and work with opt-level 3!) --- .../rustc_codegen_llvm/src/coverageinfo/mod.rs | 37 +++++++++++----------- .../rustc_codegen_ssa/src/traits/coverageinfo.rs | 4 --- 2 files changed, 19 insertions(+), 22 deletions(-) (limited to 'compiler/rustc_codegen_ssa/src') diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs index db61be407c9..2854717bb6e 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs @@ -67,25 +67,12 @@ impl CoverageInfoMethods<'tcx> for CodegenCx<'ll, 'tcx> { let mut pgo_func_name_var_map = coverage_context.pgo_func_name_var_map.borrow_mut(); pgo_func_name_var_map .entry(instance) - .or_insert_with(|| self.create_pgo_func_name_var(instance)) + .or_insert_with(|| create_pgo_func_name_var(self, instance)) } else { bug!("Could not get the `coverage_context`"); } } - /// Calls llvm::createPGOFuncNameVar() with the given function instance's - /// mangled function name. The LLVM API returns an llvm::GlobalVariable - /// containing the function name, with the specific variable name and - /// linkage required by LLVM InstrProf source-based coverage - /// instrumentation. Use `bx.get_pgo_func_name_var()` to ensure the variable - /// is only created once per `Instance`. - fn create_pgo_func_name_var(&self, instance: Instance<'tcx>) -> &'ll llvm::Value { - let mangled_fn_name = CString::new(self.tcx.symbol_name(instance).name) - .expect("error converting function name to C string"); - let llfn = self.get_fn(instance); - unsafe { llvm::LLVMRustCoverageCreatePGOFuncNameVar(llfn, mangled_fn_name.as_ptr()) } - } - fn define_unused_fn(&self, def_id: DefId) { let instance = declare_unused_fn(self, &def_id); codegen_unused_fn_and_counter(self, instance); @@ -210,10 +197,8 @@ fn declare_unused_fn(cx: &CodegenCx<'ll, 'tcx>, def_id: &DefId) -> Instance<'tcx ), ); - unsafe { - llvm::LLVMRustSetLinkage(llfn, llvm::Linkage::ExternalLinkage); - llvm::LLVMRustSetVisibility(llfn, llvm::Visibility::Hidden); - } + llvm::set_linkage(llfn, llvm::Linkage::WeakAnyLinkage); + llvm::set_visibility(llfn, llvm::Visibility::Hidden); cx.instances.borrow_mut().insert(instance, llfn); @@ -261,6 +246,22 @@ fn add_function_coverage(cx: &CodegenCx<'ll, 'tcx>, instance: Instance<'tcx>, de } } +/// Calls llvm::createPGOFuncNameVar() with the given function instance's +/// mangled function name. The LLVM API returns an llvm::GlobalVariable +/// containing the function name, with the specific variable name and linkage +/// required by LLVM InstrProf source-based coverage instrumentation. Use +/// `bx.get_pgo_func_name_var()` to ensure the variable is only created once per +/// `Instance`. +fn create_pgo_func_name_var( + cx: &CodegenCx<'ll, 'tcx>, + instance: Instance<'tcx>, +) -> &'ll llvm::Value { + let mangled_fn_name = CString::new(cx.tcx.symbol_name(instance).name) + .expect("error converting function name to C string"); + let llfn = cx.get_fn(instance); + unsafe { llvm::LLVMRustCoverageCreatePGOFuncNameVar(llfn, mangled_fn_name.as_ptr()) } +} + pub(crate) fn write_filenames_section_to_buffer<'a>( filenames: impl IntoIterator, buffer: &RustString, diff --git a/compiler/rustc_codegen_ssa/src/traits/coverageinfo.rs b/compiler/rustc_codegen_ssa/src/traits/coverageinfo.rs index c7157528396..b2b0b9eac91 100644 --- a/compiler/rustc_codegen_ssa/src/traits/coverageinfo.rs +++ b/compiler/rustc_codegen_ssa/src/traits/coverageinfo.rs @@ -29,10 +29,6 @@ pub trait CoverageInfoMethods<'tcx>: BackendTypes { /// `instrprof.increment()`. The `Value` is only created once per instance. /// Multiple invocations with the same instance return the same `Value`. fn get_pgo_func_name_var(&self, instance: Instance<'tcx>) -> Self::Value; - - /// Creates a new PGO function name variable. This should only be called - /// to fill in the unused function names array. - fn create_pgo_func_name_var(&self, instance: Instance<'tcx>) -> Self::Value; } pub trait CoverageInfoBuilderMethods<'tcx>: BackendTypes { -- cgit 1.4.1-3-g733a5 From 0859cec65255e34221b3b443f4bdd751549fd4c3 Mon Sep 17 00:00:00 2001 From: Rich Kadel Date: Tue, 23 Mar 2021 14:25:52 -0700 Subject: Changes from review comments --- .../rustc_codegen_llvm/src/coverageinfo/mod.rs | 26 +++++++++++++++++++--- .../rustc_codegen_ssa/src/traits/coverageinfo.rs | 19 ++++------------ compiler/rustc_typeck/src/collect.rs | 14 +++++------- 3 files changed, 33 insertions(+), 26 deletions(-) (limited to 'compiler/rustc_codegen_ssa/src') diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs index 2854717bb6e..32f4fc76b3d 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs @@ -73,10 +73,26 @@ impl CoverageInfoMethods<'tcx> for CodegenCx<'ll, 'tcx> { } } + /// Functions with MIR-based coverage are normally codegenned _only_ if + /// called. LLVM coverage tools typically expect every function to be + /// defined (even if unused), with at least one call to LLVM intrinsic + /// `instrprof.increment`. + /// + /// Codegen a small function that will never be called, with one counter + /// that will never be incremented. + /// + /// For used/called functions, the coverageinfo was already added to the + /// `function_coverage_map` (keyed by function `Instance`) during codegen. + /// But in this case, since the unused function was _not_ previously + /// codegenned, collect the coverage `CodeRegion`s from the MIR and add + /// them. The first `CodeRegion` is used to add a single counter, with the + /// same counter ID used in the injected `instrprof.increment` intrinsic + /// call. Since the function is never called, all other `CodeRegion`s can be + /// added as `unreachable_region`s. fn define_unused_fn(&self, def_id: DefId) { let instance = declare_unused_fn(self, &def_id); codegen_unused_fn_and_counter(self, instance); - add_function_coverage(self, instance, def_id); + add_unused_function_coverage(self, instance, def_id); } } @@ -200,7 +216,7 @@ fn declare_unused_fn(cx: &CodegenCx<'ll, 'tcx>, def_id: &DefId) -> Instance<'tcx llvm::set_linkage(llfn, llvm::Linkage::WeakAnyLinkage); llvm::set_visibility(llfn, llvm::Visibility::Hidden); - cx.instances.borrow_mut().insert(instance, llfn); + assert!(cx.instances.borrow_mut().insert(instance, llfn).is_none()); instance } @@ -221,7 +237,11 @@ fn codegen_unused_fn_and_counter(cx: &CodegenCx<'ll, 'tcx>, instance: Instance<' bx.ret_void(); } -fn add_function_coverage(cx: &CodegenCx<'ll, 'tcx>, instance: Instance<'tcx>, def_id: DefId) { +fn add_unused_function_coverage( + cx: &CodegenCx<'ll, 'tcx>, + instance: Instance<'tcx>, + def_id: DefId, +) { let tcx = cx.tcx; let mut function_coverage = FunctionCoverage::unused(tcx, instance); diff --git a/compiler/rustc_codegen_ssa/src/traits/coverageinfo.rs b/compiler/rustc_codegen_ssa/src/traits/coverageinfo.rs index b2b0b9eac91..cbf570dba4c 100644 --- a/compiler/rustc_codegen_ssa/src/traits/coverageinfo.rs +++ b/compiler/rustc_codegen_ssa/src/traits/coverageinfo.rs @@ -6,22 +6,11 @@ use rustc_middle::ty::Instance; pub trait CoverageInfoMethods<'tcx>: BackendTypes { fn coverageinfo_finalize(&self); - /// Functions with MIR-based coverage are normally codegenned _only_ if - /// called. LLVM coverage tools typically expect every function to be - /// defined (even if unused), with at least one call to LLVM intrinsic - /// `instrprof.increment`. - /// /// Codegen a small function that will never be called, with one counter - /// that will never be incremented. - /// - /// For used/called functions, the coverageinfo was already added to the - /// `function_coverage_map` (keyed by function `Instance`) during codegen. - /// But in this case, since the unused function was _not_ previously - /// codegenned, collect the coverage `CodeRegion`s from the MIR and add - /// them. The first `CodeRegion` is used to add a single counter, with the - /// same counter ID used in the injected `instrprof.increment` intrinsic - /// call. Since the function is never called, all other `CodeRegion`s can be - /// added as `unreachable_region`s. + /// that will never be incremented, that gives LLVM coverage tools a + /// function definition it needs in order to resolve coverage map references + /// to unused functions. This is necessary so unused functions will appear + /// as uncovered (coverage execution count `0`) in LLVM coverage reports. fn define_unused_fn(&self, def_id: DefId); /// For LLVM codegen, returns a function-specific `Value` for a global diff --git a/compiler/rustc_typeck/src/collect.rs b/compiler/rustc_typeck/src/collect.rs index f69674ea422..fbcc6720780 100644 --- a/compiler/rustc_typeck/src/collect.rs +++ b/compiler/rustc_typeck/src/collect.rs @@ -2876,14 +2876,12 @@ fn codegen_fn_attrs(tcx: TyCtxt<'_>, id: DefId) -> CodegenFnAttrs { InlineAttr::None } else if list_contains_name(&items[..], sym::always) { if tcx.sess.instrument_coverage() { - // Forced inlining will discard functions marked with `#[inline(always)]`. - // If `-Z instrument-coverage` is enabled, the generated coverage map may - // hold references to functions that no longer exist, causing errors in - // coverage reports. (Note, this fixes #82875. I added some tests that - // also include `#[inline(always)]` functions, used and unused, and within - // and across crates, but could not reproduce the reported error in the - // `rustc` test suite. I am able to reproduce the error, following the steps - // described in #82875, and this change does fix that issue.) + // Fixes Issue #82875. Forced inlining allows LLVM to discard functions + // marked with `#[inline(always)]`, which can break coverage reporting if + // that function was referenced from a coverage map. + // + // FIXME(#83429): Is there a better place, e.g., in codegen, to check and + // convert `Always` to `Hint`? InlineAttr::Hint } else { InlineAttr::Always -- cgit 1.4.1-3-g733a5