Address review comments

Author: wesleywiser

compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs

@@ -273,20 +273,34 @@ fn save_function_record(
 /// `codegened_and_inlined_items`).
 ///
 /// These unused functions are then codegen'd in one of the CGUs which is marked as the
-/// "code coverage dead code cgu" during the partitioning process.
+/// "code coverage dead code cgu" during the partitioning process. This prevents us from generating
+/// code regions for the same function more than once which can lead to linker errors regarding
+/// duplicate symbols.
 fn add_unused_functions<'ll, 'tcx>(cx: &CodegenCx<'ll, 'tcx>) {
     assert!(cx.codegen_unit.is_code_coverage_dead_code_cgu());
 
     let tcx = cx.tcx;
 
     let ignore_unused_generics = tcx.sess.instrument_coverage_except_unused_generics();
 
-    let all_def_ids: DefIdSet = tcx
+    let eligible_def_ids: DefIdSet = tcx
         .mir_keys(())
         .iter()
         .filter_map(|local_def_id| {
             let def_id = local_def_id.to_def_id();
-            if ignore_unused_generics && tcx.generics_of(def_id).requires_monomorphization(tcx) {
+            let kind = tcx.def_kind(def_id);
+            // `mir_keys` will give us `DefId`s for all kinds of things, not
+            // just "functions", like consts, statics, etc. Filter those out.
+            // If `ignore_unused_generics` was specified, filter out any
+            // generic functions from consideration as well.
+            if !matches!(
+                kind,
+                DefKind::Fn | DefKind::AssocFn | DefKind::Closure | DefKind::Generator
+            ) {
+                return None;
+            } else if ignore_unused_generics
+                && tcx.generics_of(def_id).requires_monomorphization(tcx)
+            {
                 return None;
             }
             Some(local_def_id.to_def_id())
@@ -295,24 +309,17 @@ fn add_unused_functions<'ll, 'tcx>(cx: &CodegenCx<'ll, 'tcx>) {
 
     let codegenned_def_ids = tcx.codegened_and_inlined_items(());
 
-    for &non_codegenned_def_id in all_def_ids.difference(codegenned_def_ids) {
-        // `all_def_ids` contains things besides just "functions" such as constants,
-        // statics, etc. We need to filter those out.
-        let kind = tcx.def_kind(non_codegenned_def_id);
-        if matches!(kind, DefKind::Fn | DefKind::AssocFn | DefKind::Closure | DefKind::Generator) {
-            let codegen_fn_attrs = tcx.codegen_fn_attrs(non_codegenned_def_id);
-
-            // If a function is marked `#[no_coverage]`, then skip generating a
-            // dead code stub for it.
-            if codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::NO_COVERAGE) {
-                debug!("skipping unused fn marked #[no_coverage]: {:?}", non_codegenned_def_id);
-                continue;
-            }
+    for &non_codegenned_def_id in eligible_def_ids.difference(codegenned_def_ids) {
+        let codegen_fn_attrs = tcx.codegen_fn_attrs(non_codegenned_def_id);
 
-            debug!("generating unused fn: {:?}", non_codegenned_def_id);
-            cx.define_unused_fn(non_codegenned_def_id);
-        } else {
-            debug!("skipping unused {:?}: {:?}", kind, non_codegenned_def_id);
+        // If a function is marked `#[no_coverage]`, then skip generating a
+        // dead code stub for it.
+        if codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::NO_COVERAGE) {
+            debug!("skipping unused fn marked #[no_coverage]: {:?}", non_codegenned_def_id);
+            continue;
         }
+
+        debug!("generating unused fn: {:?}", non_codegenned_def_id);
+        cx.define_unused_fn(non_codegenned_def_id);
     }
 }

compiler/rustc_monomorphize/src/partitioning/mod.rs

@@ -205,17 +205,33 @@ pub fn partition<'tcx>(
         tcx.sess.instrument_coverage() && !tcx.sess.instrument_coverage_except_unused_functions();
 
     if instrument_dead_code {
+        assert!(
+            post_inlining.codegen_units.len() > 0,
+            "There must be at least one CGU that code coverage data can be generated in."
+        );
+
         // Find the smallest CGU that has exported symbols and put the dead
         // function stubs in that CGU. We look for exported symbols to increase
-        // the likelyhood the linker won't throw away the dead functions.
-        let mut cgus_with_exported_symbols: Vec<_> = post_inlining
-            .codegen_units
-            .iter_mut()
+        // the likelihood the linker won't throw away the dead functions.
+        // FIXME(#92165): In order to truly resolve this, we need to make sure
+        // the object file (CGU) containing the dead function stubs is included
+        // in the final binary. This will probably require forcing these
+        // function symbols to be included via `-u` or `/include` linker args.
+        let mut cgus: Vec<_> = post_inlining.codegen_units.iter_mut().collect();
+        cgus.sort_by_key(|cgu| cgu.size_estimate());
+
+        let dead_code_cgu = if let Some(cgu) = cgus
+            .into_iter()
+            .rev()
             .filter(|cgu| cgu.items().iter().any(|(_, (linkage, _))| *linkage == Linkage::External))
-            .collect();
-        cgus_with_exported_symbols.sort_by_key(|cgu| cgu.size_estimate());
-
-        let dead_code_cgu = cgus_with_exported_symbols.last_mut().unwrap();
+            .next()
+        {
+            cgu
+        } else {
+            // If there are no CGUs that have externally linked items,
+            // then we just pick the first CGU as a fallback.
+            &mut post_inlining.codegen_units[0]
+        };
         dead_code_cgu.make_code_coverage_dead_code_cgu();
     }
 

src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.uses_crate.txt

@@ -19,12 +19,12 @@
    18|      2|    println!("used_only_from_bin_crate_generic_function with {:?}", arg);
    19|      2|}
   ------------------
-  | used_crate::used_only_from_bin_crate_generic_function::<&str>:
+  | used_crate::used_only_from_bin_crate_generic_function::<&alloc::vec::Vec<i32>>:
   |   17|      1|pub fn used_only_from_bin_crate_generic_function<T: Debug>(arg: T) {
   |   18|      1|    println!("used_only_from_bin_crate_generic_function with {:?}", arg);
   |   19|      1|}
   ------------------
-  | used_crate::used_only_from_bin_crate_generic_function::<&alloc::vec::Vec<i32>>:
+  | used_crate::used_only_from_bin_crate_generic_function::<&str>:
   |   17|      1|pub fn used_only_from_bin_crate_generic_function<T: Debug>(arg: T) {
   |   18|      1|    println!("used_only_from_bin_crate_generic_function with {:?}", arg);
   |   19|      1|}
@@ -36,12 +36,12 @@
    22|      2|    println!("used_only_from_this_lib_crate_generic_function with {:?}", arg);
    23|      2|}
   ------------------
-  | used_crate::used_only_from_this_lib_crate_generic_function::<&str>:
+  | used_crate::used_only_from_this_lib_crate_generic_function::<alloc::vec::Vec<i32>>:
   |   21|      1|pub fn used_only_from_this_lib_crate_generic_function<T: Debug>(arg: T) {
   |   22|      1|    println!("used_only_from_this_lib_crate_generic_function with {:?}", arg);
   |   23|      1|}
   ------------------
-  | used_crate::used_only_from_this_lib_crate_generic_function::<alloc::vec::Vec<i32>>:
+  | used_crate::used_only_from_this_lib_crate_generic_function::<&str>:
   |   21|      1|pub fn used_only_from_this_lib_crate_generic_function<T: Debug>(arg: T) {
   |   22|      1|    println!("used_only_from_this_lib_crate_generic_function with {:?}", arg);
   |   23|      1|}

src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.uses_inline_crate.txt

@@ -42,12 +42,12 @@
    40|      2|    println!("used_only_from_bin_crate_generic_function with {:?}", arg);
    41|      2|}
   ------------------
-  | used_inline_crate::used_only_from_bin_crate_generic_function::<&alloc::vec::Vec<i32>>:
+  | used_inline_crate::used_only_from_bin_crate_generic_function::<&str>:
   |   39|      1|pub fn used_only_from_bin_crate_generic_function<T: Debug>(arg: T) {
   |   40|      1|    println!("used_only_from_bin_crate_generic_function with {:?}", arg);
   |   41|      1|}
   ------------------
-  | used_inline_crate::used_only_from_bin_crate_generic_function::<&str>:
+  | used_inline_crate::used_only_from_bin_crate_generic_function::<&alloc::vec::Vec<i32>>:
   |   39|      1|pub fn used_only_from_bin_crate_generic_function<T: Debug>(arg: T) {
   |   40|      1|    println!("used_only_from_bin_crate_generic_function with {:?}", arg);
   |   41|      1|}
@@ -61,12 +61,12 @@
    46|      4|    println!("used_only_from_this_lib_crate_generic_function with {:?}", arg);
    47|      4|}
   ------------------
-  | used_inline_crate::used_only_from_this_lib_crate_generic_function::<alloc::vec::Vec<i32>>:
+  | used_inline_crate::used_only_from_this_lib_crate_generic_function::<&str>:
   |   45|      2|pub fn used_only_from_this_lib_crate_generic_function<T: Debug>(arg: T) {
   |   46|      2|    println!("used_only_from_this_lib_crate_generic_function with {:?}", arg);
   |   47|      2|}
   ------------------
-  | used_inline_crate::used_only_from_this_lib_crate_generic_function::<&str>:
+  | used_inline_crate::used_only_from_this_lib_crate_generic_function::<alloc::vec::Vec<i32>>:
   |   45|      2|pub fn used_only_from_this_lib_crate_generic_function<T: Debug>(arg: T) {
   |   46|      2|    println!("used_only_from_this_lib_crate_generic_function with {:?}", arg);
   |   47|      2|}