From c8ff96fe8fe85a71d32705bde170bfa81ba6926e Mon Sep 17 00:00:00 2001 From: Frank Zingsheim Date: Sun, 20 Nov 2016 15:14:49 +0100 Subject: [PATCH] Fixed #6180 (Usage of variable after std::move or std::forward) --- lib/checkother.cpp | 75 ++++++++++++++++ lib/checkother.h | 9 ++ lib/standards.h | 4 +- lib/symboldatabase.cpp | 9 ++ lib/symboldatabase.h | 6 ++ lib/token.cpp | 6 ++ lib/token.h | 9 ++ lib/tokenlist.cpp | 57 ++++++++---- lib/valueflow.cpp | 119 +++++++++++++++++++++--- lib/valueflow.h | 26 +++++- test/testother.cpp | 200 +++++++++++++++++++++++++++++++++++++++++ test/testvalueflow.cpp | 99 ++++++++++++++++++++ 12 files changed, 587 insertions(+), 32 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 918db2387..ce0fc185a 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -2697,3 +2697,78 @@ void CheckOther::unknownEvaluationOrder(const Token* tok) "Expression '" + (tok ? tok->expressionString() : std::string("x = x++;")) + "' depends on order of evaluation of side effects", CWE768, false); } +void CheckOther::checkAccessOfMovedVariable() +{ + if (!_tokenizer->isCPP() || _settings->standards.cpp < Standards::CPP11) + return; + const bool reportInconclusive = _settings->inconclusive; + const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase(); + const std::size_t functions = symbolDatabase->functionScopes.size(); + for (std::size_t i = 0; i < functions; ++i) { + const Scope * scope = symbolDatabase->functionScopes[i]; + const Token * scopeStart = scope->classStart; + if (scope->function) { + const Token * memberInitializationStart = scope->function->constructorMemberInitialization(); + if (memberInitializationStart) + scopeStart = memberInitializationStart; + } + for (const Token* tok = scopeStart->next(); tok != scope->classEnd; tok = tok->next()) { + const ValueFlow::Value * movedValue = tok->getMovedValue(); + if (!movedValue) + continue; + if (movedValue->inconclusive && !reportInconclusive) + continue; + + bool inconclusive = false; + bool accessOfMoved = false; + if (tok->strAt(1) == ".") { + if (tok->next()->originalName() == "->") + accessOfMoved = true; + else + inconclusive = true; + } else { + bool isVariableChanged = isVariableChangedByFunctionCall(tok, _settings, &inconclusive); + accessOfMoved = !isVariableChanged; + if (inconclusive) { + accessOfMoved = !isMovedParameterAllowedForInconclusiveFunction(tok); + if (accessOfMoved) + inconclusive = false; + } + } + if (accessOfMoved || (inconclusive && reportInconclusive)) + accessMovedError(tok, tok->str(), movedValue->moveKind, inconclusive || movedValue->inconclusive); + } + } +} + +bool CheckOther::isMovedParameterAllowedForInconclusiveFunction(const Token * tok) +{ + if (Token::simpleMatch(tok->tokAt(-4), "std :: move (")) + return false; + const Token * tokAtM2 = tok->tokAt(-2); + if (Token::simpleMatch(tokAtM2, "> (") && tokAtM2->link()) { + const Token * leftAngle = tokAtM2->link(); + if (Token::simpleMatch(leftAngle->tokAt(-3), "std :: forward <")) + return false; + } + return true; +} + +void CheckOther::accessMovedError(const Token *tok, const std::string &varname, ValueFlow::Value::MoveKind moveKind, bool inconclusive) +{ + const char * errorId = nullptr; + const char * kindString = nullptr; + switch (moveKind) { + case ValueFlow::Value::MovedVariable: + errorId = "accessMoved"; + kindString = "moved"; + break; + case ValueFlow::Value::ForwardedVariable: + errorId = "accessForwarded"; + kindString = "forwarded"; + break; + } + const std::string errmsg(std::string("Access of ") + kindString + " variable " + varname + "."); + reportError(tok, Severity::warning, errorId, errmsg, CWE(0U), inconclusive); +} + diff --git a/lib/checkother.h b/lib/checkother.h index 87f480db4..8cc76e7a7 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -94,6 +94,7 @@ public: checkOther.checkRedundantCopy(); checkOther.checkSuspiciousEqualityComparison(); checkOther.checkComparisonFunctionIsAlwaysTrueOrFalse(); + checkOther.checkAccessOfMovedVariable(); } /** @brief Clarify calculation for ".. a * b ? .." */ @@ -203,6 +204,9 @@ public: /** @brief %Check for expression that depends on order of evaluation of side effects */ void checkEvaluationOrder(); + /** @brief %Check for access of moved or forwarded variable */ + void checkAccessOfMovedVariable(); + private: // Error messages.. void checkComparisonFunctionIsAlwaysTrueOrFalseError(const Token* tok, const std::string &strFunctionName, const std::string &varName, const bool result); @@ -252,6 +256,8 @@ private: void raceAfterInterlockedDecrementError(const Token* tok); void unusedLabelError(const Token* tok, bool inSwitch); void unknownEvaluationOrder(const Token* tok); + static bool isMovedParameterAllowedForInconclusiveFunction(const Token * tok); + void accessMovedError(const Token *tok, const std::string &varname, ValueFlow::Value::MoveKind moveKind, bool inconclusive); void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const { CheckOther c(nullptr, settings, errorLogger); @@ -308,6 +314,8 @@ private: c.unusedLabelError(nullptr, true); c.unusedLabelError(nullptr, false); c.unknownEvaluationOrder(nullptr); + c.accessMovedError(nullptr, "v", ValueFlow::Value::MovedVariable, false); + c.accessMovedError(nullptr, "v", ValueFlow::Value::ForwardedVariable, false); } static std::string myName() { @@ -331,6 +339,7 @@ private: // warning "- either division by zero or useless condition\n" "- memset() with a value out of range as the 2nd parameter\n" + "- access of moved or forwarded variable.\n" // performance "- redundant data copying for const variable\n" diff --git a/lib/standards.h b/lib/standards.h index 288962c3b..f9e85b3e2 100644 --- a/lib/standards.h +++ b/lib/standards.h @@ -31,10 +31,10 @@ */ struct Standards { /** C code C89/C99/C11 standard */ - enum cstd_t { C89, C99, C11 } c; + enum cstd_t { C89, C99, C11, CLatest=C11 } c; /** C++ code standard */ - enum cppstd_t { CPP03, CPP11 } cpp; + enum cppstd_t { CPP03, CPP11, CPPLatest=CPP11 } cpp; /** Code is posix */ bool posix; diff --git a/lib/symboldatabase.cpp b/lib/symboldatabase.cpp index ac8a79d1e..27171595f 100644 --- a/lib/symboldatabase.cpp +++ b/lib/symboldatabase.cpp @@ -1838,6 +1838,15 @@ bool Function::argsMatch(const Scope *scope, const Token *first, const Token *se return false; } +const Token * Function::constructorMemberInitialization() const +{ + if (!isConstructor() || !functionScope || !functionScope->classStart) + return nullptr; + if (Token::Match(token, "%name% (") && Token::simpleMatch(token->linkAt(1), ") :")) + return token->linkAt(1)->next(); + return nullptr; +} + Function* SymbolDatabase::addGlobalFunction(Scope*& scope, const Token*& tok, const Token *argStart, const Token* funcStart) { Function* function = nullptr; diff --git a/lib/symboldatabase.h b/lib/symboldatabase.h index 275e6ab0f..a7371fb1b 100644 --- a/lib/symboldatabase.h +++ b/lib/symboldatabase.h @@ -857,6 +857,12 @@ public: static bool argsMatch(const Scope *info, const Token *first, const Token *second, const std::string &path, unsigned int depth); + /** + * @return token to ":" if the function is a constructor + * and it contains member initialization otherwise a nullptr is returned + */ + const Token * constructorMemberInitialization() const; + private: bool isImplicitlyVirtual_rec(const ::Type* baseType, bool& safe) const; diff --git a/lib/token.cpp b/lib/token.cpp index e6a6419c1..f65044a57 100644 --- a/lib/token.cpp +++ b/lib/token.cpp @@ -1335,6 +1335,9 @@ void Token::printValueFlow(bool xml, std::ostream &out) const case ValueFlow::Value::FLOAT: out << "floatvalue=\"" << it->floatValue << '\"'; break; + case ValueFlow::Value::MOVED: + out << "movedvalue=\"" << ValueFlow::Value::toString(it->moveKind) << '\"'; + break; } if (it->condition) out << " condition-line=\"" << it->condition->linenr() << '\"'; @@ -1358,6 +1361,9 @@ void Token::printValueFlow(bool xml, std::ostream &out) const case ValueFlow::Value::FLOAT: out << it->floatValue; break; + case ValueFlow::Value::MOVED: + out << ValueFlow::Value::toString(it->moveKind); + break; } } } diff --git a/lib/token.h b/lib/token.h index accb8485c..298933937 100644 --- a/lib/token.h +++ b/lib/token.h @@ -774,6 +774,15 @@ public: return ret; } + const ValueFlow::Value * getMovedValue() const { + std::list::const_iterator it; + for (it = values.begin(); it != values.end(); ++it) { + if (it->isMovedValue()) + return &(*it); + } + return nullptr; + } + const ValueFlow::Value * getValueLE(const MathLib::bigint val, const Settings *settings) const; const ValueFlow::Value * getValueGE(const MathLib::bigint val, const Settings *settings) const; diff --git a/lib/tokenlist.cpp b/lib/tokenlist.cpp index 44a9c784a..045bbd9ca 100644 --- a/lib/tokenlist.cpp +++ b/lib/tokenlist.cpp @@ -678,6 +678,7 @@ static void compilePrecedence2(Token *&tok, AST_state& state) // - Nest the round bracket under the square bracket. // - Nest what follows the lambda (if anything) with the lambda opening [ // - Compile the content of the lambda function as separate tree (this is done later) + // this must be consistant with isLambdaCaptureList Token* squareBracket = tok; Token* roundBracket = squareBracket->link()->next(); Token* curlyBracket = roundBracket->link()->next(); @@ -976,6 +977,23 @@ static void compileExpression(Token *&tok, AST_state& state) compileComma(tok, state); } +static bool isLambdaCaptureList(Token * tok) +{ + // a lambda expression '[x](y){}' is compiled as: + // [ + // `-( + // `-{ + // see compilePrecedence2 + if (tok->str() != "[") + return false; + if (!tok->astOperand1() || tok->astOperand1()->str() != "(") + return false; + const Token * params = tok->astOperand1(); + if (!params || !params->astOperand1() || params->astOperand1()->str() != "{") + return false; + return true; +} + static Token * createAstAtToken(Token *tok, bool cpp) { if (Token::simpleMatch(tok, "for (")) { @@ -1060,25 +1078,30 @@ static Token * createAstAtToken(Token *tok, bool cpp) // Compile inner expressions inside inner ({..}) and lambda bodies for (tok = tok1->next(); tok && tok != endToken; tok = tok ? tok->next() : nullptr) { - if (tok->str() != "{") - continue; + if (tok->str() == "{") { + if (Token::simpleMatch(tok->previous(), "( {")) + ; + else if (Token::simpleMatch(tok->astParent(), "(") && + Token::simpleMatch(tok->astParent()->astParent(), "[") && + tok->astParent()->astParent()->astOperand1() && + tok == tok->astParent()->astParent()->astOperand1()->astOperand1()) + ; + else + continue; - if (Token::simpleMatch(tok->previous(), "( {")) - ; - else if (Token::simpleMatch(tok->astParent(), "(") && - Token::simpleMatch(tok->astParent()->astParent(), "[") && - tok->astParent()->astParent()->astOperand1() && - tok == tok->astParent()->astParent()->astOperand1()->astOperand1()) - ; - else - continue; + if (Token::simpleMatch(tok->previous(), "( { .")) + break; - if (Token::simpleMatch(tok->previous(), "( { .")) - break; - - const Token * const endToken2 = tok->link(); - for (; tok && tok != endToken && tok != endToken2; tok = tok ? tok->next() : nullptr) - tok = createAstAtToken(tok, cpp); + const Token * const endToken2 = tok->link(); + for (; tok && tok != endToken && tok != endToken2; tok = tok ? tok->next() : nullptr) + tok = createAstAtToken(tok, cpp); + } else if (tok->str() == "[") { + if (isLambdaCaptureList(tok)) { + const Token * const endToken2 = tok->link(); + for (; tok && tok != endToken && tok != endToken2; tok = tok ? tok->next() : nullptr) + tok = createAstAtToken(tok, cpp); + } + } } return endToken ? endToken->previous() : nullptr; diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index f14d436d6..6d45d1aa3 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -1284,7 +1284,7 @@ static bool valueFlowForward(Token * const startToken, } // conditional block of code that assigns variable.. - else if (Token::Match(tok2, "%name% (") && Token::simpleMatch(tok2->linkAt(1), ") {")) { + else if (!tok2->varId() && Token::Match(tok2, "%name% (") && Token::simpleMatch(tok2->linkAt(1), ") {")) { // is variable changed in condition? if (isVariableChanged(tok2->next(), tok2->next()->link(), varid, settings)) { if (settings->debugwarnings) @@ -1673,6 +1673,19 @@ static bool valueFlowForward(Token * const startToken, it->changeKnownToPossible(); } } + if (tok2->strAt(1) == "." && tok2->next()->originalName() != "->") { + if (settings->inconclusive) { + std::list::iterator it; + for (it = values.begin(); it != values.end(); ++it) { + it->inconclusive = true; + it->changeKnownToPossible(); + } + } else { + if (settings->debugwarnings) + bailout(tokenlist, errorLogger, tok2, "possible assignment of " + tok2->str() + " by member function"); + return false; + } + } } // Lambda function @@ -1690,6 +1703,99 @@ static bool valueFlowForward(Token * const startToken, return true; } +static bool isStdMoveOrStdForwarded(Token * tok, ValueFlow::Value::MoveKind * moveKind, Token ** varTok = nullptr) +{ + if (tok->str() != "std") + return false; + bool isMovedOrForwarded = false; + ValueFlow::Value::MoveKind kind = ValueFlow::Value::MovedVariable; + Token * variableToken = nullptr; + if (Token::Match(tok, "std :: move ( %var% )")) { + variableToken = tok->tokAt(4); + isMovedOrForwarded = true; + kind = ValueFlow::Value::MovedVariable; + } else if (Token::simpleMatch(tok, "std :: forward <")) { + Token * leftAngle = tok->tokAt(3); + Token * rightAngle = leftAngle->link(); + if (Token::Match(rightAngle, "> ( %var% )")) { + variableToken = rightAngle->tokAt(2); + isMovedOrForwarded = true; + kind = ValueFlow::Value::ForwardedVariable; + } + } + if (!isMovedOrForwarded) + return false; + if (variableToken->strAt(2) == ".") // Only partially moved + return false; + + if (moveKind != nullptr) + *moveKind = kind; + if (varTok != nullptr) + *varTok = variableToken; + return true; +} + +static void valueFlowAfterMove(TokenList *tokenlist, SymbolDatabase* symboldatabase, ErrorLogger *errorLogger, const Settings *settings) +{ + if (!tokenlist->isCPP() || settings->standards.cpp < Standards::CPP11) + return; + const std::size_t functions = symboldatabase->functionScopes.size(); + for (std::size_t i = 0; i < functions; ++i) { + const Scope * scope = symboldatabase->functionScopes[i]; + if (!scope) + continue; + const Token * start = scope->classStart; + if (scope->function) { + const Token * memberInitializationTok = scope->function->constructorMemberInitialization(); + if (memberInitializationTok) + start = memberInitializationTok; + } + + for (Token* tok = const_cast(start); tok != scope->classEnd; tok = tok->next()) { + Token * varTok; + ValueFlow::Value::MoveKind moveKind; + if (!isStdMoveOrStdForwarded(tok, &moveKind, &varTok)) + continue; + // x is not MOVED after assignment if code is: x = ... std::move(x) .. ; + const Token *parent = tok->astParent(); + while (parent && parent->str() != "=" && parent->str() != "return") + parent = parent->astParent(); + if (parent && parent->str() == "return") // MOVED in return statement + continue; + if (parent && parent->astOperand1()->str() == varTok->str()) + continue; + const unsigned int varid = varTok->varId(); + const Variable *var = varTok->variable(); + if (!var) + continue; + const Token * const endOfVarScope = var->typeStartToken()->scope()->classEnd; + + ValueFlow::Value value; + value.valueType = ValueFlow::Value::MOVED; + value.moveKind = moveKind; + value.setKnown(); + std::list values; + values.push_back(value); + + valueFlowForward(varTok->next(), endOfVarScope, var, varid, values, false, tokenlist, errorLogger, settings); + } + } +} + +static const Token * nextAfterAstRightmostLeaf(Token const * tok) +{ + const Token * rightmostLeaf = tok; + if (!rightmostLeaf || !rightmostLeaf->astOperand1()) + return nullptr; + do { + if (rightmostLeaf->astOperand2()) + rightmostLeaf = rightmostLeaf->astOperand2(); + else + rightmostLeaf = rightmostLeaf->astOperand1(); + } while (rightmostLeaf->astOperand1()); + return rightmostLeaf->next(); +} + static void valueFlowAfterAssign(TokenList *tokenlist, SymbolDatabase* symboldatabase, ErrorLogger *errorLogger, const Settings *settings) { const std::size_t functions = symboldatabase->functionScopes.size(); @@ -1725,15 +1831,7 @@ static void valueFlowAfterAssign(TokenList *tokenlist, SymbolDatabase* symboldat } // Skip RHS - const Token *nextExpression = tok->astOperand2(); - for (;;) { - if (nextExpression->astOperand2()) - nextExpression = nextExpression->astOperand2(); - else if (nextExpression->isUnaryPreOp()) - nextExpression = nextExpression->astOperand1(); - else break; - } - nextExpression = nextExpression->next(); + const Token * nextExpression = nextAfterAstRightmostLeaf(tok); valueFlowForward(const_cast(nextExpression), endOfVarScope, var, varid, values, constValue, tokenlist, errorLogger, settings); } @@ -2623,6 +2721,7 @@ void ValueFlow::setValues(TokenList *tokenlist, SymbolDatabase* symboldatabase, valueFlowOppositeCondition(symboldatabase, settings); valueFlowForLoop(tokenlist, symboldatabase, errorLogger, settings); valueFlowBeforeCondition(tokenlist, symboldatabase, errorLogger, settings); + valueFlowAfterMove(tokenlist, symboldatabase, errorLogger, settings); valueFlowAfterAssign(tokenlist, symboldatabase, errorLogger, settings); valueFlowAfterCondition(tokenlist, symboldatabase, errorLogger, settings); valueFlowSwitchVariable(tokenlist, symboldatabase, errorLogger, settings); diff --git a/lib/valueflow.h b/lib/valueflow.h index 8f40007a3..ffffd3c48 100644 --- a/lib/valueflow.h +++ b/lib/valueflow.h @@ -33,8 +33,8 @@ class Settings; namespace ValueFlow { class CPPCHECKLIB Value { public: - explicit Value(long long val = 0) : valueType(INT), intvalue(val), tokvalue(nullptr), floatValue(0.0), varvalue(val), condition(0), varId(0U), conditional(false), inconclusive(false), defaultArg(false), valueKind(ValueKind::Possible) {} - Value(const Token *c, long long val) : valueType(INT), intvalue(val), tokvalue(nullptr), floatValue(0.0), varvalue(val), condition(c), varId(0U), conditional(false), inconclusive(false), defaultArg(false), valueKind(ValueKind::Possible) {} + explicit Value(long long val = 0) : valueType(INT), intvalue(val), tokvalue(nullptr), floatValue(0.0), moveKind(MovedVariable), varvalue(val), condition(0), varId(0U), conditional(false), inconclusive(false), defaultArg(false), valueKind(ValueKind::Possible) {} + Value(const Token *c, long long val) : valueType(INT), intvalue(val), tokvalue(nullptr), floatValue(0.0), moveKind(MovedVariable), varvalue(val), condition(c), varId(0U), conditional(false), inconclusive(false), defaultArg(false), valueKind(ValueKind::Possible) {} bool operator==(const Value &rhs) const { if (valueType != rhs.valueType) @@ -53,6 +53,10 @@ namespace ValueFlow { if (floatValue > rhs.floatValue || floatValue < rhs.floatValue) return false; break; + case MOVED: + if (moveKind != rhs.moveKind) + return false; + break; }; return varvalue == rhs.varvalue && @@ -64,7 +68,7 @@ namespace ValueFlow { valueKind == rhs.valueKind; } - enum ValueType { INT, TOK, FLOAT } valueType; + enum ValueType { INT, TOK, FLOAT, MOVED } valueType; bool isIntValue() const { return valueType == INT; } @@ -74,6 +78,9 @@ namespace ValueFlow { bool isFloatValue() const { return valueType == FLOAT; } + bool isMovedValue() const { + return valueType == MOVED; + } /** int value */ long long intvalue; @@ -84,6 +91,9 @@ namespace ValueFlow { /** float value */ double floatValue; + /** kind of moved */ + enum MoveKind {MovedVariable, ForwardedVariable} moveKind; + /** For calculated values - variable value that calculated value depends on */ long long varvalue; @@ -102,6 +112,16 @@ namespace ValueFlow { /** Is this value passed as default parameter to the function? */ bool defaultArg; + static const char * toString(MoveKind moveKind) { + switch (moveKind) { + case MovedVariable: + return "MovedVariable"; + case ForwardedVariable: + return "ForwardedVariable"; + } + return ""; + } + /** How known is this value */ enum ValueKind { /** This value is possible, other unlisted values may also be possible */ diff --git a/test/testother.cpp b/test/testother.cpp index 56aebe810..ce67cef5b 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -172,6 +172,23 @@ private: TEST_CASE(testEvaluationOrderSizeof); TEST_CASE(testUnsignedLessThanZero); + + TEST_CASE(doubleMove1); + TEST_CASE(doubleMoveMemberInitialization1); + TEST_CASE(doubleMoveMemberInitialization2); + TEST_CASE(moveAndAssign1); + TEST_CASE(moveAndAssign2); + TEST_CASE(moveAssignMoveAssign); + TEST_CASE(moveAndFunctionParameter); + TEST_CASE(moveAndFunctionParameterReference); + TEST_CASE(moveAndFunctionParameterConstReference); + TEST_CASE(moveAndFunctionParameterUnknown); + TEST_CASE(moveAndReturn); + TEST_CASE(moveAndClear); + TEST_CASE(movedPointer); + TEST_CASE(partiallyMoved); + TEST_CASE(moveAndLambda); + TEST_CASE(forwardAndUsed); } void check(const char code[], const char *filename = nullptr, bool experimental = false, bool inconclusive = true, bool runSimpleChecks=true, Settings* settings = 0) { @@ -185,6 +202,8 @@ private: settings->addEnabled("warning"); settings->addEnabled("portability"); settings->addEnabled("performance"); + settings->standards.c = Standards::CLatest; + settings->standards.cpp = Standards::CPPLatest; settings->inconclusive = inconclusive; settings->experimental = experimental; @@ -6085,6 +6104,187 @@ private: "[test.c:12]: (style) Checking if unsigned variable 'd.n' is less than zero.\n", errout.str()); } + + void doubleMove1() { + check("void g(A a);\n" + "void f() {\n" + " A a;\n" + " g(std::move(a));\n" + " g(std::move(a));\n" + "}"); + ASSERT_EQUALS("[test.cpp:5]: (warning) Access of moved variable a.\n", errout.str()); + } + + void doubleMoveMemberInitialization1() { + check("class A\n" + "{\n" + " A(B && b)\n" + " :b1(std::move(b))\n" + " {\n" + " b2 = std::move(b);\n" + " }\n" + " B b1;\n" + " B b2;\n" + "};"); + ASSERT_EQUALS("[test.cpp:6]: (warning) Access of moved variable b.\n", errout.str()); + } + + void doubleMoveMemberInitialization2() { + check("class A\n" + "{\n" + " A(B && b)\n" + " :b1(std::move(b)),\n" + " b2(std::move(b))\n" + " {}\n" + " B b1;\n" + " B b2;\n" + "};"); + ASSERT_EQUALS("[test.cpp:5]: (warning) Access of moved variable b.\n", errout.str()); + } + + void moveAndAssign1() { + check("A g(A a);\n" + "void f() {\n" + " A a;\n" + " a = g(std::move(a));\n" + " a = g(std::move(a));\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } + + void moveAndAssign2() { + check("A g(A a);\n" + "void f() {\n" + " A a;\n" + " B b = g(std::move(a));\n" + " C c = g(std::move(a));\n" + "}"); + ASSERT_EQUALS("[test.cpp:5]: (warning) Access of moved variable a.\n", errout.str()); + } + + void moveAssignMoveAssign() { + check("void h(A a);\n" + "void f() {" + " A a;\n" + " g(std::move(a));\n" + " h(a);\n" + " a = b;\n" + " h(a);\n" + " g(std::move(a));\n" + " h(a);\n" + " a = b;\n" + " h(a);\n" + "}"); + ASSERT_EQUALS("[test.cpp:4]: (warning) Access of moved variable a.\n" + "[test.cpp:8]: (warning) Access of moved variable a.\n", errout.str()); + } + + void moveAndFunctionParameter() { + check("void g(A a);\n" + "void f() {\n" + " A a;\n" + " A b = std::move(a);\n" + " g(a);\n" + " A c = a;\n" + "}"); + ASSERT_EQUALS("[test.cpp:5]: (warning) Access of moved variable a.\n" + "[test.cpp:6]: (warning) Access of moved variable a.\n", errout.str()); + } + + void moveAndFunctionParameterReference() { + check("void g(A & a);\n" + "void f() {\n" + " A a;\n" + " A b = std::move(a);\n" + " g(a);\n" + " A c = a;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } + + void moveAndFunctionParameterConstReference() { + check("void g(A const & a);\n" + "void f() {\n" + " A a;\n" + " A b = std::move(a);\n" + " g(a);\n" + " A c = a;\n" + "}"); + ASSERT_EQUALS("[test.cpp:5]: (warning) Access of moved variable a.\n" + "[test.cpp:6]: (warning) Access of moved variable a.\n", errout.str()); + } + + void moveAndFunctionParameterUnknown() { + check("void f() {\n" + " A a;\n" + " A b = std::move(a);\n" + " g(a);\n" + " A c = a;\n" + "}"); + ASSERT_EQUALS("[test.cpp:4]: (warning, inconclusive) Access of moved variable a.\n" + "[test.cpp:5]: (warning, inconclusive) Access of moved variable a.\n", errout.str()); + } + + void moveAndReturn() { + check("int f(int i) {\n" + " A a;\n" + " A b;\n" + " g(std::move(a));\n" + " if (i)\n" + " return g(std::move(b));\n" + " return h(std::move(a),std::move(b));\n" + "}"); + ASSERT_EQUALS("[test.cpp:7]: (warning) Access of moved variable a.\n", errout.str()); + } + + void moveAndClear() { + check("void f() {\n" + " V v;\n" + " g(std::move(v));\n" + " v.clear();\n" + " if (v.empty()) {}\n" + "}"); + ASSERT_EQUALS("[test.cpp:4]: (warning, inconclusive) Access of moved variable v.\n" + "[test.cpp:5]: (warning, inconclusive) Access of moved variable v.\n", errout.str()); + } + + void movedPointer() { + check("void f() {\n" + " P p;\n" + " g(std::move(p));\n" + " x = p->x;\n" + " y = p->y;\n" + "}"); + ASSERT_EQUALS("[test.cpp:4]: (warning) Access of moved variable p.\n" + "[test.cpp:5]: (warning) Access of moved variable p.\n", errout.str()); + } + + void partiallyMoved() { + check("void f() {\n" + " A a;\n" + " gx(std::move(a).x());\n" + " gy(std::move(a).y());\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } + + void moveAndLambda() { + check("void f() {\n" + " A a;\n" + " auto h = [a=std::move(a)](){return g(std::move(a));};" + " b = a;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } + + void forwardAndUsed() { + check("template\n" + "void f(T && t) {\n" + " g(std::forward(t));\n" + " T s = t;\n" + "}"); + ASSERT_EQUALS("[test.cpp:4]: (warning) Access of forwarded variable t.\n", errout.str()); + } }; REGISTER_TEST(TestOther) diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index 29b9d767f..e57de0212 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -46,6 +46,7 @@ private: TEST_CASE(valueFlowString); TEST_CASE(valueFlowPointerAlias); TEST_CASE(valueFlowArrayElement); + TEST_CASE(valueFlowMove); TEST_CASE(valueFlowBitAnd); @@ -120,6 +121,25 @@ private: return false; } + bool testValueOfX(const char code[], unsigned int linenr, ValueFlow::Value::MoveKind moveKind) { + // 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) { + std::list::const_iterator it; + for (it = tok->values.begin(); it != tok->values.end(); ++it) { + if (it->isMovedValue() && it->moveKind == moveKind) + return true; + } + } + } + + return false; + } + bool testConditionalValueOfX(const char code[], unsigned int linenr, int value) { // Tokenize.. Tokenizer tokenizer(&settings, this); @@ -264,6 +284,77 @@ private: ASSERT_EQUALS(0, valueOfTok(code, "[").intvalue); } + void valueFlowMove() { + const char *code; + + code = "void f() {\n" + " X x;\n" + " g(std::move(x));\n" + " y=x;\n" + "}"; + ASSERT_EQUALS(true, testValueOfX(code, 4U, ValueFlow::Value::MovedVariable)); + + code = "void f() {\n" + " X x;\n" + " g(std::forward(x));\n" + " y=x;\n" + "}"; + ASSERT_EQUALS(true, testValueOfX(code, 4U, ValueFlow::Value::ForwardedVariable)); + + code = "void f() {\n" + " X x;\n" + " g(std::move(x).getA());\n" // Only parts of x might be moved out + " y=x;\n" + "}"; + ASSERT_EQUALS(false, testValueOfX(code, 4U, ValueFlow::Value::MovedVariable)); + + code = "void f() {\n" + " X x;\n" + " g(std::forward(x).getA());\n" // Only parts of x might be moved out + " y=x;\n" + "}"; + ASSERT_EQUALS(false, testValueOfX(code, 4U, ValueFlow::Value::ForwardedVariable)); + + code = "void f() {\n" + " X x;\n" + " g(std::move(x));\n" + " x.clear();\n" + " y=x;\n" + "}"; + ASSERT_EQUALS(false, testValueOfX(code, 5U, ValueFlow::Value::MovedVariable)); + + code = "void f() {\n" + " X x;\n" + " g(std::move(x));\n" + " y=x->y;\n" + " z=x->z;\n" + "}"; + ASSERT_EQUALS(true, testValueOfX(code, 5U, ValueFlow::Value::MovedVariable)); + + code = "void f(int i) {\n" + " X x;\n" + " z = g(std::move(x));\n" + " y = x;\n" + "}"; + ASSERT_EQUALS(true, testValueOfX(code, 4U, ValueFlow::Value::MovedVariable)); + + code = "void f(int i) {\n" + " X x;\n" + " x = g(std::move(x));\n" + " y = x;\n" + "}"; + ASSERT_EQUALS(false, testValueOfX(code, 4U, ValueFlow::Value::MovedVariable)); + + code = "A f(int i) {\n" + " X x;\n" + " if (i)" + " return g(std::move(x));\n" + " return h(std::move(x));\n" + "}"; + ASSERT_EQUALS(false, testValueOfX(code, 5U, ValueFlow::Value::MovedVariable)); + + } + void valueFlowCalculations() { const char *code; @@ -1821,6 +1912,14 @@ private: "}"; ASSERT_EQUALS(6, valueOfTok(code, "*").intvalue); ASSERT_EQUALS(true, valueOfTok(code, "*").isKnown()); + + code = "int f(int i, X x) {\n" + " if (i)\n" + " return g(std::move(x));\n" + " g(x);\n" + " return 0;\n" + "}"; + ASSERT_EQUALS(false, testValueOfX(code, 4U, ValueFlow::Value::MovedVariable)); } void valueFlowFunctionDefaultParameter() {