walkero
/
cppcheck
1
0
Fork 0
Code Issues Pull Requests Projects Releases 1 Wiki Activity

Add a pass in valueflow for terminating conditions (#1323) 

* Add valueflow for terminating conditions

* Add valueflow test

* Dont check for same expressions for now to avoid double diagnostics

* Check nesting

* Add more tests

* Ensure conditions happen in order

* Check for null

* Add error path

* Support same expression check as well

* Use early continue

* Skip checking the same token

* Avoid double condtion diagnosis

* Fix FP when in switch statements

* Fix FP when time function

* Skip conditional escapes

* Use simpleMatch

* Fix naming

* Fix typo
This commit is contained in:
Paul Fultz II 2018-11-06 23:49:07 -06:00 committed by Daniel Marjamäki
parent 43b6a391d8
commit 7373be2bfa
6 changed files with 221 additions and 19 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
lib/astutils.cpp
View File

@ -347,8 +347,6 @@ bool isSameExpression(bool cpp, bool macro, const Token *tok1, const Token *tok2
return false;
if (pure && tok1->isName() && tok1->next()->str() == "(" && tok1->str() != "sizeof") {
if (!tok1->function()) {
if (!Token::Match(tok1->previous(), ".|::") && !library.isFunctionConst(tok1) && !tok1->isAttributeConst() && !tok1->isAttributePure())
return false;
if (Token::simpleMatch(tok1->previous(), ".")) {
const Token *lhs = tok1->previous();
while (Token::Match(lhs, "(|.|["))
@ -358,6 +356,12 @@ bool isSameExpression(bool cpp, bool macro, const Token *tok1, const Token *tok2
(Token::Match(lhs, "%var% . %name% (") && library.isFunctionConst(lhs->tokAt(2)));
if (!lhsIsConst)
return false;
} else {
const Token * ftok = tok1;
if (Token::simpleMatch(tok1->previous(), "::"))
ftok = tok1->previous();
if (!library.isFunctionConst(ftok) && !ftok->isAttributeConst() && !ftok->isAttributePure())
return false;
}
} else {
if (tok1->function() && !tok1->function()->isConst() && !tok1->function()->isAttributeConst() && !tok1->function()->isAttributePure())

21
lib/checkcondition.cpp
View File

@ -51,6 +51,18 @@ namespace {
CheckCondition instance;
}
bool CheckCondition::diag(const Token* tok, bool insert)
{
if(!tok)
return false;
if(mCondDiags.find(tok) == mCondDiags.end()) {
if(insert)
mCondDiags.insert(tok);
return false;
}
return true;
}
bool CheckCondition::isAliased(const std::set<unsigned int> &vars) const
{
for (const Token *tok = mTokenizer->tokens(); tok; tok = tok->next()) {
@ -705,6 +717,8 @@ static std::string innerSmtString(const Token * tok)
void CheckCondition::oppositeInnerConditionError(const Token *tok1, const Token* tok2, ErrorPath errorPath)
{
if(diag(tok1) & diag(tok2))
return;
const std::string s1(tok1 ? tok1->expressionString() : "x");
const std::string s2(tok2 ? tok2->expressionString() : "!x");
const std::string innerSmt = innerSmtString(tok2);
@ -718,6 +732,8 @@ void CheckCondition::oppositeInnerConditionError(const Token *tok1, const Token*
void CheckCondition::identicalInnerConditionError(const Token *tok1, const Token* tok2, ErrorPath errorPath)
{
if(diag(tok1) & diag(tok2))
return;
const std::string s1(tok1 ? tok1->expressionString() : "x");
const std::string s2(tok2 ? tok2->expressionString() : "x");
const std::string innerSmt = innerSmtString(tok2);
@ -731,6 +747,8 @@ void CheckCondition::identicalInnerConditionError(const Token *tok1, const Token
void CheckCondition::identicalConditionAfterEarlyExitError(const Token *cond1, const Token* cond2, ErrorPath errorPath)
{
if(diag(cond1) & diag(cond2))
return;
const std::string cond(cond1 ? cond1->expressionString() : "x");
errorPath.emplace_back(ErrorPathItem(cond1, "first condition"));
errorPath.emplace_back(ErrorPathItem(cond2, "second condition"));
@ -1259,6 +1277,9 @@ void CheckCondition::alwaysTrueFalse()
continue;
if (!tok->hasKnownIntValue())
continue;
// Skip already diagnosed values
if(diag(tok, false))
continue;
if (Token::Match(tok, "%num%|%bool%|%char%"))
continue;
if (Token::Match(tok, "! %num%|%bool%|%char%"))

3
lib/checkcondition.h
View File

@ -118,6 +118,9 @@ public:
void checkPointerAdditionResultNotNull();
private:
// The conditions that have been diagnosed
std::set<const Token*> mCondDiags;
bool diag(const Token* tok, bool insert=true);
bool isAliased(const std::set<unsigned int> &vars) const;
bool isOverlappingCond(const Token * const cond1, const Token * const cond2, bool pure) const;
void assignIfError(const Token *tok1, const Token *tok2, const std::string &condition, bool result);

116
lib/valueflow.cpp
View File

@ -304,6 +304,15 @@ static const Token * skipValueInConditionalExpression(const Token * const valuet
return nullptr;
}
static bool isEscapeScope(const Token* tok, TokenList * tokenlist)
{
if (!Token::simpleMatch(tok, "{"))
return false;
const Token * termTok = Token::findmatch(tok, "return|continue|break|throw|goto", tok->link());
return (termTok && termTok->scope() == tok->scope()) ||
(tokenlist && tokenlist->getSettings()->library.isScopeNoReturn(tok->link(), nullptr));
}
static bool bailoutSelfAssignment(const Token * const tok)
{
const Token *parent = tok;
@ -1089,6 +1098,104 @@ static void valueFlowBitAnd(TokenList *tokenlist)
}
}
static void valueFlowTerminatingCondition(TokenList *tokenlist, SymbolDatabase* symboldatabase, const Settings *settings)
{
const bool cpp = symboldatabase->isCPP();
typedef std::pair<const Token*, const Scope*> Condition;
for (const Scope * scope : symboldatabase->functionScopes) {
std::vector<Condition> conds;
for (const Token* tok = scope->bodyStart; tok != scope->bodyEnd; tok = tok->next()) {
if (!Token::simpleMatch(tok, "if ("))
continue;
// Skip known values
if (tok->next()->hasKnownValue())
continue;
const Token * condTok = tok->next();
if (!Token::simpleMatch(condTok->link(), ") {"))
continue;
const Token * blockTok = condTok->link()->tokAt(1);
// Check if the block terminates early
if(!isEscapeScope(blockTok, tokenlist))
continue;
// Check if any variables are modified in scope
bool bail = false;
for(const Token * tok2=condTok->next();tok2 != condTok->link();tok2 = tok2->next()) {
const Variable * var = tok2->variable();
if(!var)
continue;
const Token * endToken = var->scope()->bodyEnd;
if(!var->isLocal() && !var->isConst() && !var->isArgument()) {
bail = true;
break;
}
if(var->isStatic() && !var->isConst()) {
bail = true;
break;
}
if(!var->isConst() && var->declEndToken() && isVariableChanged(var->declEndToken()->next(), endToken, tok2->varId(), false, settings, cpp)) {
bail = true;
break;
}
}
if(bail)
continue;
// TODO: Handle multiple conditions
if(Token::Match(condTok->astOperand2(), "%oror%|%or%|&|&&"))
continue;
const Scope * condScope = nullptr;
for(const Scope * parent = condTok->scope();parent;parent = parent->nestedIn) {
if (parent->type == Scope::eIf ||
parent->type == Scope::eSwitch) {
condScope = parent;
break;
}
}
conds.emplace_back(condTok->astOperand2(), condScope);
}
for(Condition cond:conds) {
if(!cond.first)
continue;
for (Token* tok = cond.first->next(); tok != scope->bodyEnd; tok = tok->next()) {
if(tok == cond.first)
continue;
if (!Token::Match(tok, "%comp%"))
continue;
// Skip known values
if (tok->hasKnownValue())
continue;
if(cond.second) {
bool bail = true;
for(const Scope * parent = tok->scope()->nestedIn;parent;parent = parent->nestedIn) {
if(parent == cond.second) {
bail = false;
break;
}
}
if(bail)
continue;
}
ErrorPath errorPath;
if(isOppositeCond(true, cpp, tok, cond.first, settings->library, true, &errorPath)) {
ValueFlow::Value val(1);
val.setKnown();
val.condition = cond.first;
val.errorPath = errorPath;
val.errorPath.emplace_back(cond.first, "Assuming condition '" + cond.first->expressionString() + "' is false");
setTokenValue(tok, val, tokenlist->getSettings());
} else if(isSameExpression(cpp, true, tok, cond.first, settings->library, true, &errorPath)) {
ValueFlow::Value val(0);
val.setKnown();
val.condition = cond.first;
val.errorPath = errorPath;
val.errorPath.emplace_back(cond.first, "Assuming condition '" + cond.first->expressionString() + "' is false");
setTokenValue(tok, val, tokenlist->getSettings());
}
}
}
}
}
static bool getExpressionRange(const Token *expr, MathLib::bigint *minvalue, MathLib::bigint *maxvalue)
{
if (expr->hasKnownIntValue()) {
@ -1688,14 +1795,6 @@ static bool evalAssignment(ValueFlow::Value &lhsValue, const std::string &assign
return true;
}
static bool isEscapeScope(const Token* tok, TokenList * tokenlist)
{
if (!Token::simpleMatch(tok, "{"))
return false;
return Token::findmatch(tok, "return|continue|break|throw|goto", tok->link()) ||
(tokenlist && tokenlist->getSettings()->library.isScopeNoReturn(tok->link(), nullptr));
}
static bool valueFlowForward(Token * const startToken,
const Token * const endToken,
const Variable * const var,
@ -4012,6 +4111,7 @@ void ValueFlow::setValues(TokenList *tokenlist, SymbolDatabase* symboldatabase,
values = getTotalValues(tokenlist);
valueFlowRightShift(tokenlist);
valueFlowOppositeCondition(symboldatabase, settings);
valueFlowTerminatingCondition(tokenlist, symboldatabase, settings);
valueFlowBeforeCondition(tokenlist, symboldatabase, errorLogger, settings);
valueFlowAfterMove(tokenlist, symboldatabase, errorLogger, settings);
valueFlowAfterAssign(tokenlist, symboldatabase, errorLogger, settings);

20
test/testcondition.cpp
View File

@ -1907,8 +1907,7 @@ private:
void oppositeInnerCondition3() {
check("void f3(char c) { if(c=='x') if(c=='y') {}} ");
ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:1]: (warning) Opposite inner 'if' condition leads to a dead code block.\n"
"[test.cpp:1] -> [test.cpp:1]: (style) Condition 'c=='y'' is always false\n", errout.str());
ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:1]: (warning) Opposite inner 'if' condition leads to a dead code block.\n", errout.str());
check("void f4(char *p) { if(*p=='x') if(*p=='y') {}} ");
ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:1]: (warning) Opposite inner 'if' condition leads to a dead code block.\n", errout.str());
@ -1926,8 +1925,7 @@ private:
ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:1]: (warning) Opposite inner 'if' condition leads to a dead code block.\n", errout.str());
check("void f8(int i) { if(i==4) if(i==2) {}} ");
ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:1]: (warning) Opposite inner 'if' condition leads to a dead code block.\n"
"[test.cpp:1] -> [test.cpp:1]: (style) Condition 'i==2' is always false\n", errout.str());
ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:1]: (warning) Opposite inner 'if' condition leads to a dead code block.\n", errout.str());
check("void f9(int *p) { if (*p==4) if(*p==2) {}} ");
ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:1]: (warning) Opposite inner 'if' condition leads to a dead code block.\n", errout.str());
@ -1959,8 +1957,7 @@ private:
ASSERT_EQUALS("", errout.str());
check("void f(int x) { if (x == 1) if (x != 1) {} }");
ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:1]: (warning) Opposite inner 'if' condition leads to a dead code block.\n"
"[test.cpp:1] -> [test.cpp:1]: (style) Condition 'x!=1' is always false\n", errout.str());
ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:1]: (warning) Opposite inner 'if' condition leads to a dead code block.\n", errout.str());
}
void oppositeInnerConditionAnd() {
@ -1992,8 +1989,7 @@ private:
ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:1]: (warning) Opposite inner 'if' condition leads to a dead code block.\n", errout.str());
check("void f1(QString s) { if(s.isEmpty()) if(s.length() > 42) {}} ");
ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:1]: (warning) Opposite inner 'if' condition leads to a dead code block.\n"
"[test.cpp:1] -> [test.cpp:1]: (style) Condition 's.length()>42' is always false\n", errout.str());
ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:1]: (warning) Opposite inner 'if' condition leads to a dead code block.\n", errout.str());
check("void f1(const std::string &s) { if(s.empty()) if(s.size() == 0) {}} ");
ASSERT_EQUALS("", errout.str());
@ -2164,7 +2160,7 @@ private:
" X(do);\n"
" if (x > 100) {}\n"
"}");
ASSERT_EQUALS("", errout.str());
ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:4]: (style) Condition 'x>100' is always false\n", errout.str());
check("void f(const int *i) {\n"
" if (!i) return;\n"
@ -2671,6 +2667,12 @@ private:
" }\n"
"};\n");
ASSERT_EQUALS("", errout.str());
check("bool f(long maxtime) {\n"
" if (std::time(0) > maxtime)\n"
" return std::time(0) > maxtime;\n"
"}\n");
ASSERT_EQUALS("", errout.str());
}
void multiConditionAlwaysTrue() {

72
test/testvalueflow.cpp
View File

@ -107,6 +107,8 @@ private:
TEST_CASE(valueFlowUninit);
TEST_CASE(valueFlowTerminatingCond);
TEST_CASE(valueFlowContainerSize);
}
@ -3304,6 +3306,76 @@ private:
ASSERT_EQUALS(true, values.front().intvalue == 0 || values.back().intvalue == 0);
}
void valueFlowTerminatingCond() {
const char* code;
// opposite condition
code = "void f(int i, int j) {\n"
" if (i == j) return;\n"
" if(i != j) {}\n"
"}\n";
ASSERT_EQUALS(true, valueOfTok(code, "!=").intvalue == 1);
code = "void f(int i, int j) {\n"
" if (i == j) return;\n"
" i++;\n"
" if (i != j) {}\n"
"}\n";
ASSERT_EQUALS(false, valueOfTok(code, "!=").intvalue == 1);
code = "void f(int i, int j, bool a) {\n"
" if (a) {\n"
" if (i == j) return;\n"
" }\n"
" if (i != j) {}\n"
"}\n";
ASSERT_EQUALS(false, valueOfTok(code, "!=").intvalue == 1);
code = "void f(int i, int j, bool a) {\n"
" if (i != j) {}\n"
" if (i == j) return; \n"
"}\n";
ASSERT_EQUALS(false, valueOfTok(code, "!=").intvalue == 1);
// same expression
code = "void f(int i, int j) {\n"
" if (i != j) return;\n"
" bool x = (i != j);\n"
" bool b = x;\n"
"}\n";
ASSERT_EQUALS(true, testValueOfXKnown(code, 4U, 0));
code = "void f(int i, int j) {\n"
" if (i != j) return;\n"
" i++;\n"
" bool x = (i != j);\n"
" bool b = x;\n"
"}\n";
ASSERT_EQUALS(false, testValueOfXKnown(code, 5U, 0));
code = "void f(int i, int j, bool a) {\n"
" if (a) {\n"
" if (i != j) return;\n"
" }\n"
" bool x = (i != j);\n"
" bool b = x;\n"
"}\n";
ASSERT_EQUALS(false, testValueOfXKnown(code, 6U, 0));
code = "void f(int i, int j, bool a) {\n"
" bool x = (i != j);\n"
" bool b = x;\n"
" if (i != j) return; \n"
"}\n";
ASSERT_EQUALS(false, testValueOfXKnown(code, 3U, 0));
code = "void f(int i, int j, bool b) {\n"
" if (i == j) { if(b) return; }\n"
" if(i != j) {}\n"
"}\n";
ASSERT_EQUALS(false, valueOfTok(code, "!=").intvalue == 1);
}
static std::string isPossibleContainerSizeValue(const std::list<ValueFlow::Value> &values, MathLib::bigint i) {
if (values.size() != 1)
return "values.size():" + std::to_string(values.size());