[clang] [llvm] [clang][CoverageMapping] Refactor when setting MC/DC True/False (PR #78202)

Alan Phipps via cfe-commits cfe-commits at lists.llvm.org
Tue Jan 16 12:30:49 PST 2024


https://github.com/evodius96 updated https://github.com/llvm/llvm-project/pull/78202

>From 8751417f4889a120193a604e2ea91627f3b064e8 Mon Sep 17 00:00:00 2001
From: Alan Phipps <a-phipps at ti.com>
Date: Mon, 15 Jan 2024 12:24:36 -0600
Subject: [PATCH 1/3] [clang][CoverageMapping] Refactor when setting MC/DC
 True/False Condition IDs

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.
---
 clang/lib/CodeGen/CoverageMappingGen.cpp      | 127 +++++++++++-------
 .../mcdc-logical-stmt-ids-all.cpp             |  50 +++++++
 .../ProfileData/Coverage/CoverageMapping.h    |   2 +-
 3 files changed, 133 insertions(+), 46 deletions(-)

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) {}
 

>From 768a040ebdb4d2ef446cd7914ee64d69711b8378 Mon Sep 17 00:00:00 2001
From: Alan Phipps <a-phipps at ti.com>
Date: Mon, 15 Jan 2024 12:45:17 -0600
Subject: [PATCH 2/3] Update formatting changes

---
 clang/lib/CodeGen/CoverageMappingGen.cpp | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp b/clang/lib/CodeGen/CoverageMappingGen.cpp
index ca7b65e3f4b08d7..499e0e5d04d8631 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -717,17 +717,17 @@ struct MCDCCoverageBuilder {
 
   /// Set the given condition's ID.
   void setCondID(const Expr *Cond, MCDCConditionID ID) {
-      CondIDs[CodeGenFunction::stripCond(Cond)] = 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;
+    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;
+    FalseCondIDs[CodeGenFunction::stripCond(Cond)] = ID;
   }
 
   /// Return the ID of a given condition.

>From cd516da06fa326229995b442f929781c010e4a27 Mon Sep 17 00:00:00 2001
From: Alan Phipps <a-phipps at ti.com>
Date: Tue, 16 Jan 2024 13:29:32 -0600
Subject: [PATCH 3/3] Cleanup Decision push/pop and Nesting Level Tracking

---
 clang/lib/CodeGen/CoverageMappingGen.cpp      | 248 +++++++-----------
 .../mcdc-logical-stmt-ids-all.cpp             |  35 ++-
 .../ProfileData/Coverage/CoverageMapping.h    |  13 +-
 3 files changed, 137 insertions(+), 159 deletions(-)

diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp b/clang/lib/CodeGen/CoverageMappingGen.cpp
index 499e0e5d04d8631..ad1ee264cd9670c 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -616,14 +616,14 @@ struct MCDCCoverageBuilder {
   ///
   /// A node ID of '0' always means MC/DC isn't being tracked.
   ///
-  /// As the AST walk proceeds recursively, the algorithm will also use stacks
+  /// As the AST walk proceeds recursively, the algorithm will also use a stack
   /// to track the IDs of logical-AND and logical-OR operations on the RHS so
   /// that it can be determined which nodes are executed next, depending on how
   /// a LHS or RHS of a logical-AND or logical-OR is evaluated.  This
   /// information relies on the assigned IDs and are embedded within the
   /// coverage region IDs of each branch region associated with a leaf-level
   /// condition. This information helps the visualization tool reconstruct all
-  /// possible test vectors for the purposes of MC/DC analysis. if a "next" node
+  /// possible test vectors for the purposes of MC/DC analysis. If a "next" node
   /// ID is '0', it means it's the end of the test vector. The following rules
   /// are used:
   ///
@@ -663,13 +663,14 @@ struct MCDCCoverageBuilder {
 private:
   CodeGenModule &CGM;
 
-  llvm::SmallVector<MCDCConditionID> AndRHS;
-  llvm::SmallVector<MCDCConditionID> OrRHS;
-  llvm::SmallVector<const BinaryOperator *> NestLevel;
+  struct DecisionIDPair {
+    MCDCConditionID TrueID = 0;
+    MCDCConditionID FalseID = 0;
+  };
+
+  llvm::SmallVector<DecisionIDPair, 1> DecisionStack;
   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;
 
@@ -678,58 +679,27 @@ struct MCDCCoverageBuilder {
     return E->getOpcode() == BO_LAnd;
   }
 
-  /// Push an ID onto the corresponding RHS stack.
-  void pushRHS(const BinaryOperator *E) {
-    llvm::SmallVector<MCDCConditionID> &rhs = isLAnd(E) ? AndRHS : OrRHS;
-    rhs.push_back(CondIDs[CodeGenFunction::stripCond(E->getRHS())]);
-  }
-
-  /// Pop an ID from the corresponding RHS stack.
-  void popRHS(const BinaryOperator *E) {
-    llvm::SmallVector<MCDCConditionID> &rhs = isLAnd(E) ? AndRHS : OrRHS;
-    if (!rhs.empty())
-      rhs.pop_back();
-  }
-
-  /// If the expected ID is on top, pop it off the corresponding RHS stack.
-  void popRHSifTop(const BinaryOperator *E) {
-    if (!OrRHS.empty() && CondIDs[E] == OrRHS.back())
-      OrRHS.pop_back();
-    else if (!AndRHS.empty() && CondIDs[E] == AndRHS.back())
-      AndRHS.pop_back();
-  }
-
 public:
   MCDCCoverageBuilder(CodeGenModule &CGM,
                       llvm::DenseMap<const Stmt *, MCDCConditionID> &CondIDMap,
                       llvm::DenseMap<const Stmt *, unsigned> &MCDCBitmapMap)
-      : CGM(CGM), CondIDs(CondIDMap), MCDCBitmapMap(MCDCBitmapMap) {}
+      : CGM(CGM), DecisionStack(1), CondIDs(CondIDMap),
+        MCDCBitmapMap(MCDCBitmapMap) {}
 
-  /// Return the ID of the RHS of the next, upper nest-level logical-OR.
-  MCDCConditionID getNextLOrCondID() const {
-    return OrRHS.empty() ? 0 : OrRHS.back();
-  }
+  /// Return whether the control flow map is not presently being built. This
+  /// can be used to determine whether the flow is at the root node of an
+  /// expression if that expression is mapped.
+  bool isIdle() { return (NextID == 1 && !NotMapped); }
 
-  /// Return the ID of the RHS of the next, upper nest-level logical-AND.
-  MCDCConditionID getNextLAndCondID() const {
-    return AndRHS.empty() ? 0 : AndRHS.back();
-  }
+  /// Return whether the control flow map is in the process of being built for
+  /// a mapped expression.
+  bool isBuilding() { return (NextID > 1); }
 
   /// 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));
@@ -739,22 +709,10 @@ 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;
+  /// Return the LHS Decision ({0,0} if not set).
+  const DecisionIDPair &back() {
+    assert(DecisionStack.size() >= 1);
+    return DecisionStack.back();
   }
 
   /// Push the binary operator statement to track the nest level and assign IDs
@@ -765,99 +723,70 @@ struct MCDCCoverageBuilder {
       return;
 
     // If binary expression is disqualified, don't do mapping.
-    if (NestLevel.empty() &&
-        !MCDCBitmapMap.contains(CodeGenFunction::stripCond(E)))
+    if (!isBuilding() && !MCDCBitmapMap.contains(CodeGenFunction::stripCond(E)))
       NotMapped = true;
 
-    // Push Stmt on 'NestLevel' stack to keep track of nest location.
-    NestLevel.push_back(E);
-
     // Don't go any further if we don't need to map condition IDs.
     if (NotMapped)
       return;
 
+    assert(DecisionStack.size() >= 1);
+    const DecisionIDPair &ParentDecision = DecisionStack.back();
+
     // If the operator itself has an assigned ID, this means it represents a
-    // larger subtree.  In this case, pop its ID out of the RHS stack and
-    // 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
+    // larger subtree.  In this case, assign that ID to its LHS node.  Its RHS
+    // will receive a new ID below. Otherwise, assign ID+1 to LHS.
+    if (CondIDs.contains(CodeGenFunction::stripCond(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
-      // incorrectly re-used as the node executed next.
-      popRHSifTop(E);
-    } else {
-      // Otherwise, assign ID+1 to LHS.
+    else
       setCondID(E->getLHS(), NextID++);
-    }
 
-    // Assign ID+1 to RHS.
-    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());
-    }
+    // Assign a ID+1 for the RHS.
+    MCDCConditionID RHSid = NextID++;
+    setCondID(E->getRHS(), RHSid);
+
+    // Push the LHS decision IDs onto the DecisionStack.
+    if (isLAnd(E))
+      DecisionStack.push_back({RHSid, ParentDecision.FalseID});
+    else
+      DecisionStack.push_back({ParentDecision.TrueID, RHSid});
+  }
+
+  /// Pop and return the LHS Decision ({0,0} if not set).
+  DecisionIDPair pop() {
+    assert(DecisionStack.size() >= 1);
+
+    if (!CGM.getCodeGenOpts().MCDCCoverage || NotMapped)
+      return DecisionStack.front();
 
-    // Push ID of Stmt's RHS so that LHS nodes know about it.
-    pushRHS(E);
+    assert(DecisionStack.size() > 1);
+    DecisionIDPair D = DecisionStack.back();
+    DecisionStack.pop_back();
+    return D;
   }
 
-  /// Pop the binary operator from the next level. If the walk is at the top of
-  /// the next, assign the total number of conditions.
-  unsigned popAndReturnCondCount(const BinaryOperator *E) {
+  /// Return the total number of conditions and reset the state. The number of
+  /// conditions is zero if the expression isn't mapped.
+  unsigned getTotalConditionsAndReset(const BinaryOperator *E) {
     if (!CGM.getCodeGenOpts().MCDCCoverage)
       return 0;
 
-    unsigned TotalConds = 0;
-
-    // Pop Stmt from 'NestLevel' stack.
-    assert(NestLevel.back() == E);
-    NestLevel.pop_back();
+    assert(!isIdle());
+    assert(DecisionStack.size() == 1);
 
     // Reset state if not doing mapping.
-    if (NestLevel.empty() && NotMapped) {
+    if (NotMapped) {
       NotMapped = false;
+      assert(NextID == 1);
       return 0;
     }
 
-    // Pop RHS ID.
-    popRHS(E);
+    // Set number of conditions and reset.
+    unsigned TotalConds = NextID - 1;
 
-    // If at the parent (NestLevel=0), set conds and reset.
-    if (NestLevel.empty()) {
-      TotalConds = NextID - 1;
+    // Reset ID back to beginning.
+    NextID = 1;
 
-      // Reset ID back to beginning.
-      NextID = 1;
-    }
     return TotalConds;
   }
 };
@@ -1088,7 +1017,9 @@ 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) {
+  void createBranchRegion(const Expr *C, Counter TrueCnt, Counter FalseCnt,
+                          MCDCConditionID ID = 0, MCDCConditionID TrueID = 0,
+                          MCDCConditionID FalseID = 0) {
     // Check for NULL conditions.
     if (!C)
       return;
@@ -1098,11 +1029,6 @@ 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.
@@ -1891,20 +1817,28 @@ struct CounterCoverageMappingBuilder
   }
 
   void VisitBinLAnd(const BinaryOperator *E) {
-    // Keep track of Binary Operator and assign MCDC condition IDs
+    bool IsRootNode = MCDCBuilder.isIdle();
+
+    // Keep track of Binary Operator and assign MCDC condition IDs.
     MCDCBuilder.pushAndAssignIDs(E);
 
     extendRegion(E->getLHS());
     propagateCounts(getRegion().getCounter(), E->getLHS());
     handleFileExit(getEnd(E->getLHS()));
 
+    // Track LHS True/False Decision.
+    auto DecisionLHS = MCDCBuilder.pop();
+
     // Counter tracks the right hand side of a logical and operator.
     extendRegion(E->getRHS());
     propagateCounts(getRegionCounter(E), E->getRHS());
 
-    // Process Binary Operator and create MCDC Decision Region if top-level.
+    // Track RHS True/False Decision.
+    const auto &DecisionRHS = MCDCBuilder.back();
+
+    // Create MCDC Decision Region if at top-level (root).
     unsigned NumConds = 0;
-    if ((NumConds = MCDCBuilder.popAndReturnCondCount(E)))
+    if (IsRootNode && (NumConds = MCDCBuilder.getTotalConditionsAndReset(E)))
       createDecisionRegion(E, getRegionBitmap(E), NumConds);
 
     // Extract the RHS's Execution Counter.
@@ -1916,13 +1850,18 @@ struct CounterCoverageMappingBuilder
     // Extract the Parent Region Counter.
     Counter ParentCnt = getRegion().getCounter();
 
+    MCDCConditionID LHSid = MCDCBuilder.getCondID(E->getLHS());
+    MCDCConditionID RHSid = MCDCBuilder.getCondID(E->getRHS());
+
     // Create Branch Region around LHS condition.
     createBranchRegion(E->getLHS(), RHSExecCnt,
-                       subtractCounters(ParentCnt, RHSExecCnt));
+                       subtractCounters(ParentCnt, RHSExecCnt), LHSid,
+                       DecisionLHS.TrueID, DecisionLHS.FalseID);
 
     // Create Branch Region around RHS condition.
     createBranchRegion(E->getRHS(), RHSTrueCnt,
-                       subtractCounters(RHSExecCnt, RHSTrueCnt));
+                       subtractCounters(RHSExecCnt, RHSTrueCnt), RHSid,
+                       DecisionRHS.TrueID, DecisionRHS.FalseID);
   }
 
   // Determine whether the right side of OR operation need to be visited.
@@ -1936,20 +1875,28 @@ struct CounterCoverageMappingBuilder
   }
 
   void VisitBinLOr(const BinaryOperator *E) {
-    // Keep track of Binary Operator and assign MCDC condition IDs
+    bool IsRootNode = MCDCBuilder.isIdle();
+
+    // Keep track of Binary Operator and assign MCDC condition IDs.
     MCDCBuilder.pushAndAssignIDs(E);
 
     extendRegion(E->getLHS());
     Counter OutCount = propagateCounts(getRegion().getCounter(), E->getLHS());
     handleFileExit(getEnd(E->getLHS()));
 
+    // Track LHS True/False Decision.
+    auto DecisionLHS = MCDCBuilder.pop();
+
     // Counter tracks the right hand side of a logical or operator.
     extendRegion(E->getRHS());
     propagateCounts(getRegionCounter(E), E->getRHS());
 
-    // Process Binary Operator and create MCDC Decision Region if top-level.
+    // Track RHS True/False Decision.
+    const auto &DecisionRHS = MCDCBuilder.back();
+
+    // Create MCDC Decision Region if at top-level (root).
     unsigned NumConds = 0;
-    if ((NumConds = MCDCBuilder.popAndReturnCondCount(E)))
+    if (IsRootNode && (NumConds = MCDCBuilder.getTotalConditionsAndReset(E)))
       createDecisionRegion(E, getRegionBitmap(E), NumConds);
 
     // Extract the RHS's Execution Counter.
@@ -1965,13 +1912,18 @@ struct CounterCoverageMappingBuilder
     // Extract the Parent Region Counter.
     Counter ParentCnt = getRegion().getCounter();
 
+    MCDCConditionID LHSid = MCDCBuilder.getCondID(E->getLHS());
+    MCDCConditionID RHSid = MCDCBuilder.getCondID(E->getRHS());
+
     // Create Branch Region around LHS condition.
     createBranchRegion(E->getLHS(), subtractCounters(ParentCnt, RHSExecCnt),
-                       RHSExecCnt);
+                       RHSExecCnt, LHSid, DecisionLHS.TrueID,
+                       DecisionLHS.FalseID);
 
     // Create Branch Region around RHS condition.
     createBranchRegion(E->getRHS(), subtractCounters(RHSExecCnt, RHSFalseCnt),
-                       RHSFalseCnt);
+                       RHSFalseCnt, RHSid, DecisionRHS.TrueID,
+                       DecisionRHS.FalseID);
   }
 
   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 b1d32bdd746cb40..6f47a4b901a8a75 100644
--- a/clang/test/CoverageMapping/mcdc-logical-stmt-ids-all.cpp
+++ b/clang/test/CoverageMapping/mcdc-logical-stmt-ids-all.cpp
@@ -130,7 +130,7 @@ bool func_ternary_or(bool a, bool b, bool c, bool d, bool e, bool f) {
 // 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) {
+bool func_if_nested_if(bool a, bool b, bool c, bool d, bool e) {
   if (a || (b && c) || d || e)
     return true;
   else
@@ -144,7 +144,7 @@ bool func_if_nested_and_if(bool a, bool b, bool c, bool d, bool e) {
 // 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) {
+bool func_ternary_nested_if(bool a, bool b, bool c, bool d, bool e) {
   return (a || (b && c) || d || e) ? true : false;
 }
 
@@ -155,7 +155,7 @@ bool func_ternary_nested_and_if(bool a, bool b, bool c, bool d, bool e) {
 // 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) {
+bool func_if_nested_if_2(bool a, bool b, bool c, bool d, bool e) {
   if (a || ((b && c) || d) && e)
     return true;
   else
@@ -169,7 +169,7 @@ bool func_if_nested_and_if_2(bool a, bool b, bool c, bool d, bool e) {
 // 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) {
+bool func_ternary_nested_if_2(bool a, bool b, bool c, bool d, bool e) {
   return (a || ((b && c) || d) && e) ? true : false;
 }
 
@@ -179,3 +179,30 @@ bool func_ternary_nested_and_if_2(bool a, bool b, bool c, bool d, bool e) {
 // 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]
+
+bool func_if_nested_if_3(bool a, bool b, bool c, bool d, bool e, bool f) {
+  if ((a && (b || c) || (d && e)) && f)
+    return true;
+  else
+    return false;
+}
+
+// CHECK-LABEL: Decision,File 0, 184:7 -> 184:39 = M:0, C:6
+// CHECK:  Branch,File 0, 184:8 -> 184:9 = #5, (#0 - #5) [1,4,3]
+// CHECK:  Branch,File 0, 184:14 -> 184:15 = (#5 - #6), #6 [4,2,5]
+// CHECK:  Branch,File 0, 184:19 -> 184:20 = (#6 - #7), #7 [5,2,3]
+// CHECK:  Branch,File 0, 184:26 -> 184:27 = #8, (#4 - #8) [3,6,0]
+// CHECK:  Branch,File 0, 184:31 -> 184:32 = #9, (#8 - #9) [6,2,0]
+// CHECK:  Branch,File 0, 184:38 -> 184:39 = #3, (#2 - #3) [2,0,0]
+
+bool func_ternary_nested_if_3(bool a, bool b, bool c, bool d, bool e, bool f) {
+  return ((a && (b || c) || (d && e)) && f) ? true : false;
+}
+
+// CHECK-LABEL: Decision,File 0, 199:11 -> 199:43 = M:0, C:6
+// CHECK:  Branch,File 0, 199:12 -> 199:13 = #5, (#0 - #5) [1,4,3]
+// CHECK:  Branch,File 0, 199:18 -> 199:19 = (#5 - #6), #6 [4,2,5]
+// CHECK:  Branch,File 0, 199:23 -> 199:24 = (#6 - #7), #7 [5,2,3]
+// CHECK:  Branch,File 0, 199:30 -> 199:31 = #8, (#4 - #8) [3,6,0]
+// CHECK:  Branch,File 0, 199:35 -> 199:36 = #9, (#8 - #9) [6,2,0]
+// CHECK:  Branch,File 0, 199:42 -> 199:43 = #3, (#2 - #3) [2,0,0]
diff --git a/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h b/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
index 367c884957ff6d7..2a857917136a841 100644
--- a/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
+++ b/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
@@ -295,12 +295,11 @@ struct CounterMappingRegion {
         Kind(Kind) {}
 
   CounterMappingRegion(MCDCParameters MCDCParams, unsigned FileID,
-                       unsigned ExpandedFileID, unsigned LineStart,
-                       unsigned ColumnStart, unsigned LineEnd,
-                       unsigned ColumnEnd, RegionKind Kind)
-      : MCDCParams(MCDCParams), FileID(FileID), ExpandedFileID(ExpandedFileID),
-        LineStart(LineStart), ColumnStart(ColumnStart), LineEnd(LineEnd),
-        ColumnEnd(ColumnEnd), Kind(Kind) {}
+                       unsigned LineStart, unsigned ColumnStart,
+                       unsigned LineEnd, unsigned ColumnEnd, RegionKind Kind)
+      : MCDCParams(MCDCParams), FileID(FileID), LineStart(LineStart),
+        ColumnStart(ColumnStart), LineEnd(LineEnd), ColumnEnd(ColumnEnd),
+        Kind(Kind) {}
 
   static CounterMappingRegion
   makeRegion(Counter Count, unsigned FileID, unsigned LineStart,
@@ -354,7 +353,7 @@ struct CounterMappingRegion {
   makeDecisionRegion(MCDCParameters MCDCParams, unsigned FileID,
                      unsigned LineStart, unsigned ColumnStart, unsigned LineEnd,
                      unsigned ColumnEnd) {
-    return CounterMappingRegion(MCDCParams, FileID, 0, LineStart, ColumnStart,
+    return CounterMappingRegion(MCDCParams, FileID, LineStart, ColumnStart,
                                 LineEnd, ColumnEnd, MCDCDecisionRegion);
   }
 



More information about the cfe-commits mailing list