[llvm] [coverage] keep relative order of mcdc branch and decision mapping regions (PR #91600)

via llvm-commits llvm-commits at lists.llvm.org
Thu May 9 07:38:14 PDT 2024


https://github.com/Lambdaris created https://github.com/llvm/llvm-project/pull/91600

This patch fixes possible exceptions due to processing mcdc coverage code from rust with [nested decisions](https://github.com/rust-lang/rust/pull/124255).

In rust now it supports nested decisions such as 
```rust
fn inner_decision(a: bool) -> bool {
    !a
}

fn foo(a: bool, b: bool, c: bool, d: bool) {
    if inner_decision(a || b || c) && d {
        println!("true");
    }
}

fn main() {
    foo(true, false, true, false);
}
```
But this example could make llvm-cov panic. We can reproduce it with:
```bash
# Ensure installed latest nightly rust
cargo +nightly rustc --bin foo -- -Cinstrument-coverage -Zcoverage-options=mcdc
LLVM_PROFILE_FILE="foo.profraw" target/debug/foo
llvm-profdata merge -sparse foo.profraw -o foo.profdata
llvm-cov show target/debug/foo -instr-profile="foo.profdata" --show-mcdc
```

For now llvm-cov could generate wrong result with such kind of code when:
* Some conditions of the nested decision are located in front of at least one condition of the outer.
* The nested decision's start location is behind of the outer decision.

What's more , it would panic when:
* The nested decision has more conditions than the outer.

Let's consider that example still. The example code generate 2 decision:
* D1: the nested decision, comes from `a || b || c`, with 3 conditions `a`, `b`, `c`.
* D2: the outer decision, comes from `inner_decision(a || b || c) && d` , with 2 conditions `inner_decision(a || b || c)`, `d`.

After the sort, the order of mappings would become:
Conditions: `inner_decision(a || b || c)`, `a`, `b`, `c`, `d`
Decisions:   D2, D1
due to the start location comparison.
Then llvm-cov reconstructs mapping between decisions and conditions like:
1. Try to bind `inner_decision(a || b || c)` with D2, ok. -> right
2. Try to bind `a` with D2, but D2 already has a condition with id 1. So bind `a` with D1, ok -> right
3. Try to bind `b` with D2, clearly span of D2 dominates `b`, thus ok -> wrong, `b` should be a condition of D1.

Note that `b` has a false next condition id 3 and is taken as condition of D2. Hence when llvm-cov tries to construct the decision tree of D2, it attempts to visit the third element of a vector with length 2, leading to boom.

Though we can forbid people to write code in such style, macros in rust also could generate code like it, even something more dreadful like `if if a || b || c { true } else { false } && d`.

What's more important, we persist the nested decision implementation because rust has pattern matching and there might be some code like 
```rust
// let-chains
if let Ok(Some(IpAddr::V4(addr)) = ip && let Some(size) = val { /*...*/ }
```
Here `Ok(Some(IpAddr::V4(addr))` could generate a mcdc decision because it also leads some branches in CFG.

The most painless way to fix it might keep the relative order of branch mappings and decision mappins, so that llvm-cov can regroup them in same order.

>From e469a3ebc8708fce8a129187862a7f5613bd58d6 Mon Sep 17 00:00:00 2001
From: Lambdaris <lambdaris at outlook.com>
Date: Thu, 9 May 2024 21:35:33 +0800
Subject: [PATCH] [coverage] keep relative order of mcdc branch and decision
 mapping regions

---
 .../lib/ProfileData/Coverage/CoverageMappingWriter.cpp | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp b/llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp
index 5036bde5aca72..c58fc9a46f57d 100644
--- a/llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp
+++ b/llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp
@@ -165,7 +165,15 @@ void CoverageMappingWriter::write(raw_ostream &OS) {
                                        const CounterMappingRegion &RHS) {
     if (LHS.FileID != RHS.FileID)
       return LHS.FileID < RHS.FileID;
-    if (LHS.startLoc() != RHS.startLoc())
+
+    auto ignoreLocationComparison = [](const CounterMappingRegion &LHS,
+                                       const CounterMappingRegion &RHS) {
+      return (LHS.Kind == CounterMappingRegion::MCDCBranchRegion ||
+              LHS.Kind == CounterMappingRegion::MCDCDecisionRegion) &&
+             LHS.Kind == RHS.Kind;
+    };
+
+    if (!ignoreLocationComparison(LHS, RHS) && LHS.startLoc() != RHS.startLoc())
       return LHS.startLoc() < RHS.startLoc();
 
     // Put `Decision` before `Expansion`.



More information about the llvm-commits mailing list