Skip to content

ProgramMemory: avoid duplicated lookups / removed at() #7768

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

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

ProgramMemory: avoid duplicated lookups / removed at() #7768

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
43ed3fd
ProgramMemory: consistently added `impossible` parameter to lookup fu…
firewave Sep 15, 2025
28064ac
ProgramMemory: avoid duplicated lookups / removed `at()`
firewave Aug 25, 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
143 changes: 75 additions & 68 deletions lib/programmemory.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,10 +96,25 @@
return nullptr;
}

ValueFlow::Value* ProgramMemory::getValue(nonneg int exprid, bool impossible)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should return a const ValueFlow::Value*. We dont ever mutate the ValueFlow::Value directly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We do. This was previously done from the non-const at().

{
if (mValues->empty())
return nullptr;

// TODO: avoid copy if no value is found
copyOnWrite();

const auto it = find(exprid);
const bool found = it != mValues->cend() && (impossible || !it->second.isImpossible());
Copy link
Contributor

Choose a reason for hiding this comment

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

The isImpossible can be checked by the consumers.

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 was thinking about doing that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ugh - it is already handled inside some of the getters. So it was even more inconsistent as it seemed to be. They also return the underlying MathLib::bigint so the impossible handling cannot just be externalized...

if (found)
return &it->second;
return nullptr;
}

// cppcheck-suppress unusedFunction
bool ProgramMemory::getIntValue(nonneg int exprid, MathLib::bigint& result) const
bool ProgramMemory::getIntValue(nonneg int exprid, MathLib::bigint& result, bool impossible) const
{
const ValueFlow::Value* value = getValue(exprid);
const ValueFlow::Value* value = getValue(exprid, impossible);
if (value && value->isIntValue()) {
result = value->intvalue;
return true;
Expand All @@ -115,9 +130,9 @@
setValue(expr, v);
}

bool ProgramMemory::getTokValue(nonneg int exprid, const Token*& result) const
bool ProgramMemory::getTokValue(nonneg int exprid, const Token*& result, bool impossible) const
{
const ValueFlow::Value* value = getValue(exprid);
const ValueFlow::Value* value = getValue(exprid, impossible);
if (value && value->isTokValue()) {
result = value->tokvalue;
return true;
Expand All @@ -126,18 +141,18 @@
}

// cppcheck-suppress unusedFunction
bool ProgramMemory::getContainerSizeValue(nonneg int exprid, MathLib::bigint& result) const
bool ProgramMemory::getContainerSizeValue(nonneg int exprid, MathLib::bigint& result, bool impossible) const
{
const ValueFlow::Value* value = getValue(exprid);
const ValueFlow::Value* value = getValue(exprid, impossible);
if (value && value->isContainerSizeValue()) {
result = value->intvalue;
return true;
}
return false;
}
bool ProgramMemory::getContainerEmptyValue(nonneg int exprid, MathLib::bigint& result) const
bool ProgramMemory::getContainerEmptyValue(nonneg int exprid, MathLib::bigint& result, bool impossible) const
{
const ValueFlow::Value* value = getValue(exprid, true);
const ValueFlow::Value* value = getValue(exprid, impossible);
if (value && value->isContainerSizeValue()) {
if (value->isImpossible() && value->intvalue == 0) {
result = false;
Expand Down Expand Up @@ -166,28 +181,10 @@
(*mValues)[expr].valueType = ValueFlow::Value::ValueType::UNINIT;
}

bool ProgramMemory::hasValue(nonneg int exprid) const
bool ProgramMemory::hasValue(nonneg int exprid, bool impossible) const
{
const auto it = find(exprid);
return it != mValues->cend();
}

const ValueFlow::Value& ProgramMemory::at(nonneg int exprid) const {
const auto it = find(exprid);
if (it == mValues->cend()) {
throw std::out_of_range("ProgramMemory::at");
}
return it->second;
}

ValueFlow::Value& ProgramMemory::at(nonneg int exprid) {
copyOnWrite();

const auto it = find(exprid);
if (it == mValues->end()) {
throw std::out_of_range("ProgramMemory::at");
}
return it->second;
return it != mValues->cend() && (impossible || !it->second.isImpossible());
}

void ProgramMemory::erase_if(const std::function<bool(const ExprIdToken&)>& pred)
Expand Down Expand Up @@ -253,6 +250,7 @@
return cvalues.find(ExprIdToken::create(exprid));
}

// need to call copyOnWrite() before calling this
ProgramMemory::Map::iterator ProgramMemory::find(nonneg int exprid)
{
return mValues->find(ExprIdToken::create(exprid));
Expand Down Expand Up @@ -1350,10 +1348,9 @@

ValueFlow::Value executeMultiCondition(bool b, const Token* expr)
{
if (pm->hasValue(expr->exprId())) {
const ValueFlow::Value& v = utils::as_const(*pm).at(expr->exprId());
if (v.isIntValue())
return v;
if (const ValueFlow::Value* v = utils::as_const(*pm).getValue(expr->exprId(), true)) {
if (v->isIntValue())
return *v;
}

// Evaluate recursively if there are no exprids
Expand Down Expand Up @@ -1482,18 +1479,18 @@
if (rhs.isUninitValue())
return unknown();
if (expr->str() != "=") {
if (!pm->hasValue(expr->astOperand1()->exprId()))
ValueFlow::Value* lhs = pm->getValue(expr->astOperand1()->exprId(), true);
if (!lhs)
return unknown();
ValueFlow::Value& lhs = pm->at(expr->astOperand1()->exprId());
rhs = evaluate(removeAssign(expr->str()), lhs, rhs);
if (lhs.isIntValue())
ValueFlow::Value::visitValue(rhs, std::bind(assign{}, std::ref(lhs.intvalue), std::placeholders::_1));
else if (lhs.isFloatValue())
rhs = evaluate(removeAssign(expr->str()), *lhs, rhs);
if (lhs->isIntValue())
ValueFlow::Value::visitValue(rhs, std::bind(assign{}, std::ref(lhs->intvalue), std::placeholders::_1));
else if (lhs->isFloatValue())
ValueFlow::Value::visitValue(rhs,
std::bind(assign{}, std::ref(lhs.floatValue), std::placeholders::_1));
std::bind(assign{}, std::ref(lhs->floatValue), std::placeholders::_1));
else
return unknown();
return lhs;
return *lhs;
}
pm->setValue(expr->astOperand1(), rhs);
return rhs;
Expand All @@ -1505,20 +1502,20 @@
execute(expr->astOperand1());
return execute(expr->astOperand2());
} else if (expr->tokType() == Token::eIncDecOp && expr->astOperand1() && expr->astOperand1()->exprId() != 0) {
if (!pm->hasValue(expr->astOperand1()->exprId()))
ValueFlow::Value* lhs = pm->getValue(expr->astOperand1()->exprId(), true);
if (!lhs)
return ValueFlow::Value::unknown();
ValueFlow::Value& lhs = pm->at(expr->astOperand1()->exprId());
if (!lhs.isIntValue())
if (!lhs->isIntValue())
return unknown();
// overflow
if (!lhs.isImpossible() && lhs.intvalue == 0 && expr->str() == "--" && astIsUnsigned(expr->astOperand1()))
if (!lhs->isImpossible() && lhs->intvalue == 0 && expr->str() == "--" && astIsUnsigned(expr->astOperand1()))
return unknown();

if (expr->str() == "++")
lhs.intvalue++;
lhs->intvalue++;
else
lhs.intvalue--;
return lhs;
lhs->intvalue--;
return *lhs;
} else if (expr->str() == "[" && expr->astOperand1() && expr->astOperand2()) {
const Token* tokvalue = nullptr;
if (!pm->getTokValue(expr->astOperand1()->exprId(), tokvalue)) {
Expand Down Expand Up @@ -1609,13 +1606,16 @@
}
return execute(expr->astOperand1());
}
if (expr->exprId() > 0 && pm->hasValue(expr->exprId())) {
ValueFlow::Value result = utils::as_const(*pm).at(expr->exprId());
if (result.isImpossible() && result.isIntValue() && result.intvalue == 0 && isUsedAsBool(expr, settings)) {
result.intvalue = !result.intvalue;
result.setKnown();
if (expr->exprId() > 0) {
if (const ValueFlow::Value* v = utils::as_const(*pm).getValue(expr->exprId(), true))
{
ValueFlow::Value result = *v;
if (result.isImpossible() && result.isIntValue() && result.intvalue == 0 && isUsedAsBool(expr, settings)) {
result.intvalue = !result.intvalue;
result.setKnown();
}
return result;
}
return result;
}

if (Token::Match(expr->previous(), ">|%name% {|(")) {
Expand Down Expand Up @@ -1665,14 +1665,16 @@
}
// Check if function modifies argument
visitAstNodes(expr->astOperand2(), [&](const Token* child) {
if (child->exprId() > 0 && pm->hasValue(child->exprId())) {
ValueFlow::Value& v = pm->at(child->exprId());
if (v.valueType == ValueFlow::Value::ValueType::CONTAINER_SIZE) {
if (ValueFlow::isContainerSizeChanged(child, v.indirect, settings))
v = unknown();
} else if (v.valueType != ValueFlow::Value::ValueType::UNINIT) {
if (isVariableChanged(child, v.indirect, settings))
v = unknown();
if (child->exprId() > 0) {
if (ValueFlow::Value* v = pm->getValue(child->exprId(), true))
{
if (v->valueType == ValueFlow::Value::ValueType::CONTAINER_SIZE) {
if (ValueFlow::isContainerSizeChanged(child, v->indirect, settings))
*v = unknown();
} else if (v->valueType != ValueFlow::Value::ValueType::UNINIT) {
if (isVariableChanged(child, v->indirect, settings))
*v = unknown();
}
}
}
return ChildrenToVisit::op1_and_op2;
Expand Down Expand Up @@ -1724,22 +1726,27 @@
return v;
if (!expr)
return v;
if (expr->exprId() > 0 && pm->hasValue(expr->exprId())) {
if (updateValue(v, utils::as_const(*pm).at(expr->exprId())))
return v;
if (expr->exprId() > 0) {
if (const ValueFlow::Value* val = utils::as_const(*pm).getValue(expr->exprId(), true))
{
if (updateValue(v, *val))
return v;
}
}
// Find symbolic values
for (const ValueFlow::Value& value : expr->values()) {
if (!value.isSymbolicValue())
continue;
if (!value.isKnown())
continue;
if (value.tokvalue->exprId() > 0 && !pm->hasValue(value.tokvalue->exprId()))
if (value.tokvalue->exprId() == 0)
continue;
Comment on lines -1737 to +1743
Copy link
Collaborator Author

@firewave firewave Aug 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This should have been separate before since an at() call with no exprid would have failed.

const ValueFlow::Value* v_p = utils::as_const(*pm).getValue(value.tokvalue->exprId(), true);
if (!v_p)
continue;
const ValueFlow::Value& v_ref = utils::as_const(*pm).at(value.tokvalue->exprId());
if (!v_ref.isIntValue() && value.intvalue != 0)
if (!v_p->isIntValue() && value.intvalue != 0)
continue;
ValueFlow::Value v2 = v_ref;
ValueFlow::Value v2 = *v_p;
v2.intvalue += value.intvalue;
return v2;
}
Expand Down
14 changes: 6 additions & 8 deletions lib/programmemory.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,21 +110,19 @@ struct CPPCHECKLIB ProgramMemory {

void setValue(const Token* expr, const ValueFlow::Value& value);
const ValueFlow::Value* getValue(nonneg int exprid, bool impossible = false) const;
ValueFlow::Value* getValue(nonneg int exprid, bool impossible = false);

bool getIntValue(nonneg int exprid, MathLib::bigint& result) const;
bool getIntValue(nonneg int exprid, MathLib::bigint& result, bool impossible = false) const;
void setIntValue(const Token* expr, MathLib::bigint value, bool impossible = false);

bool getContainerSizeValue(nonneg int exprid, MathLib::bigint& result) const;
bool getContainerEmptyValue(nonneg int exprid, MathLib::bigint& result) const;
bool getContainerSizeValue(nonneg int exprid, MathLib::bigint& result, bool impossible = false) const;
bool getContainerEmptyValue(nonneg int exprid, MathLib::bigint& result, bool impossible = false) const;
Copy link
Collaborator Author

@firewave firewave Sep 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I flipped the default behavior on getContainerEmptyValue() but addressed it via an explicit parameter in its only usage in vf_analyzers.cpp.

Copy link
Contributor

Choose a reason for hiding this comment

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

Dont add the impossible parameters. They dont make sense at all.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Read my comment. It was to make it clear what they are doing as they were inconsistent. But if we move the impossible logic out of them they will go away.

void setContainerSizeValue(const Token* expr, MathLib::bigint value, bool isEqual = true);

void setUnknown(const Token* expr);

bool getTokValue(nonneg int exprid, const Token*& result) const;
bool hasValue(nonneg int exprid) const;

const ValueFlow::Value& at(nonneg int exprid) const;
ValueFlow::Value& at(nonneg int exprid);
bool getTokValue(nonneg int exprid, const Token*& result, bool impossible = false) const;
bool hasValue(nonneg int exprid, bool impossible = true) const;

void erase_if(const std::function<bool(const ExprIdToken&)>& pred);

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 @@ -715,7 +715,8 @@ struct ValueFlowAnalyzer : Analyzer {
return {value->intvalue == 0};
ProgramMemory pm = pms.get(tok, ctx, getProgramState());
MathLib::bigint out = 0;
if (pm.getContainerEmptyValue(tok->exprId(), out))
// TODO: do we really went to return an impossible value?
if (pm.getContainerEmptyValue(tok->exprId(), out, true))
Copy link
Contributor

Choose a reason for hiding this comment

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

It is the correct behaviour, but the parameter and the comment makes it look like its wrong. The getContainerEmptyValue returns true or false if the container is empty or not. It will use impossible values internally to determine this as well(its really an implementation detail though).

This is why the impossible parameter should not be added.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is inconsistent with the other getters. But if we move the impossible logic out of those of them as suggested, things will be clear.

return {static_cast<int>(out)};
return {};
}
Expand Down
21 changes: 7 additions & 14 deletions test/testprogrammemory.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ class TestProgramMemory : public TestFixture {
TEST_CASE(copyOnWrite);
TEST_CASE(hasValue);
TEST_CASE(getValue);
TEST_CASE(at);
}

void copyOnWrite() const {
Expand All @@ -42,19 +41,19 @@ class TestProgramMemory : public TestFixture {
tok->exprId(id);

ProgramMemory pm;
const ValueFlow::Value* v = pm.getValue(id);
const ValueFlow::Value* v = utils::as_const(pm).getValue(id, false);
ASSERT(!v);
pm.setValue(tok, ValueFlow::Value{41});

v = pm.getValue(id);
v = utils::as_const(pm).getValue(id, false);
ASSERT(v);
ASSERT_EQUALS(41, v->intvalue);

// create a copy
ProgramMemory pm2 = pm;

// make sure the value was copied
v = pm2.getValue(id);
v = utils::as_const(pm2).getValue(id, false);
ASSERT(v);
ASSERT_EQUALS(41, v->intvalue);

Expand All @@ -68,17 +67,17 @@ class TestProgramMemory : public TestFixture {
pm3.setValue(tok, ValueFlow::Value{43});

// make sure the value was set
v = pm2.getValue(id);
v = utils::as_const(pm2).getValue(id, false);
ASSERT(v);
ASSERT_EQUALS(42, v->intvalue);

// make sure the value was set
v = pm3.getValue(id);
v = utils::as_const(pm3).getValue(id, false);
ASSERT(v);
ASSERT_EQUALS(43, v->intvalue);

// make sure the original value remains unchanged
v = pm.getValue(id);
v = utils::as_const(pm).getValue(id, false);
ASSERT(v);
ASSERT_EQUALS(41, v->intvalue);
}
Expand All @@ -90,13 +89,7 @@ class TestProgramMemory : public TestFixture {

void getValue() const {
ProgramMemory pm;
ASSERT(!pm.getValue(123));
}

void at() const {
ProgramMemory pm;
ASSERT_THROW_EQUALS_2(pm.at(123), std::out_of_range, "ProgramMemory::at");
ASSERT_THROW_EQUALS_2(utils::as_const(pm).at(123), std::out_of_range, "ProgramMemory::at");
ASSERT(!utils::as_const(pm).getValue(123, false));
}
};

Expand Down