From 9f6890512c78cfa975e5f157fa293435903940ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Thu, 31 Dec 2015 12:05:23 +0100 Subject: [PATCH] Refactoring CheckType checkers. Use ValueType. --- lib/checktype.cpp | 178 ++++++++++++---------------------------------- test/testtype.cpp | 11 +-- 2 files changed, 51 insertions(+), 138 deletions(-) diff --git a/lib/checktype.cpp b/lib/checktype.cpp index 51419acf0..63e0de35b 100644 --- a/lib/checktype.cpp +++ b/lib/checktype.cpp @@ -30,82 +30,6 @@ namespace { CheckType instance; } -static bool astIsIntResult(const Token *tok) -{ - if (!tok) - return false; - if (tok->astOperand1()) - return astIsIntResult(tok->astOperand1()) && astIsIntResult(tok->astOperand2()); - // todo: handle numbers - if (!tok->variable()) - return false; - const Token *type = tok->variable()->typeStartToken(); - return Token::Match(type, "char|short|int") && !type->isLong(); -} - -static bool astGetSizeSign(const Settings *settings, const Token *tok, unsigned int *size, char *sign) -{ - if (!tok) - return false; - if (tok->isArithmeticalOp()) { - if (!astGetSizeSign(settings, tok->astOperand1(), size, sign)) - return false; - return !tok->astOperand2() || astGetSizeSign(settings, tok->astOperand2(), size, sign); - } - if (tok->isNumber() && MathLib::isInt(tok->str())) { - if (tok->str().find('L') != std::string::npos) - return false; - MathLib::bigint value = MathLib::toLongNumber(tok->str()); - unsigned int sz; - if (value >= -(1<<7) && value <= (1<<7)-1) - sz = 8; - else if (value >= -(1<<15) && value <= (1<<15)-1) - sz = 16; - else if (value >= -(1LL<<31) && value <= (1LL<<31)-1) - sz = 32; - else - return false; - if (sz < 8 * settings->sizeof_int) - sz = 8 * settings->sizeof_int; - if (*size < sz) - *size = sz; - if (tok->str().find('U') != std::string::npos) - *sign = 'u'; - if (*sign != 'u') - *sign = 's'; - return true; - } - if (tok->isName()) { - const Variable *var = tok->variable(); - if (!var) - return false; - unsigned int sz = 0; - for (const Token *type = var->typeStartToken(); type; type = type->next()) { - if (type->str() == "*") - return false; // <- FIXME: handle pointers - if (Token::Match(type, "char|short|int")) { - sz = 8 * settings->sizeof_int; - if (type->isUnsigned()) - *sign = 'u'; - else if (*sign != 'u') - *sign = 's'; - } else if (Token::Match(type, "float|double|long")) { - return false; - } else { - // TODO: try to lookup type info in library - } - if (type == var->typeEndToken()) - break; - } - if (sz == 0) - return false; - if (*size < sz) - *size = sz; - return true; - } - return false; -} - //--------------------------------------------------------------------------- // Checking for shift by too many bits //--------------------------------------------------------------------------- @@ -124,7 +48,7 @@ void CheckType::checkTooBigBitwiseShift() for (std::size_t i = 0; i < functions; ++i) { const Scope * scope = symbolDatabase->functionScopes[i]; for (const Token* tok = scope->classStart->next(); tok != scope->classEnd; tok = tok->next()) { - if (tok->str() != "<<" && tok->str() != ">>") + if (!Token::Match(tok, "<<|>>|<<=|>>=")) continue; if (!tok->astOperand1() || !tok->astOperand2()) @@ -198,15 +122,10 @@ void CheckType::checkIntegerOverflow() if (!value) continue; - // get size and sign of result.. - unsigned int size = 0; - char sign = 0; - if (!astGetSizeSign(_settings, tok, &size, &sign)) - continue; - if (sign != 's') // only signed integer overflow is UB - continue; - - integerOverflowError(tok, *value); + // is result signed integer? + const ValueType *vt = tok->valueType(); + if (vt && vt->type == ValueType::Type::INT && vt->sign == ValueType::Sign::SIGNED) + integerOverflowError(tok, *value); } } } @@ -214,14 +133,18 @@ void CheckType::checkIntegerOverflow() void CheckType::integerOverflowError(const Token *tok, const ValueFlow::Value &value) { const std::string expr(tok ? tok->expressionString() : ""); - const std::string cond(value.condition ? - ". See condition at line " + MathLib::toString(value.condition->linenr()) + "." : - ""); + + std::string msg; + if (value.condition) + msg = ValueFlow::eitherTheConditionIsRedundant(value.condition) + + " or there is signed integer overflow for expression '" + expr + "'."; + else + msg = "Signed integer overflow for expression '" + expr + "'."; reportError(tok, value.condition ? Severity::warning : Severity::error, "integerOverflow", - "Signed integer overflow for expression '"+expr+"'"+cond, + msg, 0U, value.inconclusive); } @@ -243,15 +166,11 @@ void CheckType::checkSignConversion() if (!tok->isArithmeticalOp() || Token::Match(tok,"+|-")) continue; - unsigned int size = 0; - char sign = 0; - if (!astGetSizeSign(_settings, tok, &size, &sign)) + // Is result unsigned? + if (!(tok->valueType() && tok->valueType()->sign == ValueType::Sign::UNSIGNED)) continue; - if (sign != 'u') - continue; - - // Check if there are signed operands that can be negative.. + // Check if an operand can be negative.. std::stack tokens; tokens.push(tok->astOperand1()); tokens.push(tok->astOperand2()); @@ -260,30 +179,10 @@ void CheckType::checkSignConversion() tokens.pop(); if (!tok1) continue; - if (tok1->str() == "(") - continue; // Todo: properly handle casts, function calls, etc - const Variable *var = tok1->variable(); - if (var && tok1->getValueLE(-1,_settings)) { - bool signedvar = true; // assume that variable is signed since it can have a negative value - for (const Token *type = var->typeStartToken();; type = type->next()) { - if (type->isUnsigned()) { - signedvar = false; - break; - } - if (type->isSigned()) - break; - if (type->isName() && !Token::Match(type, "char|short|int|long|const")) { - signedvar = false; - break; - } - if (type == var->typeEndToken()) - break; - } - if (signedvar) { - signConversionError(tok1); - break; - } - } + if (!tok1->getValueLE(-1,_settings)) + continue; + if (tok1->valueType() && tok1->valueType()->sign != ValueType::Sign::UNSIGNED) + signConversionError(tok1); } } } @@ -311,13 +210,23 @@ void CheckType::checkLongCast() // Assignments.. for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) { - if (!Token::Match(tok, "%var% =")) + if (tok->str() != "=" || !Token::Match(tok->astOperand2(), "*|<<")) continue; - if (!tok->variable() || !tok->variable()->isConst() || tok->variable()->typeStartToken()->str() != "long") + + const ValueType *lhstype = tok->astOperand1() ? tok->astOperand1()->valueType() : nullptr; + const ValueType *rhstype = tok->astOperand2()->valueType(); + + if (!lhstype || !rhstype) continue; - if (!tok->variable()->typeStartToken()->originalName().empty()) - continue; - if (Token::Match(tok->next()->astOperand2(), "*|<<") && astIsIntResult(tok->next()->astOperand2())) + + // assign int result to long/longlong const nonpointer? + if (rhstype->type == ValueType::Type::INT && + rhstype->pointer == 0U && + rhstype->originalTypeName.empty() && + (lhstype->type == ValueType::Type::LONG || lhstype->type == ValueType::Type::LONGLONG) && + lhstype->pointer == 0U && + lhstype->constness == 1U && + lhstype->originalTypeName.empty()) longCastAssignError(tok); } @@ -340,21 +249,24 @@ void CheckType::checkLongCast() if (!islong) continue; - // find return statement - // todo.. this is slow, we should only check last statement in each child scope + // return statements const Token *ret = nullptr; for (const Token *tok = scope->classStart; tok != scope->classEnd; tok = tok->next()) { if (tok->str() == "return") { - if (!ret) - ret = tok; - else { - ret = nullptr; + if (Token::Match(tok->astOperand1(), "<<|*")) { + const ValueType *type = tok->astOperand1()->valueType(); + if (type->type == ValueType::Type::INT && type->pointer == 0U && type->originalTypeName.empty()) + ret = tok; + } + // All return statements must have problem otherwise no warning + if (ret != tok) { + ret = nullptr; break; } } } - if (ret && Token::Match(ret->astOperand1(), "*|<<") && astIsIntResult(ret->astOperand1())) + if (ret) longCastReturnError(ret); } } diff --git a/test/testtype.cpp b/test/testtype.cpp index 9f4c64e9d..5950eacd6 100644 --- a/test/testtype.cpp +++ b/test/testtype.cpp @@ -99,20 +99,21 @@ private: void checkIntegerOverflow() { Settings settings; settings.platform(Settings::Unix32); + settings.addEnabled("warning"); - check("int foo(int x) {\n" + check("int foo(signed int x) {\n" " if (x==123456) {}\n" " return x * x;\n" "}",&settings); - ASSERT_EQUALS("[test.cpp:3]: (warning) Signed integer overflow for expression 'x*x'. See condition at line 2.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (warning) Either the condition 'x==123456' is redundant or there is signed integer overflow for expression 'x*x'.\n", errout.str()); - check("int foo(int x) {\n" + check("int foo(signed int x) {\n" " if (x==123456) {}\n" " return -123456 * x;\n" "}",&settings); - ASSERT_EQUALS("[test.cpp:3]: (warning) Signed integer overflow for expression '-123456*x'. See condition at line 2.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (warning) Either the condition 'x==123456' is redundant or there is signed integer overflow for expression '-123456*x'.\n", errout.str()); - check("int foo(int x) {\n" + check("int foo(signed int x) {\n" " if (x==123456) {}\n" " return 123456U * x;\n" "}",&settings);