From f1cc3ada86bc0c83a6c55f5737d2fca87d949ac6 Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Thu, 21 Jan 2021 13:18:53 -0600 Subject: [PATCH] Refactor valueFlowTerminatingCondition to handle inner conditions and complex conditions (#3060) --- lib/checkcondition.cpp | 8 +-- lib/valueflow.cpp | 114 +++++++++++++++++++---------------- test/testcondition.cpp | 14 ++--- test/testsimplifytypedef.cpp | 4 +- test/testsymboldatabase.cpp | 2 +- test/testvalueflow.cpp | 74 ++++++++++++++++++++--- 6 files changed, 139 insertions(+), 77 deletions(-) diff --git a/lib/checkcondition.cpp b/lib/checkcondition.cpp index e15a6c74c..ecf2c4a9b 100644 --- a/lib/checkcondition.cpp +++ b/lib/checkcondition.cpp @@ -507,14 +507,12 @@ void CheckCondition::multiCondition() break; tok2 = tok2->tokAt(4); - if (tok2->astOperand2() && - !cond1->hasKnownIntValue() && - !tok2->astOperand2()->hasKnownIntValue()) { + if (tok2->astOperand2()) { ErrorPath errorPath; if (isOverlappingCond(cond1, tok2->astOperand2(), true)) - overlappingElseIfConditionError(tok2, cond1->linenr()); + overlappingElseIfConditionError(tok2->astOperand2(), cond1->linenr()); else if (isOppositeCond(true, mTokenizer->isCPP(), cond1, tok2->astOperand2(), mSettings->library, true, true, &errorPath)) - oppositeElseIfConditionError(cond1, tok2, errorPath); + oppositeElseIfConditionError(cond1, tok2->astOperand2(), errorPath); } } } diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 14e515168..42d4477d3 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -1427,37 +1427,6 @@ static void valueFlowRightShift(TokenList *tokenList, const Settings* settings) } } -static void valueFlowOppositeCondition(SymbolDatabase *symboldatabase, const Settings *settings) -{ - for (const Scope &scope : symboldatabase->scopeList) { - if (scope.type != Scope::eIf) - continue; - Token *tok = const_cast(scope.classDef); - if (!Token::simpleMatch(tok, "if (")) - continue; - const Token *cond1 = tok->next()->astOperand2(); - if (!cond1 || !cond1->isComparisonOp()) - continue; - const bool cpp = symboldatabase->isCPP(); - Token *tok2 = tok->linkAt(1); - while (Token::simpleMatch(tok2, ") {")) { - tok2 = tok2->linkAt(1); - if (!Token::simpleMatch(tok2, "} else { if (")) - break; - Token *ifOpenBraceTok = tok2->tokAt(4); - Token *cond2 = ifOpenBraceTok->astOperand2(); - if (!cond2 || !cond2->isComparisonOp()) - continue; - if (isOppositeCond(true, cpp, cond1, cond2, settings->library, true, true)) { - ValueFlow::Value value(1); - value.setKnown(); - setTokenValue(cond2, value, settings); - } - tok2 = ifOpenBraceTok->link(); - } - } -} - static void valueFlowEnumValue(SymbolDatabase * symboldatabase, const Settings * settings) { @@ -3756,7 +3725,19 @@ static const Token* findIncompleteVar(const Token* start, const Token* end) return nullptr; } -static void valueFlowTerminatingCondition(TokenList *tokenlist, SymbolDatabase* symboldatabase, ErrorLogger *errorLogger, const Settings *settings) +ValueFlow::Value makeConditionValue(long long val, const Token* condTok, bool assume) +{ + ValueFlow::Value v(val); + v.setKnown(); + v.condition = condTok; + if (assume) + v.errorPath.emplace_back(condTok, "Assuming condition '" + condTok->expressionString() + "' is true"); + else + v.errorPath.emplace_back(condTok, "Assuming condition '" + condTok->expressionString() + "' is false"); + return v; +} +// +static void valueFlowConditionExpressions(TokenList *tokenlist, SymbolDatabase* symboldatabase, ErrorLogger *errorLogger, const Settings *settings) { for (const Scope * scope : symboldatabase->functionScopes) { if (const Token* incompleteTok = findIncompleteVar(scope->bodyStart, scope->bodyEnd)) { @@ -3773,28 +3754,58 @@ static void valueFlowTerminatingCondition(TokenList *tokenlist, SymbolDatabase* // Skip known values if (tok->next()->hasKnownValue()) continue; - const Token * parenTok = tok->next(); + Token * parenTok = tok->next(); if (!Token::simpleMatch(parenTok->link(), ") {")) continue; - const Token * blockTok = parenTok->link()->tokAt(1); - // Check if the block terminates early - if (!isEscapeScope(blockTok, tokenlist)) - continue; - + Token * blockTok = parenTok->link()->tokAt(1); const Token* condTok = parenTok->astOperand2(); - ValueFlow::Value v1(0); - v1.setKnown(); - v1.condition = condTok; - v1.errorPath.emplace_back(condTok, "Assuming condition '" + condTok->expressionString() + "' is true"); - ExpressionAnalyzer a1(condTok, v1, tokenlist); - valueFlowGenericForward(blockTok->link()->next(), scope->bodyEnd, a1, settings); - ValueFlow::Value v2(1); - v2.setKnown(); - v2.condition = condTok; - v2.errorPath.emplace_back(condTok, "Assuming condition '" + condTok->expressionString() + "' is false"); - OppositeExpressionAnalyzer a2(true, condTok, v2, tokenlist); - valueFlowGenericForward(blockTok->link()->next(), scope->bodyEnd, a2, settings); + Token* startTok = blockTok; + // Inner condition + { + std::vector conds = {condTok}; + if (Token::simpleMatch(condTok, "&&")) { + std::vector args = astFlatten(condTok, "&&"); + conds.insert(conds.end(), args.begin(), args.end()); + } + for(const Token* condTok2:conds) { + ExpressionAnalyzer a1(condTok2, makeConditionValue(1, condTok2, true), tokenlist); + valueFlowGenericForward(startTok, startTok->link(), a1, settings); + + OppositeExpressionAnalyzer a2(true, condTok2, makeConditionValue(0, condTok2, true), tokenlist); + valueFlowGenericForward(startTok, startTok->link(), a2, settings); + } + } + + std::vector conds = {condTok}; + if (Token::simpleMatch(condTok, "||")) { + std::vector args = astFlatten(condTok, "||"); + conds.insert(conds.end(), args.begin(), args.end()); + } + + // Check else block + if (Token::simpleMatch(startTok->link(), "} else {")) { + startTok = startTok->link()->tokAt(2); + for(const Token* condTok2:conds) { + ExpressionAnalyzer a1(condTok2, makeConditionValue(0, condTok2, false), tokenlist); + valueFlowGenericForward(startTok, startTok->link(), a1, settings); + + OppositeExpressionAnalyzer a2(true, condTok2, makeConditionValue(1, condTok2, false), tokenlist); + valueFlowGenericForward(startTok, startTok->link(), a2, settings); + } + } + + // Check if the block terminates early + if (isEscapeScope(blockTok, tokenlist)) { + for(const Token* condTok2:conds) { + ExpressionAnalyzer a1(condTok2, makeConditionValue(0, condTok2, false), tokenlist); + valueFlowGenericForward(startTok->link()->next(), scope->bodyEnd, a1, settings); + + OppositeExpressionAnalyzer a2(true, condTok2, makeConditionValue(1, condTok2, false), tokenlist); + valueFlowGenericForward(startTok->link()->next(), scope->bodyEnd, a2, settings); + } + } + } } } @@ -6624,6 +6635,7 @@ void ValueFlow::setValues(TokenList *tokenlist, SymbolDatabase* symboldatabase, valueFlowLifetime(tokenlist, symboldatabase, errorLogger, settings); valueFlowBitAnd(tokenlist); valueFlowSameExpressions(tokenlist); + valueFlowConditionExpressions(tokenlist, symboldatabase, errorLogger, settings); valueFlowFwdAnalysis(tokenlist, settings); std::size_t values = 0; @@ -6633,8 +6645,6 @@ void ValueFlow::setValues(TokenList *tokenlist, SymbolDatabase* symboldatabase, valueFlowPointerAliasDeref(tokenlist); valueFlowArrayBool(tokenlist); valueFlowRightShift(tokenlist, settings); - valueFlowOppositeCondition(symboldatabase, settings); - valueFlowTerminatingCondition(tokenlist, symboldatabase, errorLogger, settings); valueFlowAfterMove(tokenlist, symboldatabase, errorLogger, settings); valueFlowCondition(SimpleConditionHandler{}, tokenlist, symboldatabase, errorLogger, settings); valueFlowInferCondition(tokenlist, settings); diff --git a/test/testcondition.cpp b/test/testcondition.cpp index d4beb476c..e7cde3c7b 100644 --- a/test/testcondition.cpp +++ b/test/testcondition.cpp @@ -533,27 +533,27 @@ private: " if (a) { b = 1; }\n" " else { if (a) { b = 2; } }\n" "}"); - ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Condition 'a' is always false\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (style) Expression is always false because 'else if' condition matches previous condition at line 2.\n", errout.str()); check("void f(int a, int &b) {\n" " if (a) { b = 1; }\n" " else { if (a) { b = 2; } }\n" "}"); - ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Condition 'a' is always false\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (style) Expression is always false because 'else if' condition matches previous condition at line 2.\n", errout.str()); check("void f(int a, int &b) {\n" " if (a == 1) { b = 1; }\n" " else { if (a == 2) { b = 2; }\n" " else { if (a == 1) { b = 3; } } }\n" "}"); - ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:4]: (style) Condition 'a==1' is always false\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (style) Expression is always false because 'else if' condition matches previous condition at line 2.\n", errout.str()); check("void f(int a, int &b) {\n" " if (a == 1) { b = 1; }\n" " else { if (a == 2) { b = 2; }\n" " else { if (a == 2) { b = 3; } } }\n" "}"); - ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (style) Condition 'a==2' is always false\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (style) Expression is always false because 'else if' condition matches previous condition at line 3.\n", errout.str()); check("void f(int a, int &b) {\n" " if (a++) { b = 1; }\n" @@ -721,9 +721,9 @@ private: " if (x) {}\n" " else if (!x) {}\n" "}"); - ASSERT_EQUALS("test.cpp:3:style:Condition '!x' is always true\n" - "test.cpp:2:note:condition 'x'\n" - "test.cpp:3:note:Condition '!x' is always true\n", errout.str()); + ASSERT_EQUALS("test.cpp:3:style:Expression is always true because 'else if' condition is opposite to previous condition at line 2.\n" + "test.cpp:2:note:first condition\n" + "test.cpp:3:note:else if condition is opposite to first condition\n", errout.str()); check("void f(int x) {\n" " int y = x;\n" diff --git a/test/testsimplifytypedef.cpp b/test/testsimplifytypedef.cpp index 8a1847329..8dff6116d 100644 --- a/test/testsimplifytypedef.cpp +++ b/test/testsimplifytypedef.cpp @@ -1670,7 +1670,7 @@ private: "( ( int ( * * ( * ) ( char * , char * , int , int ) ) ( ) ) global [ 6 ] ) ( \"assoc\" , \"eggdrop\" , 106 , 0 ) ; " "}"; ASSERT_EQUALS(expected, tok(code)); - ASSERT_EQUALS_WITHOUT_LINENUMBERS("[test.cpp:3]: (debug) valueflow.cpp:1319:valueFlowTerminatingCondition bailout: Skipping function due to incomplete variable global\n", errout.str()); + ASSERT_EQUALS_WITHOUT_LINENUMBERS("[test.cpp:3]: (debug) valueflow.cpp:1319:valueFlowConditionExpressions bailout: Skipping function due to incomplete variable global\n", errout.str()); } void simplifyTypedef68() { // ticket #2355 @@ -1877,7 +1877,7 @@ private: " B * b = new B;\n" " b->f = new A::F * [ 10 ];\n" "}"); - ASSERT_EQUALS_WITHOUT_LINENUMBERS("[test.cpp:12]: (debug) valueflow.cpp:1319:valueFlowTerminatingCondition bailout: Skipping function due to incomplete variable idx\n", errout.str()); + ASSERT_EQUALS_WITHOUT_LINENUMBERS("[test.cpp:12]: (debug) valueflow.cpp:1319:valueFlowConditionExpressions bailout: Skipping function due to incomplete variable idx\n", errout.str()); } void simplifyTypedef83() { // ticket #2620 diff --git a/test/testsymboldatabase.cpp b/test/testsymboldatabase.cpp index 1b57f3ca6..90a019233 100644 --- a/test/testsymboldatabase.cpp +++ b/test/testsymboldatabase.cpp @@ -2743,7 +2743,7 @@ private: check("::y(){x}"); ASSERT_EQUALS_WITHOUT_LINENUMBERS("[test.cpp:1]: (debug) Executable scope 'y' with unknown function.\n" - "[test.cpp:1]: (debug) valueflow.cpp:1321:valueFlowTerminatingCondition bailout: Skipping function due to incomplete variable x\n", errout.str()); + "[test.cpp:1]: (debug) valueflow.cpp:1321:valueFlowConditionExpressions bailout: Skipping function due to incomplete variable x\n", errout.str()); } void symboldatabase20() { diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index d67fcf663..8415c4382 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -122,7 +122,7 @@ private: TEST_CASE(valueFlowUninit); - TEST_CASE(valueFlowTerminatingCond); + TEST_CASE(valueFlowConditionExpressions); TEST_CASE(valueFlowContainerSize); @@ -1266,7 +1266,7 @@ private: " if (x == 123) {}\n" "}"); ASSERT_EQUALS_WITHOUT_LINENUMBERS( - "[test.cpp:2]: (debug) valueflow.cpp::valueFlowTerminatingCondition bailout: Skipping function due to incomplete variable y\n", + "[test.cpp:2]: (debug) valueflow.cpp::valueFlowConditionExpressions bailout: Skipping function due to incomplete variable y\n", errout.str()); } @@ -1383,7 +1383,7 @@ private: " y = ((x<0) ? x : ((x==2)?3:4));\n" "}"); ASSERT_EQUALS_WITHOUT_LINENUMBERS( - "[test.cpp:2]: (debug) valueflow.cpp::valueFlowTerminatingCondition bailout: Skipping function due to incomplete variable y\n", + "[test.cpp:2]: (debug) valueflow.cpp::valueFlowConditionExpressions bailout: Skipping function due to incomplete variable y\n", errout.str()); bailout("int f(int x) {\n" @@ -1448,7 +1448,7 @@ private: " if (x == 123) {}\n" "}"); ASSERT_EQUALS_WITHOUT_LINENUMBERS( - "[test.cpp:2]: (debug) valueflow.cpp::valueFlowTerminatingCondition bailout: Skipping function due to incomplete variable b\n", + "[test.cpp:2]: (debug) valueflow.cpp::valueFlowConditionExpressions bailout: Skipping function due to incomplete variable b\n", errout.str()); code = "void f(int x, bool abc) {\n" @@ -1497,7 +1497,7 @@ private: " };\n" "}"); ASSERT_EQUALS_WITHOUT_LINENUMBERS( - "[test.cpp:3]: (debug) valueflow.cpp::valueFlowTerminatingCondition bailout: Skipping function due to incomplete variable a\n", + "[test.cpp:3]: (debug) valueflow.cpp::valueFlowConditionExpressions bailout: Skipping function due to incomplete variable a\n", errout.str()); bailout("void f(int x, int y) {\n" @@ -1507,7 +1507,7 @@ private: " };\n" "}"); ASSERT_EQUALS_WITHOUT_LINENUMBERS( - "[test.cpp:3]: (debug) valueflow.cpp::valueFlowTerminatingCondition bailout: Skipping function due to incomplete variable a\n", + "[test.cpp:3]: (debug) valueflow.cpp::valueFlowConditionExpressions bailout: Skipping function due to incomplete variable a\n", errout.str()); } @@ -1519,7 +1519,7 @@ private: " M;\n" "}"); ASSERT_EQUALS_WITHOUT_LINENUMBERS( - "[test.cpp:3]: (debug) valueflow.cpp::valueFlowTerminatingCondition bailout: Skipping function due to incomplete variable a\n" + "[test.cpp:3]: (debug) valueflow.cpp::valueFlowConditionExpressions bailout: Skipping function due to incomplete variable a\n" "[test.cpp:4]: (debug) valueflow.cpp:1260:(valueFlow) bailout: variable 'x', condition is defined in macro\n", errout.str()); @@ -1529,7 +1529,7 @@ private: " FREE(x);\n" "}"); ASSERT_EQUALS_WITHOUT_LINENUMBERS( - "[test.cpp:3]: (debug) valueflow.cpp::valueFlowTerminatingCondition bailout: Skipping function due to incomplete variable a\n" + "[test.cpp:3]: (debug) valueflow.cpp::valueFlowConditionExpressions bailout: Skipping function due to incomplete variable a\n" "[test.cpp:4]: (debug) valueflow.cpp:1260:(valueFlow) bailout: variable 'x', condition is defined in macro\n", errout.str()); } @@ -1543,7 +1543,7 @@ private: " 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:3]: (debug) valueflow.cpp::valueFlowConditionExpressions bailout: Skipping function due to incomplete variable a\n", errout.str()); // #5721 - FP @@ -4202,7 +4202,7 @@ private: ASSERT_EQUALS(0, values.size()); } - void valueFlowTerminatingCond() { + void valueFlowConditionExpressions() { const char* code; // opposite condition @@ -4272,6 +4272,60 @@ private: "}\n"; ASSERT_EQUALS(false, valueOfTok(code, "!=").intvalue == 1); + code = "void f(bool b, int i, int j) {\n" + " if (b || i == j) return;\n" + " if(i != j) {}\n" + "}\n"; + ASSERT_EQUALS(true, valueOfTok(code, "!=").intvalue == 1); + + code = "void f(bool b, int i, int j) {\n" + " if (b && i == j) return;\n" + " if(i != j) {}\n" + "}\n"; + ASSERT_EQUALS(true, tokenValues(code, "!=").empty()); + + code = "void f(int i, int j) {\n" + " if (i == j) {\n" + " if (i != j) {}\n" + " }\n" + "}\n"; + ASSERT_EQUALS(true, valueOfTok(code, "!=").intvalue == 0); + + code = "void f(int i, int j) {\n" + " if (i == j) {} else {\n" + " if (i != j) {}\n" + " }\n" + "}\n"; + ASSERT_EQUALS(true, valueOfTok(code, "!=").intvalue == 1); + + code = "void f(bool b, int i, int j) {\n" + " if (b && i == j) {\n" + " if (i != j) {}\n" + " }\n" + "}\n"; + ASSERT_EQUALS(true, valueOfTok(code, "!=").intvalue == 0); + + code = "void f(bool b, int i, int j) {\n" + " if (b || i == j) {\n" + " if (i != j) {}\n" + " }\n" + "}\n"; + ASSERT_EQUALS(true, tokenValues(code, "!=").empty()); + + code = "void f(bool b, int i, int j) {\n" + " if (b || i == j) {} else {\n" + " if (i != j) {}\n" + " }\n" + "}\n"; + ASSERT_EQUALS(true, valueOfTok(code, "!=").intvalue == 1); + + code = "void f(bool b, int i, int j) {\n" + " if (b && i == j) {} else {\n" + " if (i != j) {}\n" + " }\n" + "}\n"; + ASSERT_EQUALS(true, tokenValues(code, "!=").empty()); + code = "void foo()\n" // #8924 "{\n" " if ( this->FileIndex >= 0 )\n"