Skip to content

Commit 7373be2

Browse files
pfultz2danmar
pfultz2
authored and
danmar
committed
Add a pass in valueflow for terminating conditions (cppcheck-opensource#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
1 parent 43b6a39 commit 7373be2

6 files changed

Lines changed: 221 additions & 19 deletions

File tree

‎lib/astutils.cpp‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -347,8 +347,6 @@ bool isSameExpression(bool cpp, bool macro, const Token *tok1, const Token *tok2
347347
return false;
348348
if (pure && tok1->isName() && tok1->next()->str() == "(" && tok1->str() != "sizeof") {
349349
if (!tok1->function()) {
350-
if (!Token::Match(tok1->previous(), ".|::") && !library.isFunctionConst(tok1) && !tok1->isAttributeConst() && !tok1->isAttributePure())
351-
return false;
352350
if (Token::simpleMatch(tok1->previous(), ".")) {
353351
const Token *lhs = tok1->previous();
354352
while (Token::Match(lhs, "(|.|["))
@@ -358,6 +356,12 @@ bool isSameExpression(bool cpp, bool macro, const Token *tok1, const Token *tok2
358356
(Token::Match(lhs, "%var% . %name% (") && library.isFunctionConst(lhs->tokAt(2)));
359357
if (!lhsIsConst)
360358
return false;
359+
} else {
360+
const Token * ftok = tok1;
361+
if (Token::simpleMatch(tok1->previous(), "::"))
362+
ftok = tok1->previous();
363+
if (!library.isFunctionConst(ftok) && !ftok->isAttributeConst() && !ftok->isAttributePure())
364+
return false;
361365
}
362366
} else {
363367
if (tok1->function() && !tok1->function()->isConst() && !tok1->function()->isAttributeConst() && !tok1->function()->isAttributePure())

‎lib/checkcondition.cpp‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,18 @@ namespace {
5151
CheckCondition instance;
5252
}
5353

54+
bool CheckCondition::diag(const Token* tok, bool insert)
55+
{
56+
if(!tok)
57+
return false;
58+
if(mCondDiags.find(tok) == mCondDiags.end()) {
59+
if(insert)
60+
mCondDiags.insert(tok);
61+
return false;
62+
}
63+
return true;
64+
}
65+
5466
bool CheckCondition::isAliased(const std::set<unsigned int> &vars) const
5567
{
5668
for (const Token *tok = mTokenizer->tokens(); tok; tok = tok->next()) {
@@ -705,6 +717,8 @@ static std::string innerSmtString(const Token * tok)
705717

706718
void CheckCondition::oppositeInnerConditionError(const Token *tok1, const Token* tok2, ErrorPath errorPath)
707719
{
720+
if(diag(tok1) & diag(tok2))
721+
return;
708722
const std::string s1(tok1 ? tok1->expressionString() : "x");
709723
const std::string s2(tok2 ? tok2->expressionString() : "!x");
710724
const std::string innerSmt = innerSmtString(tok2);
@@ -718,6 +732,8 @@ void CheckCondition::oppositeInnerConditionError(const Token *tok1, const Token*
718732

719733
void CheckCondition::identicalInnerConditionError(const Token *tok1, const Token* tok2, ErrorPath errorPath)
720734
{
735+
if(diag(tok1) & diag(tok2))
736+
return;
721737
const std::string s1(tok1 ? tok1->expressionString() : "x");
722738
const std::string s2(tok2 ? tok2->expressionString() : "x");
723739
const std::string innerSmt = innerSmtString(tok2);
@@ -731,6 +747,8 @@ void CheckCondition::identicalInnerConditionError(const Token *tok1, const Token
731747

732748
void CheckCondition::identicalConditionAfterEarlyExitError(const Token *cond1, const Token* cond2, ErrorPath errorPath)
733749
{
750+
if(diag(cond1) & diag(cond2))
751+
return;
734752
const std::string cond(cond1 ? cond1->expressionString() : "x");
735753
errorPath.emplace_back(ErrorPathItem(cond1, "first condition"));
736754
errorPath.emplace_back(ErrorPathItem(cond2, "second condition"));
@@ -1259,6 +1277,9 @@ void CheckCondition::alwaysTrueFalse()
12591277
continue;
12601278
if (!tok->hasKnownIntValue())
12611279
continue;
1280+
// Skip already diagnosed values
1281+
if(diag(tok, false))
1282+
continue;
12621283
if (Token::Match(tok, "%num%|%bool%|%char%"))
12631284
continue;
12641285
if (Token::Match(tok, "! %num%|%bool%|%char%"))

‎lib/checkcondition.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,9 @@ class CPPCHECKLIB CheckCondition : public Check {
118118
void checkPointerAdditionResultNotNull();
119119

120120
private:
121+
// The conditions that have been diagnosed
122+
std::set<const Token*> mCondDiags;
123+
bool diag(const Token* tok, bool insert=true);
121124
bool isAliased(const std::set<unsigned int> &vars) const;
122125
bool isOverlappingCond(const Token * const cond1, const Token * const cond2, bool pure) const;
123126
void assignIfError(const Token *tok1, const Token *tok2, const std::string &condition, bool result);

‎lib/valueflow.cpp‎

Lines changed: 108 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -304,6 +304,15 @@ static const Token * skipValueInConditionalExpression(const Token * const valuet
304304
return nullptr;
305305
}
306306

307+
static bool isEscapeScope(const Token* tok, TokenList * tokenlist)
308+
{
309+
if (!Token::simpleMatch(tok, "{"))
310+
return false;
311+
const Token * termTok = Token::findmatch(tok, "return|continue|break|throw|goto", tok->link());
312+
return (termTok && termTok->scope() == tok->scope()) ||
313+
(tokenlist && tokenlist->getSettings()->library.isScopeNoReturn(tok->link(), nullptr));
314+
}
315+
307316
static bool bailoutSelfAssignment(const Token * const tok)
308317
{
309318
const Token *parent = tok;
@@ -1089,6 +1098,104 @@ static void valueFlowBitAnd(TokenList *tokenlist)
10891098
}
10901099
}
10911100

1101+
static void valueFlowTerminatingCondition(TokenList *tokenlist, SymbolDatabase* symboldatabase, const Settings *settings)
1102+
{
1103+
const bool cpp = symboldatabase->isCPP();
1104+
typedef std::pair<const Token*, const Scope*> Condition;
1105+
for (const Scope * scope : symboldatabase->functionScopes) {
1106+
std::vector<Condition> conds;
1107+
for (const Token* tok = scope->bodyStart; tok != scope->bodyEnd; tok = tok->next()) {
1108+
if (!Token::simpleMatch(tok, "if ("))
1109+
continue;
1110+
// Skip known values
1111+
if (tok->next()->hasKnownValue())
1112+
continue;
1113+
const Token * condTok = tok->next();
1114+
if (!Token::simpleMatch(condTok->link(), ") {"))
1115+
continue;
1116+
const Token * blockTok = condTok->link()->tokAt(1);
1117+
// Check if the block terminates early
1118+
if(!isEscapeScope(blockTok, tokenlist))
1119+
continue;
1120+
// Check if any variables are modified in scope
1121+
bool bail = false;
1122+
for(const Token * tok2=condTok->next();tok2 != condTok->link();tok2 = tok2->next()) {
1123+
const Variable * var = tok2->variable();
1124+
if(!var)
1125+
continue;
1126+
const Token * endToken = var->scope()->bodyEnd;
1127+
if(!var->isLocal() && !var->isConst() && !var->isArgument()) {
1128+
bail = true;
1129+
break;
1130+
}
1131+
if(var->isStatic() && !var->isConst()) {
1132+
bail = true;
1133+
break;
1134+
}
1135+
if(!var->isConst() && var->declEndToken() && isVariableChanged(var->declEndToken()->next(), endToken, tok2->varId(), false, settings, cpp)) {
1136+
bail = true;
1137+
break;
1138+
}
1139+
}
1140+
if(bail)
1141+
continue;
1142+
// TODO: Handle multiple conditions
1143+
if(Token::Match(condTok->astOperand2(), "%oror%|%or%|&|&&"))
1144+
continue;
1145+
const Scope * condScope = nullptr;
1146+
for(const Scope * parent = condTok->scope();parent;parent = parent->nestedIn) {
1147+
if (parent->type == Scope::eIf ||
1148+
parent->type == Scope::eSwitch) {
1149+
condScope = parent;
1150+
break;
1151+
}
1152+
}
1153+
conds.emplace_back(condTok->astOperand2(), condScope);
1154+
1155+
}
1156+
for(Condition cond:conds) {
1157+
if(!cond.first)
1158+
continue;
1159+
for (Token* tok = cond.first->next(); tok != scope->bodyEnd; tok = tok->next()) {
1160+
if(tok == cond.first)
1161+
continue;
1162+
if (!Token::Match(tok, "%comp%"))
1163+
continue;
1164+
// Skip known values
1165+
if (tok->hasKnownValue())
1166+
continue;
1167+
if(cond.second) {
1168+
bool bail = true;
1169+
for(const Scope * parent = tok->scope()->nestedIn;parent;parent = parent->nestedIn) {
1170+
if(parent == cond.second) {
1171+
bail = false;
1172+
break;
1173+
}
1174+
}
1175+
if(bail)
1176+
continue;
1177+
}
1178+
ErrorPath errorPath;
1179+
if(isOppositeCond(true, cpp, tok, cond.first, settings->library, true, &errorPath)) {
1180+
ValueFlow::Value val(1);
1181+
val.setKnown();
1182+
val.condition = cond.first;
1183+
val.errorPath = errorPath;
1184+
val.errorPath.emplace_back(cond.first, "Assuming condition '" + cond.first->expressionString() + "' is false");
1185+
setTokenValue(tok, val, tokenlist->getSettings());
1186+
} else if(isSameExpression(cpp, true, tok, cond.first, settings->library, true, &errorPath)) {
1187+
ValueFlow::Value val(0);
1188+
val.setKnown();
1189+
val.condition = cond.first;
1190+
val.errorPath = errorPath;
1191+
val.errorPath.emplace_back(cond.first, "Assuming condition '" + cond.first->expressionString() + "' is false");
1192+
setTokenValue(tok, val, tokenlist->getSettings());
1193+
}
1194+
}
1195+
}
1196+
}
1197+
}
1198+
10921199
static bool getExpressionRange(const Token *expr, MathLib::bigint *minvalue, MathLib::bigint *maxvalue)
10931200
{
10941201
if (expr->hasKnownIntValue()) {
@@ -1688,14 +1795,6 @@ static bool evalAssignment(ValueFlow::Value &lhsValue, const std::string &assign
16881795
return true;
16891796
}
16901797

1691-
static bool isEscapeScope(const Token* tok, TokenList * tokenlist)
1692-
{
1693-
if (!Token::simpleMatch(tok, "{"))
1694-
return false;
1695-
return Token::findmatch(tok, "return|continue|break|throw|goto", tok->link()) ||
1696-
(tokenlist && tokenlist->getSettings()->library.isScopeNoReturn(tok->link(), nullptr));
1697-
}
1698-
16991798
static bool valueFlowForward(Token * const startToken,
17001799
const Token * const endToken,
17011800
const Variable * const var,
@@ -4012,6 +4111,7 @@ void ValueFlow::setValues(TokenList *tokenlist, SymbolDatabase* symboldatabase,
40124111
values = getTotalValues(tokenlist);
40134112
valueFlowRightShift(tokenlist);
40144113
valueFlowOppositeCondition(symboldatabase, settings);
4114+
valueFlowTerminatingCondition(tokenlist, symboldatabase, settings);
40154115
valueFlowBeforeCondition(tokenlist, symboldatabase, errorLogger, settings);
40164116
valueFlowAfterMove(tokenlist, symboldatabase, errorLogger, settings);
40174117
valueFlowAfterAssign(tokenlist, symboldatabase, errorLogger, settings);

‎test/testcondition.cpp‎

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1907,8 +1907,7 @@ class TestCondition : public TestFixture {
19071907

19081908
void oppositeInnerCondition3() {
19091909
check("void f3(char c) { if(c=='x') if(c=='y') {}} ");
1910-
ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:1]: (warning) Opposite inner 'if' condition leads to a dead code block.\n"
1911-
"[test.cpp:1] -> [test.cpp:1]: (style) Condition 'c=='y'' is always false\n", errout.str());
1910+
ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:1]: (warning) Opposite inner 'if' condition leads to a dead code block.\n", errout.str());
19121911

19131912
check("void f4(char *p) { if(*p=='x') if(*p=='y') {}} ");
19141913
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 @@ class TestCondition : public TestFixture {
19261925
ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:1]: (warning) Opposite inner 'if' condition leads to a dead code block.\n", errout.str());
19271926

19281927
check("void f8(int i) { if(i==4) if(i==2) {}} ");
1929-
ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:1]: (warning) Opposite inner 'if' condition leads to a dead code block.\n"
1930-
"[test.cpp:1] -> [test.cpp:1]: (style) Condition 'i==2' is always false\n", errout.str());
1928+
ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:1]: (warning) Opposite inner 'if' condition leads to a dead code block.\n", errout.str());
19311929

19321930
check("void f9(int *p) { if (*p==4) if(*p==2) {}} ");
19331931
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 @@ class TestCondition : public TestFixture {
19591957
ASSERT_EQUALS("", errout.str());
19601958

19611959
check("void f(int x) { if (x == 1) if (x != 1) {} }");
1962-
ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:1]: (warning) Opposite inner 'if' condition leads to a dead code block.\n"
1963-
"[test.cpp:1] -> [test.cpp:1]: (style) Condition 'x!=1' is always false\n", errout.str());
1960+
ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:1]: (warning) Opposite inner 'if' condition leads to a dead code block.\n", errout.str());
19641961
}
19651962

19661963
void oppositeInnerConditionAnd() {
@@ -1992,8 +1989,7 @@ class TestCondition : public TestFixture {
19921989
ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:1]: (warning) Opposite inner 'if' condition leads to a dead code block.\n", errout.str());
19931990

19941991
check("void f1(QString s) { if(s.isEmpty()) if(s.length() > 42) {}} ");
1995-
ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:1]: (warning) Opposite inner 'if' condition leads to a dead code block.\n"
1996-
"[test.cpp:1] -> [test.cpp:1]: (style) Condition 's.length()>42' is always false\n", errout.str());
1992+
ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:1]: (warning) Opposite inner 'if' condition leads to a dead code block.\n", errout.str());
19971993

19981994
check("void f1(const std::string &s) { if(s.empty()) if(s.size() == 0) {}} ");
19991995
ASSERT_EQUALS("", errout.str());
@@ -2164,7 +2160,7 @@ class TestCondition : public TestFixture {
21642160
" X(do);\n"
21652161
" if (x > 100) {}\n"
21662162
"}");
2167-
ASSERT_EQUALS("", errout.str());
2163+
ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:4]: (style) Condition 'x>100' is always false\n", errout.str());
21682164

21692165
check("void f(const int *i) {\n"
21702166
" if (!i) return;\n"
@@ -2671,6 +2667,12 @@ class TestCondition : public TestFixture {
26712667
" }\n"
26722668
"};\n");
26732669
ASSERT_EQUALS("", errout.str());
2670+
2671+
check("bool f(long maxtime) {\n"
2672+
" if (std::time(0) > maxtime)\n"
2673+
" return std::time(0) > maxtime;\n"
2674+
"}\n");
2675+
ASSERT_EQUALS("", errout.str());
26742676
}
26752677

26762678
void multiConditionAlwaysTrue() {

‎test/testvalueflow.cpp‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,8 @@ class TestValueFlow : public TestFixture {
107107

108108
TEST_CASE(valueFlowUninit);
109109

110+
TEST_CASE(valueFlowTerminatingCond);
111+
110112
TEST_CASE(valueFlowContainerSize);
111113
}
112114

@@ -3304,6 +3306,76 @@ class TestValueFlow : public TestFixture {
33043306
ASSERT_EQUALS(true, values.front().intvalue == 0 || values.back().intvalue == 0);
33053307
}
33063308

3309+
void valueFlowTerminatingCond() {
3310+
const char* code;
3311+
3312+
// opposite condition
3313+
code = "void f(int i, int j) {\n"
3314+
" if (i == j) return;\n"
3315+
" if(i != j) {}\n"
3316+
"}\n";
3317+
ASSERT_EQUALS(true, valueOfTok(code, "!=").intvalue == 1);
3318+
3319+
code = "void f(int i, int j) {\n"
3320+
" if (i == j) return;\n"
3321+
" i++;\n"
3322+
" if (i != j) {}\n"
3323+
"}\n";
3324+
ASSERT_EQUALS(false, valueOfTok(code, "!=").intvalue == 1);
3325+
3326+
code = "void f(int i, int j, bool a) {\n"
3327+
" if (a) {\n"
3328+
" if (i == j) return;\n"
3329+
" }\n"
3330+
" if (i != j) {}\n"
3331+
"}\n";
3332+
ASSERT_EQUALS(false, valueOfTok(code, "!=").intvalue == 1);
3333+
3334+
code = "void f(int i, int j, bool a) {\n"
3335+
" if (i != j) {}\n"
3336+
" if (i == j) return; \n"
3337+
"}\n";
3338+
ASSERT_EQUALS(false, valueOfTok(code, "!=").intvalue == 1);
3339+
3340+
// same expression
3341+
code = "void f(int i, int j) {\n"
3342+
" if (i != j) return;\n"
3343+
" bool x = (i != j);\n"
3344+
" bool b = x;\n"
3345+
"}\n";
3346+
ASSERT_EQUALS(true, testValueOfXKnown(code, 4U, 0));
3347+
3348+
code = "void f(int i, int j) {\n"
3349+
" if (i != j) return;\n"
3350+
" i++;\n"
3351+
" bool x = (i != j);\n"
3352+
" bool b = x;\n"
3353+
"}\n";
3354+
ASSERT_EQUALS(false, testValueOfXKnown(code, 5U, 0));
3355+
3356+
code = "void f(int i, int j, bool a) {\n"
3357+
" if (a) {\n"
3358+
" if (i != j) return;\n"
3359+
" }\n"
3360+
" bool x = (i != j);\n"
3361+
" bool b = x;\n"
3362+
"}\n";
3363+
ASSERT_EQUALS(false, testValueOfXKnown(code, 6U, 0));
3364+
3365+
code = "void f(int i, int j, bool a) {\n"
3366+
" bool x = (i != j);\n"
3367+
" bool b = x;\n"
3368+
" if (i != j) return; \n"
3369+
"}\n";
3370+
ASSERT_EQUALS(false, testValueOfXKnown(code, 3U, 0));
3371+
3372+
code = "void f(int i, int j, bool b) {\n"
3373+
" if (i == j) { if(b) return; }\n"
3374+
" if(i != j) {}\n"
3375+
"}\n";
3376+
ASSERT_EQUALS(false, valueOfTok(code, "!=").intvalue == 1);
3377+
}
3378+
33073379
static std::string isPossibleContainerSizeValue(const std::list<ValueFlow::Value> &values, MathLib::bigint i) {
33083380
if (values.size() != 1)
33093381
return "values.size():" + std::to_string(values.size());

0 commit comments

Comments
 (0)