From 3a7ba3cd295550bb6e2903af698ac2df4fdac2b3 Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Fri, 30 Jul 2021 14:29:35 -0500 Subject: [PATCH] Add symbolic values to ValueFlow (#3367) --- lib/astutils.cpp | 2 +- lib/reverseanalyzer.cpp | 2 +- lib/token.cpp | 50 +++++++-------- lib/valueflow.cpp | 133 +++++++++++++++++++++++++++++++++++----- lib/valueflow.h | 26 +++++++- test/testvalueflow.cpp | 111 +++++++++++++++++++++++++++++++-- 6 files changed, 271 insertions(+), 53 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 1fd63948b..de192f45e 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -878,7 +878,7 @@ static bool compareKnownValue(const Token * const tok1, const Token * const tok2 if (v1 == tok1->values().end()) { return false; } - if (v1->isNonValue() || v1->isContainerSizeValue()) + if (v1->isNonValue() || v1->isContainerSizeValue() || v1->isSymbolicValue()) return false; const auto v2 = std::find_if(tok2->values().begin(), tok2->values().end(), isKnownFn); if (v2 == tok2->values().end()) { diff --git a/lib/reverseanalyzer.cpp b/lib/reverseanalyzer.cpp index ec2b39f5d..d9989c1e1 100644 --- a/lib/reverseanalyzer.cpp +++ b/lib/reverseanalyzer.cpp @@ -172,7 +172,7 @@ struct ReverseTraversal { settings); } // Assignment to - } else if (lhsAction.matches() && !assignTok->astOperand2()->hasKnownValue() && + } else if (lhsAction.matches() && !assignTok->astOperand2()->hasKnownIntValue() && isConstExpression(assignTok->astOperand2(), settings->library, true, true)) { const std::string info = "Assignment to '" + assignTok->expressionString() + "'"; ValuePtr a = analyzer->reanalyze(assignTok->astOperand2(), info); diff --git a/lib/token.cpp b/lib/token.cpp index a17283302..fef9899e0 100644 --- a/lib/token.cpp +++ b/lib/token.cpp @@ -1292,8 +1292,7 @@ std::string Token::stringifyList(const stringifyOptions& options, const std::vec if (options.linenumbers) { ret += std::to_string(lineNumber); ret += ':'; - if (lineNumber == tok->linenr()) - ret += ' '; + ret += ' '; } } else { while (lineNumber < tok->linenr()) { @@ -1742,6 +1741,10 @@ void Token::printValueFlow(bool xml, std::ostream &out) const case ValueFlow::Value::ValueType::LIFETIME: out << "lifetime=\"" << value.tokvalue << '\"'; break; + case ValueFlow::Value::ValueType::SYMBOLIC: + out << "tokvalue=\"" << value.tokvalue << '\"'; + out << " intvalue=\"" << value.intvalue << '\"'; + break; } if (value.condition) out << " condition-line=\"" << value.condition->linenr() << '\"'; @@ -1795,6 +1798,14 @@ void Token::printValueFlow(bool xml, std::ostream &out) const out << "lifetime[" << ValueFlow::Value::toString(value.lifetimeKind) << "]=(" << value.tokvalue->expressionString() << ")"; break; + case ValueFlow::Value::ValueType::SYMBOLIC: + out << "symbolic=(" << value.tokvalue->expressionString(); + if (value.intvalue > 0) + out << "+" << value.intvalue; + else if (value.intvalue < 0) + out << "-" << -value.intvalue; + out << ")"; + break; } if (value.indirect > 0) for (int i=0; imValues) { // Clear all other values of the same type since value is known - mImpl->mValues->remove_if([&](const ValueFlow::Value & x) { - return x.valueType == value.valueType; + mImpl->mValues->remove_if([&](const ValueFlow::Value& x) { + if (x.valueType != value.valueType) + return false; + // Allow multiple known symbolic values + if (x.isSymbolicValue()) + return !x.isKnown(); + return true; }); } @@ -2146,31 +2162,7 @@ bool Token::addValue(const ValueFlow::Value &value) continue; // different value => continue - bool differentValue = true; - switch (it->valueType) { - case ValueFlow::Value::ValueType::INT: - case ValueFlow::Value::ValueType::CONTAINER_SIZE: - case ValueFlow::Value::ValueType::BUFFER_SIZE: - case ValueFlow::Value::ValueType::ITERATOR_START: - case ValueFlow::Value::ValueType::ITERATOR_END: - differentValue = (it->intvalue != value.intvalue); - break; - case ValueFlow::Value::ValueType::TOK: - case ValueFlow::Value::ValueType::LIFETIME: - differentValue = (it->tokvalue != value.tokvalue); - break; - case ValueFlow::Value::ValueType::FLOAT: - // TODO: Write some better comparison - differentValue = (it->floatValue > value.floatValue || it->floatValue < value.floatValue); - break; - case ValueFlow::Value::ValueType::MOVED: - differentValue = (it->moveKind != value.moveKind); - break; - case ValueFlow::Value::ValueType::UNINIT: - differentValue = false; - break; - } - if (differentValue) + if (!it->equalValue(value)) continue; if ((value.isTokValue() || value.isLifetimeValue()) && (it->tokvalue != value.tokvalue) && (it->tokvalue->str() != value.tokvalue->str())) diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 24c059081..1bfc280be 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -1387,7 +1387,7 @@ static void valueFlowBitAnd(TokenList *tokenlist) static void valueFlowSameExpressions(TokenList *tokenlist) { for (Token *tok = tokenlist->front(); tok; tok = tok->next()) { - if (tok->hasKnownValue()) + if (tok->hasKnownIntValue()) continue; if (!tok->astOperand1() || !tok->astOperand2()) @@ -2032,7 +2032,7 @@ struct ValueFlowAnalyzer : Analyzer { const ValueFlow::Value* value = getValue(tok); if (!value) return Action::None; - if (!(value->isIntValue() || value->isFloatValue())) + if (!(value->isIntValue() || value->isFloatValue() || value->isSymbolicValue())) return Action::None; const Token* parent = tok->astParent(); @@ -2409,6 +2409,8 @@ struct ExpressionAnalyzer : SingleValueFlowAnalyzer { dependOnThis = exprDependsOnThis(expr); setupExprVarIds(expr); + if (val.isSymbolicValue()) + setupExprVarIds(val.tokvalue); } virtual const ValueType* getValueType(const Token*) const OVERRIDE { @@ -3992,6 +3994,100 @@ static void valueFlowConditionExpressions(TokenList *tokenlist, SymbolDatabase* } } +static ValueFlow::Value makeSymbolic(const Token* tok, MathLib::bigint delta = 0) +{ + ValueFlow::Value value; + value.setKnown(); + value.valueType = ValueFlow::Value::ValueType::SYMBOLIC; + value.tokvalue = tok; + value.intvalue = delta; + return value; +} + +static void valueFlowSymbolic(TokenList* tokenlist, SymbolDatabase* symboldatabase) +{ + for (const Scope* scope : symboldatabase->functionScopes) { + for (Token* tok = const_cast(scope->bodyStart); tok != scope->bodyEnd; tok = tok->next()) { + if (!Token::simpleMatch(tok, "=")) + continue; + if (!tok->astOperand1()) + continue; + if (!tok->astOperand2()) + continue; + if (tok->astOperand1()->hasKnownIntValue()) + continue; + if (tok->astOperand2()->hasKnownIntValue()) + continue; + + Token* start = nextAfterAstRightmostLeaf(tok); + const Token* end = scope->bodyEnd; + + ValueFlow::Value rhs = makeSymbolic(tok->astOperand2()); + rhs.errorPath.emplace_back(tok, + tok->astOperand1()->expressionString() + " is assigned '" + + tok->astOperand2()->expressionString() + "' here."); + valueFlowForward(start, end, tok->astOperand1(), {rhs}, tokenlist, tokenlist->getSettings()); + + ValueFlow::Value lhs = makeSymbolic(tok->astOperand1()); + lhs.errorPath.emplace_back(tok, + tok->astOperand1()->expressionString() + " is assigned '" + + tok->astOperand2()->expressionString() + "' here."); + valueFlowForward(start, end, tok->astOperand2(), {lhs}, tokenlist, tokenlist->getSettings()); + } + } +} + +static void valueFlowSymbolicInfer(TokenList* tokenlist, SymbolDatabase* symboldatabase) +{ + for (const Scope* scope : symboldatabase->functionScopes) { + for (Token* tok = const_cast(scope->bodyStart); tok != scope->bodyEnd; tok = tok->next()) { + if (!Token::Match(tok, "-|%comp%")) + continue; + if (tok->hasKnownIntValue()) + continue; + if (!tok->astOperand1()) + continue; + if (!tok->astOperand2()) + continue; + if (tok->astOperand1()->hasKnownIntValue()) + continue; + if (tok->astOperand2()->hasKnownIntValue()) + continue; + + MathLib::bigint rhsvalue = 0; + const ValueFlow::Value* rhs = + ValueFlow::findValue(tok->astOperand2()->values(), nullptr, [&](const ValueFlow::Value& v) { + return v.isSymbolicValue() && v.tokvalue && v.tokvalue->exprId() == tok->astOperand1()->exprId(); + }); + if (rhs) + rhsvalue = rhs->intvalue; + MathLib::bigint lhsvalue = 0; + const ValueFlow::Value* lhs = + ValueFlow::findValue(tok->astOperand1()->values(), nullptr, [&](const ValueFlow::Value& v) { + return v.isSymbolicValue() && v.tokvalue && v.tokvalue->exprId() == tok->astOperand2()->exprId(); + }); + if (lhs) + lhsvalue = lhs->intvalue; + if (!lhs && !rhs) + continue; + + ValueFlow::Value value{calculate(tok->str(), lhsvalue, rhsvalue)}; + if (lhs && rhs) { + if (lhs->isImpossible() != rhs->isImpossible()) + continue; + combineValueProperties(*lhs, *rhs, &value); + } else if (lhs) { + value.valueKind = lhs->valueKind; + value.errorPath = lhs->errorPath; + } else if (rhs) { + value.valueKind = rhs->valueKind; + value.errorPath = rhs->errorPath; + } + setTokenValue(tok, value, tokenlist->getSettings()); + } + } +} + static void valueFlowForwardAssign(Token* const tok, const Token* expr, std::vector vars, @@ -4044,17 +4140,13 @@ static void valueFlowForwardAssign(Token* const tok, // Skip RHS const Token * nextExpression = tok->astParent() ? nextAfterAstRightmostLeaf(tok->astParent()) : tok->next(); - if (std::any_of(values.begin(), values.end(), std::mem_fn(&ValueFlow::Value::isTokValue))) { - std::list tokvalues; - std::copy_if(values.begin(), - values.end(), - std::back_inserter(tokvalues), - std::mem_fn(&ValueFlow::Value::isTokValue)); - valueFlowForward(const_cast(nextExpression), endOfVarScope, expr, values, tokenlist, settings); - values.remove_if(std::mem_fn(&ValueFlow::Value::isTokValue)); - } - for (ValueFlow::Value& value:values) + for (ValueFlow::Value& value : values) { + if (value.isSymbolicValue()) + continue; + if (value.isTokValue()) + continue; value.tokvalue = tok; + } valueFlowForward(const_cast(nextExpression), endOfVarScope, expr, values, tokenlist, settings); } @@ -4163,7 +4255,7 @@ static void valueFlowAfterAssign(TokenList *tokenlist, SymbolDatabase* symboldat std::set types; if (tok->astOperand1()->hasKnownValue()) { for (const ValueFlow::Value& value:tok->astOperand1()->values()) { - if (value.isKnown()) + if (value.isKnown() && !value.isSymbolicValue()) types.insert(value.valueType); } } @@ -4719,7 +4811,7 @@ struct SimpleConditionHandler : ConditionHandler { ValueFlow::Value false_value; const Token *vartok = parseCompareInt(tok, true_value, false_value); if (vartok) { - if (vartok->hasKnownValue()) + if (vartok->hasKnownIntValue()) return {}; if (vartok->str() == "=" && vartok->astOperand1() && vartok->astOperand2()) vartok = vartok->astOperand1(); @@ -4887,7 +4979,7 @@ static void valueFlowInferCondition(TokenList* tokenlist, for (Token* tok = tokenlist->front(); tok; tok = tok->next()) { if (!tok->astParent()) continue; - if (tok->hasKnownValue()) + if (tok->hasKnownIntValue()) continue; if (tok->variable() && (Token::Match(tok->astParent(), "?|&&|!|%oror%") || Token::Match(tok->astParent()->previous(), "if|while ("))) { @@ -6365,7 +6457,7 @@ static void valueFlowContainerSize(TokenList *tokenlist, SymbolDatabase* symbold continue; if (!astIsContainer(var->nameToken())) continue; - if (var->nameToken()->hasKnownValue()) + if (var->nameToken()->hasKnownValue(ValueFlow::Value::ValueType::CONTAINER_SIZE)) continue; if (!Token::Match(var->nameToken(), "%name% ;") && !(Token::Match(var->nameToken(), "%name% {") && Token::simpleMatch(var->nameToken()->next()->link(), "} ;"))) @@ -6824,6 +6916,13 @@ std::string ValueFlow::Value::infoString() const return "end=" + MathLib::toString(intvalue); case ValueType::LIFETIME: return "lifetime=" + tokvalue->str(); + case ValueType::SYMBOLIC: + std::string result = "symbolic=" + tokvalue->expressionString(); + if (intvalue > 0) + result += "+" + MathLib::toString(intvalue); + else if (intvalue < 0) + result += "-" + MathLib::toString(-intvalue); + return result; } throw InternalError(nullptr, "Invalid ValueFlow Value type"); } @@ -6892,6 +6991,7 @@ void ValueFlow::setValues(TokenList *tokenlist, SymbolDatabase* symboldatabase, valueFlowGlobalStaticVar(tokenlist, settings); valueFlowPointerAlias(tokenlist); valueFlowLifetime(tokenlist, symboldatabase, errorLogger, settings); + valueFlowSymbolic(tokenlist, symboldatabase); valueFlowBitAnd(tokenlist); valueFlowSameExpressions(tokenlist); valueFlowConditionExpressions(tokenlist, symboldatabase, errorLogger, settings); @@ -6901,6 +7001,7 @@ void ValueFlow::setValues(TokenList *tokenlist, SymbolDatabase* symboldatabase, while (n > 0 && values < getTotalValues(tokenlist)) { values = getTotalValues(tokenlist); valueFlowImpossibleValues(tokenlist, settings); + valueFlowSymbolicInfer(tokenlist, symboldatabase); valueFlowArrayBool(tokenlist); valueFlowRightShift(tokenlist, settings); valueFlowAfterMove(tokenlist, symboldatabase, settings); diff --git a/lib/valueflow.h b/lib/valueflow.h index 996d7eab3..485d33cbb 100644 --- a/lib/valueflow.h +++ b/lib/valueflow.h @@ -26,6 +26,7 @@ #include "mathlib.h" #include "utils.h" +#include #include #include #include @@ -132,6 +133,13 @@ namespace ValueFlow { case ValueType::LIFETIME: if (tokvalue != rhs.tokvalue) return false; + break; + case ValueType::SYMBOLIC: + if (tokvalue != rhs.tokvalue) + return false; + if (intvalue != rhs.intvalue) + return false; + break; } return true; } @@ -140,6 +148,7 @@ namespace ValueFlow { static void visitValue(T& self, F f) { switch (self.valueType) { case ValueType::INT: + case ValueType::SYMBOLIC: case ValueType::BUFFER_SIZE: case ValueType::CONTAINER_SIZE: case ValueType::ITERATOR_START: @@ -175,6 +184,8 @@ namespace ValueFlow { template bool compareValue(const Value& rhs, Compare compare) const { + assert((!this->isSymbolicValue() && !rhs.isSymbolicValue()) || + (this->valueType == rhs.valueType && this->tokvalue == rhs.tokvalue)); bool result = false; visitValue( *this, @@ -229,7 +240,19 @@ namespace ValueFlow { std::string infoString() const; - enum class ValueType { INT, TOK, FLOAT, MOVED, UNINIT, CONTAINER_SIZE, LIFETIME, BUFFER_SIZE, ITERATOR_START, ITERATOR_END } valueType; + enum class ValueType { + INT, + TOK, + FLOAT, + MOVED, + UNINIT, + CONTAINER_SIZE, + LIFETIME, + BUFFER_SIZE, + ITERATOR_START, + ITERATOR_END, + SYMBOLIC + } valueType; bool isIntValue() const { return valueType == ValueType::INT; } @@ -263,6 +286,7 @@ namespace ValueFlow { bool isIteratorEndValue() const { return valueType == ValueType::ITERATOR_END; } + bool isSymbolicValue() const { return valueType == ValueType::SYMBOLIC; } bool isLocalLifetimeValue() const { return valueType == ValueType::LIFETIME && lifetimeScope == LifetimeScope::Local; diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index a1365868d..da5a03762 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -24,16 +24,17 @@ #include "tokenize.h" #include "valueflow.h" -#include #include #include +#include +#include +#include #include #include +#include #include #include #include -#include -#include class TestValueFlow : public TestFixture { public: @@ -142,6 +143,7 @@ private: TEST_CASE(valueFlowIdempotent); TEST_CASE(valueFlowUnsigned); TEST_CASE(valueFlowMod); + TEST_CASE(valueFlowSymbolic); } static bool isNotTokValue(const ValueFlow::Value &val) { @@ -152,6 +154,8 @@ private: return !val.isLifetimeValue(); } + static bool isNotUninitValue(const ValueFlow::Value& val) { return !val.isUninitValue(); } + static bool isNotPossible(const ValueFlow::Value& val) { return !val.isPossible(); } @@ -177,6 +181,8 @@ private: for (const Token *tok = tokenizer.tokens(); tok; tok = tok->next()) { if (tok->str() == "x" && tok->linenr() == linenr) { for (const ValueFlow::Value& val:tok->values()) { + if (val.isSymbolicValue()) + continue; if (val.isKnown() && val.intvalue == value) return true; } @@ -186,6 +192,26 @@ private: return false; } + bool testValueOfXKnown(const char code[], unsigned int linenr, const std::string& expr, int value) { + // Tokenize.. + Tokenizer tokenizer(&settings, this); + std::istringstream istr(code); + tokenizer.tokenize(istr, "test.cpp"); + + for (const Token* tok = tokenizer.tokens(); tok; tok = tok->next()) { + if (tok->str() == "x" && tok->linenr() == linenr) { + for (const ValueFlow::Value& val : tok->values()) { + if (!val.isSymbolicValue()) + continue; + if (val.isKnown() && val.intvalue == value && val.tokvalue->expressionString() == expr) + return true; + } + } + } + + return false; + } + bool testValueOfXImpossible(const char code[], unsigned int linenr, int value) { // Tokenize.. Tokenizer tokenizer(&settings, this); @@ -195,6 +221,8 @@ private: for (const Token *tok = tokenizer.tokens(); tok; tok = tok->next()) { if (tok->str() == "x" && tok->linenr() == linenr) { for (const ValueFlow::Value& val:tok->values()) { + if (val.isSymbolicValue()) + continue; if (val.isImpossible() && val.intvalue == value) return true; } @@ -213,6 +241,8 @@ private: for (const Token *tok = tokenizer.tokens(); tok; tok = tok->next()) { if (tok->str() == "x" && tok->linenr() == linenr) { for (const ValueFlow::Value& val:tok->values()) { + if (val.isSymbolicValue()) + continue; if (val.isInconclusive() && val.intvalue == value) return true; } @@ -1152,7 +1182,8 @@ private: " y += 12;\n" " if (y == 32) {}" "}\n"; - ASSERT_EQUALS("5,Assuming that condition 'y==32' is not redundant\n" + ASSERT_EQUALS("2,x is assigned 'y' here.\n" + "5,Assuming that condition 'y==32' is not redundant\n" "4,Compound assignment '+=', assigned value is 20\n" "2,Assignment 'x=y', assigned value is 20\n", getErrorPathForX(code, 3U)); @@ -1173,7 +1204,10 @@ private: " for (x = a; x < 50; x++) {}\n" " b = x;\n" "}\n"; - ASSERT_EQUALS("3,After for loop, x has value 50\n", + ASSERT_EQUALS("3,x is assigned 'a' here.\n" + "3,x is incremented', new value is symbolic=a+1\n" + "3,x is incremented', new value is symbolic=a+2\n" + "3,After for loop, x has value 50\n", getErrorPathForX(code, 4U)); } @@ -4298,6 +4332,7 @@ private: " a = x + 1;\n" "}\n"; values = tokenValues(code, "x +"); + values.remove_if(&isNotUninitValue); ASSERT_EQUALS(true, values.empty()); // #8494 - overloaded operator & @@ -4682,6 +4717,7 @@ private: static std::string isPossibleContainerSizeValue(std::list values, MathLib::bigint i, bool unique = true) { + values.remove_if(std::mem_fn(&ValueFlow::Value::isSymbolicValue)); if (!unique) values.remove_if(&isNotPossible); if (values.size() != 1) @@ -4698,6 +4734,7 @@ private: static std::string isImpossibleContainerSizeValue(std::list values, MathLib::bigint i, bool unique = true) { + values.remove_if(std::mem_fn(&ValueFlow::Value::isSymbolicValue)); if (!unique) values.remove_if(&isNotImpossible); if (values.size() != 1) @@ -4714,6 +4751,7 @@ private: static std::string isInconclusiveContainerSizeValue(std::list values, MathLib::bigint i, bool unique = true) { + values.remove_if(std::mem_fn(&ValueFlow::Value::isSymbolicValue)); if (!unique) values.remove_if(&isNotInconclusive); if (values.size() != 1) @@ -4728,6 +4766,7 @@ private: } static std::string isKnownContainerSizeValue(std::list values, MathLib::bigint i, bool unique = true) { + values.remove_if(std::mem_fn(&ValueFlow::Value::isSymbolicValue)); if (!unique) values.remove_if(&isNotKnown); if (values.size() != 1) @@ -5898,6 +5937,68 @@ private: ASSERT_EQUALS(false, testValueOfXImpossible(code, 3U, 0)); ASSERT_EQUALS(false, testValueOfXImpossible(code, 3U, 1)); } + + void valueFlowSymbolic() { + const char* code; + + code = "int f(int i) {\n" + " int j = i;\n" + " int x = i;\n" + " return x;\n" + "}\n"; + ASSERT_EQUALS(true, testValueOfXKnown(code, 4U, "j", 0)); + ASSERT_EQUALS(true, testValueOfXKnown(code, 4U, "i", 0)); + + code = "int f(int i) {\n" + " int j = i;\n" + " int x = j;\n" + " return x;\n" + "}\n"; + ASSERT_EQUALS(true, testValueOfXKnown(code, 4U, "i", 0)); + ASSERT_EQUALS(true, testValueOfXKnown(code, 4U, "j", 0)); + + code = "void g(int&);\n" + "int f(int i) {\n" + " int j = i;\n" + " g(i);\n" + " int x = i;\n" + " return x;\n" + "}\n"; + ASSERT_EQUALS(true, testValueOfXKnown(code, 6U, "i", 0)); + ASSERT_EQUALS(false, testValueOfXKnown(code, 6U, "j", 0)); + + code = "int f(int i) {\n" + " int j = i;\n" + " j++;\n" + " int x = i == j;\n" + " return x;\n" + "}\n"; + ASSERT_EQUALS(true, testValueOfXKnown(code, 5U, 0)); + + code = "int f(int i) {\n" + " int j = i;\n" + " i++;\n" + " int x = i - j;\n" + " return x;\n" + "}\n"; + ASSERT_EQUALS(true, testValueOfXKnown(code, 5U, 1)); + + code = "int f(int i) {\n" + " int j = i;\n" + " i++;\n" + " int x = i > j;\n" + " return x;\n" + "}\n"; + ASSERT_EQUALS(true, testValueOfXKnown(code, 5U, 1)); + + code = "int f(int i) {\n" + " int j = i;\n" + " j++;\n" + " int x = j > i;\n" + " return x;\n" + "}\n"; + ASSERT_EQUALS(true, testValueOfXKnown(code, 5U, 1)); + } }; REGISTER_TEST(TestValueFlow)