[clang][CoverageMapping] Refactor setting MC/DC True/False Condition IDs (#78202)

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 issue
#77873 in which a record's
control flow map can be malformed due to an incorrect calculation of the
True/False condition IDs.

Author: evodius96

clang/lib/CodeGen/CoverageMappingGen.cpp

@@ -573,6 +573,11 @@ struct EmptyCoverageMappingBuilder : public CoverageMappingBuilder {
 /// creation.
 struct MCDCCoverageBuilder {
 
+  struct DecisionIDPair {
+    MCDCConditionID TrueID = 0;
+    MCDCConditionID FalseID = 0;
+  };
+
   /// The AST walk recursively visits nested logical-AND or logical-OR binary
   /// operator nodes and then visits their LHS and RHS children nodes.  As this
   /// happens, the algorithm will assign IDs to each operator's LHS and RHS side
@@ -616,14 +621,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,54 +668,40 @@ struct MCDCCoverageBuilder {
 private:
   CodeGenModule &CGM;
 
-  llvm::SmallVector<MCDCConditionID> AndRHS;
-  llvm::SmallVector<MCDCConditionID> OrRHS;
-  llvm::SmallVector<const BinaryOperator *> NestLevel;
+  llvm::SmallVector<DecisionIDPair> DecisionStack;
   llvm::DenseMap<const Stmt *, MCDCConditionID> &CondIDs;
   llvm::DenseMap<const Stmt *, unsigned> &MCDCBitmapMap;
   MCDCConditionID NextID = 1;
   bool NotMapped = false;
 
+  /// Represent a sentinel value of [0,0] for the bottom of DecisionStack.
+  static constexpr DecisionIDPair DecisionStackSentinel{0, 0};
+
   /// Is this a logical-AND operation?
   bool isLAnd(const BinaryOperator *E) const {
     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, DecisionStackSentinel), 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 build of the control flow map is at the top-level
+  /// (root) of a logical operator nest in a boolean expression prior to the
+  /// assignment of condition IDs.
+  bool isIdle() const { return (NextID == 1 && !NotMapped); }
+
+  /// Return whether any IDs have been assigned in the build of the control
+  /// flow map, indicating that the map is being generated for this boolean
+  /// expression.
+  bool isBuilding() const { return (NextID > 1); }
 
-  /// Return the ID of the RHS of the next, upper nest-level logical-AND.
-  MCDCConditionID getNextLAndCondID() const {
-    return AndRHS.empty() ? 0 : AndRHS.back();
+  /// Set the given condition's ID.
+  void setCondID(const Expr *Cond, MCDCConditionID ID) {
+    CondIDs[CodeGenFunction::stripCond(Cond)] = ID;
   }
 
   /// Return the ID of a given condition.
@@ -722,6 +713,9 @@ struct MCDCCoverageBuilder {
       return I->second;
   }
 
+  /// Return the LHS Decision ([0,0] if not set).
+  const DecisionIDPair &back() const { return DecisionStack.back(); }
+
   /// 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.
@@ -730,68 +724,67 @@ 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;
 
+    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
-      CondIDs[CodeGenFunction::stripCond(E->getLHS())] = CondIDs[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.
-      CondIDs[CodeGenFunction::stripCond(E->getLHS())] = NextID++;
-    }
+    // 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));
+    else
+      setCondID(E->getLHS(), NextID++);
 
-    // Assign ID+1 to RHS.
-    CondIDs[CodeGenFunction::stripCond(E->getRHS())] = NextID++;
+    // Assign a ID+1 for the RHS.
+    MCDCConditionID RHSid = NextID++;
+    setCondID(E->getRHS(), RHSid);
 
-    // Push ID of Stmt's RHS so that LHS nodes know about it
-    pushRHS(E);
+    // 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() {
+    if (!CGM.getCodeGenOpts().MCDCCoverage || NotMapped)
+      return DecisionStack.front();
+
+    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;
   }
 };
@@ -1018,13 +1011,15 @@ struct CounterCoverageMappingBuilder
     return (Cond->EvaluateAsInt(Result, CVM.getCodeGenModule().getContext()));
   }
 
+  using MCDCDecisionIDPair = MCDCCoverageBuilder::DecisionIDPair;
+
   /// Create a Branch Region around an instrumentable condition for coverage
   /// 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,
+                     const MCDCDecisionIDPair &IDPair = MCDCDecisionIDPair()) {
     // Check for NULL conditions.
     if (!C)
       return;
@@ -1034,6 +1029,10 @@ struct CounterCoverageMappingBuilder
     // function's SourceRegions) because it doesn't apply to any other source
     // code other than the Condition.
     if (CodeGenFunction::isInstrumentedCondition(C)) {
+      MCDCConditionID ID = MCDCBuilder.getCondID(C);
+      MCDCConditionID TrueID = IDPair.TrueID;
+      MCDCConditionID FalseID = IDPair.FalseID;
+
       // 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.
@@ -1822,20 +1821,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.
+    const 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.
@@ -1847,30 +1854,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), DecisionLHS);
 
     // 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), DecisionRHS);
   }
 
   // Determine whether the right side of OR operation need to be visited.
@@ -1884,20 +1874,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.
+    const 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.
@@ -1913,28 +1911,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, DecisionLHS);
 
     // 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, DecisionRHS);
   }
 
   void VisitLambdaExpr(const LambdaExpr *LE) {