[clang] [clang-tools-extra] [analyzer][clang-tidy][NFC] Clean up eagerly-assume handling (PR #112209)
Donát Nagy via cfe-commits
cfe-commits at lists.llvm.org
Mon Oct 14 10:14:21 PDT 2024
https://github.com/NagyDonat updated https://github.com/llvm/llvm-project/pull/112209
>From ea6ab3fe84e5ac89f82def877c37c8409889d01d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Mon, 14 Oct 2024 15:34:55 +0200
Subject: [PATCH 1/4] [analyzer][clang-tidy][NFC] Clean up eagerly-assume
handling
This commit is a collection of several very minor code quality
improvements. The main goal is removing the misleading "Bin" substring
from the names of several methods and variables (like
`evalEagerlyAssumedBinOpBifurcation`) that are also applied for the
unary logical not operator.
In addition to this, I clarified the doc-comment of the method
`evalEagerlyAssumedBinOpBifurcation` and refactored the body of this
method to fix the capitalization of variable names and replace an
obsolete use of `std::tie` with a structured binding.
Finally, the data member `eagerlyAssumeBinOpBifurcation` of the class
`AnalyzerOptions` was completely removed (including a line in clang-tidy
that sets it to true), because it was never read by any code.
Note that the eagerly-assume mode of the analyzer is controlled by a
different boolean member of `AnalyzerOptions` which is called
`ShouldEagerlyAssume` and is defined via the macro magic from
`AnalyzerOptions.def`.
---
clang-tools-extra/clang-tidy/ClangTidy.cpp | 1 -
.../StaticAnalyzer/Core/AnalyzerOptions.h | 8 ++--
.../Core/PathSensitive/ExprEngine.h | 12 +++---
.../Core/BugReporterVisitors.cpp | 2 +-
clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 41 +++++++++----------
5 files changed, 29 insertions(+), 35 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/ClangTidy.cpp b/clang-tools-extra/clang-tidy/ClangTidy.cpp
index 62f9d19b2a362f..c4cac7d27b77c2 100644
--- a/clang-tools-extra/clang-tidy/ClangTidy.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidy.cpp
@@ -458,7 +458,6 @@ ClangTidyASTConsumerFactory::createASTConsumer(
if (!AnalyzerOptions.CheckersAndPackages.empty()) {
setStaticAnalyzerCheckerOpts(Context.getOptions(), AnalyzerOptions);
AnalyzerOptions.AnalysisDiagOpt = PD_NONE;
- AnalyzerOptions.eagerlyAssumeBinOpBifurcation = true;
std::unique_ptr<ento::AnalysisASTConsumer> AnalysisConsumer =
ento::CreateAnalysisConsumer(Compiler);
AnalysisConsumer->AddDiagnosticConsumer(
diff --git a/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h b/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
index 3a3c1a13d67dd5..2f4cd277cccdc6 100644
--- a/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
+++ b/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
@@ -229,8 +229,6 @@ class AnalyzerOptions : public RefCountedBase<AnalyzerOptions> {
unsigned AnalyzerDisplayProgress : 1;
unsigned AnalyzerNoteAnalysisEntryPoints : 1;
- unsigned eagerlyAssumeBinOpBifurcation : 1;
-
unsigned TrimGraph : 1;
unsigned visualizeExplodedGraphWithGraphViz : 1;
unsigned UnoptimizedCFG : 1;
@@ -293,9 +291,9 @@ class AnalyzerOptions : public RefCountedBase<AnalyzerOptions> {
ShowConfigOptionsList(false),
ShouldEmitErrorsOnInvalidConfigValue(false), AnalyzeAll(false),
AnalyzerDisplayProgress(false), AnalyzerNoteAnalysisEntryPoints(false),
- eagerlyAssumeBinOpBifurcation(false), TrimGraph(false),
- visualizeExplodedGraphWithGraphViz(false), UnoptimizedCFG(false),
- PrintStats(false), NoRetryExhausted(false), AnalyzerWerror(false) {}
+ TrimGraph(false), visualizeExplodedGraphWithGraphViz(false),
+ UnoptimizedCFG(false), PrintStats(false), NoRetryExhausted(false),
+ AnalyzerWerror(false) {}
/// Interprets an option's string value as a boolean. The "true" string is
/// interpreted as true and the "false" string is interpreted as false.
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
index 04eacd1df7ffe2..ad21943f56fc8e 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -583,14 +583,14 @@ class ExprEngine {
ExplodedNode *Pred,
ExplodedNodeSet &Dst);
- /// evalEagerlyAssumeBinOpBifurcation - Given the nodes in 'Src', eagerly assume symbolic
- /// expressions of the form 'x != 0' and generate new nodes (stored in Dst)
- /// with those assumptions.
- void evalEagerlyAssumeBinOpBifurcation(ExplodedNodeSet &Dst, ExplodedNodeSet &Src,
- const Expr *Ex);
+ /// evalEagerlyAssumeOpBifurcation - Given the nodes in 'Src', eagerly assume
+ /// comparison operator expressions like 'x != 0' or logical negation like
+ /// '!foo' and generate new nodes (stored in Dst) with those assumptions.
+ void evalEagerlyAssumeOpBifurcation(ExplodedNodeSet &Dst,
+ ExplodedNodeSet &Src, const Expr *Ex);
static std::pair<const ProgramPointTag *, const ProgramPointTag *>
- geteagerlyAssumeBinOpBifurcationTags();
+ getEagerlyAssumeOpBifurcationTags();
ProgramStateRef handleLValueBitCast(ProgramStateRef state, const Expr *Ex,
const LocationContext *LCtx, QualType T,
diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index 68c8a8dc682507..de2d89bf09c3b9 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -2695,7 +2695,7 @@ ConditionBRVisitor::VisitNodeImpl(const ExplodedNode *N,
PathSensitiveBugReport &BR) {
ProgramPoint ProgPoint = N->getLocation();
const std::pair<const ProgramPointTag *, const ProgramPointTag *> &Tags =
- ExprEngine::geteagerlyAssumeBinOpBifurcationTags();
+ ExprEngine::getEagerlyAssumeOpBifurcationTags();
// If an assumption was made on a branch, it should be caught
// here by looking at the state transition.
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
index 43ab646d398b31..9bfe93b6fc7ba6 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -2129,7 +2129,7 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred,
(B->isRelationalOp() || B->isEqualityOp())) {
ExplodedNodeSet Tmp;
VisitBinaryOperator(cast<BinaryOperator>(S), Pred, Tmp);
- evalEagerlyAssumeBinOpBifurcation(Dst, Tmp, cast<Expr>(S));
+ evalEagerlyAssumeOpBifurcation(Dst, Tmp, cast<Expr>(S));
}
else
VisitBinaryOperator(cast<BinaryOperator>(S), Pred, Dst);
@@ -2402,7 +2402,7 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred,
if (AMgr.options.ShouldEagerlyAssume && (U->getOpcode() == UO_LNot)) {
ExplodedNodeSet Tmp;
VisitUnaryOperator(U, Pred, Tmp);
- evalEagerlyAssumeBinOpBifurcation(Dst, Tmp, U);
+ evalEagerlyAssumeOpBifurcation(Dst, Tmp, U);
}
else
VisitUnaryOperator(U, Pred, Dst);
@@ -3742,20 +3742,18 @@ void ExprEngine::evalLocation(ExplodedNodeSet &Dst,
BldrTop.addNodes(Tmp);
}
-std::pair<const ProgramPointTag *, const ProgramPointTag*>
-ExprEngine::geteagerlyAssumeBinOpBifurcationTags() {
- static SimpleProgramPointTag
- eagerlyAssumeBinOpBifurcationTrue(TagProviderName,
- "Eagerly Assume True"),
- eagerlyAssumeBinOpBifurcationFalse(TagProviderName,
- "Eagerly Assume False");
- return std::make_pair(&eagerlyAssumeBinOpBifurcationTrue,
- &eagerlyAssumeBinOpBifurcationFalse);
+std::pair<const ProgramPointTag *, const ProgramPointTag *>
+ExprEngine::getEagerlyAssumeOpBifurcationTags() {
+ static SimpleProgramPointTag eagerlyAssumeOpBifurcationTrue(
+ TagProviderName, "Eagerly Assume True"),
+ eagerlyAssumeOpBifurcationFalse(TagProviderName, "Eagerly Assume False");
+ return std::make_pair(&eagerlyAssumeOpBifurcationTrue,
+ &eagerlyAssumeOpBifurcationFalse);
}
-void ExprEngine::evalEagerlyAssumeBinOpBifurcation(ExplodedNodeSet &Dst,
- ExplodedNodeSet &Src,
- const Expr *Ex) {
+void ExprEngine::evalEagerlyAssumeOpBifurcation(ExplodedNodeSet &Dst,
+ ExplodedNodeSet &Src,
+ const Expr *Ex) {
StmtNodeBuilder Bldr(Src, Dst, *currBldrCtx);
for (const auto Pred : Src) {
@@ -3767,28 +3765,27 @@ void ExprEngine::evalEagerlyAssumeBinOpBifurcation(ExplodedNodeSet &Dst,
continue;
}
- ProgramStateRef state = Pred->getState();
- SVal V = state->getSVal(Ex, Pred->getLocationContext());
+ ProgramStateRef State = Pred->getState();
+ SVal V = State->getSVal(Ex, Pred->getLocationContext());
std::optional<nonloc::SymbolVal> SEV = V.getAs<nonloc::SymbolVal>();
if (SEV && SEV->isExpression()) {
- const std::pair<const ProgramPointTag *, const ProgramPointTag*> &tags =
- geteagerlyAssumeBinOpBifurcationTags();
+ const std::pair<const ProgramPointTag *, const ProgramPointTag *> &Tags =
+ getEagerlyAssumeOpBifurcationTags();
- ProgramStateRef StateTrue, StateFalse;
- std::tie(StateTrue, StateFalse) = state->assume(*SEV);
+ auto [StateTrue, StateFalse] = State->assume(*SEV);
// First assume that the condition is true.
if (StateTrue) {
SVal Val = svalBuilder.makeIntVal(1U, Ex->getType());
StateTrue = StateTrue->BindExpr(Ex, Pred->getLocationContext(), Val);
- Bldr.generateNode(Ex, Pred, StateTrue, tags.first);
+ Bldr.generateNode(Ex, Pred, StateTrue, Tags.first);
}
// Next, assume that the condition is false.
if (StateFalse) {
SVal Val = svalBuilder.makeIntVal(0U, Ex->getType());
StateFalse = StateFalse->BindExpr(Ex, Pred->getLocationContext(), Val);
- Bldr.generateNode(Ex, Pred, StateFalse, tags.second);
+ Bldr.generateNode(Ex, Pred, StateFalse, Tags.second);
}
}
}
>From fc143aff52931d6b17a2cacb87cee5092dfc4bad Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Mon, 14 Oct 2024 18:32:20 +0200
Subject: [PATCH 2/4] Rewrite the documentation of eagerly-assume
---
.../clang/StaticAnalyzer/Core/AnalyzerOptions.def | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def b/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
index 737bc8e86cfb6a..ad2dbffe88326d 100644
--- a/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
+++ b/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
@@ -299,13 +299,12 @@ ANALYZER_OPTION(
ANALYZER_OPTION(
bool, ShouldEagerlyAssume, "eagerly-assume",
- "Whether we should eagerly assume evaluations of conditionals, thus, "
- "bifurcating the path. This indicates how the engine should handle "
- "expressions such as: 'x = (y != 0)'. When this is true then the "
- "subexpression 'y != 0' will be eagerly assumed to be true or false, thus "
- "evaluating it to the integers 0 or 1 respectively. The upside is that "
- "this can increase analysis precision until we have a better way to lazily "
- "evaluate such logic. The downside is that it eagerly bifurcates paths.",
+ "If this is enabled (the default behavior), when the analyzer encounters "
+ "a comparison operator or logical negation, it immediately splits the "
+ "state to separate the case when the expression is true and the case when "
+ "it's false. The upside is that this can increase analysis precision until "
+ "we have a better way to lazily evaluate such logic; the downside is that "
+ "it eagerly bifurcates paths.",
true)
ANALYZER_OPTION(
>From f03306869da1790d6fe4b96a4e2d0b2f61c8b2e4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Mon, 14 Oct 2024 18:33:22 +0200
Subject: [PATCH 3/4] Improve doc comment of eagerlyAssumeOpBifurcation
---
.../clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
index ad21943f56fc8e..d087d87aec984f 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -584,8 +584,7 @@ class ExprEngine {
ExplodedNodeSet &Dst);
/// evalEagerlyAssumeOpBifurcation - Given the nodes in 'Src', eagerly assume
- /// comparison operator expressions like 'x != 0' or logical negation like
- /// '!foo' and generate new nodes (stored in Dst) with those assumptions.
+ /// concrete boolean valuse for 'Ex', storing the resulting nodes in 'Dst'.
void evalEagerlyAssumeOpBifurcation(ExplodedNodeSet &Dst,
ExplodedNodeSet &Src, const Expr *Ex);
>From 8c4ec720cf7446dd94ca208a9afaef959fa4719e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Mon, 14 Oct 2024 19:13:45 +0200
Subject: [PATCH 4/4] s/OpBifurcation/Bifurcation/, simplify the code
---
.../Core/PathSensitive/ExprEngine.h | 10 +++----
.../Core/BugReporterVisitors.cpp | 2 +-
clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 30 +++++++++----------
3 files changed, 20 insertions(+), 22 deletions(-)
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
index d087d87aec984f..8c7493e27fcaa6 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -583,13 +583,13 @@ class ExprEngine {
ExplodedNode *Pred,
ExplodedNodeSet &Dst);
- /// evalEagerlyAssumeOpBifurcation - Given the nodes in 'Src', eagerly assume
- /// concrete boolean valuse for 'Ex', storing the resulting nodes in 'Dst'.
- void evalEagerlyAssumeOpBifurcation(ExplodedNodeSet &Dst,
- ExplodedNodeSet &Src, const Expr *Ex);
+ /// evalEagerlyAssumeBifurcation - Given the nodes in 'Src', eagerly assume
+ /// concrete boolean values for 'Ex', storing the resulting nodes in 'Dst'.
+ void evalEagerlyAssumeBifurcation(ExplodedNodeSet &Dst, ExplodedNodeSet &Src,
+ const Expr *Ex);
static std::pair<const ProgramPointTag *, const ProgramPointTag *>
- getEagerlyAssumeOpBifurcationTags();
+ getEagerlyAssumeBifurcationTags();
ProgramStateRef handleLValueBitCast(ProgramStateRef state, const Expr *Ex,
const LocationContext *LCtx, QualType T,
diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index de2d89bf09c3b9..c4479db14b791d 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -2695,7 +2695,7 @@ ConditionBRVisitor::VisitNodeImpl(const ExplodedNode *N,
PathSensitiveBugReport &BR) {
ProgramPoint ProgPoint = N->getLocation();
const std::pair<const ProgramPointTag *, const ProgramPointTag *> &Tags =
- ExprEngine::getEagerlyAssumeOpBifurcationTags();
+ ExprEngine::getEagerlyAssumeBifurcationTags();
// If an assumption was made on a branch, it should be caught
// here by looking at the state transition.
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
index 9bfe93b6fc7ba6..48506f85246f33 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -2129,7 +2129,7 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred,
(B->isRelationalOp() || B->isEqualityOp())) {
ExplodedNodeSet Tmp;
VisitBinaryOperator(cast<BinaryOperator>(S), Pred, Tmp);
- evalEagerlyAssumeOpBifurcation(Dst, Tmp, cast<Expr>(S));
+ evalEagerlyAssumeBifurcation(Dst, Tmp, cast<Expr>(S));
}
else
VisitBinaryOperator(cast<BinaryOperator>(S), Pred, Dst);
@@ -2402,7 +2402,7 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred,
if (AMgr.options.ShouldEagerlyAssume && (U->getOpcode() == UO_LNot)) {
ExplodedNodeSet Tmp;
VisitUnaryOperator(U, Pred, Tmp);
- evalEagerlyAssumeOpBifurcation(Dst, Tmp, U);
+ evalEagerlyAssumeBifurcation(Dst, Tmp, U);
}
else
VisitUnaryOperator(U, Pred, Dst);
@@ -3743,20 +3743,19 @@ void ExprEngine::evalLocation(ExplodedNodeSet &Dst,
}
std::pair<const ProgramPointTag *, const ProgramPointTag *>
-ExprEngine::getEagerlyAssumeOpBifurcationTags() {
- static SimpleProgramPointTag eagerlyAssumeOpBifurcationTrue(
- TagProviderName, "Eagerly Assume True"),
- eagerlyAssumeOpBifurcationFalse(TagProviderName, "Eagerly Assume False");
- return std::make_pair(&eagerlyAssumeOpBifurcationTrue,
- &eagerlyAssumeOpBifurcationFalse);
+ExprEngine::getEagerlyAssumeBifurcationTags() {
+ static SimpleProgramPointTag TrueTag(TagProviderName, "Eagerly Assume True"),
+ FalseTag(TagProviderName, "Eagerly Assume False");
+
+ return std::make_pair(&TrueTag, &FalseTag);
}
-void ExprEngine::evalEagerlyAssumeOpBifurcation(ExplodedNodeSet &Dst,
- ExplodedNodeSet &Src,
- const Expr *Ex) {
+void ExprEngine::evalEagerlyAssumeBifurcation(ExplodedNodeSet &Dst,
+ ExplodedNodeSet &Src,
+ const Expr *Ex) {
StmtNodeBuilder Bldr(Src, Dst, *currBldrCtx);
- for (const auto Pred : Src) {
+ for (ExplodedNode *Pred : Src) {
// Test if the previous node was as the same expression. This can happen
// when the expression fails to evaluate to anything meaningful and
// (as an optimization) we don't generate a node.
@@ -3769,8 +3768,7 @@ void ExprEngine::evalEagerlyAssumeOpBifurcation(ExplodedNodeSet &Dst,
SVal V = State->getSVal(Ex, Pred->getLocationContext());
std::optional<nonloc::SymbolVal> SEV = V.getAs<nonloc::SymbolVal>();
if (SEV && SEV->isExpression()) {
- const std::pair<const ProgramPointTag *, const ProgramPointTag *> &Tags =
- getEagerlyAssumeOpBifurcationTags();
+ auto [TrueTag, FalseTag] = getEagerlyAssumeBifurcationTags();
auto [StateTrue, StateFalse] = State->assume(*SEV);
@@ -3778,14 +3776,14 @@ void ExprEngine::evalEagerlyAssumeOpBifurcation(ExplodedNodeSet &Dst,
if (StateTrue) {
SVal Val = svalBuilder.makeIntVal(1U, Ex->getType());
StateTrue = StateTrue->BindExpr(Ex, Pred->getLocationContext(), Val);
- Bldr.generateNode(Ex, Pred, StateTrue, Tags.first);
+ Bldr.generateNode(Ex, Pred, StateTrue, TrueTag);
}
// Next, assume that the condition is false.
if (StateFalse) {
SVal Val = svalBuilder.makeIntVal(0U, Ex->getType());
StateFalse = StateFalse->BindExpr(Ex, Pred->getLocationContext(), Val);
- Bldr.generateNode(Ex, Pred, StateFalse, Tags.second);
+ Bldr.generateNode(Ex, Pred, StateFalse, FalseTag);
}
}
}
More information about the cfe-commits
mailing list