diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 9f2613c26..5b9635fa2 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -534,6 +534,188 @@ static void valueFlowBeforeCondition(TokenList *tokenlist, ErrorLogger *errorLog } } +static void valueFlowForward(Token * const startToken, + const Token * const endToken, + const Variable * const var, + const unsigned int varid, + std::list values, + const bool constValue, + TokenList * const tokenlist, + ErrorLogger * const errorLogger, + const Settings * const settings) +{ + int indentlevel = 0; + unsigned int number_of_if = 0; + int varusagelevel = -1; + bool returnStatement = false; // current statement is a return, stop analysis at the ";" + + for (Token *tok2 = startToken; tok2 && tok2 != endToken; tok2 = tok2->next()) { + if (indentlevel >= 0 && tok2->str() == "{") + ++indentlevel; + else if (indentlevel >= 0 && tok2->str() == "}") + --indentlevel; + + if (Token::Match(tok2, "sizeof|typeof|typeid (")) + tok2 = tok2->linkAt(1); + + // conditional block of code that assigns variable.. + else if (Token::Match(tok2, "%var% (") && Token::simpleMatch(tok2->linkAt(1), ") {")) { + // Should scope be skipped because variable value is checked? + bool skip = false; + for (std::list::iterator it = values.begin(); it != values.end(); ++it) { + if (conditionIsFalse(tok2->next()->astOperand2(), varid, *it)) { + skip = true; + break; + } + } + if (skip) { + // goto '{' + tok2 = tok2->linkAt(1)->next(); + // goto '}' + tok2 = tok2->link(); + continue; + } + + 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) : + (nullptr != Token::findmatch(start, "%varid%", end, varid)); + if (varusage) { + varusagelevel = indentlevel; + + // TODO: don't check noreturn scopes + if (number_of_if > 0U || Token::findmatch(tok2, "%varid%", start, varid)) { + if (settings->debugwarnings) + bailout(tokenlist, errorLogger, tok2, "variable " + var->nameToken()->str() + " is assigned in conditional code"); + break; + } + + if (var->isStatic()) { + if (settings->debugwarnings) + bailout(tokenlist, errorLogger, tok2, "variable " + var->nameToken()->str() + " bailout when conditional code that contains var is seen"); + break; + } + + // Remove conditional values + std::list::iterator it; + for (it = values.begin(); it != values.end();) { + if (it->condition || it->conditional) + values.erase(it++); + else + ++it; + } + } + + // noreturn scopes.. + if ((number_of_if > 0 || Token::findmatch(tok2, "%varid%", start, varid)) && + (Token::findmatch(start, "return|continue|break", end) || + (Token::simpleMatch(end,"} else {") && Token::findmatch(end, "return|continue|break", end->linkAt(2))))) { + if (settings->debugwarnings) + bailout(tokenlist, errorLogger, tok2, "variable " + var->nameToken()->str() + ". noreturn conditional scope."); + break; + } + + if (isVariableChanged(start, end, varid)) { + if (number_of_if == 0 && + Token::simpleMatch(tok2, "if (") && + !(Token::simpleMatch(end, "} else {") && + (Token::findmatch(end, "%varid%", end->linkAt(2), varid) || + Token::findmatch(end, "return|continue|break", end->linkAt(2))))) { + ++number_of_if; + tok2 = end; + } else { + if (settings->debugwarnings) + bailout(tokenlist, errorLogger, tok2, "variable " + var->nameToken()->str() + " is assigned in conditional code"); + break; + } + } + } + + else if (tok2->str() == "}" && indentlevel == varusagelevel) { + ++number_of_if; + + // Set "conditional" flag for all values + std::list::iterator it; + for (it = values.begin(); it != values.end(); ++it) + it->conditional = true; + + if (Token::simpleMatch(tok2,"} else {")) + tok2 = tok2->linkAt(2); + } + + else if (indentlevel <= 0 && Token::Match(tok2, "break|continue|goto")) { + if (settings->debugwarnings) + bailout(tokenlist, errorLogger, tok2, "variable " + var->nameToken()->str() + ". noreturn conditional scope."); + break; + } + + else if (indentlevel <= 0 && tok2->str() == "return") + returnStatement = true; + + else if (returnStatement && tok2->str() == ";") + break; + + if (tok2->varId() == varid) { + // bailout: assignment + if (Token::Match(tok2->previous(), "!!* %var% %op%") && tok2->next()->isAssignmentOp()) { + if (settings->debugwarnings) + bailout(tokenlist, errorLogger, tok2, "assignment of " + tok2->str()); + break; + } + + // bailout increment/decrement for now.. + if (Token::Match(tok2->previous(), "++|-- %var%") || Token::Match(tok2, "%var% ++|--")) { + if (settings->debugwarnings) + bailout(tokenlist, errorLogger, tok2, "increment/decrement of " + tok2->str()); + break; + } + + // bailout: possible assignment using >> + if (Token::Match(tok2->previous(), ">> %var% >>|;")) { + const Token *parent = tok2->previous(); + while (Token::simpleMatch(parent,">>")) + parent = parent->astParent(); + if (!parent) { + if (settings->debugwarnings) + bailout(tokenlist, errorLogger, tok2, "Possible assignment of " + tok2->str() + " using >>"); + break; + } + } + + // skip if variable is conditionally used in ?: expression + if (const Token *parent = skipValueInConditionalExpression(tok2)) { + if (settings->debugwarnings) + bailout(tokenlist, + errorLogger, + tok2, + "no simplification of " + tok2->str() + " within " + (Token::Match(parent,"[?:]") ? "?:" : parent->str()) + " expression"); + continue; + } + + { + std::list::const_iterator it; + for (it = values.begin(); it != values.end(); ++it) + setTokenValue(tok2, *it); + } + + // assigned by subfunction? + bool inconclusive = false; + if (bailoutFunctionPar(tok2, ValueFlow::Value(), settings, &inconclusive)) { + if (settings->debugwarnings) + bailout(tokenlist, errorLogger, tok2, "possible assignment of " + tok2->str() + " by subfunction"); + break; + } + if (inconclusive) { + std::list::iterator it; + for (it = values.begin(); it != values.end(); ++it) + it->inconclusive = true; + } + } + } + +} + static void valueFlowAfterAssign(TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings) { for (Token *tok = tokenlist->front(); tok; tok = tok->next()) { @@ -564,174 +746,47 @@ static void valueFlowAfterAssign(TokenList *tokenlist, ErrorLogger *errorLogger, std::list values = tok->astOperand2()->values; const bool constValue = tok->astOperand2()->isNumber(); - int indentlevel = 0; - unsigned int number_of_if = 0; - int varusagelevel = -1; - bool returnStatement = false; // current statement is a return, stop analysis at the ";" + valueFlowForward(tok, endToken, var, varid, values, constValue, tokenlist, errorLogger, settings); + } +} - for (Token *tok2 = tok; tok2 && tok2 != endToken; tok2 = tok2->next()) { - if (indentlevel >= 0 && tok2->str() == "{") - ++indentlevel; - else if (indentlevel >= 0 && tok2->str() == "}") - --indentlevel; +static void valueFlowAfterCondition(TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings) +{ + for (Token *tok = tokenlist->front(); tok; tok = tok->next()) { + // Comparison + if (!tok->isComparisonOp() || !Token::Match(tok,"==|!=|>=|<=")) + continue; - if (Token::Match(tok2, "sizeof|typeof|typeid (")) - tok2 = tok2->linkAt(1); + if (!tok->astOperand1() || !tok->astOperand2()) + continue; - // conditional block of code that assigns variable.. - else if (Token::Match(tok2, "%var% (") && Token::simpleMatch(tok2->linkAt(1), ") {")) { - // Should scope be skipped because variable value is checked? - bool skip = false; - for (std::list::iterator it = values.begin(); it != values.end(); ++it) { - if (conditionIsFalse(tok2->next()->astOperand2(), varid, *it)) { - skip = true; - break; - } - } - if (skip) { - // goto '{' - tok2 = tok2->linkAt(1)->next(); - // goto '}' - tok2 = tok2->link(); - continue; - } + const Token *vartok, *numtok; + if (tok->astOperand1()->isName()) { + vartok = tok->astOperand1(); + numtok = tok->astOperand2(); + } else { + vartok = tok->astOperand2(); + numtok = tok->astOperand1(); + } + if (!vartok->isName() || !numtok->isNumber()) + continue; + const unsigned int varid = vartok->varId(); + if (varid == 0U) + continue; + const Variable *var = vartok->variable(); + if (!var || !(var->isLocal() || var->isArgument())) + continue; + std::list values = numtok->values; - 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) : - (nullptr != Token::findmatch(start, "%varid%", end, varid)); - if (varusage) { - varusagelevel = indentlevel; - - // TODO: don't check noreturn scopes - if (number_of_if > 0U || Token::findmatch(tok2, "%varid%", start, varid)) { - if (settings->debugwarnings) - bailout(tokenlist, errorLogger, tok2, "variable " + var->nameToken()->str() + " is assigned in conditional code"); - break; - } - - if (var->isStatic()) { - if (settings->debugwarnings) - bailout(tokenlist, errorLogger, tok2, "variable " + var->nameToken()->str() + " bailout when conditional code that contains var is seen"); - break; - } - - // Remove conditional values - std::list::iterator it; - for (it = values.begin(); it != values.end();) { - if (it->condition || it->conditional) - values.erase(it++); - else - ++it; - } - } - - // noreturn scopes.. - if ((number_of_if > 0 || Token::findmatch(tok2, "%varid%", start, varid)) && - (Token::findmatch(start, "return|continue|break", end) || - (Token::simpleMatch(end,"} else {") && Token::findmatch(end, "return|continue|break", end->linkAt(2))))) { - if (settings->debugwarnings) - bailout(tokenlist, errorLogger, tok2, "variable " + var->nameToken()->str() + ". noreturn conditional scope."); - break; - } - - if (isVariableChanged(start, end, varid)) { - if (number_of_if == 0 && - Token::simpleMatch(tok2, "if (") && - !(Token::simpleMatch(end, "} else {") && - (Token::findmatch(end, "%varid%", end->linkAt(2), varid) || - Token::findmatch(end, "return|continue|break", end->linkAt(2))))) { - ++number_of_if; - tok2 = end; - } else { - if (settings->debugwarnings) - bailout(tokenlist, errorLogger, tok2, "variable " + var->nameToken()->str() + " is assigned in conditional code"); - break; - } - } - } - - else if (tok2->str() == "}" && indentlevel == varusagelevel) { - ++number_of_if; - - // Set "conditional" flag for all values - std::list::iterator it; - for (it = values.begin(); it != values.end(); ++it) - it->conditional = true; - - if (Token::simpleMatch(tok2,"} else {")) - tok2 = tok2->linkAt(2); - } - - else if (indentlevel <= 0 && Token::Match(tok2, "break|continue|goto")) { - if (settings->debugwarnings) - bailout(tokenlist, errorLogger, tok2, "variable " + var->nameToken()->str() + ". noreturn conditional scope."); - break; - } - - else if (indentlevel <= 0 && tok2->str() == "return") - returnStatement = true; - - else if (returnStatement && tok2->str() == ";") - break; - - if (tok2->varId() == varid) { - // bailout: assignment - if (Token::Match(tok2->previous(), "!!* %var% %op%") && tok2->next()->isAssignmentOp()) { - if (settings->debugwarnings) - bailout(tokenlist, errorLogger, tok2, "assignment of " + tok2->str()); - break; - } - - // bailout increment/decrement for now.. - if (Token::Match(tok2->previous(), "++|-- %var%") || Token::Match(tok2, "%var% ++|--")) { - if (settings->debugwarnings) - bailout(tokenlist, errorLogger, tok2, "increment/decrement of " + tok2->str()); - break; - } - - // bailout: possible assignment using >> - if (Token::Match(tok2->previous(), ">> %var% >>|;")) { - const Token *parent = tok2->previous(); - while (Token::simpleMatch(parent,">>")) - parent = parent->astParent(); - if (!parent) { - if (settings->debugwarnings) - bailout(tokenlist, errorLogger, tok2, "Possible assignment of " + tok2->str() + " using >>"); - break; - } - } - - // skip if variable is conditionally used in ?: expression - if (const Token *parent = skipValueInConditionalExpression(tok2)) { - if (settings->debugwarnings) - bailout(tokenlist, - errorLogger, - tok2, - "no simplification of " + tok2->str() + " within " + (Token::Match(parent,"[?:]") ? "?:" : parent->str()) + " expression"); - continue; - } - - { - std::list::const_iterator it; - for (it = values.begin(); it != values.end(); ++it) - setTokenValue(tok2, *it); - } - - // assigned by subfunction? - bool inconclusive = false; - if (bailoutFunctionPar(tok2, ValueFlow::Value(), settings, &inconclusive)) { - if (settings->debugwarnings) - bailout(tokenlist, errorLogger, tok2, "possible assignment of " + tok2->str() + " by subfunction"); - break; - } - if (inconclusive) { - std::list::iterator it; - for (it = values.begin(); it != values.end(); ++it) - it->inconclusive = true; - } - } + const Token *top = tok->astTop(); + if (top && Token::simpleMatch(top->previous(), "if (")) { + Token *startToken = nullptr; + if (Token::Match(tok, "==|>=|<=") && Token::simpleMatch(top->link(), ") {")) + startToken = top->link()->next(); + else if (tok->str() == "!=" && Token::simpleMatch(top->link()->linkAt(1), "} else {")) + startToken = top->link()->linkAt(1)->tokAt(2); + if (startToken) + valueFlowForward(startToken->next(), startToken->link(), var, varid, values, true, tokenlist, errorLogger, settings); } } } @@ -1080,5 +1135,6 @@ void ValueFlow::setValues(TokenList *tokenlist, ErrorLogger *errorLogger, const valueFlowForLoop(tokenlist, errorLogger, settings); valueFlowBeforeCondition(tokenlist, errorLogger, settings); valueFlowAfterAssign(tokenlist, errorLogger, settings); + valueFlowAfterCondition(tokenlist, errorLogger, settings); valueFlowSubFunction(tokenlist, errorLogger, settings); } diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index 0e4689c4f..8209de0bf 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -55,6 +55,8 @@ private: TEST_CASE(valueFlowAfterAssign); + TEST_CASE(valueFlowAfterCondition); + TEST_CASE(valueFlowForLoop); TEST_CASE(valueFlowSubFunction); } @@ -470,7 +472,9 @@ private: "out:" " if (x==123){}\n" "}"); - ASSERT_EQUALS("[test.cpp:4]: (debug) ValueFlow bailout: variable x stopping on goto label\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (debug) ValueFlow bailout: variable x stopping on goto label\n" + "[test.cpp:2]: (debug) ValueFlow bailout: variable x. noreturn conditional scope.\n" + , errout.str()); // #5721 - FP bailout("static void f(int rc) {\n" @@ -716,6 +720,38 @@ private: ASSERT_EQUALS(false, testValueOfX(code, 8U, 2)); // x is not 2 at line 8 } + void valueFlowAfterCondition() { + const char *code; + + // if + code = "void f(int x) {\n" + " if (x == 123) {\n" + " a = x;\n" + " }\n" + "}"; + ASSERT_EQUALS(true, testValueOfX(code, 3U, 123)); + + code = "void f(int x) {\n" + " if (x != 123) {\n" + " a = x;\n" + " }\n" + "}"; + ASSERT_EQUALS(false, testValueOfX(code, 3U, 123)); + + // else + code = "void f(int x) {\n" + " if (x == 123) {}\n" + " else a = x;\n" + "}"; + ASSERT_EQUALS(false, testValueOfX(code, 3U, 123)); + + code = "void f(int x) {\n" + " if (x != 123) {}\n" + " else a = x;\n" + "}"; + ASSERT_EQUALS(true, testValueOfX(code, 3U, 123)); + } + void valueFlowBitAnd() { const char *code;