coverage: Avoid overflow when the MC/DC condition limit is exceeded by Zalathar · Pull Request #125700 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation10 Commits2 Checks6 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Fix for the test failure seen in #124571 (comment).
If we perform this subtraction first, it can sometimes overflow to -1 before the addition can bring its value back to 0.
That behaviour seems to be benign, but it nevertheless causes test failures in compiler configurations that check for overflow.
@rustbot label +A-code-coverage
r? @nnethercote
rustbot has assigned @nnethercote.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.
Use r? to explicitly pick a reviewer
Status: Awaiting review from the assignee but also interested parties.
Relevant to the compiler team, which will review and decide on the PR/issue.
Area: Source-based code coverage (-Cinstrument-coverage)
labels
| @@ -217,7 +217,7 @@ impl MCDCInfoBuilder { |
|---|
| } |
| _ => { |
| // Do not generate mcdc mappings and statements for decisions with too many conditions. |
| let rebase_idx = self.branch_spans.len() - decision.conditions_num + 1; |
| let rebase_idx = self.branch_spans.len() + 1 - decision.conditions_num; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment about the ordering here seems warranted.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After considering what to write for the comment, I noticed that in context this would be better off subtracting 1 from the RHS instead.
r=me with the comments added.
@bors delegate=Zalathar
✌️ @Zalathar, you can now approve this pull request!
If @nnethercote told you to "r=me" after making some further change, please make that change, then do @bors r=@nnethercote
If we perform this subtraction and then add 1, the subtraction can sometimes overflow to -1 before the addition can bring its value back to 0. That behaviour seems to be benign, but it nevertheless causes test failures in compiler configurations that check for overflow.
We can avoid the overflow by instead subtracting (N - 1), which is algebraically equivalent, and more closely matches what the code is actually trying to do.
@nnethercote I ended up changing the code in a small but significant way (diff), which is a bit of a grey area for the delegated approval, so I'm going to err on the side of caution and not just enqueue it immediately.
📌 Commit 34a1828 has been approved by nnethercote
It is now in the queue for this repository.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
…cote
coverage: Avoid overflow when the MC/DC condition limit is exceeded
Fix for the test failure seen in rust-lang#124571 (comment).
If we perform this subtraction first, it can sometimes overflow to -1 before the addition can bring its value back to 0.
That behaviour seems to be benign, but it nevertheless causes test failures in compiler configurations that check for overflow.
@rustbot label +A-code-coverage
bors added a commit to rust-lang-ci/rust that referenced this pull request
…iaskrgr
Rollup of 6 pull requests
Successful merges:
- rust-lang#107099 (rustdoc: Add support for --remap-path-prefix)
- rust-lang#125693 (Format all source files in
tests/coverage/) - rust-lang#125700 (coverage: Avoid overflow when the MC/DC condition limit is exceeded)
- rust-lang#125705 (Reintroduce name resolution check for trying to access locals from an inline const)
- rust-lang#125708 (tier 3 target policy: clarify the point about producing assembly)
- rust-lang#125715 (remove unneeded extern crate in rmake test)
r? @ghost
@rustbot modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request
…iaskrgr
Rollup of 7 pull requests
Successful merges:
- rust-lang#124655 (Add
-Zfixed-x18) - rust-lang#125693 (Format all source files in
tests/coverage/) - rust-lang#125700 (coverage: Avoid overflow when the MC/DC condition limit is exceeded)
- rust-lang#125705 (Reintroduce name resolution check for trying to access locals from an inline const)
- rust-lang#125708 (tier 3 target policy: clarify the point about producing assembly)
- rust-lang#125715 (remove unneeded extern crate in rmake test)
- rust-lang#125719 (Extract coverage-specific code out of
compiletest::runtest)
r? @ghost
@rustbot modify labels: rollup
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
Rollup merge of rust-lang#125700 - Zalathar:limit-overflow, r=nnethercote
coverage: Avoid overflow when the MC/DC condition limit is exceeded
Fix for the test failure seen in rust-lang#124571 (comment).
If we perform this subtraction first, it can sometimes overflow to -1 before the addition can bring its value back to 0.
That behaviour seems to be benign, but it nevertheless causes test failures in compiler configurations that check for overflow.
@rustbot label +A-code-coverage
Status: This is awaiting some action (such as code changes or more information) from the author.
and removed S-waiting-on-bors
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
labels
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
coverage: Rename MC/DC conditions_num to num_conditions
Updated version of rust-lang#124571, without the other changes that were split out into rust-lang#125108 and rust-lang#125700.
This value represents a quantity of conditions, not an ID, so the new spelling is more appropriate.
Some of the code touched by this PR could perhaps use some other changes, but I would prefer to keep this PR as a simple renaming and avoid scope creep.
@rustbot label +A-code-coverage
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
Rollup merge of rust-lang#125754 - Zalathar:conditions-num, r=lqd
coverage: Rename MC/DC conditions_num to num_conditions
Updated version of rust-lang#124571, without the other changes that were split out into rust-lang#125108 and rust-lang#125700.
This value represents a quantity of conditions, not an ID, so the new spelling is more appropriate.
Some of the code touched by this PR could perhaps use some other changes, but I would prefer to keep this PR as a simple renaming and avoid scope creep.
@rustbot label +A-code-coverage
Labels
Area: Source-based code coverage (-Cinstrument-coverage)
Status: This is awaiting some action (such as code changes or more information) from the author.
Relevant to the compiler team, which will review and decide on the PR/issue.