From 81f0597316e641cb2fea089476be88e3ab5218a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Thu, 24 Dec 2015 09:13:20 +0100 Subject: [PATCH] Fixed #3206 and #7226 (New check: Undefined execution order) --- lib/checkother.cpp | 56 ++++++++++++++++++++++++++++++++++++++++++++++ lib/checkother.h | 7 ++++++ lib/token.cpp | 3 ++- test/testother.cpp | 22 +++++++++++++----- 4 files changed, 81 insertions(+), 7 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 6c9f85ef4..eaae621bc 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -2481,3 +2481,59 @@ void CheckOther::unusedLabelError(const Token* tok) reportError(tok, Severity::style, "unusedLabel", "Label '" + (tok?tok->str():emptyString) + "' is not used."); } + + +void CheckOther::checkEvaluationOrder() +{ + const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase(); + const std::size_t functions = symbolDatabase->functionScopes.size(); + for (std::size_t i = 0; i < functions; ++i) { + const Scope * functionScope = symbolDatabase->functionScopes[i]; + for (const Token* tok = functionScope->classStart; tok != functionScope->classEnd; tok = tok->next()) { + if (!Token::Match(tok, "++|--") && !tok->isAssignmentOp()) + continue; + if (!tok->astOperand1()) + continue; + for (const Token *tok2 = tok; tok2 && tok2->astParent(); tok2 = tok2->astParent()) { + const Token * const parent = tok2->astParent(); + if (Token::Match(parent, "%oror%|&&|?|:|;")) + break; + if (parent->str() == ",") { + const Token *par = parent; + while (Token::simpleMatch(par,",")) + par = par->astParent(); + if (!par || par->str() != "(") + break; + } + + // Is expression used? + bool foundError = false; + std::stack tokens; + tokens.push((parent->astOperand1() != tok2) ? parent->astOperand1() : parent->astOperand2()); + while (!tokens.empty() && !foundError) { + const Token * const tok3 = tokens.top(); + tokens.pop(); + if (!tok3) + continue; + tokens.push(tok3->astOperand1()); + tokens.push(tok3->astOperand2()); + if (isSameExpression(_tokenizer->isCPP(), tok->astOperand1(), tok3, _settings->library.functionpure)) { + foundError = true; + } + } + + if (foundError) { + unknownEvaluationOrder(parent); + break; + } + } + } + } +} + +void CheckOther::unknownEvaluationOrder(const Token* tok) +{ + reportError(tok, Severity::error, "unknownEvaluationOrder", + "Expression '" + (tok ? tok->expressionString() : std::string("x = x++;")) + "' depends on order of evaluation of side effects"); +} + diff --git a/lib/checkother.h b/lib/checkother.h index 731147172..cf326c7e0 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -70,6 +70,7 @@ public: checkOther.checkZeroDivision(); checkOther.checkInterlockedDecrement(); checkOther.checkUnusedLabel(); + checkOther.checkEvaluationOrder(); } /** @brief Run checks against the simplified token list */ @@ -203,6 +204,9 @@ public: /** @brief %Check for unused labels */ void checkUnusedLabel(); + /** @brief %Check for expression that depends on order of evaluation of side effects */ + void checkEvaluationOrder(); + private: // Error messages.. void checkComparisonFunctionIsAlwaysTrueOrFalseError(const Token* tok, const std::string &strFunctionName, const std::string &varName, const bool result); @@ -251,6 +255,7 @@ private: void redundantPointerOpError(const Token* tok, const std::string& varname, bool inconclusive); void raceAfterInterlockedDecrementError(const Token* tok); void unusedLabelError(const Token* tok); + void unknownEvaluationOrder(const Token* tok); void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const { CheckOther c(0, settings, errorLogger); @@ -305,6 +310,7 @@ private: c.commaSeparatedReturnError(0); c.redundantPointerOpError(0, "varname", false); c.unusedLabelError(0); + c.unknownEvaluationOrder(0); } static std::string myName() { @@ -323,6 +329,7 @@ private: "- provide wrong dimensioned array to pipe() system command (--std=posix)\n" "- cast the return values of getc(),fgetc() and getchar() to character and compare it to EOF\n" "- race condition with non-interlocked access after InterlockedDecrement() call\n" + "- expression 'x = x++;' depends on order of evaluation of side effects\n" // warning "- either division by zero or useless condition\n" diff --git a/lib/token.cpp b/lib/token.cpp index 2fa3245ac..34d30a765 100644 --- a/lib/token.cpp +++ b/lib/token.cpp @@ -1142,7 +1142,8 @@ std::string Token::expressionString() const start = start->astOperand1(); const Token *end = top; while (end->astOperand1() && (end->astOperand2() || end->isUnaryPreOp())) { - if (Token::Match(end,"(|[")) { + if (Token::Match(end,"(|[") && + !(Token::Match(end, "( %type%") && !end->astOperand2())) { end = end->link(); break; } diff --git a/test/testother.cpp b/test/testother.cpp index e597e4744..16802c633 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -157,6 +157,8 @@ private: TEST_CASE(raceAfterInterlockedDecrement); TEST_CASE(testUnusedLabel); + + TEST_CASE(testEvaluationOrder); } void check(const char code[], const char *filename = nullptr, bool experimental = false, bool inconclusive = true, bool runSimpleChecks=true, Settings* settings = 0) { @@ -2884,12 +2886,6 @@ private: "}"); ASSERT_EQUALS("[test.cpp:3]: (warning) Redundant assignment of 'x' to itself.\n", errout.str()); - check("void foo()\n" - "{\n" - " std::string var = var = \"test\";\n" - "}"); - TODO_ASSERT_EQUALS("[test.cpp:3]: (warning) Redundant assignment of 'var' to itself.\n", "", errout.str()); - check("struct A { int b; };\n" "void foo(A* a1, A* a2) {\n" " a1->b = a1->b;\n" @@ -6102,6 +6098,20 @@ private: "}"); ASSERT_EQUALS("", errout.str()); } + + void testEvaluationOrder() { + check("void f() {\n" + " int x = dostuff();\n" + " return x + x++;\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (error) Expression 'x+x++' depends on order of evaluation of side effects\n", errout.str()); + + // #7226 + check("long int f1(const char *exp) {\n" + " return strtol(++exp, (char **)&exp, 10);\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (error) Expression '++exp,(char**)&exp' depends on order of evaluation of side effects\n", errout.str()); + } }; REGISTER_TEST(TestOther)