Support MCDC Coverage with LLVM backend. #123358
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Nadrieril (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 (
|
…ning of a function
… upon MCDCBitmapRequire
…onditionEntry/Output markers
rustbot
added
the
T-compiler
This comment has been minimized.
This comment has been minimized.
|
Not at all my expertise, let's ask someone else for feedback r? compiler |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
… in MappingKind and BcbMappingKind
|
See also #123409, which is a different PR for MCDC coverage. I haven’t had a chance to look at either of these in detail yet, but naturally there seems to be a lot of overlap. |
|
@rustbot label +A-code-coverage |
rustbot
added
the
A-code-coverage
| extern "C" uint32_t LLVMRustCoverageMappingVersion() { | ||
| return coverage::CovMapVersion::Version6; | ||
| return coverage::CovMapVersion::Version7; | ||
| } |
There was a problem hiding this comment.
i didn't know about this MR, and I was skeptical about hardcoding the number in this manner.
Looks like my doubts were indeed justified. Thanks !
| /// Marks the first condition of a decision (boolean expression). All | ||
| /// conditions in the same decision will reference this id. | ||
| /// | ||
| /// Has no effect during codegen. | ||
| MCDCDecisionEntryMarker { decm_id: DecisionMarkerId }, | ||
|
|
||
| /// Marks one of the basic blocks following the decision referenced with `id`. | ||
| /// the outcome bool designates which branch of the decision it is: | ||
| /// `true` for the then block, `false` for the else block. | ||
| /// | ||
| /// Has no effect during codegen. | ||
| MCDCDecisionOutputMarker { decm_id: DecisionMarkerId, outcome: bool }, | ||
|
|
||
| /// Marks a basic block where a condition evaluation occurs | ||
| /// The block may end with a SwitchInt where the 2 successors BBs have a | ||
| /// MCDCConditionOutcomeMarker statement with a matching ID. | ||
| /// | ||
| /// Has no effect during codegen. | ||
| MCDCConditionEntryMarker { decm_id: DecisionMarkerId, condm_id: ConditionMarkerId }, | ||
|
|
||
| /// Marks a basic block that is branched from a condition evaluation. | ||
| /// The block may have a predecessor with a matching ID. | ||
| /// | ||
| /// Has no effect during codegen. | ||
| MCDCConditionOutputMarker { | ||
| decm_id: DecisionMarkerId, | ||
| condm_id: ConditionMarkerId, | ||
| outcome: bool, | ||
| }, | ||
|
|
There was a problem hiding this comment.
We shouldn't need all these extra kinds of marker statement.
It should be possible to use the existing BlockMarker to mark MIR blocks, and then have MC/DC information in a side table that refers to those blocks by their BlockMarkerId.
|
It looks like #123409 is more complete than this draft, since it is able to run tests, and probably needs fewer big changes to become ready. I feel bad about having to say this, but I think it makes sense to focus our attention on improving that PR. What do you think? (In any case, thanks for your efforts so far. I'm happy to see that my work over the last few months has made this sort of thing possible.) |
I think the same way. we should concentrate our work on the most complete implementation. I will make my best to provide review to #123409 . Thank you for your review and frankness. |
| // FFI equivalent of struct `llvm::coverage::CounterMappingRegion::MCDCParameters` | ||
| // https://github.com/rust-lang/llvm-project/blob/66a2881a/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h#L253-L263 | ||
| struct LLVMRustMCDCParameters { | ||
| /// Byte Index of Bitmap Coverage Object for a Decision Region. | ||
| uint32_t BitmapIdx = 0; | ||
|
|
||
| /// Number of Conditions used for a Decision Region. | ||
| uint32_t NumConditions = 0; | ||
|
|
||
| /// IDs used to represent a branch region and other branch regions | ||
| /// evaluated based on True and False branches. | ||
| uint32_t ID = 0; | ||
| uint32_t TrueID = 0; | ||
| uint32_t FalseID = 0; | ||
| }; | ||
|
|
||
| static coverage::CounterMappingRegion::MCDCParameters | ||
| fromRust(LLVMRustMCDCParameters MCDCParams) { | ||
| return coverage::CounterMappingRegion::MCDCParameters{ | ||
| MCDCParams.BitmapIdx, MCDCParams.NumConditions, | ||
| MCDCParams.ID, MCDCParams.TrueID, MCDCParams.FalseID | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
This is a good example of how we should be passing data from Rust to C++. I think something like this is what we should be using in #123409.
However, the fields should be declared as unsigned, so that they exactly match the type declarations that LLVM uses in MCDCParameters. And the corresponding struct on the Rust side should use libc::c_uint to match.
That makes it easier to verify that the FFI mapping has been done correctly.
|
Closing this, as #123409 is more advanced |
A few months ago, MCDC coverage was introduced in Clang/LLVM by @evodius96.
Also, @Zalathar recently merged branch coverage in Rustc in #122322 .
This PR is about supporting MCDC coverage thanks to the existing utilities in LLVM.
It is still in draft stage and not yet functional.
I will gladly take in any remark and suggestion about my changes (this is my first contribution).
Regards,