[clang] [llvm] [clang][CoverageMapping] Refactor when setting MC/DC True/False (PR #78202)
via llvm-commits
llvm-commits at lists.llvm.org
Mon Jan 15 10:40:00 PST 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-codegen
Author: Alan Phipps (evodius96)
<details>
<summary>Changes</summary>
Clean-up of the algorithm that assigns MC/DC True/False control-flow condition IDs when constructing an MC/DC decision region. This patch creates a common API for setting/getting the condition IDs, making the binary logical operator visitor functions much cleaner.
This patch also fixes a bug in which the decision which was malformed due to an incorrect calculation of the True/False condition IDs. Previously, this calculation was done as the logical operation stack was being popped, yielding off-by-one errors. This is now done when the logical operation stack is being pushed.
---
Full diff: https://github.com/llvm/llvm-project/pull/78202.diff
3 Files Affected:
- (modified) clang/lib/CodeGen/CoverageMappingGen.cpp (+82-45)
- (modified) clang/test/CoverageMapping/mcdc-logical-stmt-ids-all.cpp (+50)
- (modified) llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h (+1-1)
``````````diff
diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp b/clang/lib/CodeGen/CoverageMappingGen.cpp
index 916016601a93276..ca7b65e3f4b08d7 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -668,6 +668,8 @@ struct MCDCCoverageBuilder {
llvm::SmallVector<const BinaryOperator *> NestLevel;
llvm::DenseMap<const Stmt *, MCDCConditionID> &CondIDs;
llvm::DenseMap<const Stmt *, unsigned> &MCDCBitmapMap;
+ llvm::DenseMap<const Stmt *, MCDCConditionID> TrueCondIDs;
+ llvm::DenseMap<const Stmt *, MCDCConditionID> FalseCondIDs;
MCDCConditionID NextID = 1;
bool NotMapped = false;
@@ -713,6 +715,21 @@ struct MCDCCoverageBuilder {
return AndRHS.empty() ? 0 : AndRHS.back();
}
+ /// Set the given condition's ID.
+ void setCondID(const Expr *Cond, MCDCConditionID ID) {
+ CondIDs[CodeGenFunction::stripCond(Cond)] = ID;
+ }
+
+ /// Set the ID of the next condition when the given condition is True.
+ void setTrueCondID(const Expr *Cond, MCDCConditionID ID) {
+ TrueCondIDs[CodeGenFunction::stripCond(Cond)] = ID;
+ }
+
+ /// Set the ID of the next condition when the given condition is False.
+ void setFalseCondID(const Expr *Cond, MCDCConditionID ID) {
+ FalseCondIDs[CodeGenFunction::stripCond(Cond)] = ID;
+ }
+
/// Return the ID of a given condition.
MCDCConditionID getCondID(const Expr *Cond) const {
auto I = CondIDs.find(CodeGenFunction::stripCond(Cond));
@@ -722,6 +739,24 @@ struct MCDCCoverageBuilder {
return I->second;
}
+ /// Return the ID of the next condition when the given condition is True.
+ MCDCConditionID getNextIfTrueCondID(const Expr *Cond) const {
+ auto I = TrueCondIDs.find(CodeGenFunction::stripCond(Cond));
+ if (I == TrueCondIDs.end())
+ return 0;
+ else
+ return I->second;
+ }
+
+ /// Return the ID of the next condition when the given condition is False.
+ MCDCConditionID getNextIfFalseCondID(const Expr *Cond) const {
+ auto I = FalseCondIDs.find(CodeGenFunction::stripCond(Cond));
+ if (I == FalseCondIDs.end())
+ return 0;
+ else
+ return I->second;
+ }
+
/// Push the binary operator statement to track the nest level and assign IDs
/// to the operator's LHS and RHS. The RHS may be a larger subtree that is
/// broken up on successive levels.
@@ -746,7 +781,7 @@ struct MCDCCoverageBuilder {
// assign that ID to its LHS node. Its RHS will receive a new ID.
if (CondIDs.contains(CodeGenFunction::stripCond(E))) {
// If Stmt has an ID, assign its ID to LHS
- CondIDs[CodeGenFunction::stripCond(E->getLHS())] = CondIDs[E];
+ setCondID(E->getLHS(), getCondID(E));
// Since the operator's LHS assumes the operator's same ID, pop the
// operator from the RHS stack so that if LHS short-circuits, it won't be
@@ -754,13 +789,44 @@ struct MCDCCoverageBuilder {
popRHSifTop(E);
} else {
// Otherwise, assign ID+1 to LHS.
- CondIDs[CodeGenFunction::stripCond(E->getLHS())] = NextID++;
+ setCondID(E->getLHS(), NextID++);
}
// Assign ID+1 to RHS.
- CondIDs[CodeGenFunction::stripCond(E->getRHS())] = NextID++;
+ setCondID(E->getRHS(), NextID++);
+
+ // Assign the True and False condition IDs for the LHS and RHS.
+ if (isLAnd(E)) {
+ // For logical-AND ("LHS && RHS"):
+ // - If LHS is TRUE, execution goes to the RHS node.
+ // - If LHS is FALSE, execution goes to the LHS node of the next LOr.
+ // If that does not exist, execution exits (ID == 0).
+ setTrueCondID(E->getLHS(), getCondID(E->getRHS()));
+ setFalseCondID(E->getLHS(), getNextLOrCondID());
+
+ // - If RHS is TRUE, execution goes to LHS node of the next LAnd.
+ // If that does not exist, execution exits (ID == 0).
+ // - If RHS is FALSE, execution goes to the LHS node of the next LOr.
+ // If that does not exist, execution exits (ID == 0).
+ setTrueCondID(E->getRHS(), getNextLAndCondID());
+ setFalseCondID(E->getRHS(), getNextLOrCondID());
+ } else {
+ // For logical-OR ("LHS || RHS"):
+ // - If LHS is TRUE, execution goes to the LHS node of the next LAnd.
+ // If that does not exist, execution exits (ID == 0).
+ // - If LHS is FALSE, execution goes to the RHS node.
+ setTrueCondID(E->getLHS(), getNextLAndCondID());
+ setFalseCondID(E->getLHS(), getCondID(E->getRHS()));
+
+ // - If RHS is TRUE, execution goes to LHS node of the next LAnd.
+ // If that does not exist, execution exits (ID == 0).
+ // - If RHS is FALSE, execution goes to the LHS node of the next LOr.
+ // If that does not exist, execution exits (ID == 0).
+ setTrueCondID(E->getRHS(), getNextLAndCondID());
+ setFalseCondID(E->getRHS(), getNextLOrCondID());
+ }
- // Push ID of Stmt's RHS so that LHS nodes know about it
+ // Push ID of Stmt's RHS so that LHS nodes know about it.
pushRHS(E);
}
@@ -1022,9 +1088,7 @@ struct CounterCoverageMappingBuilder
/// and add it to the function's SourceRegions. A branch region tracks a
/// "True" counter and a "False" counter for boolean expressions that
/// result in the generation of a branch.
- void createBranchRegion(const Expr *C, Counter TrueCnt, Counter FalseCnt,
- MCDCConditionID ID = 0, MCDCConditionID TrueID = 0,
- MCDCConditionID FalseID = 0) {
+ void createBranchRegion(const Expr *C, Counter TrueCnt, Counter FalseCnt) {
// Check for NULL conditions.
if (!C)
return;
@@ -1034,6 +1098,11 @@ struct CounterCoverageMappingBuilder
// function's SourceRegions) because it doesn't apply to any other source
// code other than the Condition.
if (CodeGenFunction::isInstrumentedCondition(C)) {
+ // Extract the MCDC condition IDs (returns 0 if not needed).
+ MCDCConditionID ID = MCDCBuilder.getCondID(C);
+ MCDCConditionID TrueID = MCDCBuilder.getNextIfTrueCondID(C);
+ MCDCConditionID FalseID = MCDCBuilder.getNextIfFalseCondID(C);
+
// If a condition can fold to true or false, the corresponding branch
// will be removed. Create a region with both counters hard-coded to
// zero. This allows us to visualize them in a special way.
@@ -1833,7 +1902,7 @@ struct CounterCoverageMappingBuilder
extendRegion(E->getRHS());
propagateCounts(getRegionCounter(E), E->getRHS());
- // Process Binary Operator and create MCDC Decision Region if top-level
+ // Process Binary Operator and create MCDC Decision Region if top-level.
unsigned NumConds = 0;
if ((NumConds = MCDCBuilder.popAndReturnCondCount(E)))
createDecisionRegion(E, getRegionBitmap(E), NumConds);
@@ -1847,30 +1916,13 @@ struct CounterCoverageMappingBuilder
// Extract the Parent Region Counter.
Counter ParentCnt = getRegion().getCounter();
- // Extract the MCDC condition IDs (returns 0 if not needed).
- MCDCConditionID NextOrID = MCDCBuilder.getNextLOrCondID();
- MCDCConditionID NextAndID = MCDCBuilder.getNextLAndCondID();
- MCDCConditionID LHSid = MCDCBuilder.getCondID(E->getLHS());
- MCDCConditionID RHSid = MCDCBuilder.getCondID(E->getRHS());
-
// Create Branch Region around LHS condition.
- // MC/DC: For "LHS && RHS"
- // - If LHS is TRUE, execution goes to the RHS.
- // - If LHS is FALSE, execution goes to the LHS of the next logical-OR.
- // If that does not exist, execution exits (ID == 0).
createBranchRegion(E->getLHS(), RHSExecCnt,
- subtractCounters(ParentCnt, RHSExecCnt), LHSid, RHSid,
- NextOrID);
+ subtractCounters(ParentCnt, RHSExecCnt));
// Create Branch Region around RHS condition.
- // MC/DC: For "LHS && RHS"
- // - If RHS is TRUE, execution goes to LHS of the next logical-AND.
- // If that does not exist, execution exits (ID == 0).
- // - If RHS is FALSE, execution goes to the LHS of the next logical-OR.
- // If that does not exist, execution exits (ID == 0).
createBranchRegion(E->getRHS(), RHSTrueCnt,
- subtractCounters(RHSExecCnt, RHSTrueCnt), RHSid,
- NextAndID, NextOrID);
+ subtractCounters(RHSExecCnt, RHSTrueCnt));
}
// Determine whether the right side of OR operation need to be visited.
@@ -1895,7 +1947,7 @@ struct CounterCoverageMappingBuilder
extendRegion(E->getRHS());
propagateCounts(getRegionCounter(E), E->getRHS());
- // Process Binary Operator and create MCDC Decision Region if top-level
+ // Process Binary Operator and create MCDC Decision Region if top-level.
unsigned NumConds = 0;
if ((NumConds = MCDCBuilder.popAndReturnCondCount(E)))
createDecisionRegion(E, getRegionBitmap(E), NumConds);
@@ -1913,28 +1965,13 @@ struct CounterCoverageMappingBuilder
// Extract the Parent Region Counter.
Counter ParentCnt = getRegion().getCounter();
- // Extract the MCDC condition IDs (returns 0 if not needed).
- MCDCConditionID NextOrID = MCDCBuilder.getNextLOrCondID();
- MCDCConditionID NextAndID = MCDCBuilder.getNextLAndCondID();
- MCDCConditionID LHSid = MCDCBuilder.getCondID(E->getLHS());
- MCDCConditionID RHSid = MCDCBuilder.getCondID(E->getRHS());
-
// Create Branch Region around LHS condition.
- // MC/DC: For "LHS || RHS"
- // - If LHS is TRUE, execution goes to the LHS of the next logical-AND.
- // If that does not exist, execution exits (ID == 0).
- // - If LHS is FALSE, execution goes to the RHS.
createBranchRegion(E->getLHS(), subtractCounters(ParentCnt, RHSExecCnt),
- RHSExecCnt, LHSid, NextAndID, RHSid);
+ RHSExecCnt);
// Create Branch Region around RHS condition.
- // MC/DC: For "LHS || RHS"
- // - If RHS is TRUE, execution goes to LHS of the next logical-AND.
- // If that does not exist, execution exits (ID == 0).
- // - If RHS is FALSE, execution goes to the LHS of the next logical-OR.
- // If that does not exist, execution exits (ID == 0).
createBranchRegion(E->getRHS(), subtractCounters(RHSExecCnt, RHSFalseCnt),
- RHSFalseCnt, RHSid, NextAndID, NextOrID);
+ RHSFalseCnt);
}
void VisitLambdaExpr(const LambdaExpr *LE) {
diff --git a/clang/test/CoverageMapping/mcdc-logical-stmt-ids-all.cpp b/clang/test/CoverageMapping/mcdc-logical-stmt-ids-all.cpp
index 5b13cc3507e96cd..b1d32bdd746cb40 100644
--- a/clang/test/CoverageMapping/mcdc-logical-stmt-ids-all.cpp
+++ b/clang/test/CoverageMapping/mcdc-logical-stmt-ids-all.cpp
@@ -129,3 +129,53 @@ bool func_ternary_or(bool a, bool b, bool c, bool d, bool e, bool f) {
// CHECK: Branch,File 0, 122:26 -> 122:27 = (#6 - #7), #7 [4,0,3]
// CHECK: Branch,File 0, 122:31 -> 122:32 = (#4 - #5), #5 [3,0,2]
// CHECK: Branch,File 0, 122:36 -> 122:37 = (#2 - #3), #3 [2,0,0]
+
+bool func_if_nested_and_if(bool a, bool b, bool c, bool d, bool e) {
+ if (a || (b && c) || d || e)
+ return true;
+ else
+ return false;
+}
+
+// CHECK-LABEL: Decision,File 0, 134:7 -> 134:30 = M:0, C:5
+// CHECK-NEXT: Branch,File 0, 134:7 -> 134:8 = (#0 - #6), #6 [1,0,4]
+// CHECK: Branch,File 0, 134:13 -> 134:14 = #7, (#6 - #7) [4,5,3]
+// CHECK: Branch,File 0, 134:18 -> 134:19 = #8, (#7 - #8) [5,0,3]
+// CHECK: Branch,File 0, 134:24 -> 134:25 = (#4 - #5), #5 [3,0,2]
+// CHECK: Branch,File 0, 134:29 -> 134:30 = (#2 - #3), #3 [2,0,0]
+
+bool func_ternary_nested_and_if(bool a, bool b, bool c, bool d, bool e) {
+ return (a || (b && c) || d || e) ? true : false;
+}
+
+// CHECK-LABEL: Decision,File 0, 148:11 -> 148:34 = M:0, C:5
+// CHECK-NEXT: Branch,File 0, 148:11 -> 148:12 = (#0 - #6), #6 [1,0,4]
+// CHECK: Branch,File 0, 148:17 -> 148:18 = #7, (#6 - #7) [4,5,3]
+// CHECK: Branch,File 0, 148:22 -> 148:23 = #8, (#7 - #8) [5,0,3]
+// CHECK: Branch,File 0, 148:28 -> 148:29 = (#4 - #5), #5 [3,0,2]
+// CHECK: Branch,File 0, 148:33 -> 148:34 = (#2 - #3), #3 [2,0,0]
+
+bool func_if_nested_and_if_2(bool a, bool b, bool c, bool d, bool e) {
+ if (a || ((b && c) || d) && e)
+ return true;
+ else
+ return false;
+}
+
+// CHECK-LABEL: Decision,File 0, 159:7 -> 159:32 = M:0, C:5
+// CHECK-NEXT: Branch,File 0, 159:7 -> 159:8 = (#0 - #2), #2 [1,0,2]
+// CHECK: Branch,File 0, 159:14 -> 159:15 = #7, (#2 - #7) [2,5,4]
+// CHECK: Branch,File 0, 159:19 -> 159:20 = #8, (#7 - #8) [5,3,4]
+// CHECK: Branch,File 0, 159:25 -> 159:26 = (#5 - #6), #6 [4,3,0]
+// CHECK: Branch,File 0, 159:31 -> 159:32 = #4, (#3 - #4) [3,0,0]
+
+bool func_ternary_nested_and_if_2(bool a, bool b, bool c, bool d, bool e) {
+ return (a || ((b && c) || d) && e) ? true : false;
+}
+
+// CHECK-LABEL: Decision,File 0, 173:11 -> 173:36 = M:0, C:5
+// CHECK-NEXT: Branch,File 0, 173:11 -> 173:12 = (#0 - #2), #2 [1,0,2]
+// CHECK: Branch,File 0, 173:18 -> 173:19 = #7, (#2 - #7) [2,5,4]
+// CHECK: Branch,File 0, 173:23 -> 173:24 = #8, (#7 - #8) [5,3,4]
+// CHECK: Branch,File 0, 173:29 -> 173:30 = (#5 - #6), #6 [4,3,0]
+// CHECK: Branch,File 0, 173:35 -> 173:36 = #4, (#3 - #4) [3,0,0]
diff --git a/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h b/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
index 2757b8cd54a69c5..367c884957ff6d7 100644
--- a/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
+++ b/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
@@ -298,7 +298,7 @@ struct CounterMappingRegion {
unsigned ExpandedFileID, unsigned LineStart,
unsigned ColumnStart, unsigned LineEnd,
unsigned ColumnEnd, RegionKind Kind)
- : MCDCParams(MCDCParams), ExpandedFileID(ExpandedFileID),
+ : MCDCParams(MCDCParams), FileID(FileID), ExpandedFileID(ExpandedFileID),
LineStart(LineStart), ColumnStart(ColumnStart), LineEnd(LineEnd),
ColumnEnd(ColumnEnd), Kind(Kind) {}
``````````
</details>
https://github.com/llvm/llvm-project/pull/78202
More information about the llvm-commits
mailing list