-
Notifications
You must be signed in to change notification settings - Fork 14.5k
[llvm-cov] Add gap region between binary operator '&& and ||' and RHS #149085
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
base: main
Are you sure you want to change the base?
Conversation
@llvm/pr-subscribers-pgo @llvm/pr-subscribers-clang-codegen Author: None (int-zjt) ChangesIssue SummaryWe identified an inaccuracy in line coverage reporting when short-circuit evaluation occurs in multi-line conditional expressions. Specifically:
Root Cause AnalysisThe current Proposed SolutionThe current implementation correctly handles single-statement if bodies through GapRegion insertion:
Processing workflow:
Generalize the GapRegion insertion strategy from if-statements to logical operators, placing RHS-synchronized segments at LHS-line-end to isolate coverage contexts during short-circuit evaluation. Full diff: https://github.com/llvm/llvm-project/pull/149085.diff 2 Files Affected:
diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp b/clang/lib/CodeGen/CoverageMappingGen.cpp
index 4aafac349e3e9..f55290a5feee6 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -2269,6 +2269,11 @@ struct CounterCoverageMappingBuilder
// Track LHS True/False Decision.
const auto DecisionLHS = MCDCBuilder.pop();
+ if (auto Gap =
+ findGapAreaBetween(getEnd(E->getLHS()), getStart(E->getRHS()))) {
+ fillGapAreaWithCount(Gap->getBegin(), Gap->getEnd(), getRegionCounter(E));
+ }
+
// Counter tracks the right hand side of a logical and operator.
extendRegion(E->getRHS());
propagateCounts(getRegionCounter(E), E->getRHS());
@@ -2330,6 +2335,11 @@ struct CounterCoverageMappingBuilder
// Track LHS True/False Decision.
const auto DecisionLHS = MCDCBuilder.pop();
+ if (auto Gap =
+ findGapAreaBetween(getEnd(E->getLHS()), getStart(E->getRHS()))) {
+ fillGapAreaWithCount(Gap->getBegin(), Gap->getEnd(), getRegionCounter(E));
+ }
+
// Counter tracks the right hand side of a logical or operator.
extendRegion(E->getRHS());
propagateCounts(getRegionCounter(E), E->getRHS());
diff --git a/compiler-rt/test/profile/Linux/coverage_short_circuit.cpp b/compiler-rt/test/profile/Linux/coverage_short_circuit.cpp
new file mode 100644
index 0000000000000..cc4022bc3c286
--- /dev/null
+++ b/compiler-rt/test/profile/Linux/coverage_short_circuit.cpp
@@ -0,0 +1,36 @@
+// RUN: %clangxx_profgen -std=c++17 -fuse-ld=lld -fcoverage-mapping -o %t %s
+// RUN: env LLVM_PROFILE_FILE=%t.profraw %run %t
+// RUN: llvm-profdata merge -o %t.profdata %t.profraw
+// RUN: llvm-cov show %t -instr-profile=%t.profdata 2>&1 | FileCheck %s
+
+void foo() { // CHECK: [[@LINE]]| 1|void foo() {
+ bool cond1 = false; // CHECK-NEXT: [[@LINE]]| 1| bool cond1 = false;
+ bool cond2 = true; // CHECK-NEXT: [[@LINE]]| 1| bool cond2 = true;
+ if (cond1 && // CHECK-NEXT: [[@LINE]]| 1| if (cond1 &&
+ cond2) { // CHECK-NEXT: [[@LINE]]| 0| cond2) {
+ } // CHECK-NEXT: [[@LINE]]| 0| }
+} // CHECK-NEXT: [[@LINE]]| 1|}
+
+void bar() { // CHECK: [[@LINE]]| 1|void bar() {
+ bool cond1 = true; // CHECK-NEXT: [[@LINE]]| 1| bool cond1 = true;
+ bool cond2 = false; // CHECK-NEXT: [[@LINE]]| 1| bool cond2 = false;
+ if (cond1 && // CHECK-NEXT: [[@LINE]]| 1| if (cond1 &&
+ cond2) { // CHECK-NEXT: [[@LINE]]| 1| cond2) {
+ } // CHECK-NEXT: [[@LINE]]| 0| }
+} // CHECK-NEXT: [[@LINE]]| 1|}
+
+void baz() { // CHECK: [[@LINE]]| 1|void baz() {
+ bool cond1 = false; // CHECK-NEXT: [[@LINE]]| 1| bool cond1 = false;
+ bool cond2 = true; // CHECK-NEXT: [[@LINE]]| 1| bool cond2 = true;
+ if (cond1 // CHECK-NEXT: [[@LINE]]| 1| if (cond1
+ && // CHECK-NEXT: [[@LINE]]| 0| &&
+ cond2) { // CHECK-NEXT: [[@LINE]]| 0| cond2) {
+ } // CHECK-NEXT: [[@LINE]]| 0| }
+} // CHECK-NEXT: [[@LINE]]| 1|}
+
+int main() {
+ foo();
+ bar();
+ baz();
+ return 0;
+}
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Thank you! I assume this is only a problem with line coverage? In other words, region coverage is still correct when visualized. |
Yeah, region coverage is still correct, just line coverage is wrong. |
Issue Summary
We identified an inaccuracy in line coverage reporting when short-circuit evaluation occurs in multi-line conditional expressions. Specifically:
(e.g., conditionB in a non-executed && chain shows coverage)
(adjacent un-executed conditions may show 1 vs 0 line coverage)
Root Cause Analysis
The current
WrappedSegment
mechanism may propagates coverage data from the last segment of the LHS line to subsequent RHS lines. Because the LHS line's coverage data depends on count of its segments andWrappedSegment
's count, it may causes false positive coverage for un-executed RHS conditions.Proposed Solution
The current implementation correctly handles single-statement if bodies through GapRegion insertion:
Processing workflow:
Generalize the GapRegion insertion strategy from if-statements to logical operators, placing RHS-synchronized segments at LHS-line-end to isolate coverage contexts during short-circuit evaluation.