Skip to content

Commit

Permalink
fixed #11202/#11199/#11201/#12330/#12774 / refs #13508/#11200/#13506 …
Browse files Browse the repository at this point in the history 
…- fixed various floating-point comparison regressions (#7148)

Co-authored-by: Paul Fultz II <[email protected]>
  • Loading branch information
@firewave @pfultz2
firewave and pfultz2 authored Jan 16, 2025
1 parent 74da655 commit d777511
Show file tree
Hide file tree
Showing 5 changed files with 267 additions and 11 deletions.
4 changes: 2 additions & 2 deletions lib/calculate.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,14 +63,14 @@ R calculate(const std::string& s, const T& x, const T& y, bool* error = nullptr)
case '*':
return wrap(x * y);
case '/':
if (isZero(y) || (std::is_integral<T>{} && std::is_signed<T>{} && isEqual(y, T(-1)) && isEqual(x, std::numeric_limits<T>::min()))) {
if (isZero(y) || (std::is_signed<T>{} && y < 0)) {
if (error)
*error = true;
return R{};
}
return wrap(x / y);
case '%':
if (isZero(MathLib::bigint(y)) || (std::is_integral<T>{} && std::is_signed<T>{} && isEqual(y, T(-1)) && isEqual(x, std::numeric_limits<T>::min()))) {
if (isZero(MathLib::bigint(y)) || (std::is_signed<T>{} && MathLib::bigint(y) < 0)) {
if (error)
*error = true;
return R{};
Expand Down
19 changes: 15 additions & 4 deletions lib/vf_settokenvalue.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -496,6 +496,7 @@ namespace ValueFlow
}
const double floatValue1 = value1.isFloatValue() ? value1.floatValue : static_cast<double>(value1.intvalue);
const double floatValue2 = value2.isFloatValue() ? value2.floatValue : static_cast<double>(value2.intvalue);
const bool isFloat = value1.isFloatValue() || value2.isFloatValue();
const auto intValue1 = [&]() -> MathLib::bigint {
return value1.isFloatValue() ? static_cast<MathLib::bigint>(value1.floatValue) : value1.intvalue;
};
Expand Down Expand Up @@ -550,17 +551,27 @@ namespace ValueFlow
setTokenValue(parent, std::move(result), settings);
} else if (Token::Match(parent, "%op%")) {
if (Token::Match(parent, "%comp%")) {
if (!result.isFloatValue() && !value1.isIntValue() && !value2.isIntValue())
if (!isFloat && !value1.isIntValue() && !value2.isIntValue())
continue;
} else {
if (value1.isTokValue() || value2.isTokValue())
break;
}
bool error = false;
if (result.isFloatValue()) {
result.floatValue = calculate(parent->str(), floatValue1, floatValue2, &error);
if (isFloat) {
auto val = calculate(parent->str(), floatValue1, floatValue2, &error);
if (result.isFloatValue()) {
result.floatValue = val;
} else {
result.intvalue = static_cast<MathLib::bigint>(val);
}
} else {
result.intvalue = calculate(parent->str(), intValue1(), intValue2(), &error);
auto val = calculate(parent->str(), intValue1(), intValue2(), &error);
if (result.isFloatValue()) {
result.floatValue = static_cast<double>(val);
} else {
result.intvalue = val;
}
}
if (error)
continue;
Expand Down
132 changes: 129 additions & 3 deletions test/testcondition.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ class TestCondition : public TestFixture {
TEST_CASE(knownConditionIncrementLoop); // #9808
TEST_CASE(knownConditionAfterBailout); // #12526
TEST_CASE(knownConditionIncDecOperator);
TEST_CASE(knownConditionFloating);
}

#define check(...) check_(__FILE__, __LINE__, __VA_ARGS__)
Expand Down Expand Up @@ -4464,9 +4465,10 @@ class TestCondition : public TestFixture {
" float f = 9.9f;\n"
" if(f < 10) {}\n"
"}\n");
ASSERT_EQUALS("[test.cpp:3]: (style) Condition 'i>9.9' is always true\n"
"[test.cpp:5]: (style) Condition 'f<10' is always true\n",
errout_str());
ASSERT_EQUALS(
"[test.cpp:3]: (style) Condition 'i>9.9' is always true\n"
"[test.cpp:5]: (style) Condition 'f<10' is always true\n",
errout_str());
check("constexpr int f() {\n" // #11238
" return 1;\n"
"}\n"
Expand Down Expand Up @@ -5770,6 +5772,14 @@ class TestCondition : public TestFixture {
" if (other.mPA.cols > 0 && other.mPA.rows > 0)\n"
" ;\n"
"}");
ASSERT_EQUALS("", errout_str());

check("void foo() {\n" // #11202
" float f = 0x1.4p+3;\n"
" if (f > 10.0) {}\n"
" if (f < 10.0) {}\n"
"}\n");
ASSERT_EQUALS("", errout_str());
}

void checkInvalidTestForOverflow() {
Expand Down Expand Up @@ -6229,6 +6239,122 @@ class TestCondition : public TestFixture {
"}\n");
ASSERT_EQUALS("", errout_str());
}

void knownConditionFloating() {
check("void foo() {\n" // #11199
" float f = 1.0;\n"
" if (f > 1.0f) {}\n"
"}\n");
ASSERT_EQUALS("[test.cpp:3]: (style) Condition 'f>1.0f' is always false\n", errout_str());

check("void foo() {\n" // #11199
" float f = 1.0;\n"
" if (f > 1.0L) {}\n"
"}\n");
ASSERT_EQUALS("[test.cpp:3]: (style) Condition 'f>1.0L' is always false\n", errout_str());

check("void foo() {\n" // #11199
" float f = 1.0f;\n"
" if (f > 1.0) {}\n"
"}\n");
ASSERT_EQUALS("[test.cpp:3]: (style) Condition 'f>1.0' is always false\n", errout_str());

check("void foo() {\n" // #11199
" float f = 1.0f;\n"
" if (f > 1.0L) {}\n"
"}\n");
ASSERT_EQUALS("[test.cpp:3]: (style) Condition 'f>1.0L' is always false\n", errout_str());

check("void foo() {\n" // #11199
" float f = 1.0L;\n"
" if (f > 1.0) {}\n"
"}\n");
ASSERT_EQUALS("[test.cpp:3]: (style) Condition 'f>1.0' is always false\n", errout_str());

check("void foo() {\n" // #11199
" float f = 1.0L;\n"
" if (f > 1.0f) {}\n"
"}\n");
ASSERT_EQUALS("[test.cpp:3]: (style) Condition 'f>1.0f' is always false\n", errout_str());

check("void foo() {\n" // #11201
" float f = 0x1.4p+3;\n" // hex fraction 1.4 (decimal 1.25) scaled by 2^3, that is 10.0
" if (f > 9.9) {}\n"
" if (f < 9.9) {}\n"
"}\n");
ASSERT_EQUALS(
"[test.cpp:3]: (style) Condition 'f>9.9' is always true\n"
"[test.cpp:4]: (style) Condition 'f<9.9' is always false\n",
errout_str());

check("void foo() {\n" // #12330
" double d = 1.0;\n"
" if (d < 0.0) {}\n"
"}\n");
ASSERT_EQUALS("[test.cpp:3]: (style) Condition 'd<0.0' is always false\n", errout_str());

check("void foo() {\n" // #12330
" long double ld = 1.0;\n"
" if (ld < 0.0) {}\n"
"}\n");
ASSERT_EQUALS("[test.cpp:3]: (style) Condition 'ld<0.0' is always false\n", errout_str());

check("void foo() {\n" // #12330
" float f = 1.0;\n"
" if (f < 0.0) {}\n"
"}\n");
ASSERT_EQUALS("[test.cpp:3]: (style) Condition 'f<0.0' is always false\n", errout_str());

check("void foo() {\n" // #12774
" float f = 1.0f;\n"
" if (f > 1.01f) {}\n"
"}\n");
ASSERT_EQUALS(
"[test.cpp:3]: (style) Condition 'f>1.01f' is always false\n",
errout_str());

check("void foo() {\n" // #12774
" float f = 1.0;\n"
" if (f > 1.01) {}\n"
"}\n");
ASSERT_EQUALS(
"[test.cpp:3]: (style) Condition 'f>1.01' is always false\n",
errout_str());

check("void foo() {\n"
" float f = 1.0f;\n"
" if (f > 1) {}\n"
"}\n");
ASSERT_EQUALS(
"[test.cpp:3]: (style) Condition 'f>1' is always false\n",
errout_str());

check("void foo() {\n" // #13508
" float f = 1.0f;\n"
" if (f > 1.00f) {}\n"
"}\n");
TODO_ASSERT_EQUALS(
"[test.cpp:3]: (style) Condition 'f>1.00f' is always false\n",
"",
errout_str());

check("void foo() {\n"
" float f = 1.0;\n"
" if (f > 1) {}\n"
"}\n");
ASSERT_EQUALS(
"[test.cpp:3]: (style) Condition 'f>1' is always false\n",
errout_str());

check("void foo() {\n"// #13508
" float f = 1.0;\n"
" if (f > 1.00) {}\n"
"}\n");
TODO_ASSERT_EQUALS(
"[test.cpp:3]: (style) Condition 'f>1.00' is always false\n",
"",
errout_str());
}
};

REGISTER_TEST(TestCondition)
99 changes: 99 additions & 0 deletions test/testother.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,8 @@ class TestOther : public TestFixture {

TEST_CASE(knownPointerToBool);
TEST_CASE(iterateByValue);

TEST_CASE(alwaysTrueFloating);
}

#define check(...) check_(__FILE__, __LINE__, __VA_ARGS__)
Expand Down Expand Up @@ -12885,6 +12887,103 @@ class TestOther : public TestFixture {
ASSERT_EQUALS("[test.cpp:3]: (performance) Range variable 's' should be declared as const reference.\n",
errout_str());
}

void alwaysTrueFloating()
{
check("void foo() {\n" // #11200
" float f = 1.0;\n"
" if (f > 1.0) {}\n"
" if (f > -1.0) {}\n"
"}\n");
TODO_ASSERT_EQUALS(
"[test.cpp:2] -> [test.cpp:3]: (style) The comparison 'f > 1.0' is always false.\n"
"[test.cpp:2] -> [test.cpp:4]: (style) The comparison 'f > -1.0' is always false.\n",
"[test.cpp:2] -> [test.cpp:3]: (style) The comparison 'f > 1.0' is always false.\n",
errout_str());

check("void foo() {\n" // #13506
" float f = 1.0;\n"
" if (f > +1.0) {}\n"
"}\n");
TODO_ASSERT_EQUALS(
"[test.cpp:2] -> [test.cpp:3]: (style) The comparison 'f > +1.0' is always false.\n",
"",
errout_str());

check("void foo() {\n" // #11200
" float pf = +1.0;\n"
" if (pf > 1.0) {}\n"
" if (pf > -1.0) {}\n"
"}\n");
TODO_ASSERT_EQUALS(
"[test.cpp:2] -> [test.cpp:3]: (style) The comparison 'pf > 1.0' is always false.\n"
"[test.cpp:2] -> [test.cpp:4]: (style) The comparison 'pf > -1.0' is always false.\n",
"[test.cpp:2] -> [test.cpp:3]: (style) The comparison 'pf > 1.0' is always false.\n",
errout_str());

check("void foo() {\n" // #13506
" float pf = +1.0;\n"
" if (pf > +1.0) {}\n"
"}\n");
TODO_ASSERT_EQUALS(
"[test.cpp:2] -> [test.cpp:3]: (style) The comparison 'pf > +1.0' is always false.\n",
"",
errout_str());

check("void foo() {\n" // #11200
" float nf = -1.0;\n"
" if (nf > 1.0) {}\n"
" if (nf > -1.0) {}\n"
"}\n");
TODO_ASSERT_EQUALS(
"[test.cpp:2] -> [test.cpp:3]: (style) The comparison 'nf > 1.0' is always false.\n"
"[test.cpp:2] -> [test.cpp:4]: (style) The comparison 'nf > -1.0' is always false.\n",
"[test.cpp:2] -> [test.cpp:4]: (style) The comparison 'nf > -1.0' is always false.\n",
errout_str());

check("void foo() {\n" // #13506
" float nf = -1.0;\n"
" if (nf > +1.0) {}\n"
"}\n");
TODO_ASSERT_EQUALS(
"[test.cpp:2] -> [test.cpp:3]: (style) The comparison 'nf > +1.0' is always false.\n",
"",
errout_str());

check("void foo() {\n"
" float f = 1.0f;\n"
" if (f > 1.00f) {}\n"
"}\n");
ASSERT_EQUALS(
"[test.cpp:2] -> [test.cpp:3]: (style) The comparison 'f > 1.00f' is always false.\n",
errout_str());

check("void foo() {\n" // #13508
" float f = 1.0f;\n"
" if (f > 1) {}\n"
"}\n");
TODO_ASSERT_EQUALS(
"[test.cpp:2] -> [test.cpp:3]: (style) The comparison 'f > 1' is always false.\n",
"",
errout_str());

check("void foo() {\n"
" float f = 1.0;\n"
" if (f > 1.00) {}\n"
"}\n");
ASSERT_EQUALS(
"[test.cpp:2] -> [test.cpp:3]: (style) The comparison 'f > 1.00' is always false.\n",
errout_str());

check("void foo() {\n" // #13508
" float f = 1.0;\n"
" if (f > 1) {}\n"
"}\n");
TODO_ASSERT_EQUALS(
"[test.cpp:2] -> [test.cpp:3]: (style) The comparison 'f > 1' is always false.\n",
"",
errout_str());
}
};

REGISTER_TEST(TestOther)
24 changes: 22 additions & 2 deletions test/testvalueflow.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -951,15 +951,35 @@ class TestValueFlow : public TestFixture {
ASSERT_EQUALS(10, valueOfTok("x = static_cast<int>(10);", "( 10 )").intvalue);
ASSERT_EQUALS(0, valueOfTok("x = sizeof (struct {int a;}) * 0;", "*").intvalue);

// Don't calculate if there is UB
// Don't calculate or crash if there is UB or invalid operations
ASSERT(tokenValues(";-1<<10;","<<").empty());
ASSERT(tokenValues(";10<<-1;","<<").empty());
ASSERT(tokenValues(";10<<64;","<<").empty());
ASSERT(tokenValues(";-1>>10;",">>").empty());
ASSERT(tokenValues(";10>>-1;",">>").empty());
ASSERT(tokenValues(";10>>64;",">>").empty());
ASSERT_EQUALS(tokenValues(";1%-1;","%").size(), 1);
ASSERT_EQUALS(tokenValues(";1%-10;","%").size(), 1);
ASSERT_EQUALS(tokenValues(";1.5%-1;","%").size(), 1);
ASSERT_EQUALS(tokenValues(";1.5%-10;","%").size(), 1);
ASSERT(tokenValues(";1%-1.5;","%").empty());
ASSERT(tokenValues(";1%-10.5;","%").empty());
ASSERT(tokenValues(";1.5%-1.5;","%").empty());
ASSERT(tokenValues(";1.5%-10.5;","%").empty());
ASSERT(tokenValues(";1/-1;","/").empty());
ASSERT(tokenValues(";1/-10;","/").empty());
ASSERT(tokenValues(";1.5/-1;","/").empty());
ASSERT(tokenValues(";1.5/-10;","/").empty());
ASSERT(tokenValues(";1/-1.5;","/").empty());
ASSERT(tokenValues(";1/-10.5;","/").empty());
ASSERT(tokenValues(";1.5/-1.5;","/").empty());
ASSERT(tokenValues(";1.5/-10.5;","/").empty());
ASSERT(tokenValues(";1/0;","/").empty());
ASSERT(tokenValues(";1/0;","/").empty());
ASSERT(tokenValues(";1.5/0;","/").empty());
ASSERT(tokenValues(";1.5/0;","/").empty());
ASSERT(tokenValues(";((-1) * 9223372036854775807LL - 1) / (-1);", "/").empty()); // #12109
ASSERT_EQUALS(tokenValues(";((-1) * 9223372036854775807LL - 1) % (-1);", "%").size(), 1);
ASSERT_EQUALS(tokenValues(";((-1) * 9223372036854775807LL - 1) % (-1);", "%").size(), 1); // #12109

code = "float f(const uint16_t& value) {\n"
" const uint16_t uVal = value; \n"
Expand Down

0 comments on commit d777511

Please sign in to comment.