Repository navigation
Partial fix for #15174: ValueFlow speedups #9004
+198
−24
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -122,6 +122,7 @@ | |
| #include <limits> | ||
| #include <map> | ||
| #include <memory> | ||
| #include <numeric> | ||
| #include <set> | ||
| #include <string> | ||
| #include <unordered_map> | ||
|
|
@@ -2007,6 +2008,12 @@ static void valueFlowForwardLifetime(Token * tok, const TokenList &tokenlist, Er | |
| } else if (tok->variable() && tok->variable()->scope()) { | ||
| const Variable *var = tok->variable(); | ||
| const Token *endOfVarScope = var->scope()->bodyEnd; | ||
| // Non-local variable: do not forward past the end of the current function | ||
| if (!var->isLocal() && !var->isArgument()) { | ||
| const Token* endOfFunction = ValueFlow::getEndOfExprScope(tok); | ||
| if (endOfFunction && (!endOfVarScope || precedes(endOfFunction, endOfVarScope))) | ||
| endOfVarScope = endOfFunction; | ||
| } | ||
|
|
||
| std::list<ValueFlow::Value> values = tok->values(); | ||
| Token *nextExpression = nextAfterAstRightmostLeaf(parent); | ||
|
|
@@ -2829,6 +2836,9 @@ static bool isDecayedPointer(const Token *tok) | |
| return false; | ||
| if (!tok->astParent()) | ||
| return false; | ||
| // Subscript 'arr[i]': the element is accessed, the array itself is not used as a pointer | ||
| if (Token::simpleMatch(tok->astParent(), "[") && astIsLHS(tok)) | ||
| return false; | ||
| if (Token::simpleMatch(tok->astParent(), "=") && astIsRHS(tok)) | ||
| return true; | ||
| if (astIsPointer(tok->astParent()) && !Token::simpleMatch(tok->astParent(), "return")) | ||
|
|
@@ -7268,6 +7278,8 @@ struct ValueFlowPass { | |
| virtual void run(const ValueFlowState& state) const = 0; | ||
| // Returns true if pass needs C++ | ||
| virtual bool cpp() const = 0; | ||
| // Returns true if pass does not analyse the skipped functions | ||
| virtual bool usesSkippedFunctions() const = 0; | ||
| virtual ~ValueFlowPass() noexcept = default; | ||
| }; | ||
|
|
||
|
|
@@ -7288,24 +7300,53 @@ struct ValueFlowPassRunner { | |
| }); | ||
| } | ||
|
|
||
| bool run(std::initializer_list<ValuePtr<ValueFlowPass>> passes) | ||
| { | ||
| std::size_t values = 0; | ||
| std::size_t n = state.settings().vfOptions.maxIterations; | ||
| const bool doProgress = state.settings().reportProgress >= 0; | ||
| // A function is skipped in the next iteration when no value changed in the previous iteration | ||
| // and the passes that can skip it did not change it either. Other passes can remove a value that | ||
| // a skipping pass adds again; such a function must not be skipped. | ||
| const std::set<const Scope*> skippedFunctions = state.skippedFunctions; | ||
| const std::vector<const Scope*>& functionScopes = state.symboldatabase().functionScopes; | ||
| std::vector<std::size_t> functionValues = getFunctionValues(); | ||
| std::vector<bool> changedBySkippingPass(functionScopes.size(), false); | ||
| bool first = true; | ||
| while (n > 0 && values != getTotalValues()) { | ||
| if (!first) { | ||
| std::vector<std::size_t> newFunctionValues = getFunctionValues(); | ||
| state.skippedFunctions = skippedFunctions; | ||
| for (std::size_t i = 0; i < functionScopes.size(); ++i) { | ||
| if (newFunctionValues[i] == functionValues[i] && !changedBySkippingPass[i]) | ||
| state.skippedFunctions.insert(functionScopes[i]); | ||
| } | ||
| functionValues = std::move(newFunctionValues); | ||
| } | ||
| first = false; | ||
| changedBySkippingPass.assign(functionScopes.size(), false); | ||
| values = getTotalValues(); | ||
| const std::size_t passnum = state.settings().vfOptions.maxIterations - n + 1; | ||
| const std::string passnum_s = std::to_string(passnum); | ||
| if (std::any_of(passes.begin(), passes.end(), [&](const ValuePtr<ValueFlowPass>& pass) { | ||
| // the string concatination is a hot spot in TestIO::testScanfArgument and TestIO::testPrintfArgumentVariables | ||
| std::string stage = doProgress ? std::string("ValueFlow::") + pass->name() + (' ' + passnum_s) : ""; | ||
| ProgressReporter progressReporter(state.errorLogger, state.settings().reportProgress, state.tokenlist().getSourceFilePath(), std::move(stage)); | ||
| if (!pass->usesSkippedFunctions()) | ||
| return run(pass, passnum); | ||
| const std::vector<std::size_t> before = getFunctionValues(true); | ||
| const bool ret = run(pass, passnum); | ||
| const std::vector<std::size_t> after = getFunctionValues(true); | ||
| for (std::size_t i = 0; i < functionScopes.size(); ++i) { | ||
| if (before[i] != after[i]) | ||
| changedBySkippingPass[i] = true; | ||
| } | ||
| return ret; | ||
| })) | ||
| return true; | ||
| --n; | ||
| } | ||
| state.skippedFunctions = skippedFunctions; | ||
| if (state.settings().debugwarnings) { | ||
| if (n == 0 && values != getTotalValues()) { | ||
| ErrorMessage::FileLocation loc(state.tokenlist().getFiles()[0], 0, 0); | ||
|
|
@@ -7344,6 +7385,52 @@ struct ValueFlowPassRunner { | |
| return false; | ||
| } | ||
|
|
||
| static std::size_t hashValue(const ValueFlow::Value& v) | ||
| { | ||
| std::size_t h = std::hash<MathLib::bigint>{}(v.intvalue); | ||
| const auto combine = [&h](std::size_t x) { | ||
| h ^= x + 0x9e3779b97f4a7c15ULL + (h << 6) + (h >> 2); | ||
| }; | ||
| combine(static_cast<std::size_t>(v.valueType)); | ||
| combine(static_cast<std::size_t>(v.valueKind)); | ||
| combine(static_cast<std::size_t>(v.bound)); | ||
| combine(std::hash<const Token*>{}(v.tokvalue)); | ||
| combine(std::hash<const Token*>{}(v.condition)); | ||
| combine(std::hash<double>{}(v.floatValue)); | ||
| combine(std::hash<MathLib::bigint>{}(v.varvalue)); | ||
| combine(std::hash<MathLib::bigint>{}(v.path)); | ||
| combine(v.varId); | ||
| combine(static_cast<std::size_t>(v.conditional)); | ||
| combine(static_cast<std::size_t>(v.moveKind)); | ||
| combine(static_cast<std::size_t>(v.lifetimeKind)); | ||
| combine(static_cast<std::size_t>(v.lifetimeScope)); | ||
| return h; | ||
| } | ||
|
|
||
| // Hash of the values in each function scope (same order as symboldatabase.functionScopes) | ||
| // onlyAnalysed: the skipped functions get hash 0 | ||
| std::vector<std::size_t> getFunctionValues(bool onlyAnalysed = false) const | ||
| { | ||
| const std::vector<const Scope*>& functionScopes = state.symboldatabase().functionScopes; | ||
| std::vector<std::size_t> result(functionScopes.size()); | ||
| for (std::size_t i = 0; i < functionScopes.size(); ++i) { | ||
| const Scope* scope = functionScopes[i]; | ||
| if (!scope->bodyStart) | ||
| continue; | ||
| if (onlyAnalysed && state.skippedFunctions.count(scope) != 0) | ||
| continue; | ||
| std::size_t h = 0; | ||
| for (const Token* tok = scope->bodyStart; tok != scope->bodyEnd; tok = tok->next()) { | ||
| const std::size_t k = (2 * reinterpret_cast<std::uintptr_t>(tok)) + 1; | ||
| h = std::accumulate(tok->values().cbegin(), tok->values().cend(), h, [&](std::size_t acc, const ValueFlow::Value& v) { | ||
| return acc + (hashValue(v) * k); | ||
| }); | ||
| } | ||
| result[i] = h; | ||
| } | ||
| return result; | ||
| } | ||
|
|
||
| std::size_t getTotalValues() const | ||
| { | ||
| std::size_t n = 1; | ||
|
|
@@ -7404,8 +7491,10 @@ template<class F> | |
| struct ValueFlowPassAdaptor : ValueFlowPass { | ||
| const char* mName = nullptr; | ||
| bool mCPP = false; | ||
| bool mUsesSkippedFunctions = false; | ||
| F mRun; | ||
| ValueFlowPassAdaptor(const char* pname, bool pcpp, F prun) | ||
| : ValueFlowPass(), mName(pname), mCPP(pcpp), mUsesSkippedFunctions(std::strstr(pname, "skippedFunctions") != nullptr), mRun(prun) {} | ||
|
Contributor
There was a problem hiding this comment. AI review Whether a pass honours |
||
| const char* name() const override { | ||
| return mName; | ||
| } | ||
|
|
@@ -7416,6 +7505,9 @@ struct ValueFlowPassAdaptor : ValueFlowPass { | |
| bool cpp() const override { | ||
| return mCPP; | ||
| } | ||
| bool usesSkippedFunctions() const override { | ||
| return mUsesSkippedFunctions; | ||
| } | ||
| }; | ||
|
|
||
| template<class F> | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AI review
The hash leaves out
indirect,containerand a few other fields ofValueFlow::Value. If a pass replaces a value with one that differs only inindirect(for example indirect 0 vs 1 for uninit or null values) or only incontainer, the function's hash stays the same. The function is then skipped in the next iteration even though its values changed. Consider adding at leastindirectandcontainerto the hash.