From 43ed3fd57c2cf4f4d2162a7fd7efef82e58538fa Mon Sep 17 00:00:00 2001 From: firewave Date: Mon, 15 Sep 2025 08:02:29 +0200 Subject: [PATCH 1/2] ProgramMemory: consistently added `impossible` parameter to lookup functions --- lib/programmemory.cpp | 28 ++++++++++++++-------------- lib/programmemory.h | 16 ++++++++-------- lib/vf_analyzers.cpp | 3 ++- test/testprogrammemory.cpp | 14 +++++++------- 4 files changed, 31 insertions(+), 30 deletions(-) diff --git a/lib/programmemory.cpp b/lib/programmemory.cpp index 86d5e35273d..6b63f8606b2 100644 --- a/lib/programmemory.cpp +++ b/lib/programmemory.cpp @@ -97,9 +97,9 @@ const ValueFlow::Value* ProgramMemory::getValue(nonneg int exprid, bool impossib } // 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; @@ -115,9 +115,9 @@ void ProgramMemory::setIntValue(const Token* expr, MathLib::bigint value, bool i 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; @@ -126,18 +126,18 @@ bool ProgramMemory::getTokValue(nonneg int exprid, const Token*& result) const } // 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; @@ -166,25 +166,25 @@ void ProgramMemory::setUnknown(const Token* expr) { (*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(); + return it != mValues->cend() && (impossible || !it->second.isImpossible()); } -const ValueFlow::Value& ProgramMemory::at(nonneg int exprid) const { +const ValueFlow::Value& ProgramMemory::at(nonneg int exprid, bool impossible) const { const auto it = find(exprid); - if (it == mValues->cend()) { + if (it == mValues->cend() && (impossible || !it->second.isImpossible())) { throw std::out_of_range("ProgramMemory::at"); } return it->second; } -ValueFlow::Value& ProgramMemory::at(nonneg int exprid) { +ValueFlow::Value& ProgramMemory::at(nonneg int exprid, bool impossible) { copyOnWrite(); const auto it = find(exprid); - if (it == mValues->end()) { + if (it == mValues->end() && (impossible || !it->second.isImpossible())) { throw std::out_of_range("ProgramMemory::at"); } return it->second; diff --git a/lib/programmemory.h b/lib/programmemory.h index bc0a5537f7b..d871d139e11 100644 --- a/lib/programmemory.h +++ b/lib/programmemory.h @@ -109,22 +109,22 @@ struct CPPCHECKLIB ProgramMemory { explicit ProgramMemory(Map values) : mValues(new Map(std::move(values))) {} void setValue(const Token* expr, const ValueFlow::Value& value); - const ValueFlow::Value* getValue(nonneg int exprid, bool impossible = false) const; + const ValueFlow::Value* getValue(nonneg int exprid, bool impossible) const; - 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; 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; + bool getTokValue(nonneg int exprid, const Token*& result, bool impossible = false) const; + bool hasValue(nonneg int exprid, bool impossible = true) const; - const ValueFlow::Value& at(nonneg int exprid) const; - ValueFlow::Value& at(nonneg int exprid); + const ValueFlow::Value& at(nonneg int exprid, bool impossible = true) const; + ValueFlow::Value& at(nonneg int exprid, bool impossible = true); void erase_if(const std::function& pred); diff --git a/lib/vf_analyzers.cpp b/lib/vf_analyzers.cpp index 1b70b4446d1..e94ade65189 100644 --- a/lib/vf_analyzers.cpp +++ b/lib/vf_analyzers.cpp @@ -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)) return {static_cast(out)}; return {}; } diff --git a/test/testprogrammemory.cpp b/test/testprogrammemory.cpp index 13a5f8e639a..903f0a54c8a 100644 --- a/test/testprogrammemory.cpp +++ b/test/testprogrammemory.cpp @@ -42,11 +42,11 @@ class TestProgramMemory : public TestFixture { tok->exprId(id); ProgramMemory pm; - const ValueFlow::Value* v = pm.getValue(id); + const ValueFlow::Value* v = pm.getValue(id, false); ASSERT(!v); pm.setValue(tok, ValueFlow::Value{41}); - v = pm.getValue(id); + v = pm.getValue(id, false); ASSERT(v); ASSERT_EQUALS(41, v->intvalue); @@ -54,7 +54,7 @@ class TestProgramMemory : public TestFixture { ProgramMemory pm2 = pm; // make sure the value was copied - v = pm2.getValue(id); + v = pm2.getValue(id, false); ASSERT(v); ASSERT_EQUALS(41, v->intvalue); @@ -68,17 +68,17 @@ class TestProgramMemory : public TestFixture { pm3.setValue(tok, ValueFlow::Value{43}); // make sure the value was set - v = pm2.getValue(id); + v = pm2.getValue(id, false); ASSERT(v); ASSERT_EQUALS(42, v->intvalue); // make sure the value was set - v = pm3.getValue(id); + v = pm3.getValue(id, false); ASSERT(v); ASSERT_EQUALS(43, v->intvalue); // make sure the original value remains unchanged - v = pm.getValue(id); + v = pm.getValue(id, false); ASSERT(v); ASSERT_EQUALS(41, v->intvalue); } @@ -90,7 +90,7 @@ class TestProgramMemory : public TestFixture { void getValue() const { ProgramMemory pm; - ASSERT(!pm.getValue(123)); + ASSERT(!pm.getValue(123, false)); } void at() const { From 28064acd498f01a124ec526227697fea053bb1c4 Mon Sep 17 00:00:00 2001 From: firewave Date: Mon, 25 Aug 2025 15:08:27 +0200 Subject: [PATCH 2/2] ProgramMemory: avoid duplicated lookups / removed `at()` --- lib/programmemory.cpp | 123 ++++++++++++++++++++----------------- lib/programmemory.h | 6 +- test/testprogrammemory.cpp | 21 +++---- 3 files changed, 74 insertions(+), 76 deletions(-) diff --git a/lib/programmemory.cpp b/lib/programmemory.cpp index 6b63f8606b2..bb2cec13efa 100644 --- a/lib/programmemory.cpp +++ b/lib/programmemory.cpp @@ -96,6 +96,21 @@ const ValueFlow::Value* ProgramMemory::getValue(nonneg int exprid, bool impossib return nullptr; } +ValueFlow::Value* ProgramMemory::getValue(nonneg int exprid, bool impossible) +{ + 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()); + if (found) + return &it->second; + return nullptr; +} + // cppcheck-suppress unusedFunction bool ProgramMemory::getIntValue(nonneg int exprid, MathLib::bigint& result, bool impossible) const { @@ -172,24 +187,6 @@ bool ProgramMemory::hasValue(nonneg int exprid, bool impossible) const return it != mValues->cend() && (impossible || !it->second.isImpossible()); } -const ValueFlow::Value& ProgramMemory::at(nonneg int exprid, bool impossible) const { - const auto it = find(exprid); - if (it == mValues->cend() && (impossible || !it->second.isImpossible())) { - throw std::out_of_range("ProgramMemory::at"); - } - return it->second; -} - -ValueFlow::Value& ProgramMemory::at(nonneg int exprid, bool impossible) { - copyOnWrite(); - - const auto it = find(exprid); - if (it == mValues->end() && (impossible || !it->second.isImpossible())) { - throw std::out_of_range("ProgramMemory::at"); - } - return it->second; -} - void ProgramMemory::erase_if(const std::function& pred) { if (mValues->empty()) @@ -253,6 +250,7 @@ ProgramMemory::Map::const_iterator ProgramMemory::find(nonneg int exprid) const 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)); @@ -1350,10 +1348,9 @@ namespace { 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 @@ -1482,18 +1479,18 @@ namespace { 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; @@ -1505,20 +1502,20 @@ namespace { 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)) { @@ -1609,13 +1606,16 @@ namespace { } 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% {|(")) { @@ -1665,14 +1665,16 @@ namespace { } // 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; @@ -1724,9 +1726,12 @@ namespace { 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()) { @@ -1734,12 +1739,14 @@ namespace { continue; if (!value.isKnown()) continue; - if (value.tokvalue->exprId() > 0 && !pm->hasValue(value.tokvalue->exprId())) + if (value.tokvalue->exprId() == 0) + continue; + 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; } diff --git a/lib/programmemory.h b/lib/programmemory.h index d871d139e11..c31738fd56c 100644 --- a/lib/programmemory.h +++ b/lib/programmemory.h @@ -109,7 +109,8 @@ struct CPPCHECKLIB ProgramMemory { explicit ProgramMemory(Map values) : mValues(new Map(std::move(values))) {} void setValue(const Token* expr, const ValueFlow::Value& value); - const ValueFlow::Value* getValue(nonneg int exprid, bool impossible) const; + 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, bool impossible = false) const; void setIntValue(const Token* expr, MathLib::bigint value, bool impossible = false); @@ -123,9 +124,6 @@ struct CPPCHECKLIB ProgramMemory { bool getTokValue(nonneg int exprid, const Token*& result, bool impossible = false) const; bool hasValue(nonneg int exprid, bool impossible = true) const; - const ValueFlow::Value& at(nonneg int exprid, bool impossible = true) const; - ValueFlow::Value& at(nonneg int exprid, bool impossible = true); - void erase_if(const std::function& pred); void swap(ProgramMemory &pm) NOEXCEPT; diff --git a/test/testprogrammemory.cpp b/test/testprogrammemory.cpp index 903f0a54c8a..a73090360b6 100644 --- a/test/testprogrammemory.cpp +++ b/test/testprogrammemory.cpp @@ -32,7 +32,6 @@ class TestProgramMemory : public TestFixture { TEST_CASE(copyOnWrite); TEST_CASE(hasValue); TEST_CASE(getValue); - TEST_CASE(at); } void copyOnWrite() const { @@ -42,11 +41,11 @@ class TestProgramMemory : public TestFixture { tok->exprId(id); ProgramMemory pm; - const ValueFlow::Value* v = pm.getValue(id, false); + const ValueFlow::Value* v = utils::as_const(pm).getValue(id, false); ASSERT(!v); pm.setValue(tok, ValueFlow::Value{41}); - v = pm.getValue(id, false); + v = utils::as_const(pm).getValue(id, false); ASSERT(v); ASSERT_EQUALS(41, v->intvalue); @@ -54,7 +53,7 @@ class TestProgramMemory : public TestFixture { ProgramMemory pm2 = pm; // make sure the value was copied - v = pm2.getValue(id, false); + v = utils::as_const(pm2).getValue(id, false); ASSERT(v); ASSERT_EQUALS(41, v->intvalue); @@ -68,17 +67,17 @@ class TestProgramMemory : public TestFixture { pm3.setValue(tok, ValueFlow::Value{43}); // make sure the value was set - v = pm2.getValue(id, false); + 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, false); + 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, false); + v = utils::as_const(pm).getValue(id, false); ASSERT(v); ASSERT_EQUALS(41, v->intvalue); } @@ -90,13 +89,7 @@ class TestProgramMemory : public TestFixture { void getValue() const { ProgramMemory pm; - ASSERT(!pm.getValue(123, false)); - } - - 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)); } };