Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Run LLVM coverage instrumentation passes before optimization passes #83666

Merged
merged 2 commits into from
Mar 31, 2021
Merged

Run LLVM coverage instrumentation passes before optimization passes #83666

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
26d260b
Run LLVM coverage instrumentation passes before optimization passes
Amanieu Mar 27, 2021
cad9b6b
Apply review feedback
Amanieu Mar 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions compiler/rustc_codegen_llvm/src/back/write.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
9 changes: 1 addition & 8 deletions compiler/rustc_middle/src/mir/mono.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, before you make this change, we need to test that this doesn't break the brotli_decompressor

See #82875

I was not able to create a coverage test that failed in the way it fails in this issue, but by not forcing inline always, the issue was resolved.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tested the example from that issue. It fails on my main rustc (2021-03-24 nightly) but succeeds on my new branch. So the fix for inline always works.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's great.

Before merging, I'd like to try your patch on some creates I had issues with.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cherry-picked your branch commits and am testing them now.

} 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
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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
29 changes: 6 additions & 23 deletions src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.uses_inline_crate.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
12 changes: 6 additions & 6 deletions src/test/run-make-fulldeps/coverage/lib/used_inline_crate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,12 @@ pub fn used_inline_function() {
}
use_this_lib_crate();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any theories why this is no longer generating the Unexecuted instantiation? It probably doesn't have to be documented here anymore (the absence of the unexpected should be expected, so this is good), but I'm curious why it changed.

Here's a trick for keeping the diffs clean in the expected results, above. The line numbers in llvm-cov show mess things up, so I avoid changing line numbers whenever possible: Just replace these 6 deleted lines with 6 blank lines.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Unexecuted instantiation re-appears if I undo the changes in compiler/rustc_typeck/src/collect.rs. I'm not exactly sure how that causes it to re-appear though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I assumed it's related somehow.

Thanks for inserting the blank lines. Makes the expected result diffs a lot easier to read!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want me to remove the blank lines before merging?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessary. Thanks.

// 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