From b1c6f2946a69362efc6b3bf73bd26bbb9a4a9083 Mon Sep 17 00:00:00 2001 From: Rikard Falkeborn Date: Sun, 9 Feb 2020 11:16:08 +0100 Subject: [PATCH] Fix redundant FP assignment with unsigned zero (#2521) * Refactor isNullOperand out of FwdAnalysis * Improve isNullOperand * Fix redundantAssignment FP with unsigned zero * isNullValue check number * Enhance isNullOperand to handle c++ casts Also handle cast of NULL. --- lib/astutils.cpp | 23 ++++++++++++++--------- lib/astutils.h | 3 +-- lib/checkfunctions.cpp | 2 +- lib/checkother.cpp | 3 ++- lib/checkunusedvar.cpp | 2 +- lib/mathlib.cpp | 2 ++ lib/symboldatabase.cpp | 2 +- test/testastutils.cpp | 22 ++++++++++++++++++++++ test/testmathlib.cpp | 1 + test/testother.cpp | 37 +++++++++++++++++++++++++++++++++++++ 10 files changed, 82 insertions(+), 15 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index f63339c86..c0f17766b 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -1547,6 +1547,20 @@ static bool isUnchanged(const Token *startToken, const Token *endToken, const st return true; } +bool isNullOperand(const Token *expr) +{ + if (!expr) + return false; + if (Token::Match(expr, "static_cast|const_cast|dynamic_cast|reinterpret_cast <")) + expr = expr->astParent(); + else if (!expr->isCast()) + return Token::Match(expr, "NULL|nullptr"); + if (expr->valueType() && expr->valueType()->pointer == 0) + return false; + const Token *castOp = expr->astOperand2() ? expr->astOperand2() : expr->astOperand1(); + return Token::Match(castOp, "NULL|nullptr") || (MathLib::isInt(castOp->str()) && MathLib::isNullValue(castOp->str())); +} + struct FwdAnalysis::Result FwdAnalysis::checkRecursive(const Token *expr, const Token *startToken, const Token *endToken, const std::set &exprVarIds, bool local, bool inInnerClass, int depth) { // Parse the given tokens @@ -2037,12 +2051,3 @@ bool FwdAnalysis::isEscapedAlias(const Token* expr) } return false; } - -bool FwdAnalysis::isNullOperand(const Token *expr) -{ - if (!expr) - return false; - if (Token::Match(expr, "( %name% %name%| * )") && Token::Match(expr->astOperand1(), "0|NULL|nullptr")) - return true; - return Token::Match(expr, "NULL|nullptr"); -} diff --git a/lib/astutils.h b/lib/astutils.h index 0653da27f..9b9b11fed 100644 --- a/lib/astutils.h +++ b/lib/astutils.h @@ -211,6 +211,7 @@ const Variable *getLHSVariable(const Token *tok); bool isScopeBracket(const Token* tok); +bool isNullOperand(const Token *expr); /** * Forward data flow analysis for checks * - unused value @@ -252,8 +253,6 @@ public: 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/checkfunctions.cpp b/lib/checkfunctions.cpp index a9111b389..c84000d9e 100644 --- a/lib/checkfunctions.cpp +++ b/lib/checkfunctions.cpp @@ -264,7 +264,7 @@ void CheckFunctions::checkMathFunctions() // fmod ( x , y) If y is zero, then either a range error will occur or the function will return zero (implementation-defined). else if (Token::Match(tok, "fmod|fmodf|fmodl (")) { const Token* nextArg = tok->tokAt(2)->nextArgument(); - if (nextArg && nextArg->isNumber() && MathLib::isNullValue(nextArg->str())) + if (nextArg && MathLib::isNullValue(nextArg->str())) mathfunctionCallWarning(tok, 2); } // pow ( x , y) If x is zero, and y is negative --> division by zero diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 0fad60ee0..8d3efe6e7 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -448,8 +448,9 @@ void CheckOther::checkRedundantAssignment() continue; } + const Token* rhs = tok->astOperand2(); // Do not warn about assignment with 0 / NULL - if (Token::simpleMatch(tok->astOperand2(), "0") || FwdAnalysis::isNullOperand(tok->astOperand2())) + if ((rhs && MathLib::isNullValue(rhs->str())) || isNullOperand(rhs)) continue; if (tok->astOperand1()->variable() && tok->astOperand1()->variable()->isReference()) diff --git a/lib/checkunusedvar.cpp b/lib/checkunusedvar.cpp index 12a75b452..59269cb18 100644 --- a/lib/checkunusedvar.cpp +++ b/lib/checkunusedvar.cpp @@ -1171,7 +1171,7 @@ void CheckUnusedVar::checkFunctionVariableUsage() continue; } // Do not warn about assignment with NULL - if (FwdAnalysis::isNullOperand(tok->astOperand2())) + if (isNullOperand(tok->astOperand2())) continue; if (!tok->astOperand1()) diff --git a/lib/mathlib.cpp b/lib/mathlib.cpp index f8a3b149a..d8f8d8b4b 100644 --- a/lib/mathlib.cpp +++ b/lib/mathlib.cpp @@ -1342,6 +1342,8 @@ bool MathLib::isNullValue(const std::string &str) if (str.empty() || (!std::isdigit(static_cast(str[0])) && (str[0] != '.' && str[0] != '-' && str[0] != '+'))) return false; // Has to be a number + if (!isInt(str) && !isFloat(str)) + return false; bool isHex = isIntHex(str) || isFloatHex(str); for (char i : str) { if (std::isdigit(static_cast(i)) && i != '0') // May not contain digits other than 0 diff --git a/lib/symboldatabase.cpp b/lib/symboldatabase.cpp index 4d888bad4..e603e8848 100644 --- a/lib/symboldatabase.cpp +++ b/lib/symboldatabase.cpp @@ -4570,7 +4570,7 @@ const Function* Scope::findFunction(const Token *tok, bool requireConst) const else if (funcarg->isPointer() && Token::Match(arguments[j], "nullptr|NULL ,|)")) same++; - else if (arguments[j]->isNumber() && funcarg->isPointer() && MathLib::isNullValue(arguments[j]->str())) + else if (funcarg->isPointer() && MathLib::isNullValue(arguments[j]->str())) fallback1++; // Try to evaluate the apparently more complex expression diff --git a/test/testastutils.cpp b/test/testastutils.cpp index 81ad7302f..e327bcc50 100644 --- a/test/testastutils.cpp +++ b/test/testastutils.cpp @@ -35,6 +35,7 @@ private: void run() OVERRIDE { TEST_CASE(findLambdaEndToken); TEST_CASE(findLambdaStartToken); + TEST_CASE(isNullOperand); TEST_CASE(isReturnScope); TEST_CASE(isSameExpression); TEST_CASE(isVariableChanged); @@ -106,6 +107,27 @@ private: ASSERT_EQUALS(true, findLambdaStartToken("[](void) constexpr -> const * const* int { return x; }")); } + bool isNullOperand(const char code[]) { + Settings settings; + Tokenizer tokenizer(&settings, this); + std::istringstream istr(code); + tokenizer.tokenize(istr, "test.cpp"); + return ::isNullOperand(tokenizer.tokens()); + } + + void isNullOperand() { + ASSERT_EQUALS(true, isNullOperand("(void*)0;")); + ASSERT_EQUALS(true, isNullOperand("(void*)0U;")); + ASSERT_EQUALS(true, isNullOperand("(void*)0x0LL;")); + ASSERT_EQUALS(true, isNullOperand("NULL;")); + ASSERT_EQUALS(true, isNullOperand("nullptr;")); + ASSERT_EQUALS(true, isNullOperand("(void*)NULL;")); + ASSERT_EQUALS(true, isNullOperand("static_cast(0);")); + ASSERT_EQUALS(false, isNullOperand("0;")); + ASSERT_EQUALS(false, isNullOperand("(void*)0.0;")); + ASSERT_EQUALS(false, isNullOperand("(void*)1;")); + } + bool isReturnScope(const char code[], int offset) { Settings settings; Tokenizer tokenizer(&settings, this); diff --git a/test/testmathlib.cpp b/test/testmathlib.cpp index d14e9ba01..fc4b0c67d 100644 --- a/test/testmathlib.cpp +++ b/test/testmathlib.cpp @@ -1123,6 +1123,7 @@ private: ASSERT_EQUALS(false, MathLib::isNullValue("x")); ASSERT_EQUALS(false, MathLib::isNullValue("garbage")); ASSERT_EQUALS(false, MathLib::isNullValue("UL")); + ASSERT_EQUALS(false, MathLib::isNullValue("-ENOMEM")); } void incdec() const { diff --git a/test/testother.cpp b/test/testother.cpp index a18a1e63f..806f62c88 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -166,6 +166,7 @@ private: TEST_CASE(incompleteArrayFill); TEST_CASE(redundantVarAssignment); + TEST_CASE(redundantVarAssignment_trivial); TEST_CASE(redundantVarAssignment_struct); TEST_CASE(redundantVarAssignment_7133); TEST_CASE(redundantVarAssignment_stackoverflow); @@ -6604,6 +6605,42 @@ private: "test.cpp:4:note:*var is overwritten\n", errout.str()); } + void redundantVarAssignment_trivial() { + check("void f() {\n" + " int a = 0;\n" + " a = 4;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void f() {\n" + " int a;\n" + " a = 0;\n" + " a = 4;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void f() {\n" + " unsigned a;\n" + " a = 0u;\n" + " a = 2u;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void f() {\n" + " void* a;\n" + " a = (void*)0;\n" + " a = p;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void f() {\n" + " void* a;\n" + " a = (void*)0U;\n" + " a = p;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } + void redundantVarAssignment_struct() { check("struct foo {\n" " int a,b;\n"