Split mcdc code to a sub module of coverageinfo #124399

ZhuUx · 2024-04-26T10:52:29Z

A further work from #124217 . I have made relatively large changes when working on #124278 so that it would better split them from coverageinfo.rs to avoid potential troubling merge work with improved branch coverage by @Zalathar .

Besides BlockMarkerGenerator is added to avoid ownership problems (mostly needed for following change of #124278 )

All code changes are done in a37d737a while the second commit just renames the file.

cc @RenjiSann @Zalathar
This will impact your current work.

rustbot · 2024-04-26T10:52:37Z

r? @estebank

rustbot has assigned @estebank.
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 · 2024-04-26T10:52:38Z

Some changes occurred in coverage instrumentation.

cc @Zalathar

ZhuUx · 2024-04-26T10:52:47Z

r? @oli-obk

Zalathar · 2024-04-26T11:23:51Z

Hmm, I think I would prefer to leave the main file as coverageinfo.rs, without changing it to coverageinfo/mod.rs.

Moving the MC/DC code into a subdirectory is still fine, but we should consider delaying this change until after I've had a chance to review and merge #124255, to avoid disturbing that work too much.

oli-obk · 2024-04-26T11:32:54Z

@bors delegate=Zalathar

bors · 2024-04-26T11:32:57Z

✌️ @Zalathar, you can now approve this pull request!

If @oli-obk told you to "r=me" after making some further change, please make that change, then do @bors r=@oli-obk

ZhuUx · 2024-04-26T11:35:53Z

Hmm, I think I would prefer to leave the main file as coverageinfo.rs, without changing it to coverageinfo/mod.rs.

I'm not sure whether rustc accept the new module style so I kept it same with other modules. If could the second commit can be dropped.

Moving the MC/DC code into a subdirectory is still fine, but we should consider delaying this change until after I've had a chance to review and merge #124255, to avoid disturbing that work too much.

Sounds fair.

rustbot · 2024-04-29T08:25:12Z

Some changes occurred in coverage instrumentation.

cc @Zalathar

Some changes occurred in coverage instrumentation.

cc @Zalathar

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in coverage instrumentation.

cc @Zalathar

Some changes occurred in match lowering

cc @Nadrieril

Some changes occurred in coverage tests.

cc @Zalathar

bors · 2024-04-29T14:55:31Z

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

Zalathar · 2024-04-30T01:58:34Z

Would it be reasonable to split this into two commits, so that the first commit adds BlockMarkerGen (and maybe makes other adjustments) without moving anything, and the second commit only moves code without modifying anything?

(Similar to what I've done in #124545, by having a preparation commit first, and then a movement-only commit second.)

ZhuUx · 2024-04-30T01:59:45Z

Would it be reasonable to split this into two commits, so that the first commit adds BlockMarkerGen (and maybe makes other adjustments) without moving anything, and the second commit only moves code without modifying anything?

(Similar to what I've done in #124545, by having a preparation commit first, and then a movement-only commit second.)

Sure will do

Zalathar · 2024-04-30T02:00:20Z

compiler/rustc_mir_build/src/build/coverageinfo.rs


 use crate::build::{Builder, CFG};
-use crate::errors::MCDCExceedsConditionNumLimit;
+use mcdc::MCDCInfoBuilder;


Nit: Please make this use crate::build::coverageinfo::mcdc::MCDCInfoBuilder, so that all the same-crate imports are consistent.

(I don't think there's an official policy for this formatting, but personally I prefer the consistent style.)

Zalathar · 2024-04-30T02:54:29Z

compiler/rustc_mir_build/src/build/coverageinfo.rs

-        tcx.sess
-            .instrument_coverage_mcdc()
-            .then(|| Self { decision_ctx_stack: vec![MCDCDecisionCtx::default()] })
+    fn init() -> Self {


Nit: This should just be new().

Zalathar · 2024-04-30T03:07:51Z

I have some suggestions for renaming things. These are just suggestions, so you might disagree.

block_marker_gen can be markers (but BlockMarkerGen is fine)
mcdc_builder can be mcdc (but MCDCInfoBuilder is fine)

The reason for these suggestions is that I think the shorter variable names make the code easier to read, and the longer names don't add much useful information.

What do you think?

ZhuUx · 2024-04-30T03:17:22Z

I have some suggestions for renaming things. These are just suggestions, so you might disagree.
* `block_marker_gen` can be `markers` (but `BlockMarkerGen` is fine)

* `mcdc_builder` can be `mcdc` (but `MCDCInfoBuilder` is fine)
The reason for these suggestions is that I think the shorter variable names make the code easier to read, and the longer names don't add much useful information.

What do you think?

Agree that markers is better. While I'd prefer to mcdc_builder or mcdc_info but not just mcdc because mcdc might be too short to indicate what the field does for mcdc.

Zalathar · 2024-04-30T03:31:21Z

compiler/rustc_mir_build/src/build/coverageinfo.rs

+        if let Some(mcdc_info) = self
+            .coverage_branch_info
+            .as_mut()
+            .and_then(|branch_info| branch_info.mcdc_info.as_mut())
        {
-            mcdc_state.record_conditions(logical_op, span);
+            mcdc_info.state.record_conditions(logical_op, span);
        }
    }

    pub(crate) fn mcdc_increment_depth_if_enabled(&mut self) {
-        if let Some(branch_info) = self.coverage_branch_info.as_mut()
-            && let Some(mcdc_state) = branch_info.mcdc_state.as_mut()
+        if let Some(mcdc_info) = self
+            .coverage_branch_info
+            .as_mut()
+            .and_then(|branch_info| branch_info.mcdc_info.as_mut())


I think using let chains (&& let) here is clearer than using and_then.

Zalathar · 2024-04-30T03:44:21Z

Typo in commit name:

Add MCDCInfoBuilder to isolate all mcdc staff from BranchInfoBuilder
- “staff” → “stuff”

Zalathar · 2024-04-30T03:45:46Z

I'm not convinced that changing things from .expect(..) to bug! is an improvement, but I think I won't worry about that, because it's pretty minor and I don't want to keep asking you to make small changes that require you to re-do the split.

…foBuilder

ZhuUx · 2024-04-30T04:24:23Z

I'm not convinced that changing things from .expect(..) to bug! is an improvement, but I think I won't worry about that, because it's pretty minor and I don't want to keep asking you to make small changes that require you to re-do the split.

I change expect to bug! because it is not very obvious that the decision_ctx_stack is always non empty when needed. I want to report it with bug! to get a nicer output in case the invariant were broken with following development. We could change it back to expect if mcdc is stable to some extent.

Zalathar · 2024-04-30T05:07:08Z

OK, this looks good.

@bors r=Zalathar

bors · 2024-04-30T05:07:11Z

📌 Commit 6c8b492 has been approved by Zalathar

It is now in the queue for this repository.

Zalathar · 2024-04-30T05:08:48Z

@rustbot label +A-code-coverage

bors · 2024-04-30T09:03:59Z

⌛ Testing commit 6c8b492 with merge 47314eb...

bors · 2024-04-30T11:07:05Z

☀️ Test successful - checks-actions
Approved by: Zalathar
Pushing 47314eb to master...

rust-timer · 2024-04-30T12:23:32Z

Finished benchmarking commit (47314eb): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

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

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: 672.74s -> 672.522s (-0.03%)
Artifact size: 316.01 MiB -> 315.99 MiB (-0.00%)

rustbot assigned estebank Apr 26, 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 26, 2024

rustbot assigned davidtwco, oli-obk and lcnr and unassigned estebank, davidtwco, oli-obk and lcnr Apr 26, 2024

This was referenced Apr 28, 2024

Support mcdc analysis for pattern matching #124278

Draft

MCDC coverage: support nested decision coverage #124255

Merged

ZhuUx force-pushed the split-mcdc branch from ad1e629 to e7b9327 Compare April 29, 2024 08:25

ZhuUx force-pushed the split-mcdc branch from e7b9327 to 378e3fd Compare April 29, 2024 08:44

ZhuUx force-pushed the split-mcdc branch 2 times, most recently from d42431b to 035e0a8 Compare April 30, 2024 01:46

Zalathar reviewed Apr 30, 2024

View reviewed changes

ZhuUx force-pushed the split-mcdc branch from 035e0a8 to 18a1d1f Compare April 30, 2024 02:29

Zalathar reviewed Apr 30, 2024

View reviewed changes

ZhuUx force-pushed the split-mcdc branch from 18a1d1f to b33d220 Compare April 30, 2024 03:06

coverage. Add BlockMarkerGen to avoid ownership gymnastics

a76e250

ZhuUx force-pushed the split-mcdc branch from b33d220 to 16b2506 Compare April 30, 2024 03:26

Zalathar reviewed Apr 30, 2024

View reviewed changes

ZhuUx added 2 commits April 30, 2024 12:17

coverage. Add MCDCInfoBuilder to isolate all mcdc stuff from BranchIn…

e198c51

…foBuilder

coverage. Split mcdc builder to a sub module of coverageinfo

6c8b492

ZhuUx force-pushed the split-mcdc branch from 16b2506 to 6c8b492 Compare April 30, 2024 04:18

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 30, 2024

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

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 30, 2024

bors merged commit 47314eb into rust-lang:master Apr 30, 2024
10 checks passed

rustbot added this to the 1.80.0 milestone Apr 30, 2024

This was referenced Apr 30, 2024

coverage: Treat each match arm as a "branch" for branch coverage #124154

Draft

coverage: Branch coverage support for let-else and if-let #124223

Merged

RenjiSann mentioned this pull request Apr 30, 2024

Coverage: Add condition coverage #124402

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split mcdc code to a sub module of coverageinfo #124399

Split mcdc code to a sub module of coverageinfo #124399

ZhuUx commented Apr 26, 2024 •

edited

rustbot commented Apr 26, 2024

rustbot commented Apr 26, 2024

ZhuUx commented Apr 26, 2024 •

edited

Zalathar commented Apr 26, 2024

oli-obk commented Apr 26, 2024

bors commented Apr 26, 2024

ZhuUx commented Apr 26, 2024 •

edited

rustbot commented Apr 29, 2024

bors commented Apr 29, 2024

Zalathar commented Apr 30, 2024

ZhuUx commented Apr 30, 2024

Zalathar Apr 30, 2024

Zalathar Apr 30, 2024

Zalathar commented Apr 30, 2024

ZhuUx commented Apr 30, 2024

Zalathar Apr 30, 2024

Zalathar commented Apr 30, 2024

Zalathar commented Apr 30, 2024

ZhuUx commented Apr 30, 2024 •

edited

Zalathar commented Apr 30, 2024

bors commented Apr 30, 2024

Zalathar commented Apr 30, 2024

bors commented Apr 30, 2024

bors commented Apr 30, 2024

rust-timer commented Apr 30, 2024

Split mcdc code to a sub module of coverageinfo #124399

Split mcdc code to a sub module of coverageinfo #124399

Conversation

ZhuUx commented Apr 26, 2024 • edited

rustbot commented Apr 26, 2024

rustbot commented Apr 26, 2024

ZhuUx commented Apr 26, 2024 • edited

Zalathar commented Apr 26, 2024

oli-obk commented Apr 26, 2024

bors commented Apr 26, 2024

ZhuUx commented Apr 26, 2024 • edited

rustbot commented Apr 29, 2024

bors commented Apr 29, 2024

Zalathar commented Apr 30, 2024

ZhuUx commented Apr 30, 2024

Zalathar Apr 30, 2024

Choose a reason for hiding this comment

Zalathar Apr 30, 2024

Choose a reason for hiding this comment

Zalathar commented Apr 30, 2024

ZhuUx commented Apr 30, 2024

Zalathar Apr 30, 2024

Choose a reason for hiding this comment

Zalathar commented Apr 30, 2024

Zalathar commented Apr 30, 2024

ZhuUx commented Apr 30, 2024 • edited

Zalathar commented Apr 30, 2024

bors commented Apr 30, 2024

Zalathar commented Apr 30, 2024

bors commented Apr 30, 2024

bors commented Apr 30, 2024

rust-timer commented Apr 30, 2024

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

ZhuUx commented Apr 26, 2024 •

edited

ZhuUx commented Apr 26, 2024 •

edited

ZhuUx commented Apr 26, 2024 •

edited

ZhuUx commented Apr 30, 2024 •

edited