-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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.
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
Binaries generate malformed coverage profile when built with -Z instrument-coverage -C link-dead-code -O
#79175
Comments
Great bug report. I'll take a look. Thanks! |
Partial update here... I did some experimentation, making small changes to the code, and turning on and off the I was able to create a modified MCVE, that also appears to make minimal changes to the LLVM IR. Here is your sample code with a couple of additional lines, commented out. If I uncomment those lines, the example works as expected: fn bar() {
loop {}
}
pub trait Trait {
fn foo(&self) {
// if true {
bar();
// }
}
}
impl Trait for u8 {}
fn main() {
println!("hi")
} If I uncomment these lines (by replacing slashes with spaces, just to minimize the changes to code locations propagated to lower-level representations), there are some small but notable changes:
@__profc__RNvYhNtCs4fqI2P2rA04_11issue_791755Trait3fooB5_ = private global [3 x i64] zeroinitializer, section "__llvm_prf_cnts", align 8
@__profd__RNvYhNtCs4fqI2P2rA04_11issue_791755Trait3fooB5_ = private global { i64, i64, i64*, i8*, i8*, i32, [2 x i16] } { i64 -5234861019717404860, i64 -7566766773726293094, i64* getelementptr inbounds ([3 x i64], [3 x i64]* @__profc__RNvYhNtCs4fqI2P2rA04_11issue_791755Trait3fooB5_, i32 0, i32 0), i8* bitcast (void (i8*)* @_RNvYhNtCs4fqI2P2rA04_11issue_791755Trait3fooB5_ to i8*), i8* null, i32 3, [2 x i16] zeroinitializer }, section "__llvm_prf_data", align 8
%pgocount = load i64, i64* getelementptr inbounds ([3 x i64], [3 x i64]* @__profc__RNvYhNtCs4fqI2P2rA04_11issue_791755Trait3fooB5_, i64 0, i64 1), align 8
%1 = add i64 %pgocount1, 1
store i64 %0, i64* getelementptr inbounds ([3 x i64], [3 x i64]* @__profc__RNvYhNtCs4fqI2P2rA04_11issue_791755Trait3fooB5_, i64 0, i64 1), align 8 Those are the only significant changes. In both the working and non-working examples, everything else is the same (with the exception of a couple of locally-scoped variable symbols, like Note, The Intermediate conclusionI don't have a definitive answer yet, but I'm still investigating. I did reproduce the I suspect (still to be proven) that the coverage map in the LLVM IR for your original example still includes references to counters from cc: @tmandry |
Quick note: PR #79109 includes several improvements, and one important improvement is, we no longer automatically set I tried your sample with the modifications in PR #79109, and no longer get the coverage failures, with I think we can mark this Issue resolved, once that PR lands. |
I think we should still track this somewhere, but I can rename the issue to include |
Note, I suspect there are other combinations of compiler options and optimizations that could break coverage instrumentation, and they may not be easy to predict, but the documented examples should work, and I'll try to make note of other options we expect to work as well (including |
This is good to know, especially for people migrating from other coverage mechanisms. I didn't find it in the documentation in https://doc.rust-lang.org/nightly/unstable-book/compiler-flags/source-based-code-coverage.html. We should add such a statement to that doc. |
Also #64685 (comment):
There is more discussion about this topic in that issue. I'm guessing that a lot of people can work around this issue by simply removing |
-Z instrument-coverage -C link-dead-code -O
The document does include the following statement:
I don't know if there's a strong reason to add a statement about this specific option. I wouldn't object if someone wants to make that change though. |
This program generates a malformed coverage error from fn foo() {}
fn bar() {}
fn do_stuff(x: bool) {
if x {
foo()
} else {
bar()
}
}
fn main() {
do_stuff(false);
}
I think this is the same root cause, but I can open a separate issue if you prefer. |
I think this might be because of the order in which the LLVM instrumentation pass is executed relative to the other LLVM passes. Clang runs the "Frontend instrumentation-based coverage lowering" very early before any optimization passes while rustc runs it at the end after all the optimizations. rustc
clang
|
@richkadel I've confirmed that reordering the passes fixes this issue. |
Thanks @Amanieu. It's not clear to me from your posts if you tests this with the changes from #83307. From the timing of your post, I'm assuming so, but I'm also testing this (without your suggested fix) just to be sure, since my PR fixed #83307 only landed Thursday morning, and as far as I can tell, |
Can we close this issue as "Fixed"? |
Here's the patch I'm using: master...Amanieu:instrprof-order Looking at the generated LLVM IR, it seems that this is fixed by #83307 because the LLVM IR now marks these functions as |
llvm-cov
refuses to load coverage data for this code when runningrustc
with optimizations enabled:Here's how I'm generating the coverage data:
And what I'm observing:
rustc --version --verbose
:cc @richkadel #79121
The text was updated successfully, but these errors were encountered: