Skip to content

Two solution proposals for TRAC #7567 ("(a | 7) > 6U" is always true ...) #816

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
bartlomiejgrzeskowiak wants to merge 1 commit into from
Closed

Two solution proposals for TRAC #7567 ("(a | 7) > 6U" is always true ...) #816

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 48 additions & 29 deletions lib/checkcondition.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,36 +281,55 @@ void CheckCondition::comparison()
return;

for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) {
if (Token::Match(tok, "==|!=|>")) {
const Token *expr1 = tok->astOperand1();
const Token *expr2 = tok->astOperand2();
if (!expr1 || !expr2)
continue;
if (expr1->isNumber())
std::swap(expr1,expr2);
if (!expr2->isNumber())
continue;
const MathLib::bigint num2 = MathLib::toLongNumber(expr2->str());
if (num2 < 0)
continue;
if (!Token::Match(expr1,"[&|]"))
if (!tok->isComparisonOp())
continue;

const Token *expr1 = tok->astOperand1();
const Token *expr2 = tok->astOperand2();
if (!expr1 || !expr2)
continue;
if (expr1->isNumber())
std::swap(expr1,expr2);
if (!expr2->isNumber())
continue;
const MathLib::bigint num2 = MathLib::toLongNumber(expr2->str());
if (num2 < 0)
continue;
if (!Token::Match(expr1,"[&|]"))
continue;
std::list<MathLib::bigint> numbers;
getnumchildren(expr1, numbers);
for (std::list<MathLib::bigint>::const_iterator num = numbers.begin(); num != numbers.end(); ++num) {
const MathLib::bigint num1 = *num;
if (num1 < 0)
continue;
std::list<MathLib::bigint> numbers;
getnumchildren(expr1, numbers);
for (std::list<MathLib::bigint>::const_iterator num = numbers.begin(); num != numbers.end(); ++num) {
const MathLib::bigint num1 = *num;
if (num1 < 0)
continue;
if (Token::Match(tok, "==|!=")) {
if ((expr1->str() == "&" && (num1 & num2) != num2) ||
(expr1->str() == "|" && (num1 | num2) != num2)) {
const std::string& op(tok->str());
comparisonError(expr1, expr1->str(), num1, op, num2, op=="==" ? false : true);
}
} else if (Token::simpleMatch(tok, ">")) {
if ((expr1->str() == "&" && (num1 <= num2))) {
const std::string& op(tok->str());
comparisonError(expr1, expr1->str(), num1, op, num2, false);
if (Token::Match(tok, "==|!=")) {
if ((expr1->str() == "&" && (num1 & num2) != num2) ||
(expr1->str() == "|" && (num1 | num2) != num2)) {
const std::string& op(tok->str());
comparisonError(expr1, expr1->str(), num1, op, num2, op=="==" ? false : true);
}
} else if (expr1->str() == "&") {
const bool or_equal = Token::Match(tok, ">=|<=");
const std::string& op(tok->str());
if ((Token::Match(tok, ">=|<")) && (num1 < num2)) {
comparisonError(expr1, expr1->str(), num1, op, num2, or_equal ? false : true);
} else if ((Token::Match(tok, "<=|>")) && (num1 <= num2)) {
comparisonError(expr1, expr1->str(), num1, op, num2, or_equal ? true : false);
}
} else if (expr1->str() == "|") {
if ((expr1->astOperand1()->valueType()) &&
(expr1->astOperand1()->valueType()->sign == ValueType::Sign::UNSIGNED)) {
const bool or_equal = Token::Match(tok, ">=|<=");
const std::string& op(tok->str());
if ((Token::Match(tok, ">=|<")) && (num1 >= num2)) {
//"(a | 0x07) >= 7U" is always true for unsigned a
//"(a | 0x07) < 7U" is always false for unsigned a
comparisonError(expr1, expr1->str(), num1, op, num2, or_equal ? true : false);
} else if ((Token::Match(tok, "<=|>")) && (num1 > num2)) {
//"(a | 0x08) <= 7U" is always false for unsigned a
//"(a | 0x07) > 6U" is always true for unsigned a
comparisonError(expr1, expr1->str(), num1, op, num2, or_equal ? false : true);
}
}
}
Expand Down
153 changes: 59 additions & 94 deletions test/testcondition.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ class TestCondition : public TestFixture {

TEST_CASE(assignAndCompare); // assignment and comparison don't match
TEST_CASE(mismatchingBitAnd); // overlapping bitmasks
TEST_CASE(compare); // mismatching LHS/RHS in comparison
TEST_CASE(comparison); // CheckCondition::comparison test cases
TEST_CASE(multicompare); // mismatching comparisons
TEST_CASE(duplicateIf); // duplicate conditions in if and else-if

Expand Down Expand Up @@ -318,30 +318,64 @@ class TestCondition : public TestFixture {
ASSERT_EQUALS("", errout.str());
}

void compare() {
check("void foo(int x)\n"
"{\n"
" if ((x & 4) == 3);\n"
"}");
ASSERT_EQUALS("[test.cpp:3]: (style) Expression '(X & 0x4) == 0x3' is always false.\n", errout.str());

check("void foo(int x)\n"
"{\n"
" if ((x | 4) == 3);\n"
"}");
ASSERT_EQUALS("[test.cpp:3]: (style) Expression '(X | 0x4) == 0x3' is always false.\n", errout.str());

check("void foo(int x)\n"
"{\n"
" if ((x | 4) != 3);\n"
"}");
ASSERT_EQUALS("[test.cpp:3]: (style) Expression '(X | 0x4) != 0x3' is always true.\n", errout.str());

check("void foo(int x)\n"
"{\n"
" if ((x & y & 4 & z ) == 3);\n"
"}");
ASSERT_EQUALS("[test.cpp:3]: (style) Expression '(X & 0x4) == 0x3' is always false.\n", errout.str());
void comparison() {
// CheckCondition::comparison test cases
// '=='
check("void f(int a) {\n assert( (a & 0x07) == 8U );\n}");
ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X & 0x7) == 0x8' is always false.\n",errout.str());
check("void f(int a) {\n assert( (a & b & 4 & c ) == 3 );\n}");
ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X & 0x4) == 0x3' is always false.\n", errout.str());
check("void f(int a) {\n assert( (a | 0x07) == 8U );\n}");
ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X | 0x7) == 0x8' is always false.\n",errout.str());
check("void f(int a) {\n assert( (a & 0x07) == 7U );\n}");
ASSERT_EQUALS("", errout.str());
check("void f(int a) {\n assert( (a | 0x01) == -15 );\n}");
ASSERT_EQUALS("", errout.str());
// '!='
check("void f(int a) {\n assert( (a & 0x07) != 8U );\n}");
ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X & 0x7) != 0x8' is always true.\n",errout.str());
check("void f(int a) {\n assert( (a | 0x07) != 8U );\n}");
ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X | 0x7) != 0x8' is always true.\n",errout.str());
check("void f(int a) {\n assert( (a & 0x07) != 7U );\n}");
ASSERT_EQUALS("", errout.str());
check("void f(int a) {\n assert( (a | 0x07) != 7U );\n}");
ASSERT_EQUALS("", errout.str());
// '>='
check("void f(int a) {\n assert( (a & 0x07) >= 8U );\n}");
ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X & 0x7) >= 0x8' is always false.\n",errout.str());
check("void f(unsigned int a) {\n assert( (a | 0x7) >= 7U );\n}");
ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X | 0x7) >= 0x7' is always true.\n",errout.str());
check("void f(int a) {\n assert( (a & 0x07) >= 7U );\n}");
ASSERT_EQUALS("",errout.str());
check("void f(int a) {\n assert( (a | 0x07) >= 8U );\n}");
ASSERT_EQUALS("",errout.str()); //correct for negative 'a'
// '>'
check("void f(int a) {\n assert( (a & 0x07) > 7U );\n}");
ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X & 0x7) > 0x7' is always false.\n",errout.str());
check("void f(unsigned int a) {\n assert( (a | 0x7) > 6U );\n}");
ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X | 0x7) > 0x6' is always true.\n",errout.str());
check("void f(int a) {\n assert( (a & 0x07) > 6U );\n}");
ASSERT_EQUALS("",errout.str());
check("void f(int a) {\n assert( (a | 0x07) > 7U );\n}");
ASSERT_EQUALS("",errout.str()); //correct for negative 'a'
// '<='
check("void f(int a) {\n assert( (a & 0x07) <= 7U );\n}");
ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X & 0x7) <= 0x7' is always true.\n",errout.str());
check("void f(unsigned int a) {\n assert( (a | 0x08) <= 7U );\n}");
ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X | 0x8) <= 0x7' is always false.\n",errout.str());
check("void f(int a) {\n assert( (a & 0x07) <= 6U );\n}");
ASSERT_EQUALS("",errout.str());
check("void f(int a) {\n assert( (a | 0x08) <= 7U );\n}");
ASSERT_EQUALS("",errout.str()); //correct for negative 'a'
// '<'
check("void f(int a) {\n assert( (a & 0x07) < 8U );\n}");
ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X & 0x7) < 0x8' is always true.\n",errout.str());
check("void f(unsigned int a) {\n assert( (a | 0x07) < 7U );\n}");
ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X | 0x7) < 0x7' is always false.\n",errout.str());
check("void f(int a) {\n assert( (a & 0x07) < 3U );\n}");
ASSERT_EQUALS("",errout.str());
check("void f(int a) {\n assert( (a | 0x07) < 7U );\n}");
ASSERT_EQUALS("",errout.str()); //correct for negative 'a'
}

void multicompare() {
Expand Down Expand Up @@ -592,75 +626,6 @@ class TestCondition : public TestFixture {
" }\n"
"}");
ASSERT_EQUALS("", errout.str());

//Various bitmask comparison checks
//#7428 false negative: condition '(a&7)>7U' is always false
//#7522 false positive: condition '(X|7)>=6' is correct

check("void f() {\n"
"assert( (a & 0x07) == 8U );\n" // statement always false
"}");
ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X & 0x7) == 0x8' is always false.\n",
errout.str());

check("void f() {\n"
"assert( (a & 0x07) == 7U );\n" // statement correct
"assert( (a & 0x07) == 6U );\n" // statement correct
"}");
ASSERT_EQUALS("", errout.str());

check("void f() {\n"
"assert( (a | 0x07) == 8U );\n" // statement always false
"}");
ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X | 0x7) == 0x8' is always false.\n",
errout.str());

check("void f() {\n"
"assert( (a | 0x07) == 7U );\n" // statement correct
"assert( (a | 0x07) == 23U );\n" // statement correct
"}");
ASSERT_EQUALS("", errout.str());

check("void f() {\n"
"assert( (a & 0x07) != 8U );\n" // statement always true
"}");
ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X & 0x7) != 0x8' is always true.\n",
errout.str());

check("void f() {\n"
"assert( (a & 0x07) != 7U );\n" // statement correct
"assert( (a & 0x07) != 0U );\n" // statement correct
"}");
ASSERT_EQUALS("", errout.str());

check("void f() {\n"
"assert( (a | 0x07) != 8U );\n" // statement always true
"}");
ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X | 0x7) != 0x8' is always true.\n",
errout.str());

check("void f() {\n"
"assert( (a | 0x07) != 7U );\n" // statement correct
"}");
ASSERT_EQUALS("", errout.str());

//TRAC #7428 false negative: condition '(X&7)>7' is always false
check("void f() {\n"
"assert( (a & 0x07) > 7U );\n" // statement always false
"}");
ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X & 0x7) > 0x7' is always false.\n",
errout.str());

check("void f() {\n"
"assert( (a & 0x07) > 6U );\n" // statement correct
"}");
ASSERT_EQUALS("", errout.str());

//TRAC #7522 false positive: condition '(X|7)>=6' is correct (X can be negative)
check("void f() {\n"
"assert( (a | 0x07) >= 6U );\n" // statement correct (X can be negative)
"}");
ASSERT_EQUALS("",errout.str());
}


Expand Down