From a0d3c2c719cedcd68877ff911d4fb579bc39f0ed Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Sun, 14 Nov 2021 11:30:36 -0600 Subject: [PATCH] Handle for loop conditions in afterCondition (#3561) --- lib/astutils.cpp | 46 ++++++ lib/astutils.h | 6 + lib/forwardanalyzer.cpp | 28 ---- lib/programmemory.cpp | 7 +- lib/valueflow.cpp | 313 ++++++++++++++++++++++++---------------- test/testvalueflow.cpp | 11 +- 6 files changed, 253 insertions(+), 158 deletions(-) mode change 100755 => 100644 lib/astutils.cpp mode change 100755 => 100644 lib/valueflow.cpp diff --git a/lib/astutils.cpp b/lib/astutils.cpp old mode 100755 new mode 100644 index 253a23401..5fc26cd1a --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -595,6 +595,38 @@ static T* getCondTokFromEndImpl(T* endBlock) return nullptr; } +template )> +static T* getInitTokImpl(T* tok) +{ + if (!tok) + return nullptr; + if (Token::Match(tok, "%name% (")) + return getInitTokImpl(tok->next()); + if (tok->str() != "(") + return nullptr; + if (!Token::simpleMatch(tok->astOperand2(), ";")) + return nullptr; + if (Token::simpleMatch(tok->astOperand2()->astOperand1(), ";")) + return nullptr; + return tok->astOperand2()->astOperand1(); +} + +template )> +static T* getStepTokImpl(T* tok) +{ + if (!tok) + return nullptr; + if (Token::Match(tok, "%name% (")) + return getStepTokImpl(tok->next()); + if (tok->str() != "(") + return nullptr; + if (!Token::simpleMatch(tok->astOperand2(), ";")) + return nullptr; + if (!Token::simpleMatch(tok->astOperand2()->astOperand2(), ";")) + return nullptr; + return tok->astOperand2()->astOperand2()->astOperand2(); +} + Token* getCondTok(Token* tok) { return getCondTokImpl(tok); @@ -613,6 +645,20 @@ const Token* getCondTokFromEnd(const Token* endBlock) return getCondTokFromEndImpl(endBlock); } +Token* getInitTok(Token* tok) { + return getInitTokImpl(tok); +} +const Token* getInitTok(const Token* tok) { + return getInitTokImpl(tok); +} + +Token* getStepTok(Token* tok) { + return getStepTokImpl(tok); +} +const Token* getStepTok(const Token* tok) { + return getStepTokImpl(tok); +} + const Token *findNextTokenFromBreak(const Token *breakToken) { const Scope *scope = breakToken->scope(); diff --git a/lib/astutils.h b/lib/astutils.h index f9342562d..538e1846e 100755 --- a/lib/astutils.h +++ b/lib/astutils.h @@ -126,6 +126,12 @@ bool astIsRHS(const Token* tok); Token* getCondTok(Token* tok); const Token* getCondTok(const Token* tok); +Token* getInitTok(Token* tok); +const Token* getInitTok(const Token* tok); + +Token* getStepTok(Token* tok); +const Token* getStepTok(const Token* tok); + Token* getCondTokFromEnd(Token* endBlock); const Token* getCondTokFromEnd(const Token* endBlock); diff --git a/lib/forwardanalyzer.cpp b/lib/forwardanalyzer.cpp index cda1516dc..09965a022 100644 --- a/lib/forwardanalyzer.cpp +++ b/lib/forwardanalyzer.cpp @@ -784,34 +784,6 @@ struct ForwardTraversal { return parent && (parent->str() == ":" || parent->astOperand2() == tok); } - static Token* getInitTok(Token* tok) { - if (!tok) - return nullptr; - if (Token::Match(tok, "%name% (")) - return getInitTok(tok->next()); - if (tok->str() != "(") - return nullptr; - if (!Token::simpleMatch(tok->astOperand2(), ";")) - return nullptr; - if (Token::simpleMatch(tok->astOperand2()->astOperand1(), ";")) - return nullptr; - return tok->astOperand2()->astOperand1(); - } - - static Token* getStepTok(Token* tok) { - if (!tok) - return nullptr; - if (Token::Match(tok, "%name% (")) - return getStepTok(tok->next()); - if (tok->str() != "(") - return nullptr; - if (!Token::simpleMatch(tok->astOperand2(), ";")) - return nullptr; - if (!Token::simpleMatch(tok->astOperand2()->astOperand2(), ";")) - return nullptr; - return tok->astOperand2()->astOperand2()->astOperand2(); - } - static Token* getStepTokFromEnd(Token* tok) { if (!Token::simpleMatch(tok, "}")) return nullptr; diff --git a/lib/programmemory.cpp b/lib/programmemory.cpp index f52ae65a2..924775baf 100644 --- a/lib/programmemory.cpp +++ b/lib/programmemory.cpp @@ -692,8 +692,9 @@ static ValueFlow::Value execute(const Token* expr, ProgramMemory& pm) void execute(const Token* expr, ProgramMemory* const programMemory, MathLib::bigint* result, bool* error) { ValueFlow::Value v = execute(expr, *programMemory); - if (!v.isIntValue() || v.isImpossible()) - *error = true; - else + if (!v.isIntValue() || v.isImpossible()) { + if (error) + *error = true; + } else if (result) *result = v.intvalue; } diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp old mode 100755 new mode 100644 index d53e45a6e..18718f1e9 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -1695,7 +1695,7 @@ static bool isConditionKnown(const Token* tok, bool then) const Token* parent = tok->astParent(); while (parent && (parent->str() == op || parent->str() == "!")) parent = parent->astParent(); - return (parent && parent->str() == "("); + return Token::Match(parent, "(|;"); } static const std::string& invertAssign(const std::string& assign) @@ -5221,144 +5221,205 @@ struct ConditionHandler { } } - if (top && Token::Match(top->previous(), "if|while (") && !top->previous()->isExpandedMacro()) { - // if astParent is "!" we need to invert codeblock - { - const Token* tok2 = tok; - while (tok2->astParent()) { - const Token* parent = tok2->astParent(); - while (parent && parent->str() == "&&") - parent = parent->astParent(); - if (parent && (parent->str() == "!" || Token::simpleMatch(parent, "== false"))) - std::swap(thenValues, elseValues); - tok2 = parent; + if (!top) + return; + + if (top->previous()->isExpandedMacro()) + return; + + if (!Token::Match(top->previous(), "if|while|for (")) + return; + + if (top->previous()->str() == "for") { + if (!Token::Match(tok, "%comp%")) + return; + if (!Token::simpleMatch(tok->astParent(), ";")) + return; + const Token* stepTok = getStepTok(top); + if (cond.vartok->varId() == 0) + return; + if (!cond.vartok->variable()) + return; + if (!Token::Match(stepTok, "++|--")) + return; + std::set bounds; + for (const ValueFlow::Value& v : thenValues) { + if (v.bound != ValueFlow::Value::Bound::Point && v.isImpossible()) + continue; + bounds.insert(v.bound); + } + if (Token::simpleMatch(stepTok, "++") && bounds.count(ValueFlow::Value::Bound::Lower) > 0) + return; + if (Token::simpleMatch(stepTok, "--") && bounds.count(ValueFlow::Value::Bound::Upper) > 0) + return; + const Token* childTok = tok->astOperand1(); + if (!childTok) + childTok = tok->astOperand2(); + if (!childTok) + return; + if (childTok->varId() != cond.vartok->varId()) + return; + const Token* startBlock = top->link()->next(); + if (isVariableChanged(startBlock, + startBlock->link(), + cond.vartok->varId(), + cond.vartok->variable()->isGlobal(), + settings, + tokenlist->isCPP())) + return; + // Check if condition in for loop is always false + const Token* initTok = getInitTok(top); + ProgramMemory pm; + execute(initTok, &pm, nullptr, nullptr); + MathLib::bigint result = 1; + execute(tok, &pm, &result, nullptr); + if (result == 0) + return; + // Remove condition since for condition is not redundant + for (std::list* values : {&thenValues, &elseValues}) { + for (ValueFlow::Value& v : *values) { + v.condition = nullptr; + v.conditional = true; } } + } - bool deadBranch[] = {false, false}; - // start token of conditional code - Token* startTokens[] = {nullptr, nullptr}; - // determine startToken(s) - if (Token::simpleMatch(top->link(), ") {")) - startTokens[0] = top->link()->next(); - if (Token::simpleMatch(top->link()->linkAt(1), "} else {")) - startTokens[1] = top->link()->linkAt(1)->tokAt(2); - - int changeBlock = -1; - int bailBlock = -1; - - for (int i = 0; i < 2; i++) { - const Token* const startToken = startTokens[i]; - if (!startToken) - continue; - std::list& values = (i == 0 ? thenValues : elseValues); - valueFlowSetConditionToKnown(tok, values, i == 0); - - Analyzer::Result r = - forward(startTokens[i], startTokens[i]->link(), cond.vartok, values, tokenlist, settings); - deadBranch[i] = r.terminate == Analyzer::Terminate::Escape; - if (r.action.isModified() && !deadBranch[i]) - changeBlock = i; - if (r.terminate != Analyzer::Terminate::None && r.terminate != Analyzer::Terminate::Escape && - r.terminate != Analyzer::Terminate::Modified) - bailBlock = i; - changeKnownToPossible(values); + // if astParent is "!" we need to invert codeblock + { + const Token* tok2 = tok; + while (tok2->astParent()) { + const Token* parent = tok2->astParent(); + while (parent && parent->str() == "&&") + parent = parent->astParent(); + if (parent && (parent->str() == "!" || Token::simpleMatch(parent, "== false"))) + std::swap(thenValues, elseValues); + tok2 = parent; } - if (changeBlock >= 0 && !Token::simpleMatch(top->previous(), "while (")) { + } + + bool deadBranch[] = {false, false}; + // start token of conditional code + Token* startTokens[] = {nullptr, nullptr}; + // determine startToken(s) + if (Token::simpleMatch(top->link(), ") {")) + startTokens[0] = top->link()->next(); + if (Token::simpleMatch(top->link()->linkAt(1), "} else {")) + startTokens[1] = top->link()->linkAt(1)->tokAt(2); + + int changeBlock = -1; + int bailBlock = -1; + + for (int i = 0; i < 2; i++) { + const Token* const startToken = startTokens[i]; + if (!startToken) + continue; + std::list& values = (i == 0 ? thenValues : elseValues); + valueFlowSetConditionToKnown(tok, values, i == 0); + + Analyzer::Result r = + forward(startTokens[i], startTokens[i]->link(), cond.vartok, values, tokenlist, settings); + deadBranch[i] = r.terminate == Analyzer::Terminate::Escape; + if (r.action.isModified() && !deadBranch[i]) + changeBlock = i; + if (r.terminate != Analyzer::Terminate::None && r.terminate != Analyzer::Terminate::Escape && + r.terminate != Analyzer::Terminate::Modified) + bailBlock = i; + changeKnownToPossible(values); + } + if (changeBlock >= 0 && !Token::simpleMatch(top->previous(), "while (")) { + if (settings->debugwarnings) + bailout(tokenlist, + errorLogger, + startTokens[changeBlock]->link(), + "valueFlowAfterCondition: " + cond.vartok->expressionString() + + " is changed in conditional block"); + return; + } else if (bailBlock >= 0) { + if (settings->debugwarnings) + bailout(tokenlist, + errorLogger, + startTokens[bailBlock]->link(), + "valueFlowAfterCondition: bailing in conditional block"); + return; + } + + // After conditional code.. + if (Token::simpleMatch(top->link(), ") {")) { + Token* after = top->link()->linkAt(1); + bool dead_if = deadBranch[0]; + bool dead_else = deadBranch[1]; + const Token* unknownFunction = nullptr; + if (tok->astParent() && Token::simpleMatch(tok->astParent()->previous(), "while (")) + dead_if = !isBreakScope(after); + else if (!dead_if) + dead_if = isReturnScope(after, &settings->library, &unknownFunction); + + if (!dead_if && unknownFunction) { if (settings->debugwarnings) - bailout(tokenlist, - errorLogger, - startTokens[changeBlock]->link(), - "valueFlowAfterCondition: " + cond.vartok->expressionString() + - " is changed in conditional block"); - return; - } else if (bailBlock >= 0) { - if (settings->debugwarnings) - bailout(tokenlist, - errorLogger, - startTokens[bailBlock]->link(), - "valueFlowAfterCondition: bailing in conditional block"); + bailout(tokenlist, errorLogger, unknownFunction, "possible noreturn scope"); return; } - // After conditional code.. - if (Token::simpleMatch(top->link(), ") {")) { - Token* after = top->link()->linkAt(1); - bool dead_if = deadBranch[0]; - bool dead_else = deadBranch[1]; - const Token* unknownFunction = nullptr; - if (tok->astParent() && Token::simpleMatch(tok->astParent()->previous(), "while (")) - dead_if = !isBreakScope(after); - else if (!dead_if) - dead_if = isReturnScope(after, &settings->library, &unknownFunction); - - if (!dead_if && unknownFunction) { + if (Token::simpleMatch(after, "} else {")) { + after = after->linkAt(2); + unknownFunction = nullptr; + if (!dead_else) + dead_else = isReturnScope(after, &settings->library, &unknownFunction); + if (!dead_else && unknownFunction) { if (settings->debugwarnings) bailout(tokenlist, errorLogger, unknownFunction, "possible noreturn scope"); return; } - - if (Token::simpleMatch(after, "} else {")) { - after = after->linkAt(2); - unknownFunction = nullptr; - if (!dead_else) - dead_else = isReturnScope(after, &settings->library, &unknownFunction); - if (!dead_else && unknownFunction) { - if (settings->debugwarnings) - bailout(tokenlist, errorLogger, unknownFunction, "possible noreturn scope"); - return; - } - } - - if (dead_if && dead_else) - return; - - std::list values; - if (dead_if) { - values = elseValues; - } else if (dead_else) { - values = thenValues; - } else { - std::copy_if(thenValues.begin(), - thenValues.end(), - std::back_inserter(values), - std::mem_fn(&ValueFlow::Value::isPossible)); - std::copy_if(elseValues.begin(), - elseValues.end(), - std::back_inserter(values), - std::mem_fn(&ValueFlow::Value::isPossible)); - } - - if (values.empty()) - return; - - if (dead_if || dead_else) { - const Token* parent = tok->astParent(); - // Skip the not operator - while (Token::simpleMatch(parent, "!")) - parent = parent->astParent(); - bool possible = false; - if (Token::Match(parent, "&&|%oror%")) { - std::string op = parent->str(); - while (parent && parent->str() == op) - parent = parent->astParent(); - if (Token::simpleMatch(parent, "!") || Token::simpleMatch(parent, "== false")) - possible = op == "||"; - else - possible = op == "&&"; - } - if (possible) { - values.remove_if(std::mem_fn(&ValueFlow::Value::isImpossible)); - changeKnownToPossible(values); - } else { - valueFlowSetConditionToKnown(tok, values, true); - valueFlowSetConditionToKnown(tok, values, false); - } - } - if (values.empty()) - return; - forward(after, scope->bodyEnd, cond.vartok, values, tokenlist, settings); } + + if (dead_if && dead_else) + return; + + std::list values; + if (dead_if) { + values = elseValues; + } else if (dead_else) { + values = thenValues; + } else { + std::copy_if(thenValues.begin(), + thenValues.end(), + std::back_inserter(values), + std::mem_fn(&ValueFlow::Value::isPossible)); + std::copy_if(elseValues.begin(), + elseValues.end(), + std::back_inserter(values), + std::mem_fn(&ValueFlow::Value::isPossible)); + } + + if (values.empty()) + return; + + if (dead_if || dead_else) { + const Token* parent = tok->astParent(); + // Skip the not operator + while (Token::simpleMatch(parent, "!")) + parent = parent->astParent(); + bool possible = false; + if (Token::Match(parent, "&&|%oror%")) { + std::string op = parent->str(); + while (parent && parent->str() == op) + parent = parent->astParent(); + if (Token::simpleMatch(parent, "!") || Token::simpleMatch(parent, "== false")) + possible = op == "||"; + else + possible = op == "&&"; + } + if (possible) { + values.remove_if(std::mem_fn(&ValueFlow::Value::isImpossible)); + changeKnownToPossible(values); + } else { + valueFlowSetConditionToKnown(tok, values, true); + valueFlowSetConditionToKnown(tok, values, false); + } + } + if (values.empty()) + return; + forward(after, scope->bodyEnd, cond.vartok, values, tokenlist, settings); } }); } diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index f7fa2dd55..bfec5dfad 100755 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -1317,7 +1317,8 @@ private: " for (x = a; x < 50; x++) {}\n" " b = x;\n" "}\n"; - ASSERT_EQUALS("3,After for loop, x has value 50\n", + ASSERT_EQUALS("3,Assuming that condition 'x<50' is not redundant\n" + "3,Assuming that condition 'x<50' is not redundant\n", getErrorPathForX(code, 4U)); } @@ -6646,6 +6647,14 @@ private: "}\n"; ASSERT_EQUALS(true, testValueOfX(code, 11U, "d", 0)); ASSERT_EQUALS(false, testValueOfXImpossible(code, 11U, 0)); + + code = "void f(int * p, int len) {\n" + " for(int x = 0; x < len; ++x) {\n" + " p[x] = 1;\n" + " }\n" + "}\n"; + ASSERT_EQUALS(true, testValueOfX(code, 3U, "len", -1)); + ASSERT_EQUALS(true, testValueOfXImpossible(code, 3U, "len", 0)); } void valueFlowSymbolicIdentity()