From 165a22ed0f6b99a3fa672b0b46a15adc34983d01 Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Tue, 29 Jan 2019 02:47:52 -0600 Subject: [PATCH] Lifetime: Support analysis with functions that do not return a reference (#1632) * Initial support for function return * Add test case * Add support for reference parameters * Format --- lib/checkautovariables.cpp | 24 +----- lib/valueflow.cpp | 145 +++++++++++++++++++++++++++++++------ lib/valueflow.h | 24 +++++- test/testautovariables.cpp | 23 ++++++ 4 files changed, 170 insertions(+), 46 deletions(-) diff --git a/lib/checkautovariables.cpp b/lib/checkautovariables.cpp index e116be9e6..3769da5b0 100644 --- a/lib/checkautovariables.cpp +++ b/lib/checkautovariables.cpp @@ -622,7 +622,7 @@ void CheckAutoVariables::checkVarLifetimeScope(const Token * start, const Token } } for (const ValueFlow::Value& val:tok->values()) { - if (!val.isLifetimeValue()) + if (!val.isLocalLifetimeValue()) continue; // Skip temporaries for now if (val.tokvalue == tok) @@ -677,28 +677,6 @@ void CheckAutoVariables::checkVarLifetime() } } -static std::string lifetimeType(const Token *tok, const ValueFlow::Value* val) -{ - std::string result; - if (!val) - return "object"; - switch (val->lifetimeKind) { - case ValueFlow::Value::Lambda: - result = "lambda"; - break; - case ValueFlow::Value::Iterator: - result = "iterator"; - break; - case ValueFlow::Value::Object: - if (astIsPointer(tok)) - result = "pointer"; - else - result = "object"; - break; - } - return result; -} - static std::string lifetimeMessage(const Token *tok, const ValueFlow::Value *val, ErrorPath &errorPath) { const Token *vartok = val ? val->tokvalue : nullptr; diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 0361dc197..03d90cb9c 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -2609,22 +2609,59 @@ static const Token *findSimpleReturn(const Function *f) if (!scope) return nullptr; const Token *returnTok = nullptr; - for (const Token *tok = scope->bodyStart; tok && tok != scope->bodyEnd; tok = tok->next()) { + for (const Token *tok = scope->bodyStart->next(); tok && tok != scope->bodyEnd; tok = tok->next()) { if (tok->str() == "{" && tok->scope() && (tok->scope()->type == Scope::eLambda || tok->scope()->type == Scope::eClass)) { tok = tok->link(); continue; } - if (!Token::simpleMatch(tok->astParent(), "return")) - continue; - // Multiple returns - if (returnTok) - return nullptr; - returnTok = tok; + if (Token::simpleMatch(tok->astParent(), "return")) { + // Multiple returns + if (returnTok) + return nullptr; + returnTok = tok; + } + // Skip lambda functions since the scope may not be set correctly + const Token *lambdaEndToken = findLambdaEndToken(tok); + if (lambdaEndToken) { + tok = lambdaEndToken; + } } return returnTok; } +static int getArgumentPos(const Variable *var, const Function *f) +{ + auto arg_it = std::find_if(f->argumentList.begin(), f->argumentList.end(), [&](const Variable &v) { + return v.nameToken() == var->nameToken(); + }); + if (arg_it == f->argumentList.end()) + return -1; + return std::distance(f->argumentList.begin(), arg_it); +} + +std::string lifetimeType(const Token *tok, const ValueFlow::Value *val) +{ + std::string result; + if (!val) + return "object"; + switch (val->lifetimeKind) { + case ValueFlow::Value::Lambda: + result = "lambda"; + break; + case ValueFlow::Value::Iterator: + result = "iterator"; + break; + case ValueFlow::Value::Object: + if (astIsPointer(tok)) + result = "pointer"; + else + result = "object"; + break; + } + return result; +} + const Variable *getLifetimeVariable(const Token *tok, ValueFlow::Value::ErrorPath &errorPath) { if (!tok) @@ -2661,12 +2698,9 @@ const Variable *getLifetimeVariable(const Token *tok, ValueFlow::Value::ErrorPat if (!argvar) return nullptr; if (argvar->isArgument() && (argvar->isReference() || argvar->isRValueReference())) { - auto arg_it = std::find_if(f->argumentList.begin(), f->argumentList.end(), [&](const Variable &v) { - return v.nameToken() == argvar->nameToken(); - }); - if (arg_it == f->argumentList.end()) + int n = getArgumentPos(argvar, f); + if (n < 0) return nullptr; - std::size_t n = std::distance(f->argumentList.begin(), arg_it); const Token *argTok = getArguments(tok->previous()).at(n); errorPath.emplace_back(returnTok, "Return reference."); errorPath.emplace_back(tok->previous(), "Called function passing '" + argTok->str() + "'."); @@ -2761,21 +2795,29 @@ struct LifetimeStore { const Token *argtok; std::string message; ValueFlow::Value::LifetimeKind type; + ErrorPath errorPath; + + LifetimeStore(const Token *argtok, + const std::string &message, + ValueFlow::Value::LifetimeKind type = ValueFlow::Value::Object) + : argtok(argtok), message(message), type(type), errorPath() + {} template void byRef(Token *tok, TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings, Predicate pred) const { - ErrorPath errorPath; - const Variable *var = getLifetimeVariable(argtok, errorPath); + ErrorPath er = errorPath; + const Variable *var = getLifetimeVariable(argtok, er); if (!var) return; if (!pred(var)) return; - errorPath.emplace_back(argtok, message); + er.emplace_back(argtok, message); ValueFlow::Value value; value.valueType = ValueFlow::Value::LIFETIME; + value.lifetimeScope = ValueFlow::Value::Local; value.tokvalue = var->nameToken(); - value.errorPath = errorPath; + value.errorPath = er; value.lifetimeKind = type; // Don't add the value a second time if (std::find(tok->values().begin(), tok->values().end(), value) != tok->values().end()) @@ -2792,22 +2834,42 @@ struct LifetimeStore { template void byVal(Token *tok, TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings, Predicate pred) const { + if (argtok->values().empty()) { + ErrorPath er; + er.emplace_back(argtok, message); + const Variable *var = getLifetimeVariable(argtok, er); + if (var && var->isArgument()) { + ValueFlow::Value value; + value.valueType = ValueFlow::Value::LIFETIME; + value.lifetimeScope = ValueFlow::Value::Argument; + value.tokvalue = var->nameToken(); + value.errorPath = er; + value.lifetimeKind = type; + // Don't add the value a second time + if (std::find(tok->values().begin(), tok->values().end(), value) != tok->values().end()) + return; + setTokenValue(tok, value, tokenlist->getSettings()); + valueFlowForwardLifetime(tok, tokenlist, errorLogger, settings); + } + } for (const ValueFlow::Value &v : argtok->values()) { if (!v.isLifetimeValue()) continue; const Token *tok3 = v.tokvalue; - ErrorPath errorPath = v.errorPath; - const Variable *var = getLifetimeVariable(tok3, errorPath); + ErrorPath er = v.errorPath; + const Variable *var = getLifetimeVariable(tok3, er); if (!var) continue; if (!pred(var)) return; - errorPath.emplace_back(argtok, message); + er.emplace_back(argtok, message); + er.insert(er.end(), errorPath.begin(), errorPath.end()); ValueFlow::Value value; value.valueType = ValueFlow::Value::LIFETIME; + value.lifetimeScope = v.lifetimeScope; value.tokvalue = var->nameToken(); - value.errorPath = errorPath; + value.errorPath = er; value.lifetimeKind = type; // Don't add the value a second time if (std::find(tok->values().begin(), tok->values().end(), value) != tok->values().end()) @@ -2829,8 +2891,9 @@ struct LifetimeStore { if (!v.isLifetimeValue()) continue; const Token *tok2 = v.tokvalue; - ErrorPath errorPath = v.errorPath; - const Variable *var = getLifetimeVariable(tok2, errorPath); + ErrorPath er = v.errorPath; + const Variable *var = getLifetimeVariable(tok2, er); + er.insert(er.end(), errorPath.begin(), errorPath.end()); if (!var) continue; for (const Token *tok3 = tok; tok3 && tok3 != var->declEndToken(); tok3 = tok3->previous()) { @@ -2893,6 +2956,36 @@ static void valueFlowLifetimeFunction(Token *tok, TokenList *tokenlist, ErrorLog LifetimeStore{args.back(), "Added to container '" + vartok->str() + "'.", ValueFlow::Value::Object} .byVal( vartok, tokenlist, errorLogger, settings); } + } else if (tok->function()) { + const Function *f = tok->function(); + if (Function::returnsReference(f)) + return; + const Token *returnTok = findSimpleReturn(f); + if (!returnTok) + return; + for (const ValueFlow::Value &v : returnTok->values()) { + if (!v.isLifetimeValue()) + continue; + if (!v.tokvalue) + continue; + const Variable *var = v.tokvalue->variable(); + if (!var) + continue; + if (!var->isArgument()) + continue; + int n = getArgumentPos(var, f); + if (n < 0) + continue; + const Token *argtok = getArguments(tok).at(n); + LifetimeStore ls{argtok, "Passed to '" + tok->str() + "'.", ValueFlow::Value::Object}; + ls.errorPath = v.errorPath; + ls.errorPath.emplace_front(returnTok, "Return " + lifetimeType(returnTok, &v) + "."); + if (var->isReference() || var->isRValueReference()) { + ls.byRef(tok->next(), tokenlist, errorLogger, settings); + } else if (v.isArgumentLifetimeValue()) { + ls.byVal(tok->next(), tokenlist, errorLogger, settings); + } + } } } @@ -2949,6 +3042,10 @@ static bool isDecayedPointer(const Token *tok, const Settings *settings) static void valueFlowLifetime(TokenList *tokenlist, SymbolDatabase*, ErrorLogger *errorLogger, const Settings *settings) { for (Token *tok = tokenlist->front(); tok; tok = tok->next()) { + if (!tok->scope()) + continue; + if (tok->scope()->type == Scope::eGlobal) + continue; Lambda lam(tok); // Lamdas if (lam.isLambda()) { @@ -3009,6 +3106,7 @@ static void valueFlowLifetime(TokenList *tokenlist, SymbolDatabase*, ErrorLogger ValueFlow::Value value; value.valueType = ValueFlow::Value::LIFETIME; + value.lifetimeScope = ValueFlow::Value::Local; value.tokvalue = var->nameToken(); value.errorPath = errorPath; setTokenValue(tok, value, tokenlist->getSettings()); @@ -3031,6 +3129,7 @@ static void valueFlowLifetime(TokenList *tokenlist, SymbolDatabase*, ErrorLogger ValueFlow::Value value; value.valueType = ValueFlow::Value::LIFETIME; + value.lifetimeScope = ValueFlow::Value::Local; value.tokvalue = var->nameToken(); value.errorPath = errorPath; value.lifetimeKind = isIterator ? ValueFlow::Value::Iterator : ValueFlow::Value::Object; @@ -3056,6 +3155,7 @@ static void valueFlowLifetime(TokenList *tokenlist, SymbolDatabase*, ErrorLogger ValueFlow::Value value; value.valueType = ValueFlow::Value::LIFETIME; + value.lifetimeScope = ValueFlow::Value::Local; value.tokvalue = var->nameToken(); value.errorPath = errorPath; setTokenValue(tok, value, tokenlist->getSettings()); @@ -4822,6 +4922,7 @@ ValueFlow::Value::Value(const Token *c, long long val) conditional(false), defaultArg(false), lifetimeKind(Object), + lifetimeScope(Local), valueKind(ValueKind::Possible) { errorPath.emplace_back(c, "Assuming that condition '" + c->expressionString() + "' is not redundant"); diff --git a/lib/valueflow.h b/lib/valueflow.h index c329bb9eb..c2f49fc69 100644 --- a/lib/valueflow.h +++ b/lib/valueflow.h @@ -40,7 +40,21 @@ namespace ValueFlow { typedef std::pair ErrorPathItem; typedef std::list ErrorPath; - explicit Value(long long val = 0) : valueType(INT), intvalue(val), tokvalue(nullptr), floatValue(0.0), moveKind(NonMovedVariable), varvalue(val), condition(nullptr), varId(0U), conditional(false), defaultArg(false), lifetimeKind(Object), valueKind(ValueKind::Possible) {} + explicit Value(long long val = 0) + : valueType(INT), + intvalue(val), + tokvalue(nullptr), + floatValue(0.0), + moveKind(NonMovedVariable), + varvalue(val), + condition(nullptr), + varId(0U), + conditional(false), + defaultArg(false), + lifetimeKind(Object), + lifetimeScope(Local), + valueKind(ValueKind::Possible) + {} Value(const Token *c, long long val); bool operator==(const Value &rhs) const { @@ -108,6 +122,10 @@ namespace ValueFlow { return valueType == LIFETIME; } + bool isLocalLifetimeValue() const { return valueType == LIFETIME && lifetimeScope == Local; } + + bool isArgumentLifetimeValue() const { return valueType == LIFETIME && lifetimeScope == Argument; } + /** int value */ long long intvalue; @@ -139,6 +157,8 @@ namespace ValueFlow { enum LifetimeKind {Object, Lambda, Iterator} lifetimeKind; + enum LifetimeScope { Local, Argument } lifetimeScope; + static const char * toString(MoveKind moveKind) { switch (moveKind) { case NonMovedVariable: @@ -207,4 +227,6 @@ namespace ValueFlow { const Variable *getLifetimeVariable(const Token *tok, ValueFlow::Value::ErrorPath &errorPath); +std::string lifetimeType(const Token *tok, const ValueFlow::Value *val); + #endif // valueflowH diff --git a/test/testautovariables.cpp b/test/testautovariables.cpp index 48b0f3b73..366441f47 100644 --- a/test/testautovariables.cpp +++ b/test/testautovariables.cpp @@ -1752,6 +1752,29 @@ private: "[test.cpp:3] -> [test.cpp:3] -> [test.cpp:2] -> [test.cpp:3]: (error) Returning object that points to local variable 'a' that will be invalid when returning.\n", errout.str()); + check("template\n" + "auto by_value(T x) {\n" + " return [=] { return x; };\n" + "}\n" + "auto g() {\n" + " std::vector v;\n" + " return by_value(v.begin());\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:7] -> [test.cpp:7] -> [test.cpp:3] -> [test.cpp:3] -> [test.cpp:6] -> [test.cpp:7]: (error) Returning object that points to local variable 'v' that will be invalid when returning.\n", + errout.str()); + + check("auto by_ref(int& x) {\n" + " return [&] { return x; };\n" + "}\n" + "auto f() {\n" + " int i = 0;\n" + " return by_ref(i);\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:2] -> [test.cpp:1] -> [test.cpp:2] -> [test.cpp:6] -> [test.cpp:5] -> [test.cpp:6]: (error) Returning object that points to local variable 'i' that will be invalid when returning.\n", + errout.str()); + check("auto f(int x) {\n" " int a;\n" " std::tie(a) = x;\n"