Skip to content

Commit

Permalink
Fix #12726: Update for CheckOrderEvaluation (#6393)
Browse files Browse the repository at this point in the history
  • Loading branch information
@olabetskyi
olabetskyi authored May 20, 2024
1 parent 337dfc9 commit 9d4d118
Show file tree
Hide file tree
Showing 3 changed files with 134 additions and 33 deletions.
120 changes: 88 additions & 32 deletions lib/checkother.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3265,19 +3265,82 @@ void CheckOther::unusedLabelError(const Token* tok, bool inSwitch, bool hasIfdef
Certainty::normal);
}

static bool checkEvaluationOrderC(const Token * tok, const Token * tok2, const Token * parent, const Settings & settings, bool & selfAssignmentError)
{
// self assignment..
if (tok2 == tok && tok->str() == "=" && parent->str() == "=" && isSameExpression(false, tok->astOperand1(), parent->astOperand1(), settings, true, false)) {
if (settings.severity.isEnabled(Severity::warning) && isSameExpression(true, tok->astOperand1(), parent->astOperand1(), settings, true, false))
selfAssignmentError = true;
return false;
}
// Is expression used?
bool foundError = false;
visitAstNodes((parent->astOperand1() != tok2) ? parent->astOperand1() : parent->astOperand2(), [&](const Token *tok3) {
if (tok3->str() == "&" && !tok3->astOperand2())
return ChildrenToVisit::none; // don't handle address-of for now
if (tok3->str() == "(" && Token::simpleMatch(tok3->previous(), "sizeof"))
return ChildrenToVisit::none; // don't care about sizeof usage
if (isSameExpression(false, tok->astOperand1(), tok3, settings, true, false))
foundError = true;
return foundError ? ChildrenToVisit::done : ChildrenToVisit::op1_and_op2;
});

void CheckOther::checkEvaluationOrder()
return foundError;
}

static bool checkEvaluationOrderCpp11(const Token * tok, const Token * tok2, const Token * parent, const Settings & settings)
{
// This checker is not written according to C++11 sequencing rules
if (mTokenizer->isCPP() && mSettings->standards.cpp >= Standards::CPP11)
return;
if (tok->isAssignmentOp()) // TODO check assignment
return false;
if (tok->previous() == tok->astOperand1() && parent->isArithmeticalOp() && parent->isBinaryOp()) {
if (parent->astParent() && parent->astParent()->isAssignmentOp() && isSameExpression(false, tok->astOperand1(), parent->astParent()->astOperand1(), settings, true, false))
return true;
}
bool foundUndefined{false};
visitAstNodes((parent->astOperand1() != tok2) ? parent->astOperand1() : parent->astOperand2(), [&](const Token *tok3) {
if (tok3->str() == "&" && !tok3->astOperand2())
return ChildrenToVisit::none; // don't handle address-of for now
if (tok3->str() == "(" && Token::simpleMatch(tok3->previous(), "sizeof"))
return ChildrenToVisit::none; // don't care about sizeof usage
if (isSameExpression(false, tok->astOperand1(), tok3, settings, true, false))
foundUndefined = true;
return foundUndefined ? ChildrenToVisit::done : ChildrenToVisit::op1_and_op2;
});

logChecker("CheckOther::checkEvaluationOrder"); // C/C++03
return foundUndefined;
}

static bool checkEvaluationOrderCpp17(const Token * tok, const Token * tok2, const Token * parent, const Settings & settings, bool & foundUnspecified)
{
if (tok->isAssignmentOp())
return false;
bool foundUndefined{false};
visitAstNodes((parent->astOperand1() != tok2) ? parent->astOperand1() : parent->astOperand2(), [&](const Token *tok3) {
if (tok3->str() == "&" && !tok3->astOperand2())
return ChildrenToVisit::none; // don't handle address-of for now
if (tok3->str() == "(" && Token::simpleMatch(tok3->previous(), "sizeof"))
return ChildrenToVisit::none; // don't care about sizeof usage
if (isSameExpression(false, tok->astOperand1(), tok3, settings, true, false) && parent->isArithmeticalOp() && parent->isBinaryOp())
foundUndefined = true;
if (tok3->tokType() == Token::eIncDecOp && isSameExpression(false, tok->astOperand1(), tok3->astOperand1(), settings, true, false)) {
if (parent->isArithmeticalOp() && parent->isBinaryOp())
foundUndefined = true;
else
foundUnspecified = true;
}
return (foundUndefined || foundUnspecified) ? ChildrenToVisit::done : ChildrenToVisit::op1_and_op2;
});

return foundUndefined || foundUnspecified;
}

void CheckOther::checkEvaluationOrder()
{
logChecker("CheckOther::checkEvaluationOrder");
const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase();
for (const Scope * functionScope : symbolDatabase->functionScopes) {
for (const Token* tok = functionScope->bodyStart; tok != functionScope->bodyEnd; tok = tok->next()) {
if (tok->tokType() != Token::eIncDecOp && !tok->isAssignmentOp())
if (!tok->isIncDecOp() && !tok->isAssignmentOp())
continue;
if (!tok->astOperand1())
continue;
Expand Down Expand Up @@ -3308,43 +3371,36 @@ void CheckOther::checkEvaluationOrder()
if (parent->str() == "(" && parent->astOperand2())
break;

// self assignment..
if (tok2 == tok &&
tok->str() == "=" &&
parent->str() == "=" &&
isSameExpression(false, tok->astOperand1(), parent->astOperand1(), *mSettings, true, false)) {
if (mSettings->severity.isEnabled(Severity::warning) &&
isSameExpression(true, tok->astOperand1(), parent->astOperand1(), *mSettings, true, false))
selfAssignmentError(parent, tok->astOperand1()->expressionString());
break;
bool foundError{false}, foundUnspecified{false}, bSelfAssignmentError{false};
if (mTokenizer->isCPP() && mSettings->standards.cpp >= Standards::CPP11) {
if (mSettings->standards.cpp >= Standards::CPP17)
foundError = checkEvaluationOrderCpp17(tok, tok2, parent, *mSettings, foundUnspecified);
else
foundError = checkEvaluationOrderCpp11(tok, tok2, parent, *mSettings);
}

// Is expression used?
bool foundError = false;
visitAstNodes((parent->astOperand1() != tok2) ? parent->astOperand1() : parent->astOperand2(),
[&](const Token *tok3) {
if (tok3->str() == "&" && !tok3->astOperand2())
return ChildrenToVisit::none; // don't handle address-of for now
if (tok3->str() == "(" && Token::simpleMatch(tok3->previous(), "sizeof"))
return ChildrenToVisit::none; // don't care about sizeof usage
if (isSameExpression(false, tok->astOperand1(), tok3, *mSettings, true, false))
foundError = true;
return foundError ? ChildrenToVisit::done : ChildrenToVisit::op1_and_op2;
});
else
foundError = checkEvaluationOrderC(tok, tok2, parent, *mSettings, bSelfAssignmentError);

if (foundError) {
unknownEvaluationOrder(parent);
unknownEvaluationOrder(parent, foundUnspecified);
break;
}
if (bSelfAssignmentError) {
selfAssignmentError(parent, tok->astOperand1()->expressionString());
break;
}
}
}
}
}

void CheckOther::unknownEvaluationOrder(const Token* tok)
void CheckOther::unknownEvaluationOrder(const Token* tok, bool isUnspecifiedBehavior)
{
reportError(tok, Severity::error, "unknownEvaluationOrder",
"Expression '" + (tok ? tok->expressionString() : std::string("x = x++;")) + "' depends on order of evaluation of side effects", CWE768, Certainty::normal);
isUnspecifiedBehavior ?
reportError(tok, Severity::portability, "unknownEvaluationOrder",
"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)
: reportError(tok, Severity::error, "unknownEvaluationOrder",
"Expression '" + (tok ? tok->expressionString() : std::string("x = x++;")) + "' depends on order of evaluation of side effects", CWE768, Certainty::normal);
}

void CheckOther::checkAccessOfMovedVariable()
Expand Down
2 changes: 1 addition & 1 deletion lib/checkother.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,7 @@ class CPPCHECKLIB CheckOther : public Check {
void redundantPointerOpError(const Token* tok, const std::string& varname, bool inconclusive, bool addressOfDeref);
void raceAfterInterlockedDecrementError(const Token* tok);
void unusedLabelError(const Token* tok, bool inSwitch, bool hasIfdef);
void unknownEvaluationOrder(const Token* tok);
void unknownEvaluationOrder(const Token* tok, bool isUnspecifiedBehavior = false);
void accessMovedError(const Token *tok, const std::string &varname, const ValueFlow::Value *value, bool inconclusive);
void funcArgNamesDifferent(const std::string & functionName, nonneg int index, const Token* declaration, const Token* definition);
void funcArgOrderDifferent(const std::string & functionName, const Token * declaration, const Token * definition, const std::vector<const Token*> & declarations, const std::vector<const Token*> & definitions);
Expand Down
45 changes: 45 additions & 0 deletions test/testother.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10805,6 +10805,28 @@ class TestOther : public TestFixture {
ASSERT_EQUALS("[test.cpp:6]: (style) Label 'label' is not used.\n", errout_str());
}

#define checkCustomSettings(...) checkCustomSettings_(__FILE__, __LINE__, __VA_ARGS__)
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) {
if (!settings) {
settings = &_settings;
}
settings->certainty.setEnabled(Certainty::inconclusive, inconclusive);
settings->verbose = verbose;

// Tokenize..
SimpleTokenizer tokenizer(*settings, *this);
ASSERT_LOC(tokenizer.tokenize(code, cpp), file, line);

// Check..
runChecks<CheckOther>(tokenizer, this);

(void)runSimpleChecks; // TODO Remove this
}

void checkCustomSettings_(const char* file, int line, const char code[], Settings *s) {
checkCustomSettings_(file, line, code, true, true, true, false, s);
}

void testEvaluationOrder() {
check("void f() {\n"
" int x = dostuff();\n"
Expand Down Expand Up @@ -10848,6 +10870,29 @@ class TestOther : public TestFixture {
" a[x+y] = a[y+x]++;;\n"
"}\n", false);
ASSERT_EQUALS("[test.c:3]: (error) Expression 'a[x+y]=a[y+x]++' depends on order of evaluation of side effects\n", errout_str());

check("void f(int i) {\n"
" int n = ++i + i;\n"
"}");
ASSERT_EQUALS("[test.cpp:2]: (error) Expression '++i+i' depends on order of evaluation of side effects\n", errout_str());

check("long int f1(const char *exp) {\n"
" return dostuff(++exp, ++exp, 10);\n"
"}");
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"
"[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());

check("void f(int i) {\n"
" int n = (~(-(++i)) + i);\n"
"}");
ASSERT_EQUALS("[test.cpp:2]: (error) Expression '~(-(++i))+i' depends on order of evaluation of side effects\n", errout_str());

/*const*/ Settings settings11 = settingsBuilder(_settings).cpp(Standards::CPP11).build();

checkCustomSettings("void f(int i) {\n"
" i = i++ + 2;\n"
"}", &settings11);
ASSERT_EQUALS("[test.cpp:2]: (error) Expression 'i+++2' depends on order of evaluation of side effects\n", errout_str());
}

void testEvaluationOrderSelfAssignment() {
Expand Down

0 comments on commit 9d4d118

Please sign in to comment.