[analyzer][clang-tidy][NFC] Clean up eagerly-assume handling #112209
Conversation
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`.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clang Author: Donát Nagy (NagyDonat) ChangesThis 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 In addition to this, I clarified the doc-comment of the method Finally, the data member Note that the eagerly-assume mode of the analyzer is controlled by a different boolean member of Full diff: https://github.com/llvm/llvm-project/pull/112209.diff 5 Files Affected:
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);
}
}
}
|
clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
Outdated
Show resolved
Hide resolved
|
I want to have a look at this PR tomorrow. |
|
@steakhal May I merge this commit? |
| @@ -458,7 +458,6 @@ ClangTidyASTConsumerFactory::createASTConsumer( | |||
| if (!AnalyzerOptions.CheckersAndPackages.empty()) { | |||
| setStaticAnalyzerCheckerOpts(Context.getOptions(), AnalyzerOptions); | |||
| AnalyzerOptions.AnalysisDiagOpt = PD_NONE; | |||
| AnalyzerOptions.eagerlyAssumeBinOpBifurcation = true; | |||
There was a problem hiding this comment.
Is this a functionality loss from Tidy's perspective? AFAIK, there is no way to set CSA analyser engine options when invoked through Tidy.
There was a problem hiding this comment.
Clang-Tidy can set CSA options, I recently added support for it for --verify-config in #109523, where I had checked which syntax it has, and if the syntax worked beforehand.
E.g.: clang-analyzer-optin.cplusplus.UninitializedObject:Pedantic: true
There was a problem hiding this comment.
No, this is not functionality loss from Tidy's perspective, because this data member is dead, its value doesn't influence anything. The eagerly-assume feature is controlled by a different data member AnalyzerOptions.ShouldEagerlyAssume (which is true by default).
| "we have a better way to lazily evaluate such logic; the downside is that " | ||
| "it eagerly bifurcates paths.", |
There was a problem hiding this comment.
I know this part of the change is only because of formatting, but here we are saying that "the downside [of eagerly bifurcating the path] is that it eagerly bifurcates paths", which has a very low information value.
There was a problem hiding this comment.
Fair point, but I think it's still useful to highlight that bifurcating paths is a downside, not a helpful effect.
There was a problem hiding this comment.
My problem is that it does not explain well why it is a downside, considering we have a strong claim "increases analysis precision" in the other sentence.
There was a problem hiding this comment.
"the downside [of eagerly bifurcating the path] is that it eagerly bifurcates paths"
I think in this case it's more like "the downside [of eagerly assuming the value of a bool expression] is that it eagerly bifurcates paths"
My problem is that it does not explain well why it is a downside
I think in this case the why is that "it eagerly bifurcates paths".
There was a problem hiding this comment.
I think in this case it's more like "the downside [of eagerly assuming the value of a bool expression] is that it eagerly bifurcates paths"
I do not think there is a significant difference here, because the help text, as it stands now, immediately tries to explain what a "bifurcation" is (which one may or may not understand that this is actually the definition of that term) by saying "splits the state to separate […] true and […] false".
So we say that turning this option on will cause bifurcation…
I think in this case the why is that "it eagerly bifurcates paths".
…and then go on and say that the bifurcation (which is the only immediate effect of turning this option on) is a downside.
That still does not explain to me why this would be a problem, and what trade-offs I am making when the benefit is the increased precision. Will it cause more CPU use? Will it make the analysis run longer? Or, conversely, will it actually make it run shorter? There is this claim about a behavioural change with no mentions of the actual observeable difference.
And, remember, these documentation tidbits exist for the user, as it is dumped with the --help flags and similar. Users will not know the intimacies of the analyser's implementation!
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
evalEagerlyAssumedBinOpBifurcationand refactored the body of this method to fix the capitalization of variable names and replace an obsolete use ofstd::tiewith a structured binding.Finally, the data member
eagerlyAssumeBinOpBifurcationof the classAnalyzerOptionswas 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
AnalyzerOptionswhich is calledShouldEagerlyAssumeand is defined via the macro magic fromAnalyzerOptions.def.