Skip to content

Commit

Permalink
Run LLVM coverage instrumentation passes before optimization passes
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@Amanieu
Amanieu committed Mar 30, 2021
1 parent 3debe9a commit 26d260b
Show file tree
Hide file tree
Showing 9 changed files with 92 additions and 139 deletions.
5 changes: 5 additions & 0 deletions compiler/rustc_codegen_llvm/src/back/write.rs
View file
Expand Up @@ -548,6 +548,11 @@ pub(crate) unsafe fn optimize(
llvm::LLVMRustAddPass(fpm, find_pass("lint").unwrap());
continue;
}
if pass_name == "insert-gcov-profiling" || pass_name == "instrprof" {
// Instrumentation should be inserted before optimization.
llvm::LLVMRustAddPass(mpm, find_pass(pass_name).unwrap());
continue;
}

if let Some(pass) = find_pass(pass_name) {
extra_passes.push(pass);
Expand Down
9 changes: 1 addition & 8 deletions compiler/rustc_middle/src/mir/mono.rs
View file
Expand Up @@ -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) => {
Expand Down
8 changes: 1 addition & 7 deletions compiler/rustc_mir/src/monomorphize/partitioning/mod.rs
View file
Expand Up @@ -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);
}
Expand Down
12 changes: 1 addition & 11 deletions compiler/rustc_typeck/src/collect.rs
View file
Expand Up @@ -2889,17 +2889,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 {
Expand Down
2 changes: 1 addition & 1 deletion src/test/run-make-fulldeps/coverage-llvmir/Makefile
View file
Expand Up @@ -17,7 +17,7 @@ else
COMDAT_IF_SUPPORTED=, comdat
endif

DEFINE_INTERNAL=define hidden
DEFINE_INTERNAL=define internal

ifdef IS_WINDOWS
LLVM_FILECHECK_OPTIONS=\
Expand Down
4 changes: 2 additions & 2 deletions src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.generics.txt
View file
Expand Up @@ -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| }
Expand Down
4 changes: 2 additions & 2 deletions src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.uses_crate.txt
View file
Expand Up @@ -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|}
Expand Down
181 changes: 79 additions & 102 deletions src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.uses_inline_crate.txt
View file
Expand Up @@ -30,127 +30,104 @@
^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).
37| |
38| |#[inline(always)]
39| 2|pub fn used_only_from_bin_crate_generic_function<T: Debug>(arg: T) {
40| 2| println!("used_only_from_bin_crate_generic_function with {:?}", arg);
41| 2|}
31| |
32| |#[inline(always)]
33| 2|pub fn used_only_from_bin_crate_generic_function<T: Debug>(arg: T) {
34| 2| println!("used_only_from_bin_crate_generic_function with {:?}", arg);
35| 2|}
------------------
| 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|}
| 33| 1|pub fn used_only_from_bin_crate_generic_function<T: Debug>(arg: T) {
| 34| 1| println!("used_only_from_bin_crate_generic_function with {:?}", arg);
| 35| 1|}
------------------
| 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|}
| 33| 1|pub fn used_only_from_bin_crate_generic_function<T: Debug>(arg: T) {
| 34| 1| println!("used_only_from_bin_crate_generic_function with {:?}", arg);
| 35| 1|}
------------------
| Unexecuted instantiation: used_inline_crate::used_only_from_bin_crate_generic_function::<_>
------------------
42| |// Expect for above function: `Unexecuted instantiation` (see notes in `used_crate.rs`)
43| |
44| |#[inline(always)]
45| 4|pub fn used_only_from_this_lib_crate_generic_function<T: Debug>(arg: T) {
46| 4| println!("used_only_from_this_lib_crate_generic_function with {:?}", arg);
47| 4|}
36| |// Expect for above function: `Unexecuted instantiation` (see notes in `used_crate.rs`)
37| |
38| |#[inline(always)]
39| 4|pub fn used_only_from_this_lib_crate_generic_function<T: Debug>(arg: T) {
40| 4| println!("used_only_from_this_lib_crate_generic_function with {:?}", arg);
41| 4|}
------------------
| 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|}
| 39| 2|pub fn used_only_from_this_lib_crate_generic_function<T: Debug>(arg: T) {
| 40| 2| println!("used_only_from_this_lib_crate_generic_function with {:?}", arg);
| 41| 2|}
------------------
| 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|}
| 39| 2|pub fn used_only_from_this_lib_crate_generic_function<T: Debug>(arg: T) {
| 40| 2| println!("used_only_from_this_lib_crate_generic_function with {:?}", arg);
| 41| 2|}
------------------
48| |
49| |#[inline(always)]
50| 3|pub fn used_from_bin_crate_and_lib_crate_generic_function<T: Debug>(arg: T) {
51| 3| println!("used_from_bin_crate_and_lib_crate_generic_function with {:?}", arg);
52| 3|}
42| |
43| |#[inline(always)]
44| 3|pub fn used_from_bin_crate_and_lib_crate_generic_function<T: Debug>(arg: T) {
45| 3| println!("used_from_bin_crate_and_lib_crate_generic_function with {:?}", arg);
46| 3|}
------------------
| used_inline_crate::used_from_bin_crate_and_lib_crate_generic_function::<alloc::vec::Vec<i32>>:
| 50| 1|pub fn used_from_bin_crate_and_lib_crate_generic_function<T: Debug>(arg: T) {
| 51| 1| println!("used_from_bin_crate_and_lib_crate_generic_function with {:?}", arg);
| 52| 1|}
| 44| 1|pub fn used_from_bin_crate_and_lib_crate_generic_function<T: Debug>(arg: T) {
| 45| 1| println!("used_from_bin_crate_and_lib_crate_generic_function with {:?}", arg);
| 46| 1|}
------------------
| used_inline_crate::used_from_bin_crate_and_lib_crate_generic_function::<&str>:
| 50| 2|pub fn used_from_bin_crate_and_lib_crate_generic_function<T: Debug>(arg: T) {
| 51| 2| println!("used_from_bin_crate_and_lib_crate_generic_function with {:?}", arg);
| 52| 2|}
| 44| 2|pub fn used_from_bin_crate_and_lib_crate_generic_function<T: Debug>(arg: T) {
| 45| 2| println!("used_from_bin_crate_and_lib_crate_generic_function with {:?}", arg);
| 46| 2|}
------------------
53| |
54| |#[inline(always)]
55| 3|pub fn used_with_same_type_from_bin_crate_and_lib_crate_generic_function<T: Debug>(arg: T) {
56| 3| println!("used_with_same_type_from_bin_crate_and_lib_crate_generic_function with {:?}", arg);
57| 3|}
47| |
48| |#[inline(always)]
49| 3|pub fn used_with_same_type_from_bin_crate_and_lib_crate_generic_function<T: Debug>(arg: T) {
50| 3| println!("used_with_same_type_from_bin_crate_and_lib_crate_generic_function with {:?}", arg);
51| 3|}
------------------
| used_inline_crate::used_with_same_type_from_bin_crate_and_lib_crate_generic_function::<&str>:
| 55| 1|pub fn used_with_same_type_from_bin_crate_and_lib_crate_generic_function<T: Debug>(arg: T) {
| 56| 1| println!("used_with_same_type_from_bin_crate_and_lib_crate_generic_function with {:?}", arg);
| 57| 1|}
| 49| 1|pub fn used_with_same_type_from_bin_crate_and_lib_crate_generic_function<T: Debug>(arg: T) {
| 50| 1| println!("used_with_same_type_from_bin_crate_and_lib_crate_generic_function with {:?}", arg);
| 51| 1|}
------------------
| used_inline_crate::used_with_same_type_from_bin_crate_and_lib_crate_generic_function::<&str>:
| 55| 2|pub fn used_with_same_type_from_bin_crate_and_lib_crate_generic_function<T: Debug>(arg: T) {
| 56| 2| println!("used_with_same_type_from_bin_crate_and_lib_crate_generic_function with {:?}", arg);
| 57| 2|}
------------------
58| |
59| |#[inline(always)]
60| 0|pub fn unused_generic_function<T: Debug>(arg: T) {
61| 0| println!("unused_generic_function with {:?}", arg);
62| 0|}
63| |
64| |#[inline(always)]
65| 0|pub fn unused_function() {
66| 0| let is_true = std::env::args().len() == 1;
67| 0| let mut countdown = 2;
68| 0| if !is_true {
69| 0| countdown = 20;
70| 0| }
71| 0|}
72| |
73| |#[inline(always)]
74| 0|fn unused_private_function() {
75| 0| let is_true = std::env::args().len() == 1;
76| 0| let mut countdown = 2;
77| 0| if !is_true {
78| 0| countdown = 20;
79| 0| }
80| 0|}
81| |
82| 2|fn use_this_lib_crate() {
83| 2| used_from_bin_crate_and_lib_crate_generic_function("used from library used_crate.rs");
84| 2| used_with_same_type_from_bin_crate_and_lib_crate_generic_function(
85| 2| "used from library used_crate.rs",
86| 2| );
87| 2| let some_vec = vec![5, 6, 7, 8];
88| 2| used_only_from_this_lib_crate_generic_function(some_vec);
89| 2| used_only_from_this_lib_crate_generic_function("used ONLY from library used_crate.rs");
90| 2|}
| 49| 2|pub fn used_with_same_type_from_bin_crate_and_lib_crate_generic_function<T: Debug>(arg: T) {
| 50| 2| println!("used_with_same_type_from_bin_crate_and_lib_crate_generic_function with {:?}", arg);
| 51| 2|}
------------------
52| |
53| |#[inline(always)]
54| 0|pub fn unused_generic_function<T: Debug>(arg: T) {
55| 0| println!("unused_generic_function with {:?}", arg);
56| 0|}
57| |
58| |#[inline(always)]
59| 0|pub fn unused_function() {
60| 0| let is_true = std::env::args().len() == 1;
61| 0| let mut countdown = 2;
62| 0| if !is_true {
63| 0| countdown = 20;
64| 0| }
65| 0|}
66| |
67| |#[inline(always)]
68| 0|fn unused_private_function() {
69| 0| let is_true = std::env::args().len() == 1;
70| 0| let mut countdown = 2;
71| 0| if !is_true {
72| 0| countdown = 20;
73| 0| }
74| 0|}
75| |
76| 2|fn use_this_lib_crate() {
77| 2| used_from_bin_crate_and_lib_crate_generic_function("used from library used_crate.rs");
78| 2| used_with_same_type_from_bin_crate_and_lib_crate_generic_function(
79| 2| "used from library used_crate.rs",
80| 2| );
81| 2| let some_vec = vec![5, 6, 7, 8];
82| 2| used_only_from_this_lib_crate_generic_function(some_vec);
83| 2| used_only_from_this_lib_crate_generic_function("used ONLY from library used_crate.rs");
84| 2|}

6 changes: 0 additions & 6 deletions src/test/run-make-fulldeps/coverage/lib/used_inline_crate.rs
View file
Expand Up @@ -28,12 +28,6 @@ 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) {
Expand Down

0 comments on commit 26d260b

Please sign in to comment.