From d4f01583b6858030b096850fd02514810bc849c0 Mon Sep 17 00:00:00 2001 From: Frank Zingsheim Date: Sun, 23 Oct 2016 13:54:44 +0200 Subject: [PATCH] Fixed TODO (check if function parameter is non-const reference etc..) by common function --- Makefile | 2 +- lib/astutils.cpp | 81 +++++++++++++++++++++++++++++----- lib/astutils.h | 13 +++++- lib/checkcondition.cpp | 2 +- lib/valueflow.cpp | 94 ++++++++-------------------------------- test/testcondition.cpp | 47 ++++++++++++++++++++ test/testnullpointer.cpp | 2 +- test/testvalueflow.cpp | 28 ++++++++++++ 8 files changed, 178 insertions(+), 91 deletions(-) diff --git a/Makefile b/Makefile index 930420d79..f9c987836 100644 --- a/Makefile +++ b/Makefile @@ -303,7 +303,7 @@ endif $(SRCDIR)/analyzerinfo.o: lib/analyzerinfo.cpp lib/cxx11emu.h lib/analyzerinfo.h lib/config.h lib/errorlogger.h lib/suppressions.h lib/path.h $(CXX) ${INCLUDE_FOR_LIB} $(CPPFLAGS) $(CFG) $(CXXFLAGS) $(UNDEF_STRICT_ANSI) -c -o $(SRCDIR)/analyzerinfo.o $(SRCDIR)/analyzerinfo.cpp -$(SRCDIR)/astutils.o: lib/astutils.cpp lib/cxx11emu.h lib/astutils.h lib/symboldatabase.h lib/config.h lib/token.h lib/valueflow.h lib/mathlib.h lib/tokenize.h lib/errorlogger.h lib/suppressions.h lib/tokenlist.h +$(SRCDIR)/astutils.o: lib/astutils.cpp lib/cxx11emu.h lib/astutils.h lib/settings.h lib/config.h lib/library.h lib/mathlib.h lib/standards.h lib/errorlogger.h lib/suppressions.h lib/platform.h lib/importproject.h lib/timer.h lib/symboldatabase.h lib/token.h lib/valueflow.h lib/tokenize.h lib/tokenlist.h $(CXX) ${INCLUDE_FOR_LIB} $(CPPFLAGS) $(CFG) $(CXXFLAGS) $(UNDEF_STRICT_ANSI) -c -o $(SRCDIR)/astutils.o $(SRCDIR)/astutils.cpp $(SRCDIR)/check.o: lib/check.cpp lib/cxx11emu.h lib/check.h lib/config.h lib/token.h lib/valueflow.h lib/mathlib.h lib/tokenize.h lib/errorlogger.h lib/suppressions.h lib/tokenlist.h lib/settings.h lib/library.h lib/standards.h lib/platform.h lib/importproject.h lib/timer.h diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 23b06aaed..fce631511 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -19,6 +19,7 @@ //--------------------------------------------------------------------------- #include "astutils.h" +#include "settings.h" #include "symboldatabase.h" #include "token.h" #include "tokenize.h" @@ -341,7 +342,69 @@ bool isReturnScope(const Token * const endToken) return false; } -bool isVariableChanged(const Token *start, const Token *end, const unsigned int varid) +bool isVariableChangedByFunctionCall(const Token *tok, const Settings *settings, bool *inconclusive) +{ + if (!tok) + return false; + + // address of variable + const bool addressOf = tok && Token::simpleMatch(tok->previous(), "&"); + + // passing variable to subfunction? + if (Token::Match(tok->tokAt(-2), ") & %name% [,)]") && Token::Match(tok->linkAt(-2)->previous(), "[,(] (")) + ; + else if (Token::Match(tok->tokAt(addressOf?-2:-1), "[(,] &| %name% [,)]")) + ; + else + return false; + + // reinterpret_cast etc.. + if (Token::Match(tok->tokAt(-3), "> ( & %name% ) [,)]") && + tok->linkAt(-3) && + Token::Match(tok->linkAt(-3)->tokAt(-2), "[,(] %type% <")) + tok = tok->linkAt(-3); + + // goto start of function call and get argnr + unsigned int argnr = 0; + while (tok && tok->str() != "(") { + if (tok->str() == ",") + ++argnr; + else if (tok->str() == ")") + tok = tok->link(); + tok = tok->previous(); + } + tok = tok ? tok->previous() : nullptr; + if (tok && tok->link() && tok->str() == ">") + tok = tok->link()->previous(); + if (!Token::Match(tok, "%name% (")) + return false; // not a function => variable not changed + + if (tok->varId()) + return false; // Constructor call of tok => variable probably not changed by constructor call + + if (!tok->function()) { + // if the library says 0 is invalid + // => it is assumed that parameter is an in parameter (TODO: this is a bad heuristic) + if (!addressOf && settings->library.isnullargbad(tok, 1+argnr)) + return false; + // addressOf => inconclusive + if (!addressOf) { + if (inconclusive != nullptr) + *inconclusive = true; + return false; + } + return true; + } + + const Variable *arg = tok->function()->getArgumentVar(argnr); + + if (addressOf && !(arg && arg->isConst())) + return true; + + return arg && !arg->isConst() && arg->isReference(); +} + +bool isVariableChanged(const Token *start, const Token *end, const unsigned int varid, const Settings *settings) { for (const Token *tok = start; tok != end; tok = tok->next()) { if (tok->varId() == varid) { @@ -351,17 +414,11 @@ bool isVariableChanged(const Token *start, const Token *end, const unsigned int if (Token::Match(tok->previous(), "++|-- %name%")) return true; - if (Token::Match(tok->tokAt(-2), "[(,] & %var% [,)]")) - return true; // TODO: check if function parameter is const - - if (Token::Match(tok->previous(), "[(,] %var% [,)]")) { - const Token *parent = tok->astParent(); - while (parent && parent->str() == ",") - parent = parent->astParent(); - if (parent && Token::Match(parent->previous(), "%name% (") && !parent->previous()->function()) - return true; - // TODO: check if function parameter is non-const reference etc.. - } + bool inconclusive = false; + bool isChanged = isVariableChangedByFunctionCall(tok, settings, &inconclusive); + isChanged |= inconclusive; + if (isChanged) + return true; const Token *parent = tok->astParent(); while (Token::Match(parent, ".|::")) diff --git a/lib/astutils.h b/lib/astutils.h index 2c3ee7560..cc2af6f02 100644 --- a/lib/astutils.h +++ b/lib/astutils.h @@ -25,6 +25,7 @@ #include #include +class Settings; class Token; /** Is expression a 'signed char' if no promotion is used */ @@ -71,8 +72,18 @@ bool isWithoutSideEffects(bool cpp, const Token* tok); /** Is scope a return scope (scope will unconditionally return) */ bool isReturnScope(const Token *endToken); +/** Is variable changed by function call? + * In case the anser of the question is inconclusive, e.g. because the function declaration is not known + * the return value is false and the output parameter inconclusive is set to true + * + * @param tok token of varible in function call + * @param settings program settings + * @param inconclusive pointer to output variable which indicates that the answer of the question is inconclusive + */ +bool isVariableChangedByFunctionCall(const Token *tok, const Settings *settings, bool *inconclusive); + /** Is variable changed in block of code? */ -bool isVariableChanged(const Token *start, const Token *end, const unsigned int varid); +bool isVariableChanged(const Token *start, const Token *end, const unsigned int varid, const Settings *settings); /** Determines the number of arguments - if token is a function call or macro * @param start token which is supposed to be the function/macro name. diff --git a/lib/checkcondition.cpp b/lib/checkcondition.cpp index 33f6197d9..f6b44aa92 100644 --- a/lib/checkcondition.cpp +++ b/lib/checkcondition.cpp @@ -151,7 +151,7 @@ bool CheckCondition::assignIfParseScope(const Token * const assignTok, // is variable changed in loop? const Token *bodyStart = tok2->linkAt(1)->next(); const Token *bodyEnd = bodyStart ? bodyStart->link() : nullptr; - if (!bodyEnd || bodyEnd->str() != "}" || isVariableChanged(bodyStart, bodyEnd, varid)) + if (!bodyEnd || bodyEnd->str() != "}" || isVariableChanged(bodyStart, bodyEnd, varid, _settings)) continue; } diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 2d630183b..6d6bd650a 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -87,63 +87,6 @@ static void bailout(TokenList *tokenlist, ErrorLogger *errorLogger, const Token errorLogger->reportErr(errmsg); } -static bool bailoutFunctionPar(const Token *tok, const ValueFlow::Value &value, const Settings *settings, bool *inconclusive) -{ - if (!tok) - return false; - - // address of variable - const bool addressOf = tok && Token::simpleMatch(tok->previous(), "&"); - - // passing variable to subfunction? - if (Token::Match(tok->tokAt(-2), ") & %name% [,)]") && Token::Match(tok->linkAt(-2)->previous(), "[,(] (")) - ; - else if (Token::Match(tok->tokAt(addressOf?-2:-1), "[(,] &| %name% [,)]")) - ; - else - return false; - - // reinterpret_cast etc.. - if (Token::Match(tok->tokAt(-3), "> ( & %name% ) [,)]") && - tok->linkAt(-3) && - Token::Match(tok->linkAt(-3)->tokAt(-2), "[,(] %type% <")) - tok = tok->linkAt(-3); - - // goto start of function call and get argnr - unsigned int argnr = 0; - while (tok && tok->str() != "(") { - if (tok->str() == ",") - ++argnr; - else if (tok->str() == ")") - tok = tok->link(); - tok = tok->previous(); - } - tok = tok ? tok->previous() : nullptr; - if (tok && tok->link() && tok->str() == ">") - tok = tok->link()->previous(); - if (!Token::Match(tok, "%name% (")) - return false; // not a function => do not bailout - - if (!tok->function()) { - // if value is 0 and the library says 0 is invalid => do not bailout - if (value.intvalue==0 && settings->library.isnullargbad(tok, 1+argnr)) - return false; - // addressOf => inconclusive - if (!addressOf) { - *inconclusive = true; - return false; - } - return true; - } - - const Variable *arg = tok->function()->getArgumentVar(argnr); - - if (addressOf && !(arg && arg->isConst())) - return true; - - return arg && !arg->isConst() && arg->isReference(); -} - /** * Is condition always false when variable has given value? * \param condition top ast token in condition @@ -891,7 +834,7 @@ static void valueFlowReverse(TokenList *tokenlist, // assigned by subfunction? bool inconclusive = false; - if (bailoutFunctionPar(tok2,val2.condition ? val2 : val, settings, &inconclusive)) { + if (isVariableChangedByFunctionCall(tok2, settings, &inconclusive)) { if (settings->debugwarnings) bailout(tokenlist, errorLogger, tok2, "possible assignment of " + tok2->str() + " by subfunction"); break; @@ -950,7 +893,7 @@ static void valueFlowReverse(TokenList *tokenlist, const Token *start = tok2; const Token *end = start->link(); - if (isVariableChanged(start,end,varid)) { + if (isVariableChanged(start,end,varid, settings)) { if (settings->debugwarnings) bailout(tokenlist, errorLogger, tok2, "variable " + var->name() + " is assigned in loop. so valueflow analysis bailout when start of loop is reached."); break; @@ -1029,7 +972,7 @@ static void valueFlowBeforeCondition(TokenList *tokenlist, SymbolDatabase *symbo // Variable changed in 3rd for-expression if (Token::simpleMatch(tok2->previous(), "for (")) { - if (tok2->astOperand2() && tok2->astOperand2()->astOperand2() && isVariableChanged(tok2->astOperand2()->astOperand2(), tok2->link(), varid)) { + if (tok2->astOperand2() && tok2->astOperand2()->astOperand2() && isVariableChanged(tok2->astOperand2()->astOperand2(), tok2->link(), varid, settings)) { varid = 0U; if (settings->debugwarnings) bailout(tokenlist, errorLogger, tok, "variable " + var->name() + " used in loop"); @@ -1041,7 +984,7 @@ static void valueFlowBeforeCondition(TokenList *tokenlist, SymbolDatabase *symbo const Token * const start = tok2->link()->next(); const Token * const end = start->link(); - if (isVariableChanged(start,end,varid)) { + if (isVariableChanged(start,end,varid, settings)) { varid = 0U; if (settings->debugwarnings) bailout(tokenlist, errorLogger, tok, "variable " + var->name() + " used in loop"); @@ -1134,13 +1077,14 @@ static void valueFlowAST(Token *tok, unsigned int varid, const ValueFlow::Value static void handleKnownValuesInLoop(const Token *startToken, const Token *endToken, std::list *values, - unsigned int varid) + unsigned int varid, + const Settings *settings) { bool isChanged = false; for (std::list::iterator it = values->begin(); it != values->end(); ++it) { if (it->isKnown()) { if (!isChanged) { - if (!isVariableChanged(startToken, endToken, varid)) + if (!isVariableChanged(startToken, endToken, varid, settings)) break; isChanged = true; } @@ -1239,19 +1183,19 @@ static bool valueFlowForward(Token * const startToken, if (Token::simpleMatch(end, "} while (")) end = end->linkAt(2); - if (isVariableChanged(start, end, varid)) { + if (isVariableChanged(start, end, varid, settings)) { if (settings->debugwarnings) bailout(tokenlist, errorLogger, tok2, "variable " + var->name() + " valueFlowForward, assignment in do-while"); return false; } - handleKnownValuesInLoop(start, end, &values, varid); + handleKnownValuesInLoop(start, end, &values, varid, settings); } // conditional block of code that assigns variable.. else if (Token::Match(tok2, "%name% (") && Token::simpleMatch(tok2->linkAt(1), ") {")) { // is variable changed in condition? - if (isVariableChanged(tok2->next(), tok2->next()->link(), varid)) { + if (isVariableChanged(tok2->next(), tok2->next()->link(), varid, settings)) { if (settings->debugwarnings) bailout(tokenlist, errorLogger, tok2, "variable " + var->name() + " valueFlowForward, assignment in condition"); return false; @@ -1259,7 +1203,7 @@ static bool valueFlowForward(Token * const startToken, // if known variable is changed in loop body, change it to a possible value.. if (Token::Match(tok2, "for|while")) - handleKnownValuesInLoop(tok2, tok2->linkAt(1)->linkAt(1), &values, varid); + handleKnownValuesInLoop(tok2, tok2->linkAt(1)->linkAt(1), &values, varid, settings); // Set values in condition for (Token* tok3 = tok2->tokAt(2); tok3 != tok2->next()->link(); tok3 = tok3->next()) { @@ -1294,7 +1238,7 @@ static bool valueFlowForward(Token * const startToken, errorLogger, settings); - if (isVariableChanged(startToken1, startToken1->link(), varid)) { + if (isVariableChanged(startToken1, startToken1->link(), varid, settings)) { removeValues(values, truevalues); std::list::iterator it; @@ -1314,7 +1258,7 @@ static bool valueFlowForward(Token * const startToken, Token * const start = tok2->linkAt(1)->next(); Token * const end = start->link(); bool varusage = (indentlevel >= 0 && constValue && number_of_if == 0U) ? - isVariableChanged(start,end,varid) : + isVariableChanged(start,end,varid, settings) : (nullptr != Token::findmatch(start, "%varid%", end, varid)); if (!read) { read = bool(nullptr != Token::findmatch(tok2, "%varid% !!=", end, varid)); @@ -1383,7 +1327,7 @@ static bool valueFlowForward(Token * const startToken, return false; } - if (isVariableChanged(start, end, varid)) { + if (isVariableChanged(start, end, varid, settings)) { if ((!read || number_of_if == 0) && Token::simpleMatch(tok2, "if (") && !(Token::simpleMatch(end, "} else {") && @@ -1623,7 +1567,7 @@ static bool valueFlowForward(Token * const startToken, // assigned by subfunction? bool inconclusive = false; - if (bailoutFunctionPar(tok2, ValueFlow::Value(), settings, &inconclusive)) { + if (isVariableChangedByFunctionCall(tok2, settings, &inconclusive)) { if (settings->debugwarnings) bailout(tokenlist, errorLogger, tok2, "possible assignment of " + tok2->str() + " by subfunction"); return false; @@ -1642,7 +1586,7 @@ static bool valueFlowForward(Token * const startToken, Token::simpleMatch(tok2->linkAt(1), "] (") && Token::simpleMatch(tok2->linkAt(1)->linkAt(1), ") {")) { const Token *bodyStart = tok2->linkAt(1)->linkAt(1)->next(); - if (isVariableChanged(bodyStart, bodyStart->link(), varid)) { + if (isVariableChanged(bodyStart, bodyStart->link(), varid, settings)) { if (settings->debugwarnings) bailout(tokenlist, errorLogger, tok2, "valueFlowForward, " + var->name() + " is changed in lambda function"); return false; @@ -1788,7 +1732,7 @@ static void valueFlowAfterCondition(TokenList *tokenlist, SymbolDatabase* symbol // does condition reassign variable? if (tok != top->astOperand2() && Token::Match(top->astOperand2(), "%oror%|&&") && - isVariableChanged(top, top->link(), varid)) { + isVariableChanged(top, top->link(), varid, settings)) { if (settings->debugwarnings) bailout(tokenlist, errorLogger, tok, "assignment in condition"); continue; @@ -1823,7 +1767,7 @@ static void valueFlowAfterCondition(TokenList *tokenlist, SymbolDatabase* symbol if (startToken) { if (!valueFlowForward(startToken->next(), startToken->link(), var, varid, values, true, tokenlist, errorLogger, settings)) continue; - if (isVariableChanged(startToken, startToken->link(), varid)) { + if (isVariableChanged(startToken, startToken->link(), varid, settings)) { // TODO: The endToken should not be startToken->link() in the valueFlowForward call if (settings->debugwarnings) bailout(tokenlist, errorLogger, startToken->link(), "valueFlowAfterCondition: " + var->name() + " is changed in conditional block"); @@ -2119,7 +2063,7 @@ static void valueFlowForLoopSimplify(Token * const bodyStart, const unsigned int const Token * const bodyEnd = bodyStart->link(); // Is variable modified inside for loop - if (isVariableChanged(bodyStart, bodyEnd, varid)) + if (isVariableChanged(bodyStart, bodyEnd, varid, settings)) return; for (Token *tok2 = bodyStart->next(); tok2 != bodyEnd; tok2 = tok2->next()) { diff --git a/test/testcondition.cpp b/test/testcondition.cpp index 4bdfb9b50..a6a0991f7 100644 --- a/test/testcondition.cpp +++ b/test/testcondition.cpp @@ -193,6 +193,26 @@ private: "}"); ASSERT_EQUALS("", errout.str()); + check("void g(int x);\n" + "void f(int x) {\n" + " int a = 100;\n" + " while (x) {\n" + " int y = 16 | a;\n" + " while (y != 0) g(y);\n" + " }\n" + "}"); + ASSERT_EQUALS("[test.cpp:6]: (style) Condition 'y!=0' is always true\n[test.cpp:5] -> [test.cpp:6]: (style) Mismatching assignment and comparison, comparison 'y!=0' is always true.\n", errout.str()); + + check("void g(int &x);\n" + "void f(int x) {\n" + " int a = 100;\n" + " while (x) {\n" + " int y = 16 | a;\n" + " while (y != 0) g(y);\n" + " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + // calling function check("void f(int x) {\n" " int y = x & 7;\n" @@ -293,6 +313,33 @@ private: " }\n" "}\n"); ASSERT_EQUALS("", errout.str()); + + check("void f() {\n" + " int x = 100;\n" + " while (x) {\n" + " g(x);\n" + " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void g(int x);\n" + "void f() {\n" + " int x = 100;\n" + " while (x) {\n" + " g(x);\n" + " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void g(int & x);\n" + "void f() {\n" + " int x = 100;\n" + " while (x) {\n" + " g(x);\n" + " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } void mismatchingBitAnd() { diff --git a/test/testnullpointer.cpp b/test/testnullpointer.cpp index 8a0475bd0..262def468 100644 --- a/test/testnullpointer.cpp +++ b/test/testnullpointer.cpp @@ -2503,7 +2503,7 @@ private: void ticket6505() { check("void foo(MythSocket *socket) {\n" - " bool do_write=false;\n" + " bool do_write=0;\n" " if (socket) {\n" " do_write=something();\n" " }\n" diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index 8a05587dd..3a3e2aaf3 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -1865,6 +1865,34 @@ private: "}\n"; ASSERT(isNotKnownValues(code, "<")); + code = "void dostuff(int x);\n" + "void f() {\n" + " int x = 0;\n" + " dostuff(x);\n" + " if (x < 0) {}\n" + "}\n"; + value = valueOfTok(code, "<"); + ASSERT_EQUALS(0, value.intvalue); + ASSERT(value.isKnown()); + + code = "void dostuff(int & x);\n" + "void f() {\n" + " int x = 0;\n" + " dostuff(x);\n" + " if (x < 0) {}\n" + "}\n"; + ASSERT(isNotKnownValues(code, "<")); + + code = "void dostuff(const int & x);\n" + "void f() {\n" + " int x = 0;\n" + " dostuff(x);\n" + " if (x < 0) {}\n" + "}\n"; + value = valueOfTok(code, "<"); + ASSERT_EQUALS(0, value.intvalue); + ASSERT(value.isKnown()); + code = "void f() {\n" " int x = 0;\n" " do {\n"