Skip to content

introduced a cache for followAllReferences() calls #7192

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

introduced a cache for followAllReferences() calls #7192

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
90c3d46
introduced a cache for `followAllReferences()` calls
firewave Jan 7, 2025
3b5512c
astutils.cpp: avoid potential NULL dereference in `isAliasOf()`
firewave Sep 15, 2025
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
64 changes: 32 additions & 32 deletions Makefile
View file
Open in desktop

Large diffs are not rendered by default.

33 changes: 20 additions & 13 deletions lib/astutils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1064,10 +1064,12 @@ bool isAliasOf(const Token *tok, nonneg int varid, bool* inconclusive)

bool isAliasOf(const Token* tok, const Token* expr, int* indirect)
{
const Token* r = nullptr;
if (indirect)
*indirect = 1;
for (const ReferenceToken& ref : followAllReferences(tok)) {
if (!tok)
return false;
const Token* r = nullptr;
for (const ReferenceToken& ref : tok->refs()) {
const bool pointer = astIsPointer(ref.token);
r = findAstNode(expr, [&](const Token* childTok) {
if (childTok->exprId() == 0)
Expand Down Expand Up @@ -1246,11 +1248,11 @@ static void followVariableExpressionError(const Token *tok1, const Token *tok2,
errors->push_back(std::move(item));
}

SmallVector<ReferenceToken> followAllReferences(const Token* tok,
bool temporary,
bool inconclusive,
ErrorPath errors,
int depth)
static SmallVector<ReferenceToken> followAllReferencesInternal(const Token* tok,
bool temporary = true,
bool inconclusive = true,
ErrorPath errors = ErrorPath{},
int depth = 20)
{
struct ReferenceTokenLess {
bool operator()(const ReferenceToken& x, const ReferenceToken& y) const {
Expand Down Expand Up @@ -1296,16 +1298,16 @@ SmallVector<ReferenceToken> followAllReferences(const Token* tok,
return refs_result;
}
if (vartok)
return followAllReferences(vartok, temporary, inconclusive, std::move(errors), depth - 1);
return followAllReferencesInternal(vartok, temporary, inconclusive, std::move(errors), depth - 1);
}
}
} else if (Token::simpleMatch(tok, "?") && Token::simpleMatch(tok->astOperand2(), ":")) {
std::set<ReferenceToken, ReferenceTokenLess> result;
const Token* tok2 = tok->astOperand2();

auto refs = followAllReferences(tok2->astOperand1(), temporary, inconclusive, errors, depth - 1);
auto refs = followAllReferencesInternal(tok2->astOperand1(), temporary, inconclusive, errors, depth - 1);
result.insert(refs.cbegin(), refs.cend());
refs = followAllReferences(tok2->astOperand2(), temporary, inconclusive, errors, depth - 1);
refs = followAllReferencesInternal(tok2->astOperand2(), temporary, inconclusive, errors, depth - 1);
result.insert(refs.cbegin(), refs.cend());

if (!inconclusive && result.size() != 1) {
Expand Down Expand Up @@ -1333,7 +1335,7 @@ SmallVector<ReferenceToken> followAllReferences(const Token* tok,
if (returnTok == tok)
continue;
for (const ReferenceToken& rt :
followAllReferences(returnTok, temporary, inconclusive, errors, depth - returns.size())) {
followAllReferencesInternal(returnTok, temporary, inconclusive, errors, depth - returns.size())) {
const Variable* argvar = rt.token->variable();
if (!argvar) {
SmallVector<ReferenceToken> refs_result;
Expand All @@ -1358,7 +1360,7 @@ SmallVector<ReferenceToken> followAllReferences(const Token* tok,
er.emplace_back(returnTok, "Return reference.");
er.emplace_back(tok->previous(), "Called function passing '" + argTok->expressionString() + "'.");
auto refs =
followAllReferences(argTok, temporary, inconclusive, std::move(er), depth - returns.size());
followAllReferencesInternal(argTok, temporary, inconclusive, std::move(er), depth - returns.size());
result.insert(refs.cbegin(), refs.cend());
if (!inconclusive && result.size() > 1) {
SmallVector<ReferenceToken> refs_result;
Expand All @@ -1379,11 +1381,16 @@ SmallVector<ReferenceToken> followAllReferences(const Token* tok,
return refs_result;
}

SmallVector<ReferenceToken> followAllReferences(const Token* tok, bool temporary)
{
return followAllReferencesInternal(tok, temporary);
}

const Token* followReferences(const Token* tok, ErrorPath* errors)
{
if (!tok)
return nullptr;
auto refs = followAllReferences(tok, true, false);
auto refs = followAllReferencesInternal(tok, true, false);
if (refs.size() == 1) {
if (errors)
*errors = std::move(refs.front().errors);
Expand Down
6 changes: 1 addition & 5 deletions lib/astutils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,11 +252,7 @@ struct ReferenceToken {
ErrorPath errors;
};

SmallVector<ReferenceToken> followAllReferences(const Token* tok,
bool temporary = true,
bool inconclusive = true,
ErrorPath errors = ErrorPath{},
int depth = 20);
SmallVector<ReferenceToken> followAllReferences(const Token* tok, bool temporary = true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are removing the inconclusive flag, then we should add that to the ReferenceToken so we can know if it was inconclusive or not.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The removed flags were not used outside of astutils.cpp itself. If that information is need in the future we can make the suggested change.

const Token* followReferences(const Token* tok, ErrorPath* errors = nullptr);

CPPCHECKLIB bool isSameExpression(bool macro, const Token *tok1, const Token *tok2, const Settings& settings, bool pure, bool followVar, ErrorPath* errors=nullptr);
Expand Down
15 changes: 15 additions & 0 deletions lib/token.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2648,6 +2648,8 @@ TokenImpl::~TokenImpl()
delete mOriginalName;
delete mValueType;
delete mValues;
delete mRefs;
delete mRefsTemp;

if (mTemplateSimplifierPointers) {
for (auto *templateSimplifierPointer : *mTemplateSimplifierPointers) {
Expand Down Expand Up @@ -2739,3 +2741,16 @@ void Token::templateArgFrom(const Token* fromToken) {
mImpl->mTemplateArgLineNumber = fromToken ? fromToken->mImpl->mLineNumber : -1;
mImpl->mTemplateArgColumn = fromToken ? fromToken->mImpl->mColumn : -1;
}

const SmallVector<ReferenceToken>& Token::refs(bool temporary) const
{
if (temporary) {
if (!mImpl->mRefsTemp)
mImpl->mRefsTemp = new SmallVector<ReferenceToken>(followAllReferences(this, true));
return *mImpl->mRefsTemp;
}

if (!mImpl->mRefs)
mImpl->mRefs = new SmallVector<ReferenceToken>(followAllReferences(this, false));
return *mImpl->mRefs;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the right way to do this. This is a const method that is modifying the token. Instead followAllReferences should be moved to the SymbolDatabase and there should be a pass that fills this in for all of the tokens.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I was about to add a comment about this. This violates the technical const and if we would not allow this (I hope some day I will finish up that change) this would require mutable (which from my experience is acceptable for caches inside objects).

I am not sure how easy it would be to implement an earlier pass since it is not done for all tokens but there are lots of checks which are performed before we actually end up following references. That would need to be replicated I reckon - and that also has a certain visible overhead and we would need to run through that twice then.

Actually I would also have the ValueFlow behave this way so we might avoid running it for code which is not relevant.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the right way to do this. This is a const method that is modifying the token.

That should be totally fine (by precedent). We modify const Token objects all over the place in the ValueFlow and symbol database via const_cast. Obviously it would be better if we didn't but here it is much cleaner and in a single place and as stated before I think this is acceptable practice.

Actually I would also have the ValueFlow behave this way so we might avoid running it for code which is not relevant.

Please disregard this. This is wishful thinking as this would not be possible the way the ValueFlow is working. I totally forgot I already looked into this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The const_cast should be fixed, but we shouldn't add more code that needs to be fixed.

Also this is called in ValueFlowForward and ValueFlowReverse so its already called on almost every token in functionScopes, so it really won't help performance being a cache.

Furthermore, in copcheck we update the tokens through passes rather than using a cache, this makes it easier to debug and we can provide this information to addons later on. So doing a pass in SymbolDatabase would be consistent with the rest of the code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Furthermore, in copcheck we update the tokens through passes rather than using a cache, this makes it easier to debug and we can provide this information to addons later on. So doing a pass in SymbolDatabase would be consistent with the rest of the code.

Will give it a try and check how it impacts performance.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we know if it causes a perf impact or how much? It seems we are making it worse for premature optimizations.

There are other advantages to doing it the correct way too such as better debugging and addons can take advantage of this information (this seems like a useful analysis for addons). So if we enable it for addons then we will beed to run a pass regardless.

Also you could consider skipping this for functions we are skipping analysis for, if the performance is too bad, but it would be good to see some actual numbers to make this decision.

This comment was marked as duplicate.

Sign in to view

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the current approach seems like the best approach.

I meant to say "It seems like the currently best approach".

Do we know if it causes a perf impact or how much? It seems we are making it worse for premature optimizations.

Various performance numbers are in the PR. It is a massive improvement. It would also help with the runtime of the CI.

Also you could consider skipping this for functions we are skipping analysis for, if the performance is too bad, but it would be good to see some actual numbers to make this decision.

That was an idea regarding the ValueFlow (see https://trac.cppcheck.net/ticket/12528) but that won't work since not all passes are based on function scopes. But that is currently out-of-scope and is something I am looking at within another context hopefully soon.

It might actually not an issue after all because with the duplicated calls eliminated it basically no longer has any footprint. The only issue could be that we perform it for more tokens than we actually need so that would introduce new overhead but it might not be much. Will test that. Although I would prefer not to have that at all since all the overhead adds up - a lot.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized this is called when setting exprids, so it always called on every token regardless of ValueFlow analysis.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just gave it a small spin and it does not increase the Ir so that looks feasible.

8 changes: 8 additions & 0 deletions lib/token.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include "config.h"
#include "errortypes.h"
#include "mathlib.h"
#include "smallvector.h"
#include "templatesimplifier.h"
#include "utils.h"
#include "vfvalue.h"
Expand Down Expand Up @@ -53,6 +54,7 @@ class ConstTokenRange;
class Token;
struct TokensFrontBack;
class TokenList;
struct ReferenceToken;

struct ScopeInfo2 {
ScopeInfo2(std::string name_, const Token *bodyEnd_, std::set<std::string> usingNamespaces_ = std::set<std::string>()) : name(std::move(name_)), bodyEnd(bodyEnd_), usingNamespaces(std::move(usingNamespaces_)) {}
Expand Down Expand Up @@ -149,6 +151,9 @@ struct TokenImpl {
void setCppcheckAttribute(CppcheckAttributes::Type type, MathLib::bigint value);
bool getCppcheckAttribute(CppcheckAttributes::Type type, MathLib::bigint &value) const;

SmallVector<ReferenceToken>* mRefs{};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is going to be a pointer, you should use std::unique_ptr or std::shared_ptr.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I modeled it after mValues which is also just a raw pointer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should model it after mAttributeAlignas, thats a much safer choice. I think the raw pointers were used before managed pointers were available.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That requires ReferenceToken to be a complete type and the astutils.h to be added.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just move the ReferenceToken to the token.h header.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but followAllReferences() is the code which produces the ReferenceToken objects so I would prefer to keep those together.

It is unfortunate enough that smallvector.h spilled into more code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spillage could be addressed by moving TokenImpl into the implementation (hence the name). I tried that in the past but unfortunately the code relies on the implementation being exposed - I will give it another try.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Onward into the next rabbithole: #7831.

SmallVector<ReferenceToken>* mRefsTemp{};

TokenImpl() : mFunction(nullptr) {}

~TokenImpl();
Expand Down Expand Up @@ -1349,6 +1354,9 @@ class CPPCHECKLIB Token {
return mImpl->mValues ? *mImpl->mValues : TokenImpl::mEmptyValueList;
}

// provides and caches result of a followAllReferences() call
const SmallVector<ReferenceToken>& refs(bool temporary = true) const;

/**
* Sets the original name.
*/
Expand Down
4 changes: 2 additions & 2 deletions lib/valueflow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3268,7 +3268,7 @@ static void valueFlowLifetime(TokenList &tokenlist, ErrorLogger &errorLogger, co
}

for (const Token* tok2 : toks) {
for (const ReferenceToken& rt : followAllReferences(tok2, false)) {
for (const ReferenceToken& rt : tok2->refs(false)) {
ValueFlow::Value value = master;
value.tokvalue = rt.token;
value.errorPath.insert(value.errorPath.begin(), rt.errors.cbegin(), rt.errors.cend());
Expand Down Expand Up @@ -3980,7 +3980,7 @@ static void valueFlowForwardConst(Token* start,
} else {
[&] {
// Follow references
auto refs = followAllReferences(tok);
const auto& refs = tok->refs();
auto it = std::find_if(refs.cbegin(), refs.cend(), [&](const ReferenceToken& ref) {
return ref.token->varId() == var->declarationId();
});
Expand Down
3 changes: 2 additions & 1 deletion lib/vf_analyzers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -647,7 +647,8 @@ struct ValueFlowAnalyzer : Analyzer {
if (invalid())
return Action::Invalid;
// Follow references
auto refs = followAllReferences(tok);
// TODO: avoid copy
auto refs = tok->refs();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This copy is necessary since an additional entry is being added. But I think this is not necessary and I will try to refactor the code to avoid it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I adjusted this by unfortunately there is some redundant code introduced.

const bool inconclusiveRefs = refs.size() != 1;
if (std::none_of(refs.cbegin(), refs.cend(), [&](const ReferenceToken& ref) {
return tok == ref.token;
Expand Down
Loading