Skip to content

Commit 6ff482b

Browse files
committed
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`
2 parents 65b44b0 + cad9b6b commit 6ff482b

File tree

9 files changed

+29
-60
lines changed

9 files changed

+29
-60
lines changed

compiler/rustc_codegen_llvm/src/back/write.rs

+9
Original file line numberDiff line numberDiff line change
@@ -548,6 +548,15 @@ pub(crate) unsafe fn optimize(
548548
llvm::LLVMRustAddPass(fpm, find_pass("lint").unwrap());
549549
continue;
550550
}
551+
if pass_name == "insert-gcov-profiling" || pass_name == "instrprof" {
552+
// Instrumentation must be inserted before optimization,
553+
// otherwise LLVM may optimize some functions away which
554+
// breaks llvm-cov.
555+
//
556+
// This mirrors what Clang does in lib/CodeGen/BackendUtil.cpp.
557+
llvm::LLVMRustAddPass(mpm, find_pass(pass_name).unwrap());
558+
continue;
559+
}
551560

552561
if let Some(pass) = find_pass(pass_name) {
553562
extra_passes.push(pass);

compiler/rustc_middle/src/mir/mono.rs

+1-8
Original file line numberDiff line numberDiff line change
@@ -84,14 +84,7 @@ impl<'tcx> MonoItem<'tcx> {
8484
.debugging_opts
8585
.inline_in_all_cgus
8686
.unwrap_or_else(|| tcx.sess.opts.optimize != OptLevel::No)
87-
&& !tcx.sess.link_dead_code()
88-
&& !tcx.sess.instrument_coverage();
89-
// Disabled for `-Z instrument-coverage` because some LLVM optimizations can sometimes
90-
// break coverage results. A test that failed at certain optimization levels is now
91-
// validated at that optimization level (via `compile-flags` directive):
92-
// * `src/test/run-make-fulldeps/coverage/closure.rs` broke with `-C opt-level=2`, and
93-
// also required disabling `internalize_symbols` in
94-
// `rustc_mir/monomorphize/partitioning/mod.rs`
87+
&& !tcx.sess.link_dead_code();
9588

9689
match *self {
9790
MonoItem::Fn(ref instance) => {

compiler/rustc_mir/src/monomorphize/partitioning/mod.rs

+1-7
Original file line numberDiff line numberDiff line change
@@ -196,13 +196,7 @@ pub fn partition<'tcx>(
196196

197197
// Next we try to make as many symbols "internal" as possible, so LLVM has
198198
// more freedom to optimize.
199-
if !tcx.sess.link_dead_code() && !tcx.sess.instrument_coverage() {
200-
// Disabled for `-Z instrument-coverage` because some LLVM optimizations can sometimes
201-
// break coverage results. Tests that failed at certain optimization levels are now
202-
// validated at those optimization levels (via `compile-flags` directive); for example:
203-
// * `src/test/run-make-fulldeps/coverage/async.rs` broke with `-C opt-level=1`
204-
// * `src/test/run-make-fulldeps/coverage/closure.rs` broke with `-C opt-level=2`, and
205-
// also required disabling `generate_gcu_internal_copies` in `rustc_middle/mir/mono.rs`
199+
if !tcx.sess.link_dead_code() {
206200
let _prof_timer = tcx.prof.generic_activity("cgu_partitioning_internalize_symbols");
207201
partitioner.internalize_symbols(cx, &mut post_inlining);
208202
}

compiler/rustc_typeck/src/collect.rs

+1-11
Original file line numberDiff line numberDiff line change
@@ -2890,17 +2890,7 @@ fn codegen_fn_attrs(tcx: TyCtxt<'_>, id: DefId) -> CodegenFnAttrs {
28902890
.emit();
28912891
InlineAttr::None
28922892
} else if list_contains_name(&items[..], sym::always) {
2893-
if tcx.sess.instrument_coverage() {
2894-
// Fixes Issue #82875. Forced inlining allows LLVM to discard functions
2895-
// marked with `#[inline(always)]`, which can break coverage reporting if
2896-
// that function was referenced from a coverage map.
2897-
//
2898-
// FIXME(#83429): Is there a better place, e.g., in codegen, to check and
2899-
// convert `Always` to `Hint`?
2900-
InlineAttr::Hint
2901-
} else {
2902-
InlineAttr::Always
2903-
}
2893+
InlineAttr::Always
29042894
} else if list_contains_name(&items[..], sym::never) {
29052895
InlineAttr::Never
29062896
} else {

src/test/run-make-fulldeps/coverage-llvmir/Makefile

+1-1
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ else
1717
COMDAT_IF_SUPPORTED=, comdat
1818
endif
1919

20-
DEFINE_INTERNAL=define hidden
20+
DEFINE_INTERNAL=define internal
2121

2222
ifdef IS_WINDOWS
2323
LLVM_FILECHECK_OPTIONS=\

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

+2-2
Original file line numberDiff line numberDiff line change
@@ -29,12 +29,12 @@
2929
18| 2| println!("BOOM times {}!!!", self.strength);
3030
19| 2| }
3131
------------------
32-
| <generics::Firework<f64> as core::ops::drop::Drop>::drop:
32+
| <generics::Firework<i32> as core::ops::drop::Drop>::drop:
3333
| 17| 1| fn drop(&mut self) {
3434
| 18| 1| println!("BOOM times {}!!!", self.strength);
3535
| 19| 1| }
3636
------------------
37-
| <generics::Firework<i32> as core::ops::drop::Drop>::drop:
37+
| <generics::Firework<f64> as core::ops::drop::Drop>::drop:
3838
| 17| 1| fn drop(&mut self) {
3939
| 18| 1| println!("BOOM times {}!!!", self.strength);
4040
| 19| 1| }

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

+2-2
Original file line numberDiff line numberDiff line change
@@ -36,12 +36,12 @@
3636
22| 2| println!("used_only_from_this_lib_crate_generic_function with {:?}", arg);
3737
23| 2|}
3838
------------------
39-
| used_crate::used_only_from_this_lib_crate_generic_function::<alloc::vec::Vec<i32>>:
39+
| used_crate::used_only_from_this_lib_crate_generic_function::<&str>:
4040
| 21| 1|pub fn used_only_from_this_lib_crate_generic_function<T: Debug>(arg: T) {
4141
| 22| 1| println!("used_only_from_this_lib_crate_generic_function with {:?}", arg);
4242
| 23| 1|}
4343
------------------
44-
| used_crate::used_only_from_this_lib_crate_generic_function::<&str>:
44+
| used_crate::used_only_from_this_lib_crate_generic_function::<alloc::vec::Vec<i32>>:
4545
| 21| 1|pub fn used_only_from_this_lib_crate_generic_function<T: Debug>(arg: T) {
4646
| 22| 1| println!("used_only_from_this_lib_crate_generic_function with {:?}", arg);
4747
| 23| 1|}

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

+6-23
Original file line numberDiff line numberDiff line change
@@ -30,29 +30,12 @@
3030
^0
3131
29| 1| use_this_lib_crate();
3232
30| 1|}
33-
------------------
34-
| used_inline_crate::used_inline_function:
35-
| 20| 1|pub fn used_inline_function() {
36-
| 21| | // Initialize test constants in a way that cannot be determined at compile time, to ensure
37-
| 22| | // rustc and LLVM cannot optimize out statements (or coverage counters) downstream from
38-
| 23| | // dependent conditions.
39-
| 24| 1| let is_true = std::env::args().len() == 1;
40-
| 25| 1| let mut countdown = 0;
41-
| 26| 1| if is_true {
42-
| 27| 1| countdown = 10;
43-
| 28| 1| }
44-
| ^0
45-
| 29| 1| use_this_lib_crate();
46-
| 30| 1|}
47-
------------------
48-
| Unexecuted instantiation: used_inline_crate::used_inline_function
49-
------------------
50-
31| |// Expect for above function:
51-
32| |//
52-
33| |// | Unexecuted instantiation: used_crate::used_only_from_bin_crate_generic_function::<_>
53-
34| |//
54-
35| |// With `#[inline(always)]` this function is instantiated twice, in both the library crate (which
55-
36| |// does not use it) and the `uses_inline_crate` binary (which does use/call it).
33+
31| |
34+
32| |
35+
33| |
36+
34| |
37+
35| |
38+
36| |
5639
37| |
5740
38| |#[inline(always)]
5841
39| 2|pub fn used_only_from_bin_crate_generic_function<T: Debug>(arg: T) {

src/test/run-make-fulldeps/coverage/lib/used_inline_crate.rs

+6-6
Original file line numberDiff line numberDiff line change
@@ -28,12 +28,12 @@ pub fn used_inline_function() {
2828
}
2929
use_this_lib_crate();
3030
}
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+
3737

3838
#[inline(always)]
3939
pub fn used_only_from_bin_crate_generic_function<T: Debug>(arg: T) {

0 commit comments

Comments
 (0)