-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
ICE; glacier fixed/27925.rs -Zmir-opt-level=2 -Zinstrument-coverage: add_counter called with duplicate id #80060
Comments
Reduced from another code sample: pub fn main() {
let c = || {};
c();
} |
Issue: rust-lang/rust#80060
@matthiaskrgr - Thanks for the ICE report, and the reduced code sample! The reduced sample produces a couple of MIR that make it to PreCodegen, one of which is: // MIR for `main` after PreCodegen
fn main() -> () {
let mut _0: (); // return place in scope 0 at 27925-reduced.rs:1:15: 1:15
let _1: [closure@27925-reduced.rs:2:13: 2:18]; // in scope 0 at 27925-reduced.rs:2:9: 2:10
scope 1 {
debug c => _1; // in scope 1 at 27925-reduced.rs:2:9: 2:10
scope 2 (inlined main::{closure#0}) { // at 27925-reduced.rs:3:5: 3:8
}
}
bb0: {
Coverage::Expression(4294967295) = 1 + 0 for 27925-reduced.rs:2:18 - 4:2; // scope 1 at 27925-reduced.rs:3:5: 3:8
Coverage::Counter(1) for 27925-reduced.rs:1:1 - 2:13; // scope 1 at 27925-reduced.rs:3:5: 3:8
StorageLive(_1); // scope 0 at 27925-reduced.rs:2:9: 2:10
Coverage::Counter(1) for 27925-reduced.rs:2:16 - 2:18; // scope 2 at 27925-reduced.rs:3:5: 3:8
_0 = const (); // scope 0 at 27925-reduced.rs:1:15: 4:2
StorageDead(_1); // scope 0 at 27925-reduced.rs:4:1: 4:2
return; // scope 0 at 27925-reduced.rs:4:2: 4:2
}
} And you can see that there are two different counters with ID "1". Most likely, The output (shown above in this issue) shows the new warning that I recently added:
Until seeing this post, I wasn't aware that I don't believe we should be trying to support this option combination. I can replace the warning with an error (and update the message to say they are incompatible, or something like that). For this type of problem, I think the failed assertion is still valid (the MIR is invalid), and it is better to fail on the option combination, rather than try to gracefully recover somehow. WDYT? |
Issue: rust-lang/rust#80060
Yeah it looks like If that is infeasible, we should probably just have the inline pass disable itself if code coverage is enabled.
Yeah, I agree! It seems like it will be easier for us to diagnose the ICE than to try to figure out why coverage data is missing or behaving incorrectly. |
…4.0, r=wesleywiser MIR Inline is incompatible with coverage Fixes: rust-lang#80060 Fixed by disabling inlining if `-Zinstrument-coverage` is set. The PR also adds additional use cases to the coverage test for doctests. r? `@wesleywiser` cc: `@tmandry`
Update: The solution implemented by #80521 is different from what we initially agreed on. More detail can be found in the PR discussion, but to be more future-proof, we didn't want to disable everything at the same If a non-default (The warning will not be displayed if inlining is moved to the default (1) level in the future.) As noted in the PR discussion, I don't believe it's worth trying to hack around the LLVM instrprof expectation that each function actually be an LLVM function. Source-based coverage is not meant for production code, so disabling inlining seems like a much better way to address the problem; and now that it only affects the inlining part, it still allows for other optimizations, which is an improvement (in addition to preventing the ICE). Lastly, I would stress that there may (or may not) still be other compiler flags and/or other optimizations that are found to be incompatible with LLVM Instrprof coverage, as implemented. But the test in this issue does now work, with |
Code
code from glacier fixed/27925.rs
Meta
rustc --version --verbose
:Error output
Backtrace
The text was updated successfully, but these errors were encountered: