From 0859cec65255e34221b3b443f4bdd751549fd4c3 Mon Sep 17 00:00:00 2001 From: Rich Kadel Date: Tue, 23 Mar 2021 14:25:52 -0700 Subject: [PATCH] Changes from review comments --- .../src/coverageinfo/mod.rs | 26 ++++++++++++++++--- .../src/traits/coverageinfo.rs | 19 +++----------- compiler/rustc_typeck/src/collect.rs | 14 +++++----- 3 files changed, 33 insertions(+), 26 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs index 2854717bb6e74..32f4fc76b3d1f 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 b2b0b9eac9179..cbf570dba4c3e 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 f69674ea422b0..fbcc6720780c2 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