Skip to content
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

Implement Modified Condition/Decision Coverage #123409

Merged
merged 4 commits into from
Apr 20, 2024
Merged

Conversation

ZhuUx
Copy link
Contributor

@ZhuUx ZhuUx commented Apr 3, 2024

This is an implementation based on llvm backend support (>= 18) by @evodius96 and branch coverage support by @Zalathar.

Major changes:

  • Add -Zcoverage-options=mcdc as switch. Now coverage options accept either no-branch, branch, or mcdc. mcdc also enables branch because it is essential to work.
  • Add coverage mapping for MCDCBranch and MCDCDecision. Note that MCDCParameter evolves from llvm 18 to llvm 19. The mapping in rust side mainly references to 19 and is casted to 18 types in llvm wrapper.
  • Add wrapper for mcdc instrinc functions from llvm. And inject associated statements to mir.
  • Add BcbMappingKind::Decision, I'm not sure is it proper but can't find a better way temporarily.
  • Let coverage-dump support parsing MCDCBranch and MCDCDecision from llvm ir.
  • Add simple tests to check whether mcdc works.
  • Same as clang, currently rustc does not generate instrument for decision with more than 6 condtions or only 1 condition due to considerations of resource.

Implementation Details

  1. To get information about conditions and decisions, MCDCState in BranchInfoBuilder is used during hir lowering to mir. For expressions with logical op we call Builder::visit_coverage_branch_operation to record its sub conditions, generate condition ids for them and save their spans (to construct the span of whole decision). This process mainly references to the implementation in clang and is described in comments over MCDCState::record_conditions. Also true marks and false marks introduced by branch coverage are used to detect where the decision evaluation ends: the next id of the condition == 0.
  2. Once the MCDCState::decision_stack popped all recorded conditions, we can ensure that the decision is checked over and push it into decision_spans. We do not manually insert decision span to avoid complexity from then_else_break in nested if scopes.
  3. When constructing CoverageSpans, add condition info to BcbMappingKind::Branch and decision info to BcbMappingKind::Decision. If the branch mapping has non-zero condition id it will be transformed to MCDCBranch mapping and insert CondBitmapUpdate statements to its evaluated blocks. While decision bcb mapping will insert TestVectorBitmapUpdate in all its end blocks.

Usage

 echo "[build]\nprofiler=true" >> config.toml
./x build --stage 1
./x test tests/coverage/mcdc_if.rs

to build the compiler and run tests.

export PATH=path/to/llvm-build:$PATH
rustup toolchain link mcdc build/host/stage1
cargo +mcdc rustc --bin foo -- -Cinstrument-coverage -Zcoverage-options=mcdc
cd target/debug
LLVM_PROFILE_FILE="foo.profraw" ./foo
llvm-profdata merge -sparse foo.profraw -o foo.profdata   
llvm-cov show ./foo -instr-profile=foo.profdata --show-mcdc

to check "foo" code.

Problems to solve

For now decision mapping will insert statements to its all end blocks, which may be optimized by inserting a final block of the decision. To do this we must also trace the evaluated value at each end of the decision and join them separately.

This implementation is not heavily tested so there should be some unrevealed issues. We are going to check our rust products in the next. Please let me know if you had any suggestions or comments.

@rustbot
Copy link
Collaborator

rustbot commented Apr 3, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @oli-obk (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 3, 2024
@rustbot
Copy link
Collaborator

rustbot commented Apr 3, 2024

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in coverage instrumentation.

cc @Zalathar

Some changes occurred in coverage instrumentation.

cc @Zalathar

Some changes occurred in match lowering

cc @Nadrieril

Some changes occurred in coverage tests.

cc @Zalathar

Some changes occurred in coverage instrumentation.

cc @Zalathar

Some changes occurred in coverage instrumentation.

cc @Zalathar

@rust-log-analyzer

This comment has been minimized.

@ZhuUx
Copy link
Contributor Author

ZhuUx commented Apr 3, 2024

r? rust-lang/compiler

@rustbot rustbot assigned Nadrieril and unassigned oli-obk Apr 3, 2024
@ZhuUx
Copy link
Contributor Author

ZhuUx commented Apr 3, 2024

This implementation requires llvm 18 so x86_64-gnu-llvm-17 fails

@Nadrieril
Copy link
Member

r? compiler

@Zalathar
Copy link
Contributor

Zalathar commented Apr 4, 2024

@rustbot label +A-code-coverage

@rustbot rustbot added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Apr 4, 2024
@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Apr 4, 2024
@rustbot
Copy link
Collaborator

rustbot commented Apr 4, 2024

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

Some changes occurred in src/tools/compiletest

cc @jieyouxu

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Apr 20, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 20, 2024
…iaskrgr

Rollup of 3 pull requests

Successful merges:

 - rust-lang#123409 (Implement Modified Condition/Decision  Coverage)
 - rust-lang#124104 (Fix capturing duplicated lifetimes via parent in `precise_captures` (`impl use<'...>`))
 - rust-lang#124137 (Match hyphen in multi-revision comment matchers)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit efb264f into rust-lang:master Apr 20, 2024
12 of 13 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Apr 20, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 20, 2024
Rollup merge of rust-lang#123409 - ZhuUx:master, r=oli-obk

Implement Modified Condition/Decision  Coverage

This is an implementation based on llvm backend support (>= 18) by `@evodius96` and branch coverage support by `@Zalathar.`

### Major changes:

* Add -Zcoverage-options=mcdc as switch. Now coverage options accept either `no-branch`, `branch`, or `mcdc`. `mcdc` also enables `branch` because it is essential to work.
* Add coverage mapping for MCDCBranch and MCDCDecision. Note that MCDCParameter evolves from  llvm 18 to llvm 19. The mapping in rust side mainly references to 19 and is casted to 18 types in llvm wrapper.
* Add wrapper for mcdc instrinc functions from llvm. And inject associated statements to mir.
* Add BcbMappingKind::Decision, I'm not sure is it proper but can't find a better way temporarily.
* Let coverage-dump support parsing MCDCBranch and MCDCDecision from llvm ir.
* Add simple tests to check whether mcdc works.
* Same as clang, currently rustc does not generate instrument for decision with more than 6 condtions or only 1 condition due to considerations of resource.

### Implementation Details

1. To get information about conditions and decisions, `MCDCState` in `BranchInfoBuilder` is used during hir lowering to mir. For expressions with logical op we call `Builder::visit_coverage_branch_operation` to record its sub conditions, generate condition ids for them and save their spans (to construct the span of whole decision). This process mainly references to the implementation in clang and is described in comments over `MCDCState::record_conditions`. Also true marks and false marks introduced by branch coverage are used to detect where the decision evaluation ends: the next id  of the condition == 0.
2. Once the `MCDCState::decision_stack` popped all recorded conditions, we can ensure that the decision is checked over and push it into `decision_spans`. We do not manually insert decision span to avoid complexity from then_else_break in nested if scopes.
3. When constructing CoverageSpans, add condition info to BcbMappingKind::Branch and decision info to BcbMappingKind::Decision. If the branch mapping has non-zero condition id it will be transformed to MCDCBranch mapping and insert `CondBitmapUpdate` statements to its evaluated blocks. While decision bcb mapping will insert `TestVectorBitmapUpdate` in all its end blocks.

### Usage
```bash
 echo "[build]\nprofiler=true" >> config.toml
./x build --stage 1
./x test tests/coverage/mcdc_if.rs
```
to build the compiler and run tests.

```shell
export PATH=path/to/llvm-build:$PATH
rustup toolchain link mcdc build/host/stage1
cargo +mcdc rustc --bin foo -- -Cinstrument-coverage -Zcoverage-options=mcdc
cd target/debug
LLVM_PROFILE_FILE="foo.profraw" ./foo
llvm-profdata merge -sparse foo.profraw -o foo.profdata
llvm-cov show ./foo -instr-profile=foo.profdata --show-mcdc
```
to check "foo" code.

### Problems to solve

For now decision mapping will insert statements to its all end blocks, which may be optimized by inserting a final block of the decision. To do this we must also trace the evaluated value at each end of the decision and join them separately.

This implementation is not heavily tested so there should be some unrevealed issues. We are going to check our rust products in the next.  Please let me know if you had any suggestions or comments.
@Zalathar
Copy link
Contributor

After trying to rebase my own branch coverage changes on top of this, I've found that it's much harder than expected, because of how the MC/DC code interacts with the existing branch coverage code.

In the future, I want to insist on an even cleaner separation between branch-coverage code and MC/DC code, so that MC/DC doesn't cause unnecessary headaches for branch coverage.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Apr 22, 2024
coverage: Prepare for improved branch coverage

When trying to rebase my new branch coverage work (including rust-lang#124154) on top of the introduction of MC/DC coverage (rust-lang#123409), I found it a lot harder than anticipated. With the benefit of hindsight, the branch coverage code and MC/DC code have become more interdependent than I'm happy with.

This PR therefore disentangles them a bit, so that it will be easier for both areas of code to evolve independently without interference.

---

This PR also includes a few extra branch coverage tests that I had sitting around from my current branch coverage work. They mostly just demonstrate that certain language constructs listed in rust-lang#124118 currently don't have branch coverage support.

`@rustbot` label +A-code-coverage
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Apr 22, 2024
coverage: Prepare for improved branch coverage

When trying to rebase my new branch coverage work (including rust-lang#124154) on top of the introduction of MC/DC coverage (rust-lang#123409), I found it a lot harder than anticipated. With the benefit of hindsight, the branch coverage code and MC/DC code have become more interdependent than I'm happy with.

This PR therefore disentangles them a bit, so that it will be easier for both areas of code to evolve independently without interference.

---

This PR also includes a few extra branch coverage tests that I had sitting around from my current branch coverage work. They mostly just demonstrate that certain language constructs listed in rust-lang#124118 currently don't have branch coverage support.

``@rustbot`` label +A-code-coverage
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 23, 2024
Rollup merge of rust-lang#124217 - Zalathar:pre-branch, r=oli-obk

coverage: Prepare for improved branch coverage

When trying to rebase my new branch coverage work (including rust-lang#124154) on top of the introduction of MC/DC coverage (rust-lang#123409), I found it a lot harder than anticipated. With the benefit of hindsight, the branch coverage code and MC/DC code have become more interdependent than I'm happy with.

This PR therefore disentangles them a bit, so that it will be easier for both areas of code to evolve independently without interference.

---

This PR also includes a few extra branch coverage tests that I had sitting around from my current branch coverage work. They mostly just demonstrate that certain language constructs listed in rust-lang#124118 currently don't have branch coverage support.

``@rustbot`` label +A-code-coverage
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 29, 2024
…ons, r=oli-obk

MCDC coverage: support nested decision coverage

rust-lang#123409 provided the initial MCDC coverage implementation.

As referenced in rust-lang#124144, it does not currently support "nested" decisions, like the following example :

```rust
fn nested_if_in_condition(a: bool, b: bool, c: bool) {
    if a && if b || c { true } else { false } {
        say("yes");
    } else {
        say("no");
    }
}
```

Note that there is an if-expression (`if b || c ...`) embedded inside a boolean expression in the decision of an outer if-expression.

This PR proposes a workaround for this cases, by introducing a Decision context stack, and by handing several `temporary condition bitmaps` instead of just one.
When instrumenting boolean expressions, if the current node is a leaf condition (i.e. not a `||`/`&&` logical operator nor a `!` not operator), we insert a new decision context, such that if there are more boolean expressions inside the condition, they are handled as separate expressions.

On the codegen LLVM side, we allocate as many `temp_cond_bitmap`s as necessary to handle the maximum encountered decision depth.
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 29, 2024
…ons, r=Zalathar

MCDC coverage: support nested decision coverage

rust-lang#123409 provided the initial MCDC coverage implementation.

As referenced in rust-lang#124144, it does not currently support "nested" decisions, like the following example :

```rust
fn nested_if_in_condition(a: bool, b: bool, c: bool) {
    if a && if b || c { true } else { false } {
        say("yes");
    } else {
        say("no");
    }
}
```

Note that there is an if-expression (`if b || c ...`) embedded inside a boolean expression in the decision of an outer if-expression.

This PR proposes a workaround for this cases, by introducing a Decision context stack, and by handing several `temporary condition bitmaps` instead of just one.
When instrumenting boolean expressions, if the current node is a leaf condition (i.e. not a `||`/`&&` logical operator nor a `!` not operator), we insert a new decision context, such that if there are more boolean expressions inside the condition, they are handled as separate expressions.

On the codegen LLVM side, we allocate as many `temp_cond_bitmap`s as necessary to handle the maximum encountered decision depth.
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Apr 29, 2024
coverage: Avoid hard-coded values when visiting logical ops

This is a tiny little thing that I noticed during the final review of rust-lang#123409, and I didn't want to hold up the whole PR just for this.

Instead of separately hard-coding the operation being visited, we can get it from the match arm pattern by using an as-pattern.

`@rustbot` label +A-code-coverage
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 29, 2024
Rollup merge of rust-lang#124508 - Zalathar:op, r=jieyouxu

coverage: Avoid hard-coded values when visiting logical ops

This is a tiny little thing that I noticed during the final review of rust-lang#123409, and I didn't want to hold up the whole PR just for this.

Instead of separately hard-coding the operation being visited, we can get it from the match arm pattern by using an as-pattern.

`@rustbot` label +A-code-coverage
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 30, 2024
…errors

coverage: Replace boolean options with a `CoverageLevel` enum

After rust-lang#123409, and some discussion at rust-lang#79649 (comment) and rust-lang#124120, it became clear to me that we should have a unified concept of “coverage level”, instead of having several separate boolean flags that aren't actually independent.

This PR therefore introduces a `CoverageLevel` enum, to replace the existing boolean flags for `branch` and `mcdc`.

The `no-branch` value (for `-Zcoverage-options`) has been renamed to `block`, instructing the compiler to only instrument for block coverage, with no branch coverage or MD/DC instrumentation.

`@rustbot` label +A-code-coverage
cc `@ZhuUx` `@Lambdaris` `@RenjiSann`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 3, 2024
coverage: Clean up creation of MC/DC condition bitmaps

This PR improves the code for creating and initializing [MC/DC](https://en.wikipedia.org/wiki/Modified_condition/decision_coverage) condition bitmap variables, as introduced by rust-lang#123409 and modified by rust-lang#124255.

- The condition bitmap variables are now created eagerly at the start of per-function codegen, via a new `init_coverage` method in `CoverageInfoBuilderMethods`. This avoids having to retroactively create the bitmaps while doing codegen for an individual coverage statement.
- As a result, we can now create and initialize those bitmaps using existing safe APIs, instead of having to perform our own unsafe call to `llvm::LLVMBuildAlloca`.
- This PR also tweaks the way we count the number of condition bitmaps needed, by tracking the total number of bitmaps needed (max depth + 1), instead of only tracking the maximum depth. This reduces the potential for subtle off-by-one confusion.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 3, 2024
Rollup merge of rust-lang#124555 - Zalathar:init-coverage, r=nnethercote

coverage: Clean up creation of MC/DC condition bitmaps

This PR improves the code for creating and initializing [MC/DC](https://en.wikipedia.org/wiki/Modified_condition/decision_coverage) condition bitmap variables, as introduced by rust-lang#123409 and modified by rust-lang#124255.

- The condition bitmap variables are now created eagerly at the start of per-function codegen, via a new `init_coverage` method in `CoverageInfoBuilderMethods`. This avoids having to retroactively create the bitmaps while doing codegen for an individual coverage statement.
- As a result, we can now create and initialize those bitmaps using existing safe APIs, instead of having to perform our own unsafe call to `llvm::LLVMBuildAlloca`.
- This PR also tweaks the way we count the number of condition bitmaps needed, by tracking the total number of bitmaps needed (max depth + 1), instead of only tracking the maximum depth. This reduces the potential for subtle off-by-one confusion.
bors added a commit to rust-lang-ci/rust that referenced this pull request May 5, 2024
coverage: Split out MC/DC mappings from `BcbMappingKind`

These variants were added to `BcbMappingKind` as part of the [MC/DC coverage](https://en.wikipedia.org/wiki/Modified_Condition/Decision_Coverage) implementation in rust-lang#123409, because that was the path-of-least-resistance for integrating them into the existing code.

However, they ultimately represent complex concepts that the enum was not intended to handle, leading to more complexity in the code that processes them. This PR therefore follows in the footsteps of rust-lang#124545, and splits the MC/DC mappings out into their own dedicated vectors of structs.

After that, `BcbMappingKind` itself ends up having only one variant (`Code`), so this PR also flattens that enum into its enclosing struct, renamed to `mapping::CodeMapping`.

---

No functional changes.

This will conflict slightly with rust-lang#124571, but hopefully that should be easy to resolve either way.

`@rustbot` label +A-code-coverage
@ZhuUx ZhuUx deleted the master branch October 8, 2024 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.