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

Rollup of 3 pull requests #124194

Merged
merged 9 commits into from
Apr 20, 2024
Merged

Rollup of 3 pull requests #124194

merged 9 commits into from
Apr 20, 2024

Conversation

matthiaskrgr
Copy link
Member

Successful merges:

r? @ghost
@rustbot modify labels: rollup

Create a similar rollup

zhuyunxing and others added 9 commits April 19, 2024 10:43
Currently, the matcher `//[rev-foo,rev-bar]~` does not get selected by
the regex. Change the matcher to also match strings that contain a `-`.h
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.
…, r=oli-obk

Fix capturing duplicated lifetimes via parent in `precise_captures` (`impl use<'...>`)

For technical reasons related to the way that `Self` and `T::Assoc` are lowered from HIR -> `rustc_middle::ty`, an opaque may mention in its bounds both the original early-bound lifetime from the parent `impl`/`fn`, *and* the *duplicated* early-bound lifetime on the opaque.

This is fine -- and has been fine since `@cjgillot` rewrote the way we handled opaque lifetime captures, and we went further to allow this behavior explicitly in rust-lang#115659. It's worthwhile to read this PR's technical section to recall how this duplication works and when it acts surprisingly.

The problem here is that the check that make sure that `impl use<'a, 'b>` lists all of the opaque's captured lifetimes wasn't smart enough to consider both these captured lifetimes and the original lifetimes they're duplicated from to be equal. This PR fixes that.

r? oli-obk
…, r=jieyouxu

Match hyphen in multi-revision comment matchers

Currently, the matcher `//[rev-foo,rev-bar]~` does not get selected by the regex. Change the matcher to include `-`.
@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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. rollup A PR which is a rollup labels Apr 20, 2024
@matthiaskrgr
Copy link
Member Author

@bors r+ rollup=never p=3

@bors
Copy link
Contributor

bors commented Apr 20, 2024

📌 Commit f58ef08 has been approved by matthiaskrgr

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors 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 Apr 20, 2024
@bors
Copy link
Contributor

bors commented Apr 20, 2024

⌛ Testing commit f58ef08 with merge 584f183...

@bors
Copy link
Contributor

bors commented Apr 20, 2024

☀️ Test successful - checks-actions
Approved by: matthiaskrgr
Pushing 584f183 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 20, 2024
@bors bors merged commit 584f183 into rust-lang:master Apr 20, 2024
13 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Apr 20, 2024
@rust-timer
Copy link
Collaborator

📌 Perf builds for each rolled up PR:

PR# Message Perf Build Sha
#123409 Implement Modified Condition/Decision Coverage e40568f2362e3156ad5bf286c168c4df3f417910 (link)
#124104 Fix capturing duplicated lifetimes via parent in `precise_c… 3dc16c1aa6eaef37e69415254fe963b5ceb71b5d (link)
#124137 Match hyphen in multi-revision comment matchers 78db328590a05241978bbe3b3f570aa90df89d5a (link)

previous master: a61b14d15e

In the case of a perf regression, run the following command for each PR you suspect might be the cause: @rust-timer build $SHA

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (584f183): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
3.3% [3.3%, 3.3%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.3% [-0.3%, -0.3%] 1
All ❌✅ (primary) - - 0

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 670.71s -> 672.18s (0.22%)
Artifact size: 315.28 MiB -> 315.24 MiB (-0.01%)

@matthiaskrgr matthiaskrgr deleted the rollup-40s0c4q branch September 1, 2024 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. rollup A PR which is a rollup 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.

6 participants