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

[Coverage][MCDC] Prepare MCDC Builder for pattern matching and llvm 19 #126677

Closed
wants to merge 3 commits into from

Conversation

ZhuUx
Copy link
Contributor

@ZhuUx ZhuUx commented Jun 19, 2024

Prepare for pattern matching decisions and adaption to llvm-19.

Changes

  1. MCDCBuilder is refactored. The main change is the way to calculate depth of decisions and that DecisionCtx is responsible to process decisions (for now we have just BooleanDecisionCtx, while in Support mcdc analysis for pattern matching #124278 we introduce MatchingDecisionCtx). We use MCDCState to store context processing decisions. If a nested decision is to be introduced, the parent's state will be "stashed" and a new state will be created for the nested decision. Once the nested decision is finished the parent's state is "unstashed" and states for nested decisions will be removed later. The new mechanism can fix that nested decisions in top-level boolean expressions like a || func(b && c) can not be processed properly by current nightly compiler, as tests in nested_in_boolean_exprs.rs.
  2. MCDCBranchSpan and MCDCBranch can hold multiple true markers or false markers. This is for matching patterns.
  3. MCDC spans are grouped by decisions till instrument, in case some branches could not generate mappings but decision mappings still count them.

@rustbot
Copy link
Collaborator

rustbot commented Jun 19, 2024

r? @TaKO8Ki

rustbot has assigned @TaKO8Ki.
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

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 19, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jun 19, 2024

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in coverage tests.

cc @Zalathar

Some changes occurred in coverage instrumentation.

cc @Zalathar

Some changes occurred in coverage instrumentation.

cc @Zalathar

Some changes occurred in coverage instrumentation.

cc @Zalathar

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 19, 2024
@ZhuUx
Copy link
Contributor Author

ZhuUx commented Jun 19, 2024

cc @RenjiSann

@Zalathar
Copy link
Contributor

@rustbot label +A-code-coverage

@rustbot rustbot added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Jun 19, 2024
@Zalathar
Copy link
Contributor

By the way, I think the MC/DC limit warnings should probably be lints instead of ordinary warnings, so that they interact with the #[allow(..)] and #[deny(..)] attributes.

I don't think that concern should block this PR, but we should remember to fix it at some point.

@ZhuUx
Copy link
Contributor Author

ZhuUx commented Jun 19, 2024

By the way, I think the MC/DC limit warnings should probably be lints instead of ordinary warnings, so that they interact with the #[allow(..)] and #[deny(..)] attributes.

I don't think that concern should block this PR, but we should remember to fix it at some point.

That's meaningful. #82448 also introduces two options to control the limit. I think we can fix that with these options once llvm-19 is released.

@bors
Copy link
Contributor

bors commented Jun 30, 2024

☔ The latest upstream changes (presumably #127174) made this pull request unmergeable. Please resolve the merge conflicts.

@ZhuUx ZhuUx force-pushed the prepare-pattern-matching branch from 8712975 to 61b608c Compare July 3, 2024 08:35
@rustbot
Copy link
Collaborator

rustbot commented Jul 3, 2024

Some changes occurred in match lowering

cc @Nadrieril

@ZhuUx
Copy link
Contributor Author

ZhuUx commented Jul 4, 2024

Some commits are shared with #127234 now. I wish to merge that first.

@bors
Copy link
Contributor

bors commented Jul 5, 2024

☔ The latest upstream changes (presumably #127360) made this pull request unmergeable. Please resolve the merge conflicts.

@ZhuUx ZhuUx force-pushed the prepare-pattern-matching branch 2 times, most recently from a49e7b2 to becc530 Compare July 10, 2024 02:56
@ZhuUx
Copy link
Contributor Author

ZhuUx commented Jul 10, 2024

Since #127234 has been merged we can work on this, which lays down the base for #126733 and #124278

@ZhuUx
Copy link
Contributor Author

ZhuUx commented Jul 15, 2024

@Zalathar @TaKO8Ki This pr is prepared. Could we start reviewing?

&hir_info,
&mut extracted_mappings,
&basic_coverage_blocks,
&mut coverage_counters,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been thinking about this &mut coverage_counters, because I did the same thing in my draft of #124154, but now I think it's the wrong approach.

Instead, I think we should try to teach CoverageCounters::make_bcb_counters how to create the complex expressions that will be needed later, so that all the counter-creation happens together.

Copy link
Contributor Author

@ZhuUx ZhuUx Jul 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then some keys may be essential to mappings so that we can get these sum expressions from coverage_counters later, sounds a bit redundant.
Currently we only need to create Expression but not Counter here. What are the drawbacks of the current approach?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some more thought, I think it's OK to do this for now.

I still have mixed feelings about it, but I don't know what the specific alternative should look like, and I don't think this change should have to wait for me to figure that out.

@ZhuUx
Copy link
Contributor Author

ZhuUx commented Jul 15, 2024

I would add commit for each fix. After all are done I will squash them into the primary commits (the target commits are hinted at head of the message)

@rust-log-analyzer

This comment has been minimized.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 15, 2024
coverage: Restrict `ExpressionUsed` simplification to `Code` mappings

In the future, branch and MC/DC mappings might have expressions that don't correspond to any single point in the control-flow graph. That makes it trickier to keep track of which expressions should expect an `ExpressionUsed` node.

We therefore sidestep that complexity by only performing `ExpressionUsed` simplification for expressions associated directly with ordinary `Code` mappings.

(This simplification step is inherited from the original coverage implementation, which only supported `Code` mappings anyway, so there's no particular reason to extend it to other kinds of mappings unless we specifically choose to.)

Relevant to:
- rust-lang#124154
- rust-lang#126677
- rust-lang#124278

`@rustbot` label +A-code-coverage
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 15, 2024
coverage: Restrict `ExpressionUsed` simplification to `Code` mappings

In the future, branch and MC/DC mappings might have expressions that don't correspond to any single point in the control-flow graph. That makes it trickier to keep track of which expressions should expect an `ExpressionUsed` node.

We therefore sidestep that complexity by only performing `ExpressionUsed` simplification for expressions associated directly with ordinary `Code` mappings.

(This simplification step is inherited from the original coverage implementation, which only supported `Code` mappings anyway, so there's no particular reason to extend it to other kinds of mappings unless we specifically choose to.)

Relevant to:
- rust-lang#124154
- rust-lang#126677
- rust-lang#124278

``@rustbot`` label +A-code-coverage
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 15, 2024
coverage: Restrict `ExpressionUsed` simplification to `Code` mappings

In the future, branch and MC/DC mappings might have expressions that don't correspond to any single point in the control-flow graph. That makes it trickier to keep track of which expressions should expect an `ExpressionUsed` node.

We therefore sidestep that complexity by only performing `ExpressionUsed` simplification for expressions associated directly with ordinary `Code` mappings.

(This simplification step is inherited from the original coverage implementation, which only supported `Code` mappings anyway, so there's no particular reason to extend it to other kinds of mappings unless we specifically choose to.)

Relevant to:
- rust-lang#124154
- rust-lang#126677
- rust-lang#124278

```@rustbot``` label +A-code-coverage
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jul 15, 2024
Rollup merge of rust-lang#127758 - Zalathar:expression-used, r=oli-obk

coverage: Restrict `ExpressionUsed` simplification to `Code` mappings

In the future, branch and MC/DC mappings might have expressions that don't correspond to any single point in the control-flow graph. That makes it trickier to keep track of which expressions should expect an `ExpressionUsed` node.

We therefore sidestep that complexity by only performing `ExpressionUsed` simplification for expressions associated directly with ordinary `Code` mappings.

(This simplification step is inherited from the original coverage implementation, which only supported `Code` mappings anyway, so there's no particular reason to extend it to other kinds of mappings unless we specifically choose to.)

Relevant to:
- rust-lang#124154
- rust-lang#126677
- rust-lang#124278

```@rustbot``` label +A-code-coverage
@rust-log-analyzer

This comment has been minimized.

@Zalathar
Copy link
Contributor

It's not clear to me which parts of this are for LLVM 19, which parts are for pattern matching, and why they are combined in one PR.

Is there some reason why the changes needed for LLVM 19 can't proceed separately from the changes needed for pattern matching?

@ZhuUx
Copy link
Contributor Author

ZhuUx commented Jul 25, 2024

It's not clear to me which parts of this are for LLVM 19, which parts are for pattern matching, and why they are combined in one PR.

The change for llvm 19 is group decision and its conditions to mcdc_spans so that we can calculate index of test vectors at extract_mcdc_mappings.
(At first I do it in mcdc.rs, but after I found that for loops also generate pattern matching decisions here I moved it to extract_mcdc_mappings, where decisions from for loops are removed due to span).

Is there some reason why the changes needed for LLVM 19 can't proceed separately from the changes needed for pattern matching?

No, actually they can be separated indeed as long as we separate degraded mcdc branches from mcdc spans early, as the change introduced at the last commit of this pr. So I worked on patches for llvm 19 based this.
Maybe we could make that separately for llvm 19?

@ZhuUx
Copy link
Contributor Author

ZhuUx commented Jul 25, 2024

I have separated all work for llvm-19, this pr is no longer needed. Pattern matching will still be based on that.

@ZhuUx ZhuUx closed this Jul 25, 2024
@ZhuUx ZhuUx deleted the prepare-pattern-matching branch October 9, 2024 02:37
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) 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants