From 597d0fa35b7d752677394df85d06b3458c8e30b4 Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Thu, 26 Sep 2019 03:32:25 -0500 Subject: [PATCH] Support expression in valueFlowAfterCondition (#2219) * Add valueFlowForwardExpression function to forward values of an expression * Use token for expression * Fix name in bailout message * Handle expressions * Add more tests for more expressions * Add more tests * Solve the expression if possible * Formatting --- lib/astutils.cpp | 42 ++- lib/astutils.h | 9 + lib/valueflow.cpp | 568 +++++++++++++++++++++++++++-------------- test/testvalueflow.cpp | 75 +++++- 4 files changed, 495 insertions(+), 199 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 383bf7806..fdd9d60b5 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -1218,6 +1218,31 @@ bool isVariableChanged(const Variable * var, const Settings *settings, bool cpp, return isVariableChanged(start->next(), var->scope()->bodyEnd, var->declarationId(), var->isGlobal(), settings, cpp, depth); } +bool isVariablesChanged(const Token* start, + const Token* end, + int indirect, + std::vector vars, + const Settings* settings, + bool cpp) +{ + std::set varids; + std::transform(vars.begin(), vars.end(), std::inserter(varids, varids.begin()), [](const Variable* var) { + return var->declarationId(); + }); + const bool globalvar = std::any_of(vars.begin(), vars.end(), [](const Variable* var) { return var->isGlobal(); }); + for (const Token* tok = start; tok != end; tok = tok->next()) { + if (tok->varId() == 0 || varids.count(tok->varId()) == 0) { + if (globalvar && Token::Match(tok, "%name% (")) + // TODO: Is global variable really changed by function call? + return true; + continue; + } + if (isVariableChanged(tok, indirect, settings, cpp)) + return true; + } + return false; +} + int numberOfArguments(const Token *start) { int arguments=0; @@ -1716,7 +1741,7 @@ struct FwdAnalysis::Result FwdAnalysis::checkRecursive(const Token *expr, const const Token *parent = tok; bool other = false; bool same = tok->astParent() && isSameExpression(mCpp, false, expr, tok, mLibrary, false, false, nullptr); - while (!same && Token::Match(parent->astParent(), "*|.|::|[")) { + while (!same && Token::Match(parent->astParent(), "*|.|::|[|%cop%")) { parent = parent->astParent(); if (parent && isSameExpression(mCpp, false, expr, parent, mLibrary, false, false, nullptr)) { same = true; @@ -1860,7 +1885,7 @@ bool FwdAnalysis::isGlobalData(const Token *expr) const return globalData; } -FwdAnalysis::Result FwdAnalysis::check(const Token *expr, const Token *startToken, const Token *endToken) +std::set FwdAnalysis::getExprVarIds(const Token* expr, bool* localOut, bool* unknownVarIdOut) const { // all variable ids in expr. std::set exprVarIds; @@ -1885,6 +1910,19 @@ FwdAnalysis::Result FwdAnalysis::check(const Token *expr, const Token *startToke } return ChildrenToVisit::op1_and_op2; }); + if (localOut) + *localOut = local; + if (unknownVarIdOut) + *unknownVarIdOut = unknownVarId; + return exprVarIds; +} + +FwdAnalysis::Result FwdAnalysis::check(const Token* expr, const Token* startToken, const Token* endToken) +{ + // all variable ids in expr. + bool local = true; + bool unknownVarId = false; + std::set exprVarIds = getExprVarIds(expr, &local, &unknownVarId); if (unknownVarId) return Result(FwdAnalysis::Result::Type::BAILOUT); diff --git a/lib/astutils.h b/lib/astutils.h index 5786fa35b..2bb4380a6 100644 --- a/lib/astutils.h +++ b/lib/astutils.h @@ -151,6 +151,13 @@ bool isVariableChanged(const Token *tok, int indirect, const Settings *settings, bool isVariableChanged(const Variable * var, const Settings *settings, bool cpp, int depth = 20); +bool isVariablesChanged(const Token* start, + const Token* end, + int indirect, + std::vector vars, + const Settings* settings, + bool cpp); + const Token* findVariableChanged(const Token *start, const Token *end, int indirect, const nonneg int varid, bool globalvar, const Settings *settings, bool cpp, int depth = 20); Token* findVariableChanged(Token *start, const Token *end, int indirect, const nonneg int varid, bool globalvar, const Settings *settings, bool cpp, int depth = 20); @@ -292,6 +299,8 @@ public: /** Is there some possible alias for given expression */ bool possiblyAliased(const Token *expr, const Token *startToken) const; + std::set getExprVarIds(const Token* expr, bool* localOut = nullptr, bool* unknownVarIdOut = nullptr) const; + static bool isNullOperand(const Token *expr); private: static bool isEscapedAlias(const Token* expr); diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index fbd220739..87e390509 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -42,9 +42,10 @@ * + always 42 * 2 always 2 * - * All value flow analysis is executed in the ValueFlow::setValues() function. The ValueFlow analysis is executed after the tokenizer/ast/symboldatabase/etc.. - * The ValueFlow analysis is done in a series of valueFlow* function calls, where each such function call can only use results from previous function calls. - * The function calls should be arranged so that valueFlow* that do not require previous ValueFlow information should be first. + * All value flow analysis is executed in the ValueFlow::setValues() function. The ValueFlow analysis is executed after + * the tokenizer/ast/symboldatabase/etc.. The ValueFlow analysis is done in a series of valueFlow* function calls, where + * each such function call can only use results from previous function calls. The function calls should be arranged so + * that valueFlow* that do not require previous ValueFlow information should be first. * * Type of analysis * ================ @@ -59,22 +60,21 @@ * x = 3 + 4; * * The valueFlowNumber set the values for the "3" and "4" tokens by calling setTokenValue(). - * The setTokenValue() handle the calculations automatically. When both "3" and "4" have values, the "+" can be calculated. setTokenValue() recursively calls itself when parents in calculations can be calculated. + * The setTokenValue() handle the calculations automatically. When both "3" and "4" have values, the "+" can be + * calculated. setTokenValue() recursively calls itself when parents in calculations can be calculated. * * Forward / Reverse flow analysis * =============================== * - * In forward value flow analysis we know a value and see what happens when we are stepping the program forward. Like normal execution. - * The valueFlowForward is used in this analysis. + * In forward value flow analysis we know a value and see what happens when we are stepping the program forward. Like + * normal execution. The valueFlowForwardVariable is used in this analysis. * - * In reverse value flow analysis we know the value of a variable at line X. And try to "execute backwards" to determine possible values before line X. - * The valueFlowReverse is used in this analysis. + * In reverse value flow analysis we know the value of a variable at line X. And try to "execute backwards" to determine + * possible values before line X. The valueFlowReverse is used in this analysis. * * */ - - #include "valueflow.h" #include "astutils.h" @@ -2298,16 +2298,41 @@ static std::set getIndirections(const std::list& values) return result; } -static bool valueFlowForward(Token * const startToken, - const Token * const endToken, - const Variable * const var, - const nonneg int varid, - std::list values, - const bool constValue, - const bool subFunction, - TokenList * const tokenlist, - ErrorLogger * const errorLogger, - const Settings * const settings) +static void valueFlowForwardExpression(Token* startToken, + const Token* endToken, + const Token* exprTok, + const std::list& values, + const TokenList* const tokenlist, + const Settings* settings) +{ + FwdAnalysis fwdAnalysis(tokenlist->isCPP(), settings->library); + for (const FwdAnalysis::KnownAndToken read : fwdAnalysis.valueFlow(exprTok, startToken, endToken)) { + for (const ValueFlow::Value& value : values) { + // Dont set inconclusive values + if (value.isInconclusive()) + continue; + ValueFlow::Value v = value; + if (v.isImpossible()) { + if (read.known) + continue; + } else if (!read.known) { + v.valueKind = ValueFlow::Value::ValueKind::Possible; + } + setTokenValue(const_cast(read.token), v, settings); + } + } +} + +static bool valueFlowForwardVariable(Token* const startToken, + const Token* const endToken, + const Variable* const var, + const nonneg int varid, + std::list values, + const bool constValue, + const bool subFunction, + TokenList* const tokenlist, + ErrorLogger* const errorLogger, + const Settings* const settings) { int indentlevel = 0; int number_of_if = 0; @@ -2336,7 +2361,12 @@ static bool valueFlowForward(Token * const startToken, condition = nullptr; if (!condition) { if (settings->debugwarnings) - bailout(tokenlist, errorLogger, tok2, "variable " + var->name() + " valueFlowForward, bailing out since it's unknown if conditional return is executed"); + bailout( + tokenlist, + errorLogger, + tok2, + "variable " + var->name() + + " valueFlowForwardVariable, bailing out since it's unknown if conditional return is executed"); return false; } @@ -2358,7 +2388,11 @@ static bool valueFlowForward(Token * const startToken, } if (bailoutflag) { if (settings->debugwarnings) - bailout(tokenlist, errorLogger, tok2, "variable " + var->name() + " valueFlowForward, conditional return is assumed to be executed"); + bailout(tokenlist, + errorLogger, + tok2, + "variable " + var->name() + + " valueFlowForwardVariable, conditional return is assumed to be executed"); return false; } @@ -2423,7 +2457,10 @@ static bool valueFlowForward(Token * const startToken, if (isVariableChanged(start, end, varid, var->isGlobal(), settings, tokenlist->isCPP())) { if (settings->debugwarnings) - bailout(tokenlist, errorLogger, tok2, "variable " + var->name() + " valueFlowForward, assignment in do-while"); + bailout(tokenlist, + errorLogger, + tok2, + "variable " + var->name() + " valueFlowForwardVariable, assignment in do-while"); return false; } @@ -2451,7 +2488,10 @@ static bool valueFlowForward(Token * const startToken, } if (values.empty()) { if (settings->debugwarnings) - bailout(tokenlist, errorLogger, tok2, "variable " + var->name() + " valueFlowForward, assignment in condition"); + bailout(tokenlist, + errorLogger, + tok2, + "variable " + var->name() + " valueFlowForwardVariable, assignment in condition"); return false; } @@ -2502,16 +2542,16 @@ static bool valueFlowForward(Token * const startToken, // '{' const Token * const startToken1 = tok2->linkAt(1)->next(); - bool vfresult = valueFlowForward(startToken1->next(), - startToken1->link(), - var, - varid, - truevalues, - constValue, - subFunction, - tokenlist, - errorLogger, - settings); + bool vfresult = valueFlowForwardVariable(startToken1->next(), + startToken1->link(), + var, + varid, + truevalues, + constValue, + subFunction, + tokenlist, + errorLogger, + settings); if (!condAlwaysFalse && isVariableChanged(startToken1, startToken1->link(), varid, var->isGlobal(), settings, tokenlist->isCPP())) { removeValues(values, truevalues); @@ -2530,16 +2570,16 @@ static bool valueFlowForward(Token * const startToken, if (Token::simpleMatch(tok2, "} else {")) { const Token * const startTokenElse = tok2->tokAt(2); - vfresult = valueFlowForward(startTokenElse->next(), - startTokenElse->link(), - var, - varid, - falsevalues, - constValue, - subFunction, - tokenlist, - errorLogger, - settings); + vfresult = valueFlowForwardVariable(startTokenElse->next(), + startTokenElse->link(), + var, + varid, + falsevalues, + constValue, + subFunction, + tokenlist, + errorLogger, + settings); if (!condAlwaysTrue && isVariableChanged(startTokenElse, startTokenElse->link(), varid, var->isGlobal(), settings, tokenlist->isCPP())) { removeValues(values, falsevalues); @@ -2601,16 +2641,16 @@ static bool valueFlowForward(Token * const startToken, if (Token::simpleMatch(end, "} else {")) { std::list knownValues; std::copy_if(values.begin(), values.end(), std::back_inserter(knownValues), std::mem_fn(&ValueFlow::Value::isKnown)); - valueFlowForward(end->tokAt(2), - end->linkAt(2), - var, - varid, - knownValues, - constValue, - subFunction, - tokenlist, - errorLogger, - settings); + valueFlowForwardVariable(end->tokAt(2), + end->linkAt(2), + var, + varid, + knownValues, + constValue, + subFunction, + tokenlist, + errorLogger, + settings); } // Remove conditional values @@ -2759,7 +2799,10 @@ static bool valueFlowForward(Token * const startToken, if (subFunction && (astIsPointer(tok2->astOperand1()) || astIsIntegral(tok2->astOperand1(), false))) { tok2 = const_cast(nextAfterAstRightmostLeaf(tok2)); if (settings->debugwarnings) - bailout(tokenlist, errorLogger, tok2, "variable " + var->name() + " valueFlowForward, skip ternary in subfunctions"); + bailout(tokenlist, + errorLogger, + tok2, + "variable " + var->name() + " valueFlowForwardVariable, skip ternary in subfunctions"); continue; } const Token *condition = tok2->astOperand1(); @@ -2797,7 +2840,10 @@ static bool valueFlowForward(Token * const startToken, if (changed0 && changed1) { if (settings->debugwarnings) - bailout(tokenlist, errorLogger, tok2, "variable " + var->name() + " valueFlowForward, changed in both : expressions"); + bailout(tokenlist, + errorLogger, + tok2, + "variable " + var->name() + " valueFlowForwardVariable, changed in both : expressions"); return false; } @@ -2820,7 +2866,10 @@ static bool valueFlowForward(Token * const startToken, if (isVariableChanged(questionToken, questionToken->astOperand2(), varid, false, settings, tokenlist->isCPP()) && isVariableChanged(questionToken->astOperand2(), tok2, varid, false, settings, tokenlist->isCPP())) { if (settings->debugwarnings) - bailout(tokenlist, errorLogger, tok2, "variable " + var->name() + " valueFlowForward, assignment in condition"); + bailout(tokenlist, + errorLogger, + tok2, + "variable " + var->name() + " valueFlowForwardVariable, assignment in condition"); return false; } @@ -3026,7 +3075,10 @@ static bool valueFlowForward(Token * const startToken, const Token *bodyStart = tok2->linkAt(1)->linkAt(1)->next(); if (isVariableChanged(bodyStart, bodyStart->link(), varid, var->isGlobal(), settings, tokenlist->isCPP())) { if (settings->debugwarnings) - bailout(tokenlist, errorLogger, tok2, "valueFlowForward, " + var->name() + " is changed in lambda function"); + bailout(tokenlist, + errorLogger, + tok2, + "valueFlowForwardVariable, " + var->name() + " is changed in lambda function"); return false; } } @@ -3035,6 +3087,86 @@ static bool valueFlowForward(Token * const startToken, return true; } +const Token* parseBinaryIntOp(const Token* expr, MathLib::bigint& known) +{ + if (!expr) + return nullptr; + if (!expr->astOperand1() || !expr->astOperand2()) + return nullptr; + const Token* knownTok = nullptr; + const Token* varTok = nullptr; + if (expr->astOperand1()->hasKnownIntValue() && !expr->astOperand2()->hasKnownIntValue()) { + varTok = expr->astOperand2(); + knownTok = expr->astOperand1(); + } else if (expr->astOperand2()->hasKnownIntValue() && !expr->astOperand1()->hasKnownIntValue()) { + varTok = expr->astOperand1(); + knownTok = expr->astOperand2(); + } + if (knownTok) + known = knownTok->values().front().intvalue; + return varTok; +} + +template +void transformIntValues(std::list& values, F f) +{ + std::transform(values.begin(), values.end(), values.begin(), [&](ValueFlow::Value x) { + if (x.isIntValue()) + x.intvalue = f(x.intvalue); + return x; + }); +} + +const Token* solveExprValues(const Token* expr, std::list& values) +{ + MathLib::bigint intval; + const Token* binaryTok = parseBinaryIntOp(expr, intval); + if (binaryTok && expr->str().size() == 1) { + switch (expr->str()[0]) { + case '+': { + transformIntValues(values, [&](MathLib::bigint x) { return x - intval; }); + return solveExprValues(binaryTok, values); + } + case '*': { + transformIntValues(values, [&](MathLib::bigint x) { return x / intval; }); + return solveExprValues(binaryTok, values); + } + case '^': { + transformIntValues(values, [&](MathLib::bigint x) { return x ^ intval; }); + return solveExprValues(binaryTok, values); + } + } + } + return expr; +} + +static void valueFlowForward(Token* startToken, + const Token* endToken, + const Token* exprTok, + std::list values, + const bool constValue, + const bool subFunction, + TokenList* const tokenlist, + ErrorLogger* const errorLogger, + const Settings* settings) +{ + const Token* expr = solveExprValues(exprTok, values); + if (Token::Match(expr, "%var%")) { + valueFlowForwardVariable(startToken, + endToken, + expr->variable(), + expr->varId(), + values, + constValue, + subFunction, + tokenlist, + errorLogger, + settings); + } else { + valueFlowForwardExpression(startToken, endToken, expr, values, tokenlist, settings); + } +} + static const Token *findSimpleReturn(const Function *f) { const Scope *scope = f->functionScope; @@ -3413,30 +3545,22 @@ static void valueFlowForwardLifetime(Token * tok, TokenList *tokenlist, ErrorLog // Only forward lifetime values values.remove_if(&isNotLifetimeValue); - valueFlowForward(const_cast(nextExpression), - endOfVarScope, - var, - var->declarationId(), - values, - false, - false, - tokenlist, - errorLogger, - settings); + valueFlowForwardVariable(const_cast(nextExpression), + endOfVarScope, + var, + var->declarationId(), + values, + false, + false, + tokenlist, + errorLogger, + settings); if (tok->astTop() && Token::simpleMatch(tok->astTop()->previous(), "for (") && Token::simpleMatch(tok->astTop()->link(), ") {")) { Token *start = tok->astTop()->link()->next(); - valueFlowForward(start, - start->link(), - var, - var->declarationId(), - values, - false, - false, - tokenlist, - errorLogger, - settings); + valueFlowForwardVariable( + start, start->link(), var, var->declarationId(), values, false, false, tokenlist, errorLogger, settings); } // Constructor } else if (Token::simpleMatch(parent, "{") && !isScopeBracket(parent)) { @@ -3457,16 +3581,16 @@ static void valueFlowForwardLifetime(Token * tok, TokenList *tokenlist, ErrorLog const Token *nextExpression = nextAfterAstRightmostLeaf(parent); // Only forward lifetime values values.remove_if(&isNotLifetimeValue); - valueFlowForward(const_cast(nextExpression), - endOfVarScope, - var, - var->declarationId(), - values, - false, - false, - tokenlist, - errorLogger, - settings); + valueFlowForwardVariable(const_cast(nextExpression), + endOfVarScope, + var, + var->declarationId(), + values, + false, + false, + tokenlist, + errorLogger, + settings); } } @@ -4019,7 +4143,8 @@ static void valueFlowAfterMove(TokenList *tokenlist, SymbolDatabase* symboldatab const int varId = varTok->varId(); const Token * const endOfVarScope = var->typeStartToken()->scope()->bodyEnd; setTokenValue(varTok, value, settings); - valueFlowForward(varTok->next(), endOfVarScope, var, varId, values, false, false, tokenlist, errorLogger, settings); + valueFlowForwardVariable( + varTok->next(), endOfVarScope, var, varId, values, false, false, tokenlist, errorLogger, settings); continue; } ValueFlow::Value::MoveKind moveKind; @@ -4055,7 +4180,16 @@ static void valueFlowAfterMove(TokenList *tokenlist, SymbolDatabase* symboldatab const Token * openParentesisOfMove = findOpenParentesisOfMove(varTok); const Token * endOfFunctionCall = findEndOfFunctionCallForParameter(openParentesisOfMove); if (endOfFunctionCall) - valueFlowForward(const_cast(endOfFunctionCall), endOfVarScope, var, varId, values, false, false, tokenlist, errorLogger, settings); + valueFlowForwardVariable(const_cast(endOfFunctionCall), + endOfVarScope, + var, + varId, + values, + false, + false, + tokenlist, + errorLogger, + settings); } } } @@ -4106,21 +4240,30 @@ static void valueFlowForwardAssign(Token * const tok, values.end(), std::back_inserter(tokvalues), std::mem_fn(&ValueFlow::Value::isTokValue)); - valueFlowForward(const_cast(nextExpression), - endOfVarScope, - var, - var->declarationId(), - tokvalues, - constValue, - false, - tokenlist, - errorLogger, - settings); + valueFlowForwardVariable(const_cast(nextExpression), + endOfVarScope, + var, + var->declarationId(), + tokvalues, + constValue, + false, + tokenlist, + errorLogger, + settings); values.remove_if(std::mem_fn(&ValueFlow::Value::isTokValue)); } for (ValueFlow::Value& value:values) value.tokvalue = tok; - valueFlowForward(const_cast(nextExpression), endOfVarScope, var, var->declarationId(), values, constValue, false, tokenlist, errorLogger, settings); + valueFlowForwardVariable(const_cast(nextExpression), + endOfVarScope, + var, + var->declarationId(), + values, + constValue, + false, + tokenlist, + errorLogger, + settings); } static std::list truncateValues(std::list values, const ValueType *valueType, const Settings *settings) @@ -4233,6 +4376,20 @@ void insertImpossible(std::list& values, const std::list getExprVariables(const Token* expr, + const TokenList* tokenlist, + const SymbolDatabase* symboldatabase, + const Settings* settings) +{ + std::vector result; + FwdAnalysis fwdAnalysis(tokenlist->isCPP(), settings->library); + std::set varids = fwdAnalysis.getExprVarIds(expr); + std::transform(varids.begin(), varids.end(), std::back_inserter(result), [&](int id) { + return symboldatabase->getVariableFromVarId(id); + }); + return result; +} + struct ValueFlowConditionHandler { struct Condition { const Token *vartok; @@ -4241,8 +4398,8 @@ struct ValueFlowConditionHandler { Condition() : vartok(nullptr), true_values(), false_values() {} }; - std::function &values, bool constValue)> - forward; + std::function& values, bool constValue)> + forward; std::function parse; void afterCondition(TokenList *tokenlist, @@ -4254,19 +4411,27 @@ struct ValueFlowConditionHandler { for (Token *tok = const_cast(scope->bodyStart); tok != scope->bodyEnd; tok = tok->next()) { if (Token::Match(tok, "= & %var% ;")) aliased.insert(tok->tokAt(2)->varId()); + const Token* top = tok->astTop(); + if (!top) + continue; + + if (!Token::Match(top->previous(), "if|while|for (") && !Token::Match(tok->astParent(), "&&|%oror%")) + continue; Condition cond = parse(tok); if (!cond.vartok) continue; if (cond.true_values.empty() || cond.false_values.empty()) continue; - const int varid = cond.vartok->varId(); - if (varid == 0U) + + std::vector vars = getExprVariables(cond.vartok, tokenlist, symboldatabase, settings); + if (std::any_of(vars.begin(), vars.end(), [](const Variable* var) { + return !var || !(var->isLocal() || var->isGlobal() || var->isArgument()); + })) continue; - const Variable *var = cond.vartok->variable(); - if (!var || !(var->isLocal() || var->isGlobal() || var->isArgument())) - continue; - if (aliased.find(varid) != aliased.end()) { + if (std::any_of(vars.begin(), vars.end(), [&](const Variable* var) { + return aliased.find(var->declarationId()) != aliased.end(); + })) { if (settings->debugwarnings) bailout(tokenlist, errorLogger, @@ -4292,10 +4457,16 @@ struct ValueFlowConditionHandler { continue; tokens.push(rhstok->astOperand1()); tokens.push(rhstok->astOperand2()); - if (rhstok->varId() == varid) + if (isSameExpression( + tokenlist->isCPP(), false, cond.vartok, rhstok, settings->library, true, false)) setTokenValue(rhstok, cond.true_values.front(), settings); - else if (Token::Match(rhstok, "++|--|=") && - Token::Match(rhstok->astOperand1(), "%varid%", varid)) { + else if (Token::Match(rhstok, "++|--|=") && isSameExpression(tokenlist->isCPP(), + false, + cond.vartok, + rhstok->astOperand1(), + settings->library, + true, + false)) { assign = true; break; } @@ -4308,11 +4479,10 @@ struct ValueFlowConditionHandler { } } - const Token *top = tok->astTop(); if (top && Token::Match(top->previous(), "if|while (") && !top->previous()->isExpandedMacro()) { // does condition reassign variable? if (tok != top->astOperand2() && Token::Match(top->astOperand2(), "%oror%|&&") && - isVariableChanged(top, top->link(), varid, var->isGlobal(), settings, tokenlist->isCPP())) { + isVariablesChanged(top, top->link(), 0, vars, settings, tokenlist->isCPP())) { if (settings->debugwarnings) bailout(tokenlist, errorLogger, tok, "assignment in condition"); continue; @@ -4321,12 +4491,12 @@ struct ValueFlowConditionHandler { std::list thenValues; std::list elseValues; - if (!Token::Match(tok, "!=|%var%")) { + if (!Token::Match(tok, "!=|=") && tok != cond.vartok) { thenValues.insert(thenValues.end(), cond.true_values.begin(), cond.true_values.end()); if (isConditionKnown(tok, false)) insertImpossible(elseValues, cond.false_values); } - if (!Token::Match(tok, "==|!") && !Token::Match(tok->previous(), "%name% (")) { + if (!Token::Match(tok, "==|!")) { elseValues.insert(elseValues.end(), cond.false_values.begin(), cond.false_values.end()); if (isConditionKnown(tok, true)) insertImpossible(thenValues, cond.true_values); @@ -4364,8 +4534,8 @@ struct ValueFlowConditionHandler { std::list& values = (i == 0 ? thenValues : elseValues); valueFlowSetConditionToKnown(tok, values, i == 0); - // TODO: The endToken should not be startTokens[i]->link() in the valueFlowForward call - if (forward(startTokens[i], startTokens[i]->link(), var, values, true)) + // TODO: The endToken should not be startTokens[i]->link() in the valueFlowForwardVariable call + if (forward(startTokens[i], startTokens[i]->link(), cond.vartok, values, true)) changeBlock = i; changeKnownToPossible(values); } @@ -4375,7 +4545,8 @@ struct ValueFlowConditionHandler { bailout(tokenlist, errorLogger, startTokens[changeBlock]->link(), - "valueFlowAfterCondition: " + var->name() + " is changed in conditional block"); + "valueFlowAfterCondition: " + cond.vartok->expressionString() + + " is changed in conditional block"); continue; } @@ -4431,7 +4602,7 @@ struct ValueFlowConditionHandler { // TODO: constValue could be true if there are no assignments in the conditional blocks and // perhaps if there are no && and no || in the condition bool constValue = false; - forward(after, top->scope()->bodyEnd, var, values, constValue); + forward(after, top->scope()->bodyEnd, cond.vartok, values, constValue); } } } @@ -4446,14 +4617,14 @@ static void valueFlowAfterCondition(TokenList *tokenlist, const Settings *settings) { ValueFlowConditionHandler handler; - handler.forward = [&](Token *start, - const Token *stop, - const Variable *var, - const std::list &values, - bool constValue) { - valueFlowForward( - start->next(), stop, var, var->declarationId(), values, constValue, false, tokenlist, errorLogger, settings); - return isVariableChanged(start, stop, var->declarationId(), var->isGlobal(), settings, tokenlist->isCPP()); + handler.forward = [&](Token* start, + const Token* stop, + const Token* vartok, + const std::list& values, + bool constValue) { + valueFlowForward(start->next(), stop, vartok, values, constValue, false, tokenlist, errorLogger, settings); + std::vector vars = getExprVariables(vartok, tokenlist, symboldatabase, settings); + return isVariablesChanged(start, stop, 0, vars, settings, tokenlist->isCPP()); }; handler.parse = [&](const Token *tok) { ValueFlowConditionHandler::Condition cond; @@ -4463,8 +4634,6 @@ static void valueFlowAfterCondition(TokenList *tokenlist, if (vartok) { if (vartok->str() == "=" && vartok->astOperand1() && vartok->astOperand2()) vartok = vartok->astOperand1(); - if (!vartok->isName()) - return cond; cond.true_values.push_back(true_value); cond.false_values.push_back(false_value); cond.vartok = vartok; @@ -4474,12 +4643,15 @@ static void valueFlowAfterCondition(TokenList *tokenlist, if (tok->str() == "!") { vartok = tok->astOperand1(); - } else if (tok->isName() && (Token::Match(tok->astParent(), "%oror%|&&") || - Token::Match(tok->tokAt(-2), "if|while ( %var% [)=]"))) { - vartok = tok; + } else if (tok->astParent() && (Token::Match(tok->astParent(), "%oror%|&&") || + Token::Match(tok->astParent()->previous(), "if|while ("))) { + if (Token::simpleMatch(tok, "=")) + vartok = tok->astOperand1(); + else if (!Token::Match(tok, "%comp%|%assign%")) + vartok = tok; } - if (!vartok || !vartok->isName()) + if (!vartok) return cond; cond.true_values.emplace_back(tok, 0LL); cond.false_values.emplace_back(tok, 0LL); @@ -4965,16 +5137,8 @@ static void valueFlowForLoopSimplifyAfter(Token *fortok, nonneg int varid, const values.emplace_back(num); values.back().errorPath.emplace_back(fortok,"After for loop, " + var->name() + " has value " + values.back().infoString()); - valueFlowForward(fortok->linkAt(1)->linkAt(1)->next(), - endToken, - var, - varid, - values, - false, - false, - tokenlist, - errorLogger, - settings); + valueFlowForwardVariable( + fortok->linkAt(1)->linkAt(1)->next(), endToken, var, varid, values, false, false, tokenlist, errorLogger, settings); } static void valueFlowForLoop(TokenList *tokenlist, SymbolDatabase* symboldatabase, ErrorLogger *errorLogger, const Settings *settings) @@ -5035,7 +5199,16 @@ static void valueFlowInjectParameter(TokenList* tokenlist, ErrorLogger* errorLog if (!varid2) return; - valueFlowForward(const_cast(functionScope->bodyStart->next()), functionScope->bodyEnd, arg, varid2, argvalues, false, true, tokenlist, errorLogger, settings); + valueFlowForwardVariable(const_cast(functionScope->bodyStart->next()), + functionScope->bodyEnd, + arg, + varid2, + argvalues, + false, + true, + tokenlist, + errorLogger, + settings); } static void valueFlowSwitchVariable(TokenList *tokenlist, SymbolDatabase* symboldatabase, ErrorLogger *errorLogger, const Settings *settings) @@ -5093,7 +5266,16 @@ static void valueFlowSwitchVariable(TokenList *tokenlist, SymbolDatabase* symbol if (vartok->variable()->scope()) { if (known) values.back().setKnown(); - valueFlowForward(tok->tokAt(3), vartok->variable()->scope()->bodyEnd, vartok->variable(), vartok->varId(), values, values.back().isKnown(), false, tokenlist, errorLogger, settings); + valueFlowForwardVariable(tok->tokAt(3), + vartok->variable()->scope()->bodyEnd, + vartok->variable(), + vartok->varId(), + values, + values.back().isKnown(), + false, + tokenlist, + errorLogger, + settings); } } } @@ -5501,7 +5683,16 @@ static void valueFlowUninit(TokenList *tokenlist, SymbolDatabase * /*symbolDatab const bool constValue = true; const bool subFunction = false; - valueFlowForward(vardecl->next(), vardecl->scope()->bodyEnd, var, vardecl->varId(), values, constValue, subFunction, tokenlist, errorLogger, settings); + valueFlowForwardVariable(vardecl->next(), + vardecl->scope()->bodyEnd, + var, + vardecl->varId(), + values, + constValue, + subFunction, + tokenlist, + errorLogger, + settings); } } @@ -5858,13 +6049,16 @@ static void valueFlowContainerAfterCondition(TokenList *tokenlist, { ValueFlowConditionHandler handler; handler.forward = - [&](Token *start, const Token *stop, const Variable *var, const std::list &values, bool) { - // TODO: Forward multiple values - if (values.empty()) - return false; - valueFlowContainerForward(start, var->declarationId(), values.front(), settings, tokenlist->isCPP()); - return isContainerSizeChanged(var->declarationId(), start, stop); - }; + [&](Token* start, const Token* stop, const Token* vartok, const std::list& values, bool) { + // TODO: Forward multiple values + if (values.empty()) + return false; + const Variable* var = vartok->variable(); + if (!var) + return false; + valueFlowContainerForward(start, var->declarationId(), values.front(), settings, tokenlist->isCPP()); + return isContainerSizeChanged(var->declarationId(), start, stop); + }; handler.parse = [&](const Token *tok) { ValueFlowConditionHandler::Condition cond; ValueFlow::Value true_value; @@ -5938,16 +6132,12 @@ static void valueFlowFwdAnalysis(const TokenList *tokenlist, const Settings *set continue; ValueFlow::Value v(tok->astOperand2()->values().front()); v.errorPath.emplace_back(tok, tok->astOperand1()->expressionString() + " is assigned value " + MathLib::toString(v.intvalue)); - FwdAnalysis fwdAnalysis(tokenlist->isCPP(), settings->library); const Token *startToken = tok->findExpressionStartEndTokens().second->next(); const Scope *functionScope = tok->scope(); while (functionScope->nestedIn && functionScope->nestedIn->isExecutable()) functionScope = functionScope->nestedIn; const Token *endToken = functionScope->bodyEnd; - for (const FwdAnalysis::KnownAndToken read : fwdAnalysis.valueFlow(tok->astOperand1(), startToken, endToken)) { - v.valueKind = read.known ? ValueFlow::Value::ValueKind::Known : ValueFlow::Value::ValueKind::Possible; - setTokenValue(const_cast(read.token), v, settings); - } + valueFlowForwardExpression(const_cast(startToken), endToken, tok->astOperand1(), {v}, tokenlist, settings); } } @@ -6009,16 +6199,16 @@ static void valueFlowDynamicBufferSize(TokenList *tokenlist, SymbolDatabase *sym value.valueType = ValueFlow::Value::ValueType::BUFFER_SIZE; value.setKnown(); const std::list values{value}; - valueFlowForward(const_cast(rhs), - functionScope->bodyEnd, - tok->next()->variable(), - tok->next()->varId(), - values, - true, - false, - tokenlist, - errorLogger, - settings); + valueFlowForwardVariable(const_cast(rhs), + functionScope->bodyEnd, + tok->next()->variable(), + tok->next()->varId(), + values, + true, + false, + tokenlist, + errorLogger, + settings); } } } @@ -6153,16 +6343,16 @@ static void valueFlowSafeFunctions(TokenList *tokenlist, SymbolDatabase *symbold argValues.back().floatValue = isHigh ? high : 1E25f; argValues.back().errorPath.emplace_back(arg.nameToken(), "Safe checks: Assuming argument has value " + MathLib::toString(argValues.back().floatValue)); argValues.back().safe = true; - valueFlowForward(const_cast(functionScope->bodyStart->next()), - functionScope->bodyEnd, - &arg, - arg.declarationId(), - argValues, - false, - false, - tokenlist, - errorLogger, - settings); + valueFlowForwardVariable(const_cast(functionScope->bodyStart->next()), + functionScope->bodyEnd, + &arg, + arg.declarationId(), + argValues, + false, + false, + tokenlist, + errorLogger, + settings); continue; } } @@ -6180,16 +6370,16 @@ static void valueFlowSafeFunctions(TokenList *tokenlist, SymbolDatabase *symbold } if (!argValues.empty()) - valueFlowForward(const_cast(functionScope->bodyStart->next()), - functionScope->bodyEnd, - &arg, - arg.declarationId(), - argValues, - false, - false, - tokenlist, - errorLogger, - settings); + valueFlowForwardVariable(const_cast(functionScope->bodyStart->next()), + functionScope->bodyEnd, + &arg, + arg.declarationId(), + argValues, + false, + false, + tokenlist, + errorLogger, + settings); } } } diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index a73eb1cfc..e32f282da 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -82,6 +82,7 @@ private: TEST_CASE(valueFlowAfterAssign); TEST_CASE(valueFlowAfterCondition); + TEST_CASE(valueFlowAfterConditionExpr); TEST_CASE(valueFlowAfterConditionSeveralNot); TEST_CASE(valueFlowForwardCompoundAssign); TEST_CASE(valueFlowForwardCorrelatedVariables); @@ -1385,10 +1386,11 @@ private: "out:" " if (x==123){}\n" "}"); - ASSERT_EQUALS_WITHOUT_LINENUMBERS("[test.cpp:3]: (debug) valueflow.cpp::valueFlowTerminatingCondition bailout: Skipping function due to incomplete variable a\n" - "[test.cpp:4]: (debug) valueflow.cpp:1131:valueFlowReverse bailout: variable x stopping on goto label\n" - "[test.cpp:2]: (debug) valueflow.cpp:1813:valueFlowForward bailout: variable x. noreturn conditional scope.\n" - , errout.str()); + ASSERT_EQUALS_WITHOUT_LINENUMBERS( + "[test.cpp:3]: (debug) valueflow.cpp::valueFlowTerminatingCondition bailout: Skipping function due to incomplete variable a\n" + "[test.cpp:4]: (debug) valueflow.cpp:1131:valueFlowReverse bailout: variable x stopping on goto label\n" + "[test.cpp:2]: (debug) valueflow.cpp:1813:valueFlowForwardVariable bailout: variable x. noreturn conditional scope.\n", + errout.str()); // #5721 - FP bailout("static void f(int rc) {\n" @@ -1401,10 +1403,11 @@ private: "out:\n" " if (abc) {}\n" "}\n"); - ASSERT_EQUALS_WITHOUT_LINENUMBERS("[test.cpp:2]: (debug) valueflow.cpp:1035:valueFlowReverse bailout: assignment of abc\n" - "[test.cpp:8]: (debug) valueflow.cpp:1131:valueFlowReverse bailout: variable abc stopping on goto label\n" - "[test.cpp:3]: (debug) valueflow.cpp:1813:valueFlowForward bailout: variable abc. noreturn conditional scope.\n", - errout.str()); + ASSERT_EQUALS_WITHOUT_LINENUMBERS( + "[test.cpp:2]: (debug) valueflow.cpp:1035:valueFlowReverse bailout: assignment of abc\n" + "[test.cpp:8]: (debug) valueflow.cpp:1131:valueFlowReverse bailout: variable abc stopping on goto label\n" + "[test.cpp:3]: (debug) valueflow.cpp:1813:valueFlowForwardVariable bailout: variable abc. noreturn conditional scope.\n", + errout.str()); } void valueFlowAfterAssign() { @@ -2306,6 +2309,62 @@ private: ASSERT_EQUALS(true, testValueOfX(code, 6U, 0)); } + void valueFlowAfterConditionExpr() + { + const char* code; + + code = "void f(int* p) {\n" + " if (p[0] == 123) {\n" + " int x = p[0];\n" + " int a = x;\n" + " }\n" + "}"; + ASSERT_EQUALS(true, testValueOfX(code, 4U, 123)); + + code = "void f(int y) {\n" + " if (y+1 == 123) {\n" + " int x = y+1;\n" + " int a = x;\n" + " }\n" + "}"; + ASSERT_EQUALS(true, testValueOfX(code, 4U, 123)); + + code = "void f(int y) {\n" + " if (y+1 == 123) {\n" + " int x = y+2;\n" + " int a = x;\n" + " }\n" + "}"; + ASSERT_EQUALS(true, testValueOfX(code, 4U, 124)); + + code = "void f(int y, int z) {\n" + " if (y+z == 123) {\n" + " int x = y+z;\n" + " int a = x;\n" + " }\n" + "}"; + ASSERT_EQUALS(true, testValueOfX(code, 4U, 123)); + + code = "void f(int y, int z) {\n" + " if (y+z == 123) {\n" + " y++;\n" + " int x = y+z;\n" + " int a = x;\n" + " }\n" + "}"; + ASSERT_EQUALS(false, testValueOfX(code, 5U, 123)); + + code = "void f(int y) {\n" + " if (y++ == 123) {\n" + " int x = y++;\n" + " int a = x;\n" + " }\n" + "}"; + ASSERT_EQUALS(false, testValueOfX(code, 4U, 123)); + ASSERT_EQUALS(false, testValueOfX(code, 4U, 124)); + ASSERT_EQUALS(false, testValueOfX(code, 4U, 125)); + } + void valueFlowAfterConditionSeveralNot() { const char *code;