From 0e871c178f27ed51d8c4353153ff4875e3a213e0 Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Tue, 9 Feb 2021 08:27:46 -0600 Subject: [PATCH] Fix issue 10141: Errors with ref assignment (duplicateValueTenary and knownEmptyContainer) (#3093) --- lib/astutils.cpp | 113 ++++++++++++++++++++++++++--------------- lib/astutils.h | 8 +++ lib/valueflow.cpp | 81 +++++++++++++++++++++-------- test/testvalueflow.cpp | 42 +++++++++++---- 4 files changed, 174 insertions(+), 70 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 1b17719f9..dd6fed54e 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -30,6 +30,7 @@ #include "valueflow.h" #include +#include #include #include #include @@ -751,73 +752,105 @@ static void followVariableExpressionError(const Token *tok1, const Token *tok2, errors->push_back(item); } -static const Token* followReferences(const Token* tok, ErrorPath* errors, int depth = 20) +std::vector followAllReferences(const Token* tok, bool inconclusive, ErrorPath errors, int depth) { + struct ReferenceTokenLess { + bool operator()(const ReferenceToken& x, const ReferenceToken& y) const + { + return x.token < y.token; + } + }; if (!tok) - return nullptr; + return std::vector{}; if (depth < 0) - return tok; + return {{tok, std::move(errors)}}; const Variable *var = tok->variable(); if (var && var->declarationId() == tok->varId()) { if (var->nameToken() == tok) { - return tok; + return {{tok, std::move(errors)}}; } else if (var->isReference() || var->isRValueReference()) { if (!var->declEndToken()) - return tok; + return {{tok, std::move(errors)}}; if (var->isArgument()) { - if (errors) - errors->emplace_back(var->declEndToken(), "Passed to reference."); - return tok; + errors.emplace_back(var->declEndToken(), "Passed to reference."); + return {{tok, std::move(errors)}}; } else if (Token::simpleMatch(var->declEndToken(), "=")) { - if (errors) - errors->emplace_back(var->declEndToken(), "Assigned to reference."); + errors.emplace_back(var->declEndToken(), "Assigned to reference."); const Token *vartok = var->declEndToken()->astOperand2(); if (vartok == tok) - return tok; + return {{tok, std::move(errors)}}; if (vartok) - return followReferences(vartok, errors, depth - 1); + return followAllReferences(vartok, inconclusive, std::move(errors), depth - 1); } else { - return tok; + return {{tok, std::move(errors)}}; } } + } else if (Token::simpleMatch(tok, "?") && Token::simpleMatch(tok->astOperand2(), ":")) { + std::set result; + const Token* tok2 = tok->astOperand2(); + + std::vector refs; + refs = followAllReferences(tok2->astOperand1(), inconclusive, errors, depth - 1); + result.insert(refs.begin(), refs.end()); + refs = followAllReferences(tok2->astOperand2(), inconclusive, errors, depth - 1); + result.insert(refs.begin(), refs.end()); + + if (!inconclusive && result.size() != 1) + return {{tok, std::move(errors)}}; + + if (!result.empty()) + return std::vector(result.begin(), result.end()); + } else if (Token::Match(tok->previous(), "%name% (")) { const Function *f = tok->previous()->function(); if (f) { if (!Function::returnsReference(f)) - return tok; - std::set result; - ErrorPath errorPath; + return {{tok, std::move(errors)}}; + std::set result; for (const Token* returnTok : Function::findReturns(f)) { if (returnTok == tok) continue; - const Token* argvarTok = followReferences(returnTok, errors, depth - 1); - const Variable* argvar = argvarTok->variable(); - if (!argvar) - return tok; - if (argvar->isArgument() && (argvar->isReference() || argvar->isRValueReference())) { - int n = getArgumentPos(argvar, f); - if (n < 0) - return tok; - std::vector args = getArguments(tok->previous()); - if (n >= args.size()) - return tok; - const Token* argTok = args[n]; - ErrorPath er; - er.emplace_back(returnTok, "Return reference."); - er.emplace_back(tok->previous(), "Called function passing '" + argTok->expressionString() + "'."); - const Token* tok2 = followReferences(argTok, &er, depth - 1); - errorPath = er; - result.insert(tok2); + std::vector argvarRt = followAllReferences(returnTok, inconclusive, errors, depth - 1); + for(const ReferenceToken& rt:followAllReferences(returnTok, inconclusive, errors, depth - 1)) { + const Variable* argvar = rt.token->variable(); + if (!argvar) + return {{tok, std::move(errors)}}; + if (argvar->isArgument() && (argvar->isReference() || argvar->isRValueReference())) { + int n = getArgumentPos(argvar, f); + if (n < 0) + return {{tok, std::move(errors)}}; + std::vector args = getArguments(tok->previous()); + if (n >= args.size()) + return {{tok, std::move(errors)}}; + const Token* argTok = args[n]; + ErrorPath er = errors; + er.emplace_back(returnTok, "Return reference."); + er.emplace_back(tok->previous(), "Called function passing '" + argTok->expressionString() + "'."); + std::vector refs = followAllReferences(argTok, inconclusive, std::move(er), depth - 1); + result.insert(refs.begin(), refs.end()); + if (!inconclusive && result.size() > 1) + return {{tok, std::move(errors)}}; + } } } - if (result.size() == 1) { - if (errors) - errors->insert(errors->end(), errorPath.begin(), errorPath.end()); - return *result.begin(); - } + if (!result.empty()) + return std::vector(result.begin(), result.end()); } } - return tok; + return {{tok, std::move(errors)}}; +} + +const Token* followReferences(const Token* tok, ErrorPath* errors) +{ + if (!tok) + return nullptr; + std::vector refs = followAllReferences(tok, false); + if (refs.size() == 1) { + if (errors) + *errors = refs.front().errors; + return refs.front().token; + } + return nullptr; } static bool isSameLifetime(const Token * const tok1, const Token * const tok2) diff --git a/lib/astutils.h b/lib/astutils.h index 044ed4360..5bbe26aaf 100644 --- a/lib/astutils.h +++ b/lib/astutils.h @@ -140,6 +140,14 @@ bool precedes(const Token * tok1, const Token * tok2); bool exprDependsOnThis(const Token* expr, nonneg int depth = 0); +struct ReferenceToken { + const Token* token; + ErrorPath errors; +}; + +std::vector followAllReferences(const Token* tok, bool inconclusive = true, ErrorPath errors = ErrorPath{}, int depth = 20); +const Token* followReferences(const Token* tok, ErrorPath* errors = nullptr); + bool isSameExpression(bool cpp, bool macro, const Token *tok1, const Token *tok2, const Library& library, bool pure, bool followVar, ErrorPath* errors=nullptr); bool isEqualKnownValue(const Token * const tok1, const Token * const tok2); diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 703679231..61943824f 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -1915,25 +1915,37 @@ struct ValueFlowAnalyzer : Analyzer { } } - virtual Action analyze(const Token* tok, Direction d) const OVERRIDE { - if (invalid()) - return Action::Invalid; + Action analyzeMatch(const Token* tok, Direction d) const { + const Token* parent = tok->astParent(); + if (astIsPointer(tok) && (Token::Match(parent, "*|[") || (parent && parent->originalName() == "->")) && getIndirect(tok) <= 0) + return Action::Read; + + Action w = isWritable(tok, d); + if (w != Action::None) + return w; + + // Check for modifications by function calls + return isModified(tok); + } + + Action analyzeToken(const Token* ref, const Token* tok, Direction d, bool inconclusiveRef) const + { + if (!ref) + return Action::None; + // If its an inconclusiveRef then ref != tok + assert(!inconclusiveRef || ref != tok); bool inconclusive = false; - if (match(tok)) { - const Token* parent = tok->astParent(); - if (astIsPointer(tok) && (Token::Match(parent, "*|[") || (parent && parent->originalName() == "->")) && getIndirect(tok) <= 0) - return Action::Read | Action::Match; - - // Action read = Action::Read; - Action w = isWritable(tok, d); - if (w != Action::None) - return w | Action::Match; - - // Check for modifications by function calls - return isModified(tok) | Action::Match; - } else if (tok->isUnaryOp("*")) { + if (match(ref)) { + if (inconclusiveRef) { + Action a = isModified(tok); + if (a.isModified() || a.isInconclusive()) + return Action::Inconclusive; + } else { + return analyzeMatch(tok, d) | Action::Match; + } + } else if (ref->isUnaryOp("*")) { const Token* lifeTok = nullptr; - for (const ValueFlow::Value& v:tok->astOperand1()->values()) { + for (const ValueFlow::Value& v:ref->astOperand1()->values()) { if (!v.isLocalLifetimeValue()) continue; if (lifeTok) @@ -1946,17 +1958,37 @@ struct ValueFlowAnalyzer : Analyzer { a = Action::Invalid; if (Token::Match(tok->astParent(), "%assign%") && astIsLHS(tok)) a |= Action::Invalid; + if (inconclusiveRef && a.isModified()) + return Action::Inconclusive; return a; } return Action::None; - } else if (isAlias(tok, inconclusive)) { + } else if (isAlias(ref, inconclusive)) { + inconclusive |= inconclusiveRef; Action a = isAliasModified(tok); if (inconclusive && a.isModified()) return Action::Inconclusive; else return a; - } else if (Token::Match(tok, "%name% (") && !Token::simpleMatch(tok->linkAt(1), ") {")) { + } + return Action::None; + } + + virtual Action analyze(const Token* tok, Direction d) const OVERRIDE { + if (invalid()) + return Action::Invalid; + // Follow references + std::vector refs = followAllReferences(tok); + const bool inconclusiveRefs = refs.size() != 1; + for(const ReferenceToken& ref:refs) { + Action a = analyzeToken(ref.token, tok, d, inconclusiveRefs); + if (a != Action::None) + return a; + } + + // TODO: Check if function is pure + if (Token::Match(tok, "%name% (") && !Token::simpleMatch(tok->linkAt(1), ") {")) { // bailout: global non-const variables if (isGlobal()) { return Action::Invalid; @@ -4216,7 +4248,14 @@ struct ConditionHandler { } } - Token* startTok = tok->astParent() ? tok->astParent() : tok->previous(); + Token* startTok = nullptr; + if (astIsRHS(tok)) + startTok = tok->astParent(); + else if (astIsLHS(tok)) + startTok = previousBeforeAstLeftmostLeaf(tok->astParent()); + if (!startTok) + startTok = tok->previous(); + reverse(startTok, nullptr, cond.vartok, values, tokenlist, settings); }); } @@ -5789,7 +5828,7 @@ struct ContainerVariableAnalyzer : VariableAnalyzer { return Action::Invalid; if (isLikelyStreamRead(isCPP(), tok->astParent())) return Action::Invalid; - if (isContainerSizeChanged(tok)) + if (astIsContainer(tok) && isContainerSizeChanged(tok)) return Action::Invalid; return read; } diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index e0fd9bd4c..22d19157a 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -4472,6 +4472,18 @@ private: return ""; } + static std::string isInconclusiveContainerSizeValue(const std::list& values, MathLib::bigint i) { + if (values.size() != 1) + return "values.size():" + std::to_string(values.size()); + if (!values.front().isContainerSizeValue()) + return "ContainerSizeValue"; + if (!values.front().isInconclusive()) + return "Inconclusive"; + if (values.front().intvalue != i) + return "intvalue:" + std::to_string(values.front().intvalue); + return ""; + } + static std::string isKnownContainerSizeValue(const std::list &values, MathLib::bigint i) { if (values.size() != 1) return "values.size():" + std::to_string(values.size()); @@ -5026,17 +5038,29 @@ private: "}\n"; ASSERT_EQUALS(true, testValueOfXKnown(code, 4U, 0)); - code = "bool f(std::string s) {\n" - " if (!s.empty()) {\n" - " bool x = s == \"0\";\n" - " return x;\n" + code = "bool f(std::string s) {\n" + " if (!s.empty()) {\n" + " bool x = s == \"0\";\n" + " return x;\n" + " }\n" + " return false;\n" + "}\n"; + ASSERT_EQUALS(false, testValueOfXKnown(code, 4U, 0)); + ASSERT_EQUALS(false, testValueOfXKnown(code, 4U, 1)); + ASSERT_EQUALS(false, testValueOfXImpossible(code, 4U, 0)); + + code = "bool f() {\n" + " std::list x1;\n" + " std::list x2;\n" + " for (int i = 0; i < 10; ++i) {\n" + " std::list& x = (i < 5) ? x1 : x2;\n" + " x.push_back(i);\n" " }\n" - " return false;\n" + " return x1.empty() || x2.empty();\n" "}\n"; - ASSERT_EQUALS(false, testValueOfXKnown(code, 4U, 0)); - ASSERT_EQUALS(false, testValueOfXKnown(code, 4U, 1)); - ASSERT_EQUALS(false, testValueOfXImpossible(code, 4U, 0)); - + ASSERT_EQUALS("", isInconclusiveContainerSizeValue(tokenValues(code, "x1 . empty", ValueFlow::Value::ValueType::CONTAINER_SIZE), 0)); + ASSERT_EQUALS("", isInconclusiveContainerSizeValue(tokenValues(code, "x2 . empty", ValueFlow::Value::ValueType::CONTAINER_SIZE), 0)); + code = "std::vector g();\n" "int f(bool b) {\n" " std::set a;\n"