From 8c3f2c2ad96fc2c4bd14966cbf01e400c4f5e716 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Sun, 16 Mar 2014 08:38:52 +0100 Subject: [PATCH] Revert 894a65b0. abstract interpretation of for loops. there was some crashes and performance problems. I will fix those problems when I have time and recommit. --- lib/valueflow.cpp | 217 ++++++++++++------------------------- test/testbufferoverrun.cpp | 33 +++--- test/testvalueflow.cpp | 8 ++ 3 files changed, 95 insertions(+), 163 deletions(-) diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index dc31f4f28..feed600fd 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -609,171 +609,88 @@ static void valueFlowAfterAssign(TokenList *tokenlist, ErrorLogger *errorLogger, } } -static void execute(const Token *expr, - std::map * const programMemory, - MathLib::bigint *result, - bool *error) -{ - if (!expr) - *error = true; - - else if (expr->isNumber()) - *result = MathLib::toLongNumber(expr->str()); - - else if (expr->varId() > 0) { - const std::map::const_iterator var = programMemory->find(expr->varId()); - if (var == programMemory->end()) - *error = true; - else - *result = var->second; - } - - else if (expr->isComparisonOp()) { - MathLib::bigint result1, result2; - execute(expr->astOperand1(), programMemory, &result1, error); - execute(expr->astOperand2(), programMemory, &result2, error); - if (expr->str() == "<") - *result = result1 < result2; - else if (expr->str() == "<=") - *result = result1 <= result2; - else if (expr->str() == ">") - *result = result1 > result2; - else if (expr->str() == ">=") - *result = result1 >= result2; - else if (expr->str() == "==") - *result = result1 == result2; - else if (expr->str() == "!=") - *result = result1 != result2; - } - - else if (expr->str() == "=") { - execute(expr->astOperand2(), programMemory, result, error); - if (expr->astOperand1()->varId()) - (*programMemory)[expr->astOperand1()->varId()] = *result; - else - *error = true; - } - - else if (expr->str() == "++") { - if (!expr->astOperand1() || expr->astOperand1()->varId() == 0U) - *error = true; - else { - std::map::iterator var = programMemory->find(expr->astOperand1()->varId()); - if (var == programMemory->end()) - *error = true; - else { - *result = var->second + 1; - var->second = *result; - } - } - } - - else if (expr->str() == "&&") { - execute(expr->astOperand1(), programMemory, result, error); - if (*error || *result == 0) - *result = 0; - else { - execute(expr->astOperand2(), programMemory, result, error); - // If there is an error, assume the result is not important - if (*error) { - *error = false; - *result = 1; - } - } - } - - else if (expr->str() == "||") { - execute(expr->astOperand1(), programMemory, result, error); - if (*result == 0 && *error == false) - execute(expr->astOperand2(), programMemory, result, error); - } - - else - *error = true; -} - - static void valueFlowForLoop(TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings) { for (Token *tok = tokenlist->front(); tok; tok = tok->next()) { - if (!Token::simpleMatch(tok, "for (") || - !Token::simpleMatch(tok->next()->astOperand2(), ";") || - !Token::simpleMatch(tok->next()->astOperand2()->astOperand2(), ";")) + if (!Token::simpleMatch(tok, "for (")) continue; - const Token *firstExpression = tok->next()->astOperand2()->astOperand1(); - const Token *secondExpression = tok->next()->astOperand2()->astOperand2()->astOperand1(); - const Token *thirdExpression = tok->next()->astOperand2()->astOperand2()->astOperand2(); - tok = tok->linkAt(1); - - std::map programMemory; - MathLib::bigint result; - bool error = false; - execute(firstExpression, &programMemory, &result, &error); - if (error) + tok = tok->tokAt(2); + if (!Token::Match(tok,"%type%| %var% = %num% ;")) { // TODO: don't use %num% + if (settings->debugwarnings) + bailout(tokenlist, errorLogger, tok, "For loop not handled"); continue; - execute(secondExpression, &programMemory, &result, &error); - if (error) + } + Token * const vartok = tok->tokAt(Token::Match(tok, "%var% =") ? 0 : 1); + const MathLib::bigint num1 = MathLib::toLongNumber(vartok->strAt(2)); + if (vartok->varId() == 0U) + continue; + tok = vartok->tokAt(4); + const Token *num2tok = nullptr; + if (Token::Match(tok, "%varid% <|<=|!=", vartok->varId())) { + tok = tok->next(); + num2tok = tok->astOperand2(); + if (num2tok && num2tok->str() == "(" && !num2tok->astOperand2()) + num2tok = num2tok->astOperand1(); + if (!Token::Match(num2tok, "%num%")) + num2tok = 0; + } + if (!num2tok) { + if (settings->debugwarnings) + bailout(tokenlist, errorLogger, tok, "For loop not handled"); + continue; + } + const MathLib::bigint num2 = MathLib::toLongNumber(num2tok ? num2tok->str() : "0") - ((tok->str()=="<=") ? 0 : 1); + while (tok && tok->str() != ";") + tok = tok->next(); + if (!num2tok || !Token::Match(tok, "; %varid% ++ ) {", vartok->varId())) { + if (settings->debugwarnings) + bailout(tokenlist, errorLogger, tok, "For loop not handled"); continue; - const std::map startMemory(programMemory); - std::map endMemory; - - unsigned int maxcount = 10000; - while (result != 0 && !error && --maxcount) { - endMemory = programMemory; - execute(thirdExpression, &programMemory, &result, &error); - if (!error) - execute(secondExpression, &programMemory, &result, &error); } - Token * const bodyStart = tok->next(); - const Token * const bodyEnd = bodyStart->link(); + Token * const bodyStart = tok->tokAt(4); + const Token * const bodyEnd = bodyStart->link(); - for (std::map::const_iterator it = startMemory.begin(); it != startMemory.end(); ++it) { - const unsigned int varid = it->first; - const MathLib::bigint num1 = it->second; - const MathLib::bigint num2 = endMemory[varid]; - - // Is variable modified inside for loop - bool modified = false; - for (const Token *tok2 = bodyStart->next(); tok2 != bodyEnd; tok2 = tok2->next()) { - if (Token::Match(tok2, "%varid% =", varid)) { - modified = true; - break; - } + // Is variable modified inside for loop + bool modified = false; + for (const Token *tok2 = bodyStart->next(); tok2 != bodyEnd; tok2 = tok2->next()) { + if (Token::Match(tok2, "%varid% =", vartok->varId())) { + modified = true; + break; } - if (modified) - continue; + } + if (modified) + continue; - for (Token *tok2 = bodyStart->next(); tok2 != bodyEnd; tok2 = tok2->next()) { - if (tok2->varId() == varid) { - const Token * parent = tok2->astParent(); - while (parent) { - const Token * const p = parent; - parent = parent->astParent(); - if (parent && parent->str() == "?" && parent->astOperand2() == p) - break; - } - if (parent) { - if (settings->debugwarnings) - bailout(tokenlist, errorLogger, tok2, "For loop variable " + tok2->str() + " stopping on ?"); - continue; - } - - ValueFlow::Value value1(num1); - value1.varId = tok2->varId(); - setTokenValue(tok2, value1); - - ValueFlow::Value value2(num2); - value2.varId = tok2->varId(); - setTokenValue(tok2, value2); + for (Token *tok2 = bodyStart->next(); tok2 != bodyEnd; tok2 = tok2->next()) { + if (tok2->varId() == vartok->varId()) { + const Token * parent = tok2->astParent(); + while (parent) { + const Token * const p = parent; + parent = parent->astParent(); + if (parent && parent->str() == "?" && parent->astOperand2() == p) + break; } - - if (tok2->str() == "{") { + if (parent) { if (settings->debugwarnings) - bailout(tokenlist, errorLogger, tok2, "For loop variable " + tok2->str() + " stopping on {"); - break; + bailout(tokenlist, errorLogger, tok2, "For loop variable " + vartok->str() + " stopping on ?"); + continue; } + + ValueFlow::Value value1(num1); + value1.varId = tok2->varId(); + setTokenValue(tok2, value1); + + ValueFlow::Value value2(num2); + value2.varId = tok2->varId(); + setTokenValue(tok2, value2); + } + + if (tok2->str() == "{") { + if (settings->debugwarnings) + bailout(tokenlist, errorLogger, tok2, "For loop variable " + vartok->str() + " stopping on {"); + break; } } } diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp index 57a2c34c5..8882e1fad 100644 --- a/test/testbufferoverrun.cpp +++ b/test/testbufferoverrun.cpp @@ -130,6 +130,7 @@ private: TEST_CASE(array_index_negative2); // ticket #3063 TEST_CASE(array_index_for_decr); TEST_CASE(array_index_varnames); // FP: struct member. #1576 + TEST_CASE(array_index_for_break); // FP: for,break TEST_CASE(array_index_for_continue); // for,continue TEST_CASE(array_index_for); // FN: for,if TEST_CASE(array_index_for_neq); // #2211: Using != in condition @@ -457,8 +458,7 @@ private: " a[i] = 0;\n" " }\n" "}"); - ASSERT_EQUALS("[test.cpp:3]: (error) Buffer is accessed out of bounds: a\n" - "[test.cpp:3]: (error) Array 'a[10]' accessed at index 49, which is out of bounds.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (error) Buffer is accessed out of bounds: a\n", errout.str()); } } @@ -1045,8 +1045,7 @@ private: " for (int i = 3; 0 <= i; i--)\n" " a[i] = i;\n" "}"); - ASSERT_EQUALS("[test.cpp:5]: (error) Buffer is accessed out of bounds: a\n" - "[test.cpp:5]: (error) Array 'a[3]' accessed at index 3, which is out of bounds.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:5]: (error) Buffer is accessed out of bounds: a\n", errout.str()); check("void f()\n" "{\n" @@ -1767,8 +1766,7 @@ private: " data[i] = 0;\n" " }\n" "}"); - ASSERT_EQUALS("[test.cpp:5]: (error) Buffer is accessed out of bounds: data\n" - "[test.cpp:5]: (error) Array 'data[8]' accessed at index 10, which is out of bounds.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:5]: (error) Buffer is accessed out of bounds: data\n", errout.str()); check("void f()\n" "{\n" @@ -1846,6 +1844,19 @@ private: ASSERT_EQUALS("[test.cpp:5]: (error) Buffer is accessed out of bounds: data\n", errout.str()); } + void array_index_for_break() { + check("void f() {\n" + " int a[2];\n" + " for (int i = 0; i <= 2; ++i) {\n" + " a[i] = 0;\n" + " if (i==1) {\n" + " break;\n" + " }\n" + " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } + void array_index_for_continue() { // #3913 check("void f() {\n" @@ -1913,9 +1924,7 @@ private: " a[i] = 0;\n" " }\n" "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) Buffer is accessed out of bounds: a\n" - "[test.cpp:4]: (error) Array 'a[5]' accessed at index 9, which is out of bounds.\n", - errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (error) Buffer is accessed out of bounds: a\n", errout.str()); } void array_index_for_question() { @@ -2265,8 +2274,7 @@ private: " for (i = 0; i <= 10; ++i)\n" " a[i] = 0;\n" "}"); - ASSERT_EQUALS("[test.cpp:7]: (error) Buffer is accessed out of bounds: a\n" - "[test.cpp:7]: (error) Array 'a[10]' accessed at index 10, which is out of bounds.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:7]: (error) Buffer is accessed out of bounds: a\n", errout.str()); } @@ -2277,8 +2285,7 @@ private: " for (int i = 0; i < 8; ++i)\n" " p[i] = 0;\n" "}"); - ASSERT_EQUALS("[test.cpp:5]: (error) Buffer is accessed out of bounds: p\n" - "[test.cpp:5]: (error) Array 'p[2]' accessed at index 7, which is out of bounds.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:5]: (error) Buffer is accessed out of bounds: p\n", errout.str()); // No false positive check("void foo(int x, int y)\n" diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index 2f15a228f..db65c38e8 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -617,6 +617,14 @@ private: ASSERT_EQUALS(true, testValueOfX(code, 3U, 9)); ASSERT_EQUALS(false, testValueOfX(code, 3U, 10)); + code = "void f() {\n" + " for (int x = 0; x < ((short)10); x++)\n" + " a[x] = 0;\n" + "}"; + ASSERT_EQUALS(true, testValueOfX(code, 3U, 0)); + ASSERT_EQUALS(true, testValueOfX(code, 3U, 9)); + ASSERT_EQUALS(false, testValueOfX(code, 3U, 10)); + code = "void f() {\n" " for (int x = 0; x < 10; x++)\n" " x<4 ?\n"