coverage bug fixes and optimization support

compiler/rustc_codegen_llvm/src/base.rs

@@ -143,7 +143,7 @@ pub fn compile_codegen_unit(
 
             // Finalize code coverage by injecting the coverage map. Note, the coverage map will
             // also be added to the `llvm.used` variable, created next.
-            if cx.sess().opts.debugging_opts.instrument_coverage {
+            if cx.sess().instrument_coverage() {
                 cx.coverageinfo_finalize();
             }
 

compiler/rustc_codegen_llvm/src/context.rs

@@ -79,7 +79,7 @@ pub struct CodegenCx<'ll, 'tcx> {
     pub pointee_infos: RefCell<FxHashMap<(Ty<'tcx>, Size), Option<PointeeInfo>>>,
     pub isize_ty: &'ll Type,
 
-    pub coverage_cx: Option<coverageinfo::CrateCoverageContext<'tcx>>,
+    pub coverage_cx: Option<coverageinfo::CrateCoverageContext<'ll, 'tcx>>,
     pub dbg_cx: Option<debuginfo::CrateDebugContext<'ll, 'tcx>>,
 
     eh_personality: Cell<Option<&'ll Value>>,
@@ -280,7 +280,7 @@ impl<'ll, 'tcx> CodegenCx<'ll, 'tcx> {
 
         let (llcx, llmod) = (&*llvm_module.llcx, llvm_module.llmod());
 
-        let coverage_cx = if tcx.sess.opts.debugging_opts.instrument_coverage {
+        let coverage_cx = if tcx.sess.instrument_coverage() {
             let covctx = coverageinfo::CrateCoverageContext::new();
             Some(covctx)
         } else {
@@ -331,7 +331,7 @@ impl<'ll, 'tcx> CodegenCx<'ll, 'tcx> {
     }
 
     #[inline]
-    pub fn coverage_context(&'a self) -> Option<&'a coverageinfo::CrateCoverageContext<'tcx>> {
+    pub fn coverage_context(&'a self) -> Option<&'a coverageinfo::CrateCoverageContext<'ll, 'tcx>> {
         self.coverage_cx.as_ref()
     }
 }
@@ -712,7 +712,7 @@ impl CodegenCx<'b, 'tcx> {
         ifn!("llvm.va_end", fn(i8p) -> void);
         ifn!("llvm.va_copy", fn(i8p, i8p) -> void);
 
-        if self.sess().opts.debugging_opts.instrument_coverage {
+        if self.sess().instrument_coverage() {
             ifn!("llvm.instrprof.increment", fn(i8p, t_i64, t_i32, t_i32) -> void);
         }
 

compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs

@@ -1,63 +1,99 @@
 use crate::llvm;
 
+use crate::abi::{Abi, FnAbi};
 use crate::builder::Builder;
 use crate::common::CodegenCx;
 
 use libc::c_uint;
 use llvm::coverageinfo::CounterMappingRegion;
 use rustc_codegen_ssa::coverageinfo::map::{CounterExpression, FunctionCoverage};
 use rustc_codegen_ssa::traits::{
-    BaseTypeMethods, CoverageInfoBuilderMethods, CoverageInfoMethods, MiscMethods, StaticMethods,
+    BaseTypeMethods, BuilderMethods, ConstMethods, CoverageInfoBuilderMethods, CoverageInfoMethods,
+    MiscMethods, StaticMethods,
 };
 use rustc_data_structures::fx::FxHashMap;
+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, ExpressionOperandId, InjectedExpressionId, Op,
 };
+use rustc_middle::ty;
+use rustc_middle::ty::layout::FnAbiExt;
+use rustc_middle::ty::subst::InternalSubsts;
 use rustc_middle::ty::Instance;
 
 use std::cell::RefCell;
 use std::ffi::CString;
 
+use std::iter;
 use tracing::debug;
 
 pub mod mapgen;
 
+const UNUSED_FUNCTION_COUNTER_ID: CounterValueReference = CounterValueReference::START;
+
 const VAR_ALIGN_BYTES: usize = 8;
 
 /// A context object for maintaining all state needed by the coverageinfo module.
-pub struct CrateCoverageContext<'tcx> {
+pub struct CrateCoverageContext<'ll, 'tcx> {
     // Coverage data for each instrumented function identified by DefId.
     pub(crate) function_coverage_map: RefCell<FxHashMap<Instance<'tcx>, FunctionCoverage<'tcx>>>,
+    pub(crate) pgo_func_name_var_map: RefCell<FxHashMap<Instance<'tcx>, &'ll llvm::Value>>,
 }
 
-impl<'tcx> CrateCoverageContext<'tcx> {
+impl<'ll, 'tcx> CrateCoverageContext<'ll, 'tcx> {
     pub fn new() -> Self {
-        Self { function_coverage_map: Default::default() }
+        Self {
+            function_coverage_map: Default::default(),
+            pgo_func_name_var_map: Default::default(),
+        }
     }
 
     pub fn take_function_coverage_map(&self) -> FxHashMap<Instance<'tcx>, FunctionCoverage<'tcx>> {
         self.function_coverage_map.replace(FxHashMap::default())
     }
 }
 
-impl CoverageInfoMethods for CodegenCx<'ll, 'tcx> {
+impl CoverageInfoMethods<'tcx> for CodegenCx<'ll, 'tcx> {
     fn coverageinfo_finalize(&self) {
         mapgen::finalize(self)
     }
-}
 
-impl CoverageInfoBuilderMethods<'tcx> for Builder<'a, 'll, 'tcx> {
-    /// 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.
-    fn create_pgo_func_name_var(&self, instance: Instance<'tcx>) -> Self::Value {
-        let llfn = self.cx.get_fn(instance);
+    fn get_pgo_func_name_var(&self, instance: Instance<'tcx>) -> &'ll llvm::Value {
+        if let Some(coverage_context) = self.coverage_context() {
+            debug!("getting pgo_func_name_var for instance={:?}", instance);
+            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))
+        } 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);
+        add_function_coverage(self, instance, def_id);
+    }
+}
+
+impl CoverageInfoBuilderMethods<'tcx> for Builder<'a, 'll, 'tcx> {
     fn set_function_source_hash(
         &mut self,
         instance: Instance<'tcx>,
@@ -145,6 +181,86 @@ impl CoverageInfoBuilderMethods<'tcx> for Builder<'a, 'll, 'tcx> {
     }
 }
 
+fn declare_unused_fn(cx: &CodegenCx<'ll, 'tcx>, def_id: &DefId) -> Instance<'tcx> {
+    let tcx = cx.tcx;
+
+    let instance = Instance::new(
+        *def_id,
+        InternalSubsts::for_item(tcx, *def_id, |param, _| {
+            if let ty::GenericParamDefKind::Lifetime = param.kind {
+                tcx.lifetimes.re_erased.into()
+            } else {
+                tcx.mk_param_from_def(param)
+            }
+        }),
+    );
+
+    let llfn = cx.declare_fn(
+        &tcx.symbol_name(instance).name,
+        &FnAbi::of_fn_ptr(
+            cx,
+            ty::Binder::dummy(tcx.mk_fn_sig(
+                iter::once(tcx.mk_unit()),
+                tcx.mk_unit(),
+                false,
+                hir::Unsafety::Unsafe,
+                Abi::Rust,
+            )),
+            &[],
+        ),
+    );
+
+    unsafe {
+        llvm::LLVMRustSetLinkage(llfn, llvm::Linkage::ExternalLinkage);
+        llvm::LLVMRustSetVisibility(llfn, llvm::Visibility::Hidden);
+    }
+
+    cx.instances.borrow_mut().insert(instance, llfn);
+
+    instance
+}
+
+fn codegen_unused_fn_and_counter(cx: &CodegenCx<'ll, 'tcx>, instance: Instance<'tcx>) {
+    let llfn = cx.get_fn(instance);
+    let mut bx = Builder::new_block(cx, llfn, "unused_function");
+    let fn_name = bx.get_pgo_func_name_var(instance);
+    let hash = bx.const_u64(0);
+    let num_counters = bx.const_u32(1);
+    let index = bx.const_u32(u32::from(UNUSED_FUNCTION_COUNTER_ID));
+    debug!(
+        "codegen intrinsic instrprof.increment(fn_name={:?}, hash={:?}, num_counters={:?},
+            index={:?}) for unused function: {:?}",
+        fn_name, hash, num_counters, index, instance
+    );
+    bx.instrprof_increment(fn_name, hash, num_counters, index);
+    bx.ret_void();
+}
+
+fn add_function_coverage(cx: &CodegenCx<'ll, 'tcx>, instance: Instance<'tcx>, def_id: DefId) {
+    let tcx = cx.tcx;
+
+    let mut function_coverage = FunctionCoverage::unused(tcx, instance);
+    for (index, &code_region) in tcx.covered_code_regions(def_id).iter().enumerate() {
+        if index == 0 {
+            // Insert at least one real counter so the LLVM CoverageMappingReader will find expected
+            // definitions.
+            function_coverage.add_counter(UNUSED_FUNCTION_COUNTER_ID, code_region.clone());
+        }
+        // Add a Zero Counter for every code region.
+        //
+        // Even though the first coverage region already has an actual Counter, `llvm-cov` will not
+        // always report it. Re-adding an unreachable region (zero counter) for the same region
+        // seems to help produce the expected coverage.
+        function_coverage.add_unreachable_region(code_region.clone());
+    }
+
+    if let Some(coverage_context) = cx.coverage_context() {
+        coverage_context.function_coverage_map.borrow_mut().insert(instance, function_coverage);
+    } else {
+        bug!("Could not get the `coverage_context`");
+    }
+}
+
 pub(crate) fn write_filenames_section_to_buffer<'a>(
     filenames: impl IntoIterator<Item = &'a CString>,
     buffer: &RustString,
@@ -177,6 +293,7 @@ pub(crate) fn write_mapping_to_buffer(
         );
     }
 }
+
 pub(crate) fn hash_str(strval: &str) -> u64 {
     let strval = CString::new(strval).expect("null error converting hashable str to C string");
     unsafe { llvm::LLVMRustCoverageHashCString(strval.as_ptr()) }

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();
     }
 

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.

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

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<CounterValueReference, Option<CodeRegion>>,
     expressions: IndexVec<InjectedExpressionIndex, Option<Expression>>,
     unreachable_regions: Vec<CodeRegion>,
 }
 
 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<CounterExpression>, impl Iterator<Item = (Counter, &'a CodeRegion)>) {
         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
         );
 

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));

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).