Skip to content
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

fixed #11202/#11199/#11201/#12330/#12774 / refs #13508/#11200/#13506 - fixed various floating-point comparison regressions #7148

Merged
merged 4 commits into from
Jan 16, 2025
Merged

fixed #11202/#11199/#11201/#12330/#12774 / refs #13508/#11200/#13506 - fixed various floating-point comparison regressions #7148

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a04fc3b
added tests for various floating-point comparison related regressions
firewave Aug 9, 2024
50c53e9
attempt to fix various floating-point comparison regressions
firewave Aug 8, 2024
ef3337d
always calculate as float
firewave Dec 31, 2024
f8fdc21
calculate.h: cleaned up way too specific protections against potentia…
firewave Jan 5, 2025
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
4 changes: 2 additions & 2 deletions lib/calculate.h
View file
Open in desktop
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)) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chrchr-github This crash fix from #5587 was way too specific.

if (error)
*error = true;
return R{};
Expand Down
19 changes: 15 additions & 4 deletions lib/vf_settokenvalue.cpp
View file
Open in desktop
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())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The !isFloat is redundant here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the same but it is not (this is actually the important change based on my older comment). It reflects if either is a float. That doesn't mean one of them cannot be an Int.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, To make it clearer dont use the isFloat variable, write it completely: if(!value1.isFloatValue() && !value2.isFloatValue() && !value1.isIntValue() && !value2.isIntValue())

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well - I took it from the original code.

continue;
} else {
if (value1.isTokValue() || value2.isTokValue())
break;
}
bool error = false;
if (result.isFloatValue()) {
result.floatValue = calculate(parent->str(), floatValue1, floatValue2, &error);
if (isFloat) {
firewave marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, looking at the test, we just need to run calculate as both float if one operand is float and convert to the result type int or float at the end.

Also rather than use lambdas(like intValue1) to do the conversion we should instead update the getIntValue and getFloatValue methods to do the conversion. This should make the code much simpler.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes the regression introduced by my change. Thanks.

I will take a look into the remaining false negatives tomorrow. There is also a few test variations I still like to add.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like everything has already been fixed. The existing false negatives do not appear to be related to the commit in question but a much earlier one. Will clean up the tests tomorrow.

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
Open in desktop
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
Open in desktop
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 @@ -12784,6 +12786,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
Open in desktop
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
Loading