Changes from review comments

Author: richkadel

Diff for: 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 for: 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 for: 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