Fixed conflict with drop elaboration and coverage #80072
Conversation
See rust-lang#80045 (comment) Coverage statements are moved to the beginning of the BCB. This does also affect what's counted before a panic, changing some results, but I think these results may even be preferred? In any case, there are no guarantees about what's counted when a panic occurs (by design).
rust-highfive
added
the
S-waiting-on-review
| @@ -470,7 +470,7 @@ fn inject_statement( | |||
| code_region: some_code_region, | |||
| }), | |||
| }; | |||
| data.statements.push(statement); | |||
| data.statements.insert(0, statement); | |||
There was a problem hiding this comment.
This is always going to be slower than appending at the end. I'd prefer to skip over coverage statements in the elaborate drops pass rather than work around it here.
There was a problem hiding this comment.
I actually prefer it this way and I think the overall impact will be negligible.
Yes, functionally, we have the option of inserting the statement anywhere in the BCB, but it is standard practice to insert() into the statements vec in MIR processing when order is critical.
As I was trying to say in my reply here (though maybe not clearly):
I believe inserting the coverage statement between the last statement and the terminator is going to break up some sequences in unexpected ways.
It wouldn't be a problem if I could insert the coverage statement after the terminator (which is impossible without a new BB, and that's much worse).
But the Terminator is really just a special case of a Statement, and the sequence from one statement to another is sometimes important, analytically at least. (Possibly even important at runtime, for context switching reasons, in fact.)
To drive the point: Think about a typical instruction sequence like, Load followed by Add (adding to the value that was just loaded). Most people would expect these to come in uninterrupted sequence, so injecting code to increment a global counter between the two, even if possible, would not be right.
I would rather not insert Coverage statements between a Statement and it's following Terminator, and it's not just about this one issue.
There was a problem hiding this comment.
I missed your comment there, makes sense.
|
@bors r+ |
|
📌 Commit 1d6b455 has been approved by |
bors
added
S-waiting-on-bors
Rollup of 11 pull requests Successful merges: - rust-lang#79051 (Implement if-let match guards) - rust-lang#79877 (Allow `since="TBD"` for rustc_deprecated) - rust-lang#79882 (Fix issue rust-lang#78496) - rust-lang#80026 (expand-yaml-anchors: Make the output directory separator-insensitive) - rust-lang#80039 (Remove unused `TyEncoder::tcx` required method) - rust-lang#80069 (Test that `core::assert!` is valid) - rust-lang#80072 (Fixed conflict with drop elaboration and coverage) - rust-lang#80073 (Add support for target aliases) - rust-lang#80082 (Revert rust-lang#78790 - rust-src vendoring) - rust-lang#80097 (Add `popcount` and `popcnt` as doc aliases for `count_ones` methods.) - rust-lang#80103 (Remove docs for non-existent parameters in `rustc_expand`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
See
#80045 (comment)
Coverage statements are moved to the beginning of the BCB. This does
also affect what's counted before a panic, changing some results, but I
think these results may even be preferred? In any case, there are no
guarantees about what's counted when a panic occurs (by design).
r? @tmandry
FYI @wesleywiser @ecstatic-morse