Skip to content

Fix #12266 (Performance: valueFlowCondition slowdown) #5837

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
danmar merged 1 commit into from
Jan 29, 2024
Merged

Fix #12266 (Performance: valueFlowCondition slowdown) #5837

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -578,7 +578,7 @@ $(libcppdir)/errorlogger.o: lib/errorlogger.cpp externals/tinyxml2/tinyxml2.h li
$(libcppdir)/errortypes.o: lib/errortypes.cpp lib/config.h lib/errortypes.h lib/utils.h
$(CXX) ${INCLUDE_FOR_LIB} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ $(libcppdir)/errortypes.cpp

$(libcppdir)/forwardanalyzer.o: lib/forwardanalyzer.cpp lib/addoninfo.h lib/analyzer.h lib/astutils.h lib/config.h lib/errortypes.h lib/forwardanalyzer.h lib/library.h lib/mathlib.h lib/platform.h lib/settings.h lib/smallvector.h lib/sourcelocation.h lib/standards.h lib/suppressions.h lib/symboldatabase.h lib/templatesimplifier.h lib/token.h lib/utils.h lib/valueptr.h lib/vfvalue.h
$(libcppdir)/forwardanalyzer.o: lib/forwardanalyzer.cpp lib/addoninfo.h lib/analyzer.h lib/astutils.h lib/color.h lib/config.h lib/errorlogger.h lib/errortypes.h lib/forwardanalyzer.h lib/library.h lib/mathlib.h lib/platform.h lib/settings.h lib/smallvector.h lib/sourcelocation.h lib/standards.h lib/suppressions.h lib/symboldatabase.h lib/templatesimplifier.h lib/token.h lib/tokenlist.h lib/utils.h lib/valueptr.h lib/vfvalue.h
$(CXX) ${INCLUDE_FOR_LIB} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ $(libcppdir)/forwardanalyzer.cpp

$(libcppdir)/fwdanalysis.o: lib/fwdanalysis.cpp lib/astutils.h lib/config.h lib/errortypes.h lib/fwdanalysis.h lib/library.h lib/mathlib.h lib/smallvector.h lib/sourcelocation.h lib/standards.h lib/symboldatabase.h lib/templatesimplifier.h lib/token.h lib/utils.h lib/vfvalue.h
Expand Down
30 changes: 24 additions & 6 deletions lib/forwardanalyzer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,13 @@
#include "analyzer.h"
#include "astutils.h"
#include "config.h"
#include "errorlogger.h"
#include "errortypes.h"
#include "mathlib.h"
#include "settings.h"
#include "symboldatabase.h"
#include "token.h"
#include "tokenlist.h"
#include "utils.h"
#include "valueptr.h"
#include "vfvalue.h"
Expand All @@ -45,16 +47,19 @@ namespace {
struct ForwardTraversal {
enum class Progress { Continue, Break, Skip };
enum class Terminate { None, Bail, Inconclusive };
ForwardTraversal(const ValuePtr<Analyzer>& analyzer, const Settings& settings)
: analyzer(analyzer), settings(settings)
ForwardTraversal(const ValuePtr<Analyzer>& analyzer, const TokenList& tokenList, ErrorLogger* const errorLogger, const Settings& settings)
: analyzer(analyzer), tokenList(tokenList), errorLogger(errorLogger), settings(settings)
{}
ValuePtr<Analyzer> analyzer;
const TokenList& tokenList;
ErrorLogger* const errorLogger;
const Settings& settings;
Analyzer::Action actions;
bool analyzeOnly{};
bool analyzeTerminate{};
Analyzer::Terminate terminate = Analyzer::Terminate::None;
std::vector<Token*> loopEnds;
int branchCount = 0;

Progress Break(Analyzer::Terminate t = Analyzer::Terminate::None) {
if ((!analyzeOnly || analyzeTerminate) && t != Analyzer::Terminate::None)
Expand Down Expand Up @@ -648,6 +653,10 @@ namespace {
}
} else if (tok->isControlFlowKeyword() && Token::Match(tok, "if|while|for (") &&
Token::simpleMatch(tok->next()->link(), ") {")) {
if (settings.checkLevel == Settings::CheckLevel::normal && ++branchCount > 4) {
reportError(Severity::information, "normalCheckLevelMaxBranches", "Limit analysis of branches. Use --check-level=exhausive to analyze all branches.");
return Break(Analyzer::Terminate::Bail);
}
Token* endCond = tok->next()->link();
Token* endBlock = endCond->next()->link();
Token* condTok = getCondTok(tok);
Expand Down Expand Up @@ -828,6 +837,15 @@ namespace {
return Progress::Continue;
}

void reportError(Severity severity, const std::string& id, const std::string& msg) {
if (errorLogger) {
const ErrorMessage::FileLocation loc(tokenList.getSourceFilePath(), 1, 1);
const std::list<ErrorMessage::FileLocation> callstack{loc};
const ErrorMessage errmsg(callstack, tokenList.getSourceFilePath(), severity, msg, id, Certainty::normal);
errorLogger->reportErr(errmsg);
}
}

static bool isFunctionCall(const Token* tok)
{
if (!Token::simpleMatch(tok, "("))
Expand Down Expand Up @@ -886,24 +904,24 @@ namespace {
};
}

Analyzer::Result valueFlowGenericForward(Token* start, const Token* end, const ValuePtr<Analyzer>& a, const Settings& settings)
Analyzer::Result valueFlowGenericForward(Token* start, const Token* end, const ValuePtr<Analyzer>& a, const TokenList& tokenList, ErrorLogger* const errorLogger, const Settings& settings)
{
if (a->invalid())
return Analyzer::Result{Analyzer::Action::None, Analyzer::Terminate::Bail};
ForwardTraversal ft{a, settings};
ForwardTraversal ft{a, tokenList, errorLogger, settings};
if (start)
ft.analyzer->updateState(start);
ft.updateRange(start, end);
return Analyzer::Result{ ft.actions, ft.terminate };
}

Analyzer::Result valueFlowGenericForward(Token* start, const ValuePtr<Analyzer>& a, const Settings& settings)
Analyzer::Result valueFlowGenericForward(Token* start, const ValuePtr<Analyzer>& a, const TokenList& tokenList, ErrorLogger* const errorLogger, const Settings& settings)
{
if (Settings::terminated())
throw TerminateException();
if (a->invalid())
return Analyzer::Result{Analyzer::Action::None, Analyzer::Terminate::Bail};
ForwardTraversal ft{a, settings};
ForwardTraversal ft{a, tokenList, errorLogger, settings};
ft.updateRecursive(start);
return Analyzer::Result{ ft.actions, ft.terminate };
}
6 changes: 5 additions & 1 deletion lib/forwardanalyzer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,19 @@

#include "analyzer.h"

class ErrorLogger;
class Settings;
class Token;
class TokenList;
template<class T> class ValuePtr;

Analyzer::Result valueFlowGenericForward(Token* start,
const Token* end,
const ValuePtr<Analyzer>& a,
const TokenList& tokenList,
ErrorLogger* const errorLogger,
const Settings& settings);

Analyzer::Result valueFlowGenericForward(Token* start, const ValuePtr<Analyzer>& a, const Settings& settings);
Analyzer::Result valueFlowGenericForward(Token* start, const ValuePtr<Analyzer>& a, const TokenList& tokenList, ErrorLogger* const errorLogger, const Settings& settings);

#endif
22 changes: 14 additions & 8 deletions lib/reverseanalyzer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,12 @@

namespace {
struct ReverseTraversal {
ReverseTraversal(const ValuePtr<Analyzer>& analyzer, const Settings& settings)
: analyzer(analyzer), settings(settings)
ReverseTraversal(const ValuePtr<Analyzer>& analyzer, const TokenList& tokenlist, ErrorLogger* const errorLogger, const Settings& settings)
: analyzer(analyzer), tokenlist(tokenlist), errorLogger(errorLogger), settings(settings)
{}
ValuePtr<Analyzer> analyzer;
const TokenList& tokenlist;
ErrorLogger* const errorLogger;
const Settings& settings;

std::pair<bool, bool> evalCond(const Token* tok) const {
Expand Down Expand Up @@ -239,6 +241,8 @@ namespace {
valueFlowGenericForward(nextAfterAstRightmostLeaf(assignTok->astOperand2()),
assignTok->astOperand2()->scope()->bodyEnd,
a,
tokenlist,
errorLogger,
settings);
}
// Assignment to
Expand All @@ -251,8 +255,10 @@ namespace {
valueFlowGenericForward(nextAfterAstRightmostLeaf(assignTok->astOperand2()),
assignTok->astOperand2()->scope()->bodyEnd,
a,
tokenlist,
errorLogger,
settings);
valueFlowGenericReverse(assignTok->astOperand1()->previous(), end, a, settings);
valueFlowGenericReverse(assignTok->astOperand1()->previous(), end, a, tokenlist, errorLogger, settings);
}
}
}
Expand Down Expand Up @@ -285,7 +291,7 @@ namespace {
break;
if (condAction.isModified())
break;
valueFlowGenericForward(condTok, analyzer, settings);
valueFlowGenericForward(condTok, analyzer, tokenlist, errorLogger, settings);
}
Token* thenEnd;
const bool hasElse = Token::simpleMatch(tok->link()->tokAt(-2), "} else {");
Expand Down Expand Up @@ -314,7 +320,7 @@ namespace {
break;

if (!thenAction.isModified() && !elseAction.isModified())
valueFlowGenericForward(condTok, analyzer, settings);
valueFlowGenericForward(condTok, analyzer, tokenlist, errorLogger, settings);
else if (condAction.isRead())
break;
// If the condition modifies the variable then bail
Expand All @@ -333,7 +339,7 @@ namespace {
}
Token* condTok = getCondTokFromEnd(tok->link());
if (condTok) {
Analyzer::Result r = valueFlowGenericForward(condTok, analyzer, settings);
Analyzer::Result r = valueFlowGenericForward(condTok, analyzer, tokenlist, errorLogger, settings);
if (r.action.isModified())
break;
}
Expand Down Expand Up @@ -389,10 +395,10 @@ namespace {
};
}

void valueFlowGenericReverse(Token* start, const Token* end, const ValuePtr<Analyzer>& a, const Settings& settings)
void valueFlowGenericReverse(Token* start, const Token* end, const ValuePtr<Analyzer>& a, const TokenList& tokenlist, ErrorLogger* const errorLogger, const Settings& settings)
{
if (a->invalid())
return;
ReverseTraversal rt{a, settings};
ReverseTraversal rt{a, tokenlist, errorLogger, settings};
rt.traverse(start, end);
}
4 changes: 3 additions & 1 deletion lib/reverseanalyzer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,13 @@
#define reverseanalyzerH

struct Analyzer;
class ErrorLogger;
class Settings;
class Token;
class TokenList;
template<class T>
class ValuePtr;

void valueFlowGenericReverse(Token* start, const Token* end, const ValuePtr<Analyzer>& a, const Settings& settings);
void valueFlowGenericReverse(Token* start, const Token* end, const ValuePtr<Analyzer>& a, const TokenList& tokenlist, ErrorLogger* const errorLogger, const Settings& settings);

#endif
2 changes: 2 additions & 0 deletions lib/settings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,13 +243,15 @@ void Settings::loadSummaries()
void Settings::setCheckLevelExhaustive()
{
// Checking can take a little while. ~ 10 times slower than normal analysis is OK.
checkLevel = CheckLevel::exhaustive;
performanceValueFlowMaxIfCount = -1;
performanceValueFlowMaxSubFunctionArgs = 256;
}

void Settings::setCheckLevelNormal()
{
// Checking should finish in reasonable time.
checkLevel = CheckLevel::normal;
performanceValueFlowMaxSubFunctionArgs = 8;
performanceValueFlowMaxIfCount = 100;
}
Expand Down
6 changes: 6 additions & 0 deletions lib/settings.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,6 +444,12 @@ class CPPCHECKLIB WARN_UNUSED Settings {
void setCheckLevelExhaustive();
void setCheckLevelNormal();

enum class CheckLevel {
exhaustive,
normal
};
CheckLevel checkLevel = CheckLevel::normal;

private:
static std::string parseEnabled(const std::string &str, std::tuple<SimpleEnableGroup<Severity>, SimpleEnableGroup<Checks>> &groups);
std::string applyEnabled(const std::string &str, bool enable);
Expand Down
Loading