MCDC coverage: support nested decision coverage

[RenjiSann]

#123409 provided the initial MCDC coverage implementation.

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

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_bitmaps as necessary to handle the maximum encountered decision depth.

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cjgillot (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]

Some changes occurred in coverage instrumentation.

cc @Zalathar

Some changes occurred in match lowering

cc @Nadrieril

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

[RenjiSann]

@rustbot label +A-code-coverage

[oli-obk]

@bors delegate=Zalathar

[bors]

✌️ @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

[RenjiSann]

force pushed formatting

[bors]

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

[RenjiSann]

r? @Zalathar

[rustbot]

Failed to set assignee to Zalathar: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

[RenjiSann]

rebased on top of #124217

[Lambdaris]

The assignee has announced Zalathar as his delegate. So you could @ him once you prepared and he has full authority to approve this pr.

[RenjiSann]

I have rebased #124399 to this and fixed known issues in compiler/rustc_mir_build/src/build/coverageinfo.rs. Still you can choose your way to fix that and I will follow it at that pr.

Thank you !
If it is going to be refactored anyway, I'd prefer to let it like this for now.

[Zalathar]

@RenjiSann Do you still have more tweaks to make, or are you ready for me to have another look at this?

[RenjiSann]

@RenjiSann Do you still have more tweaks to make, or are you ready for me to have another look at this?

@Zalathar Nothing to add for now, you can take another look :)

[Zalathar]

I think this should actually be 0, not 1. But since this is temporary code anyway, and the only effect is an extra unused bitmap, it's OK to not worry about it.

[Zalathar]

There are a couple of things I would like to see improved (e.g. in the handling of max_decision_depth) ,but they're all minor enough that I think it's better to move forwards and get this merged as-is, and then consider some follow-up changes later.

[Zalathar]

(Just be aware that I have a few follow-up things planned after #123409 and this, and #124399 is also waiting on this. So I think any major changes after this one should wait until after we've had a chance to do some cleanup steps.)

[Zalathar]

@bors r=@oli-obk

[bors]

📌 Commit eb422d5 has been approved by oli-obk

It is now in the queue for this repository.

[Zalathar]

@RenjiSann Oh, another thing I want to mention is that if you're adding tests for changed behaviour, it's often a good idea to add and bless the tests before making the changes, and then bless them again in later commits, so that it's easy to see the exact before/after changes in test output.

(In some cases this doesn't work, because the compiler would error or crash before the changes are made. But it's a good habit to get into.)

[bors]

⌛ Testing commit eb422d5 with merge 56e04c5...

[oli-obk]

@bors r=Zalathar

[bors]

💡 This pull request was already approved, no need to approve it again.

• This pull request is currently being tested. If there's no response from the continuous integration service, you may use retry to trigger a build again.

[bors]

📌 Commit eb422d5 has been approved by Zalathar

It is now in the queue for this repository.

[bors]

⌛ Testing commit eb422d5 with merge 7a58674...

[rust-log-analyzer]

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

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

[RenjiSann]

@RenjiSann Oh, another thing I want to mention is that if you're adding tests for changed behaviour, it's often a good idea to add and bless the tests before making the changes, and then bless them again in later commits, so that it's easy to see the exact before/after changes in test output.

(In some cases this doesn't work, because the compiler would error or crash before the changes are made. But it's a good habit to get into.)

Will do ! thanks !

[bors]

☀️ Test successful - checks-actions
Approved by: Zalathar
Pushing 7a58674 to master...

[rust-timer]

Finished benchmarking commit (7a58674): 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)	2.0%	[2.0%, 2.0%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.0%	[2.0%, 2.0%]	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.94s -> 671.855s (-0.16%)
Artifact size: 315.94 MiB -> 315.98 MiB (0.01%)