Auto merge of #83666 - Amanieu:instrprof-order, r=tmandry

Run LLVM coverage instrumentation passes before optimization passes

This matches the behavior of Clang and allows us to remove several
hacks which were needed to ensure functions weren't optimized away
before reaching the instrumentation pass.

Fixes #83429

cc `@richkadel`

r? `@tmandry`

9 files changed, 29 insertions, 60 deletions

diff --git a/compiler/rustc_codegen_llvm/src/back/write.rs b/compiler/rustc_codegen_llvm/src/back/write.rs
index 388dd7ce81b..085935b94df 100644
--- a/compiler/rustc_codegen_llvm/src/back/write.rs
+++ b/compiler/rustc_codegen_llvm/src/back/write.rs
@@ -548,6 +548,15 @@ pub(crate) unsafe fn optimize(
                     llvm::LLVMRustAddPass(fpm, find_pass("lint").unwrap());
                     continue;
                 }
+                if pass_name == "insert-gcov-profiling" || pass_name == "instrprof" {
+                    // Instrumentation must be inserted before optimization,
+                    // otherwise LLVM may optimize some functions away which
+                    // breaks llvm-cov.
+                    //
+                    // This mirrors what Clang does in lib/CodeGen/BackendUtil.cpp.
+                    llvm::LLVMRustAddPass(mpm, find_pass(pass_name).unwrap());
+                    continue;
+                }
 
                 if let Some(pass) = find_pass(pass_name) {
                     extra_passes.push(pass);
diff --git a/compiler/rustc_middle/src/mir/mono.rs b/compiler/rustc_middle/src/mir/mono.rs
index 0167655bee5..6c2468b9ffe 100644
--- a/compiler/rustc_middle/src/mir/mono.rs
+++ b/compiler/rustc_middle/src/mir/mono.rs
@@ -84,14 +84,7 @@ impl<'tcx> MonoItem<'tcx> {
             .debugging_opts
             .inline_in_all_cgus
             .unwrap_or_else(|| tcx.sess.opts.optimize != OptLevel::No)
-            && !tcx.sess.link_dead_code()
-            && !tcx.sess.instrument_coverage();
-        // Disabled for `-Z instrument-coverage` because some LLVM optimizations can sometimes
-        // break coverage results. A test that failed at certain optimization levels is now
-        // validated at that optimization level (via `compile-flags` directive):
-        //   * `src/test/run-make-fulldeps/coverage/closure.rs` broke with `-C opt-level=2`, and
-        //     also required disabling `internalize_symbols` in
-        //     `rustc_mir/monomorphize/partitioning/mod.rs`
+            && !tcx.sess.link_dead_code();
 
         match *self {
             MonoItem::Fn(ref instance) => {
diff --git a/compiler/rustc_mir/src/monomorphize/partitioning/mod.rs b/compiler/rustc_mir/src/monomorphize/partitioning/mod.rs
index 04f31ec3a33..dc2379fd92b 100644
--- a/compiler/rustc_mir/src/monomorphize/partitioning/mod.rs
+++ b/compiler/rustc_mir/src/monomorphize/partitioning/mod.rs
@@ -196,13 +196,7 @@ pub fn partition<'tcx>(
 
     // Next we try to make as many symbols "internal" as possible, so LLVM has
     // more freedom to optimize.
-    if !tcx.sess.link_dead_code() && !tcx.sess.instrument_coverage() {
-        // Disabled for `-Z instrument-coverage` because some LLVM optimizations can sometimes
-        // break coverage results. Tests that failed at certain optimization levels are now
-        // validated at those optimization levels (via `compile-flags` directive); for example:
-        //   * `src/test/run-make-fulldeps/coverage/async.rs` broke with `-C opt-level=1`
-        //   * `src/test/run-make-fulldeps/coverage/closure.rs` broke with `-C opt-level=2`, and
-        //     also required disabling `generate_gcu_internal_copies` in `rustc_middle/mir/mono.rs`
+    if !tcx.sess.link_dead_code() {
         let _prof_timer = tcx.prof.generic_activity("cgu_partitioning_internalize_symbols");
         partitioner.internalize_symbols(cx, &mut post_inlining);
     }
diff --git a/compiler/rustc_typeck/src/collect.rs b/compiler/rustc_typeck/src/collect.rs
index 9b3a933beb1..0ef5f4d1c18 100644
--- a/compiler/rustc_typeck/src/collect.rs
+++ b/compiler/rustc_typeck/src/collect.rs
@@ -2890,17 +2890,7 @@ fn codegen_fn_attrs(tcx: TyCtxt<'_>, id: DefId) -> CodegenFnAttrs {
                     .emit();
                     InlineAttr::None
                 } else if list_contains_name(&items[..], sym::always) {
-                    if tcx.sess.instrument_coverage() {
-                        // 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
-                    }
+                    InlineAttr::Always
                 } else if list_contains_name(&items[..], sym::never) {
                     InlineAttr::Never
                 } else {
diff --git a/src/test/run-make-fulldeps/coverage-llvmir/Makefile b/src/test/run-make-fulldeps/coverage-llvmir/Makefile
index 86af7e41ae1..7d9121ee2f8 100644
--- a/src/test/run-make-fulldeps/coverage-llvmir/Makefile
+++ b/src/test/run-make-fulldeps/coverage-llvmir/Makefile
@@ -17,7 +17,7 @@ else
 	COMDAT_IF_SUPPORTED=, comdat
 endif
 
-DEFINE_INTERNAL=define hidden
+DEFINE_INTERNAL=define internal
 
 ifdef IS_WINDOWS
 	LLVM_FILECHECK_OPTIONS=\
diff --git a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.generics.txt b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.generics.txt
index 6f28c089093..7b38ffb87cb 100644
--- a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.generics.txt
+++ b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.generics.txt
@@ -29,12 +29,12 @@
    18|      2|        println!("BOOM times {}!!!", self.strength);
    19|      2|    }
   ------------------
-  | <generics::Firework<f64> as core::ops::drop::Drop>::drop:
+  | <generics::Firework<i32> as core::ops::drop::Drop>::drop:
   |   17|      1|    fn drop(&mut self) {
   |   18|      1|        println!("BOOM times {}!!!", self.strength);
   |   19|      1|    }
   ------------------
-  | <generics::Firework<i32> as core::ops::drop::Drop>::drop:
+  | <generics::Firework<f64> as core::ops::drop::Drop>::drop:
   |   17|      1|    fn drop(&mut self) {
   |   18|      1|        println!("BOOM times {}!!!", self.strength);
   |   19|      1|    }
diff --git a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.uses_crate.txt b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.uses_crate.txt
index 380869d62a8..cdcbd8fca94 100644
--- a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.uses_crate.txt
+++ b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.uses_crate.txt
@@ -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::<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|}
   ------------------
-  | 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|}
diff --git a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.uses_inline_crate.txt b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.uses_inline_crate.txt
index 0853dc9c014..cc98956e307 100644
--- a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.uses_inline_crate.txt
+++ b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.uses_inline_crate.txt
@@ -30,29 +30,12 @@
                    ^0
    29|      1|    use_this_lib_crate();
    30|      1|}
-  ------------------
-  | used_inline_crate::used_inline_function:
-  |   20|      1|pub fn used_inline_function() {
-  |   21|       |    // Initialize test constants in a way that cannot be determined at compile time, to ensure
-  |   22|       |    // rustc and LLVM cannot optimize out statements (or coverage counters) downstream from
-  |   23|       |    // dependent conditions.
-  |   24|      1|    let is_true = std::env::args().len() == 1;
-  |   25|      1|    let mut countdown = 0;
-  |   26|      1|    if is_true {
-  |   27|      1|        countdown = 10;
-  |   28|      1|    }
-  |                   ^0
-  |   29|      1|    use_this_lib_crate();
-  |   30|      1|}
-  ------------------
-  | Unexecuted instantiation: used_inline_crate::used_inline_function
-  ------------------
-   31|       |// Expect for above function:
-   32|       |//
-   33|       |// | Unexecuted instantiation: used_crate::used_only_from_bin_crate_generic_function::<_>
-   34|       |//
-   35|       |// With `#[inline(always)]` this function is instantiated twice, in both the library crate (which
-   36|       |// does not use it) and the `uses_inline_crate` binary (which does use/call it).
+   31|       |
+   32|       |
+   33|       |
+   34|       |
+   35|       |
+   36|       |
    37|       |
    38|       |#[inline(always)]
    39|      2|pub fn used_only_from_bin_crate_generic_function<T: Debug>(arg: T) {
diff --git a/src/test/run-make-fulldeps/coverage/lib/used_inline_crate.rs b/src/test/run-make-fulldeps/coverage/lib/used_inline_crate.rs
index f4c3dd46f76..4a052756d4e 100644
--- a/src/test/run-make-fulldeps/coverage/lib/used_inline_crate.rs
+++ b/src/test/run-make-fulldeps/coverage/lib/used_inline_crate.rs
@@ -28,12 +28,12 @@ pub fn used_inline_function() {
     }
     use_this_lib_crate();
 }
-// Expect for above function:
-//
-// | Unexecuted instantiation: used_crate::used_only_from_bin_crate_generic_function::<_>
-//
-// With `#[inline(always)]` this function is instantiated twice, in both the library crate (which
-// does not use it) and the `uses_inline_crate` binary (which does use/call it).
+
+
+
+
+
+
 
 #[inline(always)]
 pub fn used_only_from_bin_crate_generic_function<T: Debug>(arg: T) {