[MC/DC] Enable nested expressions

clang/docs/ReleaseNotes.rst

@@ -116,6 +116,8 @@ Improvements to Clang's time-trace
 Improvements to Coverage Mapping
 --------------------------------
 
+- [MC/DC] Nested expressions are handled as individual MC/DC expressions.
+
 Bug Fixes in This Version
 -------------------------
 

clang/docs/SourceBasedCodeCoverage.rst

@@ -510,13 +510,6 @@ requires 8 test vectors.
 Expressions such as ``((a0 && b0) || (a1 && b1) || ...)`` can cause the
 number of test vectors to increase exponentially.
 
-Also, if a boolean expression is embedded in the nest of another boolean
-expression but separated by a non-logical operator, this is also not supported.
-For example, in ``x = (a && b && c && func(d && f))``, the ``d && f`` case
-starts a new boolean expression that is separated from the other conditions by
-the operator ``func()``.  When this is encountered, a warning will be generated
-and the boolean expression will not be instrumented.
-
 Switch statements
 -----------------
 

clang/lib/CodeGen/CodeGenPGO.cpp

@@ -228,45 +228,46 @@ struct MapRegionCounters : public RecursiveASTVisitor<MapRegionCounters> {
   /// The stacks are also used to find error cases and notify the user.  A
   /// standard logical operator nest for a boolean expression could be in a form
   /// similar to this: "x = a && b && c && (d || f)"
-  unsigned NumCond = 0;
-  bool SplitNestedLogicalOp = false;
-  SmallVector<const Stmt *, 16> NonLogOpStack;
-  SmallVector<const BinaryOperator *, 16> LogOpStack;
+  struct DecisionState {
+    llvm::DenseSet<const Stmt *> Leaves; // Not BinOp
+    const Expr *DecisionExpr;            // Root
+    bool Split;
+
+    DecisionState() = delete;
+    DecisionState(const Expr *E, bool Split = false)
+        : DecisionExpr(E), Split(Split) {}
+  };
+
+  SmallVector<DecisionState, 1> DecisionStack;
 
   // Hook: dataTraverseStmtPre() is invoked prior to visiting an AST Stmt node.
   bool dataTraverseStmtPre(Stmt *S) {
     /// If MC/DC is not enabled, MCDCMaxCond will be set to 0. Do nothing.
     if (MCDCMaxCond == 0)
       return true;
 
-    /// At the top of the logical operator nest, reset the number of conditions,
-    /// also forget previously seen split nesting cases.
-    if (LogOpStack.empty()) {
-      NumCond = 0;
-      SplitNestedLogicalOp = false;
-    }
-
-    if (const Expr *E = dyn_cast<Expr>(S)) {
-      const BinaryOperator *BinOp = dyn_cast<BinaryOperator>(E->IgnoreParens());
-      if (BinOp && BinOp->isLogicalOp()) {
-        /// Check for "split-nested" logical operators. This happens when a new
-        /// boolean expression logical-op nest is encountered within an existing
-        /// boolean expression, separated by a non-logical operator.  For
-        /// example, in "x = (a && b && c && foo(d && f))", the "d && f" case
-        /// starts a new boolean expression that is separated from the other
-        /// conditions by the operator foo(). Split-nested cases are not
-        /// supported by MC/DC.
-        SplitNestedLogicalOp = SplitNestedLogicalOp || !NonLogOpStack.empty();
-
-        LogOpStack.push_back(BinOp);
+    /// Mark "in splitting" when a leaf is met.
+    if (!DecisionStack.empty()) {
+      auto &StackTop = DecisionStack.back();
+      if (!StackTop.Split) {
+        if (StackTop.Leaves.contains(S)) {
+          assert(!StackTop.Split);
+          StackTop.Split = true;
+        }
         return true;
       }
+
+      // Split
+      assert(StackTop.Split);
+      assert(!StackTop.Leaves.contains(S));
     }
 
-    /// Keep track of non-logical operators. These are OK as long as we don't
-    /// encounter a new logical operator after seeing one.
-    if (!LogOpStack.empty())
-      NonLogOpStack.push_back(S);
+    if (const auto *E = dyn_cast<Expr>(S)) {
+      if (const auto *BinOp =
+              dyn_cast<BinaryOperator>(CodeGenFunction::stripCond(E));
+          BinOp && BinOp->isLogicalOp())
+        DecisionStack.emplace_back(E);
+    }
 
     return true;
   }
@@ -275,49 +276,57 @@ struct MapRegionCounters : public RecursiveASTVisitor<MapRegionCounters> {
   // an AST Stmt node.  MC/DC will use it to to signal when the top of a
   // logical operation (boolean expression) nest is encountered.
   bool dataTraverseStmtPost(Stmt *S) {
-    /// If MC/DC is not enabled, MCDCMaxCond will be set to 0. Do nothing.
-    if (MCDCMaxCond == 0)
+    if (DecisionStack.empty())
       return true;
 
-    if (const Expr *E = dyn_cast<Expr>(S)) {
-      const BinaryOperator *BinOp = dyn_cast<BinaryOperator>(E->IgnoreParens());
-      if (BinOp && BinOp->isLogicalOp()) {
-        assert(LogOpStack.back() == BinOp);
-        LogOpStack.pop_back();
-
-        /// At the top of logical operator nest:
-        if (LogOpStack.empty()) {
-          /// Was the "split-nested" logical operator case encountered?
-          if (SplitNestedLogicalOp) {
-            unsigned DiagID = Diag.getCustomDiagID(
-                DiagnosticsEngine::Warning,
-                "unsupported MC/DC boolean expression; "
-                "contains an operation with a nested boolean expression. "
-                "Expression will not be covered");
-            Diag.Report(S->getBeginLoc(), DiagID);
-            return true;
-          }
-
-          /// Was the maximum number of conditions encountered?
-          if (NumCond > MCDCMaxCond) {
-            unsigned DiagID = Diag.getCustomDiagID(
-                DiagnosticsEngine::Warning,
-                "unsupported MC/DC boolean expression; "
-                "number of conditions (%0) exceeds max (%1). "
-                "Expression will not be covered");
-            Diag.Report(S->getBeginLoc(), DiagID) << NumCond << MCDCMaxCond;
-            return true;
-          }
-
-          // Otherwise, allocate the Decision.
-          MCDCState.DecisionByStmt[BinOp].ID = MCDCState.DecisionByStmt.size();
-        }
-        return true;
+    /// If MC/DC is not enabled, MCDCMaxCond will be set to 0. Do nothing.
+    assert(MCDCMaxCond > 0);
+
+    auto &StackTop = DecisionStack.back();
+
+    if (StackTop.DecisionExpr != S) {
+      if (StackTop.Leaves.contains(S)) {
+        assert(StackTop.Split);
+        StackTop.Split = false;
       }
+
+      return true;
+    }
+
+    /// Allocate the entry (with Valid=false)
+    auto &DecisionEntry =
+        MCDCState
+            .DecisionByStmt[CodeGenFunction::stripCond(StackTop.DecisionExpr)];
+
+    /// Was the "split-nested" logical operator case encountered?
+    if (false && DecisionStack.size() > 1) {
+      unsigned DiagID = Diag.getCustomDiagID(
+          DiagnosticsEngine::Warning,
+          "unsupported MC/DC boolean expression; "
+          "contains an operation with a nested boolean expression. "
+          "Expression will not be covered");
+      Diag.Report(S->getBeginLoc(), DiagID);
+      DecisionStack.pop_back();
+      return true;
+    }
+
+    /// Was the maximum number of conditions encountered?
+    auto NumCond = StackTop.Leaves.size();
+    if (NumCond > MCDCMaxCond) {
+      unsigned DiagID =
+          Diag.getCustomDiagID(DiagnosticsEngine::Warning,
+                               "unsupported MC/DC boolean expression; "
+                               "number of conditions (%0) exceeds max (%1). "
+                               "Expression will not be covered");
+      Diag.Report(S->getBeginLoc(), DiagID) << NumCond << MCDCMaxCond;
+      DecisionStack.pop_back();
+      return true;
     }
 
-    if (!LogOpStack.empty())
-      NonLogOpStack.pop_back();
+    // The Decision is validated.
+    DecisionEntry.ID = MCDCState.DecisionByStmt.size() - 1;
+
+    DecisionStack.pop_back();
 
     return true;
   }
@@ -329,14 +338,17 @@ struct MapRegionCounters : public RecursiveASTVisitor<MapRegionCounters> {
   /// order to use MC/DC, count the number of total LHS and RHS conditions.
   bool VisitBinaryOperator(BinaryOperator *S) {
     if (S->isLogicalOp()) {
-      if (CodeGenFunction::isInstrumentedCondition(S->getLHS()))
-        NumCond++;
+      if (CodeGenFunction::isInstrumentedCondition(S->getLHS())) {
+        if (!DecisionStack.empty())
+          DecisionStack.back().Leaves.insert(S->getLHS());
+      }
 
       if (CodeGenFunction::isInstrumentedCondition(S->getRHS())) {
         if (ProfileVersion >= llvm::IndexedInstrProf::Version7)
           CounterMap[S->getRHS()] = NextCounter++;
 
-        NumCond++;
+        if (!DecisionStack.empty())
+          DecisionStack.back().Leaves.insert(S->getRHS());
       }
     }
     return Base::VisitBinaryOperator(S);

clang/test/CoverageMapping/mcdc-nested-expr.cpp

@@ -1,20 +1,42 @@
-// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++11 -fcoverage-mcdc -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only %s 2> %t.stderr.txt | FileCheck %s
-// RUN: FileCheck %s --check-prefix=WARN < %t.stderr.txt
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++11 -fcoverage-mcdc -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only %s 2>&1 | FileCheck %s
+
+// CHECK-NOT: warning: unsupported MC/DC boolean expression; contains an operation with a nested boolean expression.
 
 // "Split-nest" -- boolean expressions within boolean expressions.
 extern bool bar(bool);
 // CHECK: func_split_nest{{.*}}:
 bool func_split_nest(bool a, bool b, bool c, bool d, bool e, bool f, bool g) {
-  // WARN: :[[@LINE+1]]:14: warning: unsupported MC/DC boolean expression; contains an operation with a nested boolean expression.
   bool res = a && b && c && bar(d && e) && f && g;
+  // CHECK:  Decision,File 0, [[@LINE-1]]:14 -> [[#L:@LINE-1]]:50 = M:10, C:6
+  // CHECK:  Branch,File 0, [[#L]]:14 -> [[#L]]:15 = #9, (#0 - #9) [1,6,0]
+  // CHECK:  Branch,File 0, [[#L]]:19 -> [[#L]]:20 = #10, (#9 - #10) [6,5,0]
+  // CHECK:  Branch,File 0, [[#L]]:24 -> [[#L]]:25 = #8, (#7 - #8) [5,4,0]
+  // CHECK:  Branch,File 0, [[#L]]:29 -> [[#L]]:40 = #6, (#5 - #6) [4,3,0]
+
+  // The inner expr -- "d && e" (w/o parentheses)
+  // CHECK:  Decision,File 0, [[#L]]:33 -> [[#L]]:39 = M:3, C:2
+  // CHECK:  Branch,File 0, [[#L]]:33 -> [[#L]]:34 = #11, (#5 - #11) [1,2,0]
+  // CHECK:  Branch,File 0, [[#L]]:38 -> [[#L]]:39 = #12, (#11 - #12) [2,0,0]
+
+  // CHECK:  Branch,File 0, [[#L]]:44 -> [[#L]]:45 = #4, (#3 - #4) [3,2,0]
+  // CHECK:  Branch,File 0, [[#L]]:49 -> [[#L]]:50 = #2, (#1 - #2) [2,0,0]
   return bar(res);
 }
 
 // The inner expr begins with the same Loc as the outer expr
 // CHECK: func_condop{{.*}}:
 bool func_condop(bool a, bool b, bool c) {
-  // WARN: :[[@LINE+1]]:10: warning: unsupported MC/DC boolean expression; contains an operation with a nested boolean expression.
   return (a && b ? true : false) && c;
+  // CHECK:  Decision,File 0, [[@LINE-1]]:10 -> [[#L:@LINE-1]]:38 = M:6, C:2
+  // This covers outer parenthses.
+  // CHECK:  Branch,File 0, [[#L]]:10 -> [[#L]]:33 = #1, (#0 - #1) [1,2,0]
+
+  // The inner expr "a && b" (w/o parenthses)
+  // CHECK:  Decision,File 0, [[#L]]:11 -> [[#L]]:17 = M:3, C:2
+  // CHECK:  Branch,File 0, [[#L]]:11 -> [[#L]]:12 = #4, (#0 - #4) [1,2,0]
+  // CHECK:  Branch,File 0, [[#L]]:16 -> [[#L]]:17 = #5, (#4 - #5) [2,0,0]
+
+  // CHECK:  Branch,File 0, [[#L]]:37 -> [[#L]]:38 = #2, (#1 - #2) [2,0,0]
 }
 
 // __builtin_expect

clang/test/Frontend/custom-diag-werror-interaction.c

@@ -1,9 +1,9 @@
-// RUN: %clang_cc1 -emit-llvm-only -fprofile-instrument=clang -fcoverage-mcdc -Werror -Wno-unused-value %s -verify
+// RUN: %clang_cc1 -emit-llvm-only -fprofile-instrument=clang -fcoverage-mcdc -Werror -Wno-unused-value %s -verify -fmcdc-max-conditions=2
 
 int foo(int x);
 
 int main(void) {
     int a, b, c;
-    a && foo( b && c ); // expected-warning{{unsupported MC/DC boolean expression; contains an operation with a nested boolean expression. Expression will not be covered}}
+    a && foo( a && b && c ); // expected-warning{{unsupported MC/DC boolean expression; number of conditions (3) exceeds max (2). Expression will not be covered}}
     return 0;
 }