[llvm-branch-commits] [clang] [MC/DC] Enable nested expressions (PR #125413)
NAKAMURA Takumi via llvm-branch-commits
llvm-branch-commits at lists.llvm.org
Sun Feb 2 05:36:40 PST 2025
https://github.com/chapuni created https://github.com/llvm/llvm-project/pull/125413
A warning "contains an operation with a nested boolean expression." is no longer emitted. At the moment, split expressions are treated as individual Decisions.
>From c56ecc30e9fd1a674073e362fbfcc6b43f2f52e2 Mon Sep 17 00:00:00 2001
From: NAKAMURA Takumi <geek4civic at gmail.com>
Date: Sun, 2 Feb 2025 22:06:32 +0900
Subject: [PATCH] [MC/DC] Enable nested expressions
A warning "contains an operation with a nested boolean expression." is
no longer emitter. At the moment, split expressions are treated as
individual Decisions.
---
clang/lib/CodeGen/CodeGenPGO.cpp | 150 ++++++++++--------
.../test/CoverageMapping/mcdc-nested-expr.cpp | 30 +++-
.../Frontend/custom-diag-werror-interaction.c | 4 +-
3 files changed, 109 insertions(+), 75 deletions(-)
diff --git a/clang/lib/CodeGen/CodeGenPGO.cpp b/clang/lib/CodeGen/CodeGenPGO.cpp
index 7d16f673ada419e..0c3973aa4dccfdc 100644
--- a/clang/lib/CodeGen/CodeGenPGO.cpp
+++ b/clang/lib/CodeGen/CodeGenPGO.cpp
@@ -228,10 +228,17 @@ 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) {
@@ -239,34 +246,28 @@ struct MapRegionCounters : public RecursiveASTVisitor<MapRegionCounters> {
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);
diff --git a/clang/test/CoverageMapping/mcdc-nested-expr.cpp b/clang/test/CoverageMapping/mcdc-nested-expr.cpp
index bb82873e3b600d0..8aa31fbdd2ab7bc 100644
--- a/clang/test/CoverageMapping/mcdc-nested-expr.cpp
+++ b/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
diff --git a/clang/test/Frontend/custom-diag-werror-interaction.c b/clang/test/Frontend/custom-diag-werror-interaction.c
index 997c8c11ff0e0d7..4b02e8fbf328a7b 100644
--- a/clang/test/Frontend/custom-diag-werror-interaction.c
+++ b/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;
}
More information about the llvm-branch-commits
mailing list