Skip to content
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

Jump to bottom

added (optional) lazy execution of ValueFlow #4521

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

added (optional) lazy execution of ValueFlow #4521

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dfa2fb3
Token: avoid unnecessary direct access of `TokenImpl::mValues`
firewave Sep 29, 2022
c4f1685
moved `TokensFrontBack` from `token.h` to `tokenlist.h`
firewave Sep 29, 2022
63e12eb
tokenize.cpp: small refactoring
firewave Sep 29, 2022
58dcddb
added lazy execution of ValueFlow via `DISABLE_VALUEFLOW=2` environme…
firewave Sep 29, 2022
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
66 changes: 33 additions & 33 deletions Makefile
View file
Open in desktop

Large diffs are not rendered by default.

81 changes: 39 additions & 42 deletions lib/token.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1689,19 +1689,17 @@ void Token::printValueFlow(bool xml, std::ostream &out) const
else
out << "\n\n##Value flow" << std::endl;
for (const Token *tok = this; tok; tok = tok->next()) {
if (!tok->mImpl->mValues)
continue;
if (tok->mImpl->mValues->empty()) // Values might be removed by removeContradictions
if (tok->values().empty()) // Values might be removed by removeContradictions
continue;
if (xml)
out << " <values id=\"" << tok->mImpl->mValues << "\">" << std::endl;
out << " <values id=\"" << &tok->values() << "\">" << std::endl;
else if (line != tok->linenr())
out << "Line " << tok->linenr() << std::endl;
line = tok->linenr();
if (!xml) {
const ValueFlow::Value::ValueKind valueKind = tok->mImpl->mValues->front().valueKind;
const ValueFlow::Value::ValueKind valueKind = tok->values().front().valueKind;
bool same = true;
for (const ValueFlow::Value &value : *tok->mImpl->mValues) {
for (const ValueFlow::Value &value : tok->values()) {
if (value.valueKind != valueKind) {
same = false;
break;
Expand All @@ -1722,10 +1720,10 @@ void Token::printValueFlow(bool xml, std::ostream &out) const
break;
}
}
if (tok->mImpl->mValues->size() > 1U)
if (tok->values().size() > 1U)
out << '{';
}
for (const ValueFlow::Value &value : *tok->mImpl->mValues) {
for (const ValueFlow::Value &value : tok->values()) {
if (xml) {
out << " <value ";
switch (value.valueType) {
Expand Down Expand Up @@ -1785,14 +1783,14 @@ void Token::printValueFlow(bool xml, std::ostream &out) const
}

else {
if (&value != &tok->mImpl->mValues->front())
if (&value != &tok->values().front())
out << ",";
out << value.toString();
}
}
if (xml)
out << " </values>" << std::endl;
else if (tok->mImpl->mValues->size() > 1U)
else if (tok->values().size() > 1U)
out << '}' << std::endl;
else
out << std::endl;
Expand All @@ -1803,29 +1801,29 @@ void Token::printValueFlow(bool xml, std::ostream &out) const

const ValueFlow::Value * Token::getValueLE(const MathLib::bigint val, const Settings *settings) const
{
if (!mImpl->mValues)
if (values().empty())
return nullptr;
return ValueFlow::findValue(*mImpl->mValues, settings, [&](const ValueFlow::Value& v) {
return ValueFlow::findValue(values(), settings, [&](const ValueFlow::Value& v) {
return !v.isImpossible() && v.isIntValue() && v.intvalue <= val;
});
}

const ValueFlow::Value * Token::getValueGE(const MathLib::bigint val, const Settings *settings) const
{
if (!mImpl->mValues)
if (values().empty())
Copy link
Contributor

Choose a reason for hiding this comment

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

values().empty() is redundant here.

return nullptr;
return ValueFlow::findValue(*mImpl->mValues, settings, [&](const ValueFlow::Value& v) {
return ValueFlow::findValue(values(), settings, [&](const ValueFlow::Value& v) {
return !v.isImpossible() && v.isIntValue() && v.intvalue >= val;
});
}

const ValueFlow::Value * Token::getInvalidValue(const Token *ftok, nonneg int argnr, const Settings *settings) const
{
if (!mImpl->mValues || !settings)
if (values().empty() || !settings)
Copy link
Contributor

Choose a reason for hiding this comment

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

values().empty() is redundant here.

return nullptr;
const ValueFlow::Value *ret = nullptr;
std::list<ValueFlow::Value>::const_iterator it;
for (it = mImpl->mValues->begin(); it != mImpl->mValues->end(); ++it) {
for (it = values().begin(); it != values().end(); ++it) {
if (it->isImpossible())
continue;
if ((it->isIntValue() && !settings->library.isIntArgValid(ftok, argnr, it->intvalue)) ||
Expand All @@ -1847,12 +1845,12 @@ const ValueFlow::Value * Token::getInvalidValue(const Token *ftok, nonneg int ar

const Token *Token::getValueTokenMinStrSize(const Settings *settings, MathLib::bigint* path) const
{
if (!mImpl->mValues)
if (values().empty())
Copy link
Contributor

Choose a reason for hiding this comment

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

values().empty() is redundant here.

return nullptr;
const Token *ret = nullptr;
int minsize = INT_MAX;
std::list<ValueFlow::Value>::const_iterator it;
for (it = mImpl->mValues->begin(); it != mImpl->mValues->end(); ++it) {
for (it = values().begin(); it != values().end(); ++it) {
if (it->isTokValue() && it->tokvalue && it->tokvalue->tokType() == Token::eString) {
const int size = getStrSize(it->tokvalue, settings);
if (!ret || size < minsize) {
Expand All @@ -1868,12 +1866,12 @@ const Token *Token::getValueTokenMinStrSize(const Settings *settings, MathLib::b

const Token *Token::getValueTokenMaxStrLength() const
{
if (!mImpl->mValues)
if (values().empty())
Copy link
Contributor

Choose a reason for hiding this comment

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

values().empty() is redundant here.

return nullptr;
const Token *ret = nullptr;
int maxlength = 0;
std::list<ValueFlow::Value>::const_iterator it;
for (it = mImpl->mValues->begin(); it != mImpl->mValues->end(); ++it) {
for (it = values().begin(); it != values().end(); ++it) {
if (it->isTokValue() && it->tokvalue && it->tokvalue->tokType() == Token::eString) {
const int length = getStrLength(it->tokvalue);
if (!ret || length > maxlength) {
Expand Down Expand Up @@ -2088,7 +2086,7 @@ bool Token::addValue(const ValueFlow::Value &value)
{
if (value.isKnown() && mImpl->mValues) {
// Clear all other values of the same type since value is known
mImpl->mValues->remove_if([&](const ValueFlow::Value& x) {
removeValues([&](const ValueFlow::Value& x) {
return sameValueType(x, value);
});
}
Expand Down Expand Up @@ -2316,22 +2314,21 @@ std::shared_ptr<ScopeInfo2> Token::scopeInfo() const

bool Token::hasKnownIntValue() const
{
if (!mImpl->mValues)
if (values().empty())
Copy link
Contributor

Choose a reason for hiding this comment

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

values().empty() is redundant here.

return false;
return std::any_of(mImpl->mValues->begin(), mImpl->mValues->end(), [](const ValueFlow::Value& value) {
return std::any_of(values().begin(), values().end(), [](const ValueFlow::Value& value) {
return value.isKnown() && value.isIntValue();
});
}

bool Token::hasKnownValue() const
{
return mImpl->mValues && std::any_of(mImpl->mValues->begin(), mImpl->mValues->end(), std::mem_fn(&ValueFlow::Value::isKnown));
return !values().empty() && std::any_of(values().begin(), values().end(), std::mem_fn(&ValueFlow::Value::isKnown));
Copy link
Contributor

Choose a reason for hiding this comment

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

values().empty() is redundant here.

}

bool Token::hasKnownValue(ValueFlow::Value::ValueType t) const
{
return mImpl->mValues &&
std::any_of(mImpl->mValues->begin(), mImpl->mValues->end(), [&](const ValueFlow::Value& value) {
return !values().empty() && std::any_of(values().begin(), values().end(), [&](const ValueFlow::Value& value) {
return value.isKnown() && value.valueType == t;
});
}
Expand All @@ -2340,39 +2337,39 @@ bool Token::hasKnownSymbolicValue(const Token* tok) const
{
if (tok->exprId() == 0)
return false;
return mImpl->mValues &&
std::any_of(mImpl->mValues->begin(), mImpl->mValues->end(), [&](const ValueFlow::Value& value) {
return !values().empty() &&
Copy link
Contributor

Choose a reason for hiding this comment

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

values().empty() is redundant here.

std::any_of(values().begin(), values().end(), [&](const ValueFlow::Value& value) {
return value.isKnown() && value.isSymbolicValue() && value.tokvalue &&
value.tokvalue->exprId() == tok->exprId();
});
}

const ValueFlow::Value* Token::getKnownValue(ValueFlow::Value::ValueType t) const
{
if (!mImpl->mValues)
if (values().empty())
return nullptr;
auto it = std::find_if(mImpl->mValues->begin(), mImpl->mValues->end(), [&](const ValueFlow::Value& value) {
auto it = std::find_if(values().begin(), values().end(), [&](const ValueFlow::Value& value) {
return value.isKnown() && value.valueType == t;
});
return it == mImpl->mValues->end() ? nullptr : &*it;
return it == values().end() ? nullptr : &*it;
}

const ValueFlow::Value* Token::getValue(const MathLib::bigint val) const
{
if (!mImpl->mValues)
if (values().empty())
Copy link
Contributor

Choose a reason for hiding this comment

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

values().empty() is redundant here.

return nullptr;
const auto it = std::find_if(mImpl->mValues->begin(), mImpl->mValues->end(), [=](const ValueFlow::Value& value) {
const auto it = std::find_if(values().begin(), values().end(), [=](const ValueFlow::Value& value) {
return value.isIntValue() && !value.isImpossible() && value.intvalue == val;
});
return it == mImpl->mValues->end() ? nullptr : &*it;
return it == values().end() ? nullptr : &*it;
}

const ValueFlow::Value* Token::getMaxValue(bool condition, MathLib::bigint path) const
{
if (!mImpl->mValues)
if (values().empty())
Copy link
Contributor

Choose a reason for hiding this comment

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

values().empty() is redundant here.

return nullptr;
const ValueFlow::Value* ret = nullptr;
for (const ValueFlow::Value& value : *mImpl->mValues) {
for (const ValueFlow::Value& value : values()) {
if (!value.isIntValue())
continue;
if (value.isImpossible())
Expand All @@ -2388,24 +2385,24 @@ const ValueFlow::Value* Token::getMaxValue(bool condition, MathLib::bigint path)

const ValueFlow::Value* Token::getMovedValue() const
{
if (!mImpl->mValues)
if (values().empty())
Copy link
Contributor

Choose a reason for hiding this comment

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

values().empty() is redundant here.

return nullptr;
const auto it = std::find_if(mImpl->mValues->begin(), mImpl->mValues->end(), [](const ValueFlow::Value& value) {
const auto it = std::find_if(values().begin(), values().end(), [](const ValueFlow::Value& value) {
return value.isMovedValue() && !value.isImpossible() &&
value.moveKind != ValueFlow::Value::MoveKind::NonMovedVariable;
});
return it == mImpl->mValues->end() ? nullptr : &*it;
return it == values().end() ? nullptr : &*it;
}

// cppcheck-suppress unusedFunction
const ValueFlow::Value* Token::getContainerSizeValue(const MathLib::bigint val) const
{
if (!mImpl->mValues)
if (values().empty())
Copy link
Contributor

Choose a reason for hiding this comment

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

values().empty() is redundant here.

Copy link
Collaborator Author

@firewave firewave Oct 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I kept it to avoid the function calls (which obviously won't do much) since some of these might be called loooooots of times. I haven't profiled any of this yet.

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 would not call it "redundant" but "unnecessary". Still needs to be profiled.

return nullptr;
const auto it = std::find_if(mImpl->mValues->begin(), mImpl->mValues->end(), [=](const ValueFlow::Value& value) {
const auto it = std::find_if(values().begin(), values().end(), [=](const ValueFlow::Value& value) {
return value.isContainerSizeValue() && !value.isImpossible() && value.intvalue == val;
});
return it == mImpl->mValues->end() ? nullptr : &*it;
return it == values().end() ? nullptr : &*it;
}

TokenImpl::~TokenImpl()
Expand Down
15 changes: 4 additions & 11 deletions lib/token.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

#include "config.h"
#include "mathlib.h"
#include "tokenlist.h"
#include "valueflow.h"
#include "templatesimplifier.h"
#include "utils.h"
Expand All @@ -45,19 +46,9 @@ class Settings;
class Type;
class ValueType;
class Variable;
class TokenList;
class ConstTokenRange;
class Token;

/**
* @brief This struct stores pointers to the front and back tokens of the list this token is in.
*/
struct TokensFrontBack {
Token *front;
Token *back;
const TokenList* list;
};

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_)) {}
std::string name;
Expand Down Expand Up @@ -1171,6 +1162,8 @@ class CPPCHECKLIB Token {
}

const std::list<ValueFlow::Value>& values() const {
if (mTokensFrontBack && !mTokensFrontBack->list->isValuesSet())
mTokensFrontBack->list->setValues();
return mImpl->mValues ? *mImpl->mValues : TokenImpl::mEmptyValueList;
}

Expand All @@ -1192,7 +1185,7 @@ class CPPCHECKLIB Token {

const ValueFlow::Value* getKnownValue(ValueFlow::Value::ValueType t) const;
MathLib::bigint getKnownIntValue() const {
return mImpl->mValues->front().intvalue;
return values().front().intvalue;
}

const ValueFlow::Value* getValue(const MathLib::bigint val) const;
Expand Down
32 changes: 18 additions & 14 deletions lib/tokenize.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2796,6 +2796,7 @@ bool Tokenizer::simplifyTokens1(const std::string &configuration)
createSymbolDatabase();
}

// TODO: this accesses ValueFlow information before it has been generated
if (mTimerResults) {
Timer t("Tokenizer::simplifyTokens1::setValueType", mSettings->showtime, mTimerResults);
mSymbolDatabase->setValueTypeInTokenList(true);
Expand All @@ -2806,19 +2807,6 @@ bool Tokenizer::simplifyTokens1(const std::string &configuration)
if (!mSettings->buildDir.empty())
Summaries::create(this, configuration);

// TODO: do not run valueflow if no checks are being performed at all - e.g. unusedFunctions only
const char* disableValueflowEnv = std::getenv("DISABLE_VALUEFLOW");
const bool doValueFlow = !disableValueflowEnv || (std::strcmp(disableValueflowEnv, "1") != 0);

if (doValueFlow) {
if (mTimerResults) {
Timer t("Tokenizer::simplifyTokens1::ValueFlow", mSettings->showtime, mTimerResults);
ValueFlow::setValues(&list, mSymbolDatabase, mErrorLogger, mSettings);
} else {
ValueFlow::setValues(&list, mSymbolDatabase, mErrorLogger, mSettings);
}
}

// Warn about unhandled character literals
if (mSettings->severity.isEnabled(Severity::portability)) {
for (const Token *tok = tokens(); tok; tok = tok->next()) {
Expand All @@ -2832,8 +2820,24 @@ bool Tokenizer::simplifyTokens1(const std::string &configuration)
}
}

if (doValueFlow) {
auto setValues = [&](){
if (mTimerResults) {
Timer t("Tokenizer::simplifyTokens1::ValueFlow", mSettings->showtime, mTimerResults);
ValueFlow::setValues(&list, mSymbolDatabase, mErrorLogger, mSettings);
} else {
ValueFlow::setValues(&list, mSymbolDatabase, mErrorLogger, mSettings);
}

mSymbolDatabase->setArrayDimensionsUsingValueFlow();
};

const char* disableValueflowEnv = std::getenv("DISABLE_VALUEFLOW");
const bool doLazyValueFlow = disableValueflowEnv && (std::strcmp(disableValueflowEnv, "2") == 0);
const bool doValueFlow = !disableValueflowEnv || (std::strcmp(disableValueflowEnv, "1") != 0);
if (doLazyValueFlow) {
list.setValuesFunc(std::move(setValues));
} else if (doValueFlow) {
setValues();
}

printDebugOutput(1);
Expand Down
4 changes: 3 additions & 1 deletion lib/tokenlist.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,9 @@ TokenList::TokenList(const Settings* settings) :
mTokensFrontBack(),
mSettings(settings),
mIsC(false),
mIsCpp(false)
mIsCpp(false),
mValuesSet(false),
mInSetValues(false)
{
mTokensFrontBack.list = this;
mKeywords.insert("asm");
Expand Down
Loading