Skip MIR pass UnreachablePropagation when coverage is enabled
#116166
Conversation
|
(rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
rustbot
added
A-code-coverage
|
I had also written a more complicated patch that leaves this pass enabled, but takes care not to erase coverage statements from unreachable blocks. But for now I think it's simpler to just turn off the pass when coverage is enabled, and try to fix it entirely on the coverage side (without having to modify other MIR passes) after some other coverage-related changes land. |
| @@ -13,7 +13,9 @@ pub struct UnreachablePropagation; | |||
| impl MirPass<'_> for UnreachablePropagation { | |||
| fn is_enabled(&self, sess: &rustc_session::Session) -> bool { | |||
| // Enable only under -Zmir-opt-level=2 as this can make programs less debuggable. | |||
| sess.mir_opt_level() >= 2 | |||
| // Coverage gets confused by MIR passes that can remove all coverage statements | |||
| // from an instrumented function. | |||
There was a problem hiding this comment.
Could you add a FIXME with a issue/PR number, so a future reader will know when to revert that PR?
r=me once done
There was a problem hiding this comment.
I've added a FIXME that refers to a new issue, #116171.
Zalathar
force-pushed
the
unreachable
branch
from
ddbb0f7
to
d356984
Compare
When coverage instrumentation and MIR opts are both enabled, coverage relies on two assumptions: - MIR opts that would delete `StatementKind::Coverage` statements instead move them into bb0 and change them to `CoverageKind::Unreachable`. - MIR opts won't delete all `CoverageKind::Counter` statements from an instrumented function. Most MIR opts naturally satisfy the second assumption, because they won't remove coverage statements from bb0, but `UnreachablePropagation` can do so if it finds that bb0 is unreachable. If this happens, LLVM thinks the function isn't instrumented, and it vanishes from coverage reports. A proper solution won't be possible until after per-function coverage info lands in rust-lang#116046, but for now we can avoid the problem by turning off this particular pass when coverage instrumentation is enabled.
Zalathar
force-pushed
the
unreachable
branch
from
d356984
to
64df5a8
Compare
|
@bors r=cjgillot |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (376f3f0): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 630.064s -> 629.749s (-0.05%) |
coverage: Regression test for functions with unreachable bodies This is a regression test for the coverage issue that was addressed temporarily by rust-lang#116166, and is tracked by rust-lang#116171. --- If we instrument a function for coverage, but all of its counter-increment statements are removed by MIR optimizations, LLVM will think it isn't instrumented and it will disappear from coverage maps and coverage reports. Most MIR opts won't cause this because they tend not to remove statements from bb0, but `UnreachablePropagation` can do so if it sees that bb0 ends with `TerminatorKind::Unreachable`. Currently we have worked around this by turning off `UnreachablePropagation` when coverage instrumentation is enabled, which is why this test is able to pass. --- `@rustbot` label +A-code-coverage
When coverage instrumentation and MIR opts are both enabled, coverage relies on two assumptions:
MIR opts that would delete
StatementKind::Coveragestatements instead move them into bb0 and change them toCoverageKind::Unreachable.MIR opts won't delete all
CoverageKind::Counterstatements from an instrumented function.Most MIR opts naturally satisfy the second assumption, because they won't remove coverage statements from bb0, but
UnreachablePropagationcan do so if it finds that bb0 is unreachable. If this happens, LLVM thinks the function isn't instrumented, and it vanishes from coverage reports.A proper solution won't be possible until after per-function coverage info lands in #116046, but for now we can avoid the problem by turning off this particular pass when coverage instrumentation is enabled.
cc @cjgillot since I found this while investigating coverage problems encountered by #113970
@rustbot label +A-code-coverage +A-mir-opt