Skip to content

Commit 7cb8f2b

Browse files
danmarolabetskyi
danmar
and
olabetskyi
authored
Fix #12726: Update for CheckOrderEvaluation (#6448)
cherry picked 9d4d118 Co-authored-by: olabetskyi <[email protected]>
1 parent cc7f2ea commit 7cb8f2b

File tree

3 files changed

+134
-33
lines changed

3 files changed

+134
-33
lines changed

‎lib/checkother.cpp

+88-32
Original file line numberDiff line numberDiff line change
@@ -3265,19 +3265,82 @@ void CheckOther::unusedLabelError(const Token* tok, bool inSwitch, bool hasIfdef
32653265
Certainty::normal);
32663266
}
32673267

3268+
static bool checkEvaluationOrderC(const Token * tok, const Token * tok2, const Token * parent, const Settings & settings, bool & selfAssignmentError)
3269+
{
3270+
// self assignment..
3271+
if (tok2 == tok && tok->str() == "=" && parent->str() == "=" && isSameExpression(false, tok->astOperand1(), parent->astOperand1(), settings.library, true, false)) {
3272+
if (settings.severity.isEnabled(Severity::warning) && isSameExpression(true, tok->astOperand1(), parent->astOperand1(), settings.library, true, false))
3273+
selfAssignmentError = true;
3274+
return false;
3275+
}
3276+
// Is expression used?
3277+
bool foundError = false;
3278+
visitAstNodes((parent->astOperand1() != tok2) ? parent->astOperand1() : parent->astOperand2(), [&](const Token *tok3) {
3279+
if (tok3->str() == "&" && !tok3->astOperand2())
3280+
return ChildrenToVisit::none; // don't handle address-of for now
3281+
if (tok3->str() == "(" && Token::simpleMatch(tok3->previous(), "sizeof"))
3282+
return ChildrenToVisit::none; // don't care about sizeof usage
3283+
if (isSameExpression(false, tok->astOperand1(), tok3, settings.library, true, false))
3284+
foundError = true;
3285+
return foundError ? ChildrenToVisit::done : ChildrenToVisit::op1_and_op2;
3286+
});
32683287

3269-
void CheckOther::checkEvaluationOrder()
3288+
return foundError;
3289+
}
3290+
3291+
static bool checkEvaluationOrderCpp11(const Token * tok, const Token * tok2, const Token * parent, const Settings & settings)
32703292
{
3271-
// This checker is not written according to C++11 sequencing rules
3272-
if (mTokenizer->isCPP() && mSettings->standards.cpp >= Standards::CPP11)
3273-
return;
3293+
if (tok->isAssignmentOp()) // TODO check assignment
3294+
return false;
3295+
if (tok->previous() == tok->astOperand1() && parent->isArithmeticalOp() && parent->isBinaryOp()) {
3296+
if (parent->astParent() && parent->astParent()->isAssignmentOp() && isSameExpression(false, tok->astOperand1(), parent->astParent()->astOperand1(), settings.library, true, false))
3297+
return true;
3298+
}
3299+
bool foundUndefined{false};
3300+
visitAstNodes((parent->astOperand1() != tok2) ? parent->astOperand1() : parent->astOperand2(), [&](const Token *tok3) {
3301+
if (tok3->str() == "&" && !tok3->astOperand2())
3302+
return ChildrenToVisit::none; // don't handle address-of for now
3303+
if (tok3->str() == "(" && Token::simpleMatch(tok3->previous(), "sizeof"))
3304+
return ChildrenToVisit::none; // don't care about sizeof usage
3305+
if (isSameExpression(false, tok->astOperand1(), tok3, settings.library, true, false))
3306+
foundUndefined = true;
3307+
return foundUndefined ? ChildrenToVisit::done : ChildrenToVisit::op1_and_op2;
3308+
});
32743309

3275-
logChecker("CheckOther::checkEvaluationOrder"); // C/C++03
3310+
return foundUndefined;
3311+
}
32763312

3313+
static bool checkEvaluationOrderCpp17(const Token * tok, const Token * tok2, const Token * parent, const Settings & settings, bool & foundUnspecified)
3314+
{
3315+
if (tok->isAssignmentOp())
3316+
return false;
3317+
bool foundUndefined{false};
3318+
visitAstNodes((parent->astOperand1() != tok2) ? parent->astOperand1() : parent->astOperand2(), [&](const Token *tok3) {
3319+
if (tok3->str() == "&" && !tok3->astOperand2())
3320+
return ChildrenToVisit::none; // don't handle address-of for now
3321+
if (tok3->str() == "(" && Token::simpleMatch(tok3->previous(), "sizeof"))
3322+
return ChildrenToVisit::none; // don't care about sizeof usage
3323+
if (isSameExpression(false, tok->astOperand1(), tok3, settings.library, true, false) && parent->isArithmeticalOp() && parent->isBinaryOp())
3324+
foundUndefined = true;
3325+
if (tok3->tokType() == Token::eIncDecOp && isSameExpression(false, tok->astOperand1(), tok3->astOperand1(), settings.library, true, false)) {
3326+
if (parent->isArithmeticalOp() && parent->isBinaryOp())
3327+
foundUndefined = true;
3328+
else
3329+
foundUnspecified = true;
3330+
}
3331+
return (foundUndefined || foundUnspecified) ? ChildrenToVisit::done : ChildrenToVisit::op1_and_op2;
3332+
});
3333+
3334+
return foundUndefined || foundUnspecified;
3335+
}
3336+
3337+
void CheckOther::checkEvaluationOrder()
3338+
{
3339+
logChecker("CheckOther::checkEvaluationOrder");
32773340
const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase();
32783341
for (const Scope * functionScope : symbolDatabase->functionScopes) {
32793342
for (const Token* tok = functionScope->bodyStart; tok != functionScope->bodyEnd; tok = tok->next()) {
3280-
if (tok->tokType() != Token::eIncDecOp && !tok->isAssignmentOp())
3343+
if (!tok->isIncDecOp() && !tok->isAssignmentOp())
32813344
continue;
32823345
if (!tok->astOperand1())
32833346
continue;
@@ -3308,43 +3371,36 @@ void CheckOther::checkEvaluationOrder()
33083371
if (parent->str() == "(" && parent->astOperand2())
33093372
break;
33103373

3311-
// self assignment..
3312-
if (tok2 == tok &&
3313-
tok->str() == "=" &&
3314-
parent->str() == "=" &&
3315-
isSameExpression(false, tok->astOperand1(), parent->astOperand1(), mSettings->library, true, false)) {
3316-
if (mSettings->severity.isEnabled(Severity::warning) &&
3317-
isSameExpression(true, tok->astOperand1(), parent->astOperand1(), mSettings->library, true, false))
3318-
selfAssignmentError(parent, tok->astOperand1()->expressionString());
3319-
break;
3374+
bool foundError{false}, foundUnspecified{false}, bSelfAssignmentError{false};
3375+
if (mTokenizer->isCPP() && mSettings->standards.cpp >= Standards::CPP11) {
3376+
if (mSettings->standards.cpp >= Standards::CPP17)
3377+
foundError = checkEvaluationOrderCpp17(tok, tok2, parent, *mSettings, foundUnspecified);
3378+
else
3379+
foundError = checkEvaluationOrderCpp11(tok, tok2, parent, *mSettings);
33203380
}
3321-
3322-
// Is expression used?
3323-
bool foundError = false;
3324-
visitAstNodes((parent->astOperand1() != tok2) ? parent->astOperand1() : parent->astOperand2(),
3325-
[&](const Token *tok3) {
3326-
if (tok3->str() == "&" && !tok3->astOperand2())
3327-
return ChildrenToVisit::none; // don't handle address-of for now
3328-
if (tok3->str() == "(" && Token::simpleMatch(tok3->previous(), "sizeof"))
3329-
return ChildrenToVisit::none; // don't care about sizeof usage
3330-
if (isSameExpression(false, tok->astOperand1(), tok3, mSettings->library, true, false))
3331-
foundError = true;
3332-
return foundError ? ChildrenToVisit::done : ChildrenToVisit::op1_and_op2;
3333-
});
3381+
else
3382+
foundError = checkEvaluationOrderC(tok, tok2, parent, *mSettings, bSelfAssignmentError);
33343383

33353384
if (foundError) {
3336-
unknownEvaluationOrder(parent);
3385+
unknownEvaluationOrder(parent, foundUnspecified);
3386+
break;
3387+
}
3388+
if (bSelfAssignmentError) {
3389+
selfAssignmentError(parent, tok->astOperand1()->expressionString());
33373390
break;
33383391
}
33393392
}
33403393
}
33413394
}
33423395
}
33433396

3344-
void CheckOther::unknownEvaluationOrder(const Token* tok)
3397+
void CheckOther::unknownEvaluationOrder(const Token* tok, bool isUnspecifiedBehavior)
33453398
{
3346-
reportError(tok, Severity::error, "unknownEvaluationOrder",
3347-
"Expression '" + (tok ? tok->expressionString() : std::string("x = x++;")) + "' depends on order of evaluation of side effects", CWE768, Certainty::normal);
3399+
isUnspecifiedBehavior ?
3400+
reportError(tok, Severity::portability, "unknownEvaluationOrder",
3401+
"Expression '" + (tok ? tok->expressionString() : std::string("x++, x++")) + "' depends on order of evaluation of side effects. Behavior is Unspecified according to c++17", CWE768, Certainty::normal)
3402+
: reportError(tok, Severity::error, "unknownEvaluationOrder",
3403+
"Expression '" + (tok ? tok->expressionString() : std::string("x = x++;")) + "' depends on order of evaluation of side effects", CWE768, Certainty::normal);
33483404
}
33493405

33503406
void CheckOther::checkAccessOfMovedVariable()

‎lib/checkother.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -280,7 +280,7 @@ class CPPCHECKLIB CheckOther : public Check {
280280
void redundantPointerOpError(const Token* tok, const std::string& varname, bool inconclusive, bool addressOfDeref);
281281
void raceAfterInterlockedDecrementError(const Token* tok);
282282
void unusedLabelError(const Token* tok, bool inSwitch, bool hasIfdef);
283-
void unknownEvaluationOrder(const Token* tok);
283+
void unknownEvaluationOrder(const Token* tok, bool isUnspecifiedBehavior = false);
284284
void accessMovedError(const Token *tok, const std::string &varname, const ValueFlow::Value *value, bool inconclusive);
285285
void funcArgNamesDifferent(const std::string & functionName, nonneg int index, const Token* declaration, const Token* definition);
286286
void funcArgOrderDifferent(const std::string & functionName, const Token * declaration, const Token * definition, const std::vector<const Token*> & declarations, const std::vector<const Token*> & definitions);

‎test/testother.cpp

+45
Original file line numberDiff line numberDiff line change
@@ -10799,6 +10799,28 @@ class TestOther : public TestFixture {
1079910799
ASSERT_EQUALS("[test.cpp:6]: (style) Label 'label' is not used.\n", errout_str());
1080010800
}
1080110801

10802+
#define checkCustomSettings(...) checkCustomSettings_(__FILE__, __LINE__, __VA_ARGS__)
10803+
void checkCustomSettings_(const char* file, int line, const char code[], bool cpp = true, bool inconclusive = true, bool runSimpleChecks=true, bool verbose=false, Settings* settings = nullptr) {
10804+
if (!settings) {
10805+
settings = &_settings;
10806+
}
10807+
settings->certainty.setEnabled(Certainty::inconclusive, inconclusive);
10808+
settings->verbose = verbose;
10809+
10810+
// Tokenize..
10811+
SimpleTokenizer tokenizer(*settings, *this);
10812+
ASSERT_LOC(tokenizer.tokenize(code, cpp), file, line);
10813+
10814+
// Check..
10815+
runChecks<CheckOther>(tokenizer, this);
10816+
10817+
(void)runSimpleChecks; // TODO Remove this
10818+
}
10819+
10820+
void checkCustomSettings_(const char* file, int line, const char code[], Settings *s) {
10821+
checkCustomSettings_(file, line, code, true, true, true, false, s);
10822+
}
10823+
1080210824
void testEvaluationOrder() {
1080310825
check("void f() {\n"
1080410826
" int x = dostuff();\n"
@@ -10842,6 +10864,29 @@ class TestOther : public TestFixture {
1084210864
" a[x+y] = a[y+x]++;;\n"
1084310865
"}\n", false);
1084410866
ASSERT_EQUALS("[test.c:3]: (error) Expression 'a[x+y]=a[y+x]++' depends on order of evaluation of side effects\n", errout_str());
10867+
10868+
check("void f(int i) {\n"
10869+
" int n = ++i + i;\n"
10870+
"}");
10871+
ASSERT_EQUALS("[test.cpp:2]: (error) Expression '++i+i' depends on order of evaluation of side effects\n", errout_str());
10872+
10873+
check("long int f1(const char *exp) {\n"
10874+
" return dostuff(++exp, ++exp, 10);\n"
10875+
"}");
10876+
ASSERT_EQUALS("[test.cpp:2]: (portability) Expression '++exp,++exp' depends on order of evaluation of side effects. Behavior is Unspecified according to c++17\n"
10877+
"[test.cpp:2]: (portability) Expression '++exp,++exp' depends on order of evaluation of side effects. Behavior is Unspecified according to c++17\n", errout_str());
10878+
10879+
check("void f(int i) {\n"
10880+
" int n = (~(-(++i)) + i);\n"
10881+
"}");
10882+
ASSERT_EQUALS("[test.cpp:2]: (error) Expression '~(-(++i))+i' depends on order of evaluation of side effects\n", errout_str());
10883+
10884+
/*const*/ Settings settings11 = settingsBuilder(_settings).cpp(Standards::CPP11).build();
10885+
10886+
checkCustomSettings("void f(int i) {\n"
10887+
" i = i++ + 2;\n"
10888+
"}", &settings11);
10889+
ASSERT_EQUALS("[test.cpp:2]: (error) Expression 'i+++2' depends on order of evaluation of side effects\n", errout_str());
1084510890
}
1084610891

1084710892
void testEvaluationOrderSelfAssignment() {

0 commit comments

Comments
 (0)