Implement Modified Condition/Decision Coverage by ZhuUx · Pull Request #123409 · rust-lang/rust

ZhuUx · 2024-04-03T13:00:36Z

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

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.
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.
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 · 2024-04-03T13:00:45Z

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 · 2024-04-03T13:00:46Z

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

ZhuUx · 2024-04-03T13:11:02Z

r? rust-lang/compiler

ZhuUx · 2024-04-03T13:12:22Z

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

Nadrieril · 2024-04-03T14:56:23Z

r? compiler

Zalathar · 2024-04-04T00:01:48Z

@rustbot label +A-code-coverage

compiler/rustc_llvm/llvm-wrapper/CoverageMappingWrapper.cpp

src/tools/coverage-dump/src/covfun.rs

tests/coverage/mcdc_if.coverage

compiler/rustc_session/src/options.rs

src/tools/coverage-dump/src/covfun.rs

compiler/rustc_mir_transform/src/coverage/spans.rs

rustbot · 2024-04-04T05:58:09Z

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

Some changes occurred in src/tools/compiletest

cc @jieyouxu

rust-log-analyzer · 2024-04-20T08:48:32Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

  SDKROOT: /Applications/Xcode_14.3.1.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.3.sdk
  AR: ar
##[endgroup]
curl: (28) Operation too slow. Less than 100 bytes/sec transferred the last 5 seconds
Error: Failure while executing; `/usr/bin/env /usr/local/Homebrew/Library/Homebrew/shims/shared/curl --disable --cookie /dev/null --globoff --user-agent Homebrew/4.2.18\ \(Macintosh\;\ Intel\ Mac\ OS\ X\ 13.6.6\)\ curl/8.4.0 --header Accept-Language:\ en --fail --progress-bar --silent --remote-time --output /Users/runner/Library/Caches/Homebrew/api/formula.jws.json --location --disable --cookie /dev/null --globoff --show-error --user-agent Homebrew/4.2.18\ \(Macintosh\;\ Intel\ Mac\ OS\ X\ 13.6.6\)\ curl/8.4.0 --header Accept-Language:\ en --fail --progress-bar --silent --compressed --speed-limit 100 --speed-time 5 https://formulae.brew.sh/api/formula.jws.json` exited with 28. Here's the output:

##[error]Process completed with exit code 1.
Post job cleanup.
[command]/usr/local/bin/git version

Zalathar · 2024-04-20T08:52:12Z

@bors retry (flaky ci)

…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

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 · 2024-04-21T02:10:05Z

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.

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

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

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

…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.

…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.

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

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

…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`

rustbot assigned oli-obk Apr 3, 2024

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

This comment has been minimized.

Sign in to view

rustbot assigned Nadrieril and unassigned oli-obk Apr 3, 2024

ZhuUx mentioned this pull request Apr 3, 2024

Add support for branch coverage in source-based coverage #79649

Open

Zalathar mentioned this pull request Apr 3, 2024

Support MCDC Coverage with LLVM backend. #123358

Closed

rustbot assigned compiler-errors and unassigned Nadrieril Apr 3, 2024

Zalathar mentioned this pull request Apr 4, 2024

Manually run clang-format on CoverageMappingWrapper.cpp #123437

Merged

rustbot added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Apr 4, 2024