Fixes several issues with FP exceptions, like domain errors, div-by-zero, NaN, etc.

[stolk]

This MR adds a toggle to run the vbe_test program with FP exceptions enabled.

This MR also fixes issues #2090 #2091 #2092 that were found with this facility:

FP overflow by multiplying FLT_MAX

Division by zero

Square roots of negative values.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: stolk / name: Bram Stolk (495fda9, 5190308, 60ef741, bc381ad)

[Idclip]

I wonder if we really want FE_OVERFLOW on by default. It's perfectly valid if it's handled and my understanding is it will fire regardless. For example, the change in Diagnostics.h:

maxVal2(_max < std::numeric_limits<ValueType>::max() ? _max*_max : _max)

can still overflow if _max is large. A better solution would be:

maxVal2([&](){ 
    auto m2 = _max*_max; 
    return std::isfinite(m2) ? m2 : std::numeric_limits<T>::max();
}())

But this will always trigger an overflow, even though the program is well formed. I guess it might still be useful to identify these cases - but as it causes an immediate abort it's probably not super useful.

Issue hit with FE_DIVBYZERO and FE_INVALID should (usually) always be avoided, so those definitely make sense.

[stolk]

First off: FLT_MAX is a special value to denote a lack of bounds, so will frequently be encountered. Values smaller than that, but larger than the sqrt(FLT_MAX) will not occur in practice.

They are either small, or they are FLT_MAX, nothing in between.

Hence the solution used here makes sense: special treatment of FLT_MAX, because it is a special case, used to signal something.

Second, I disagree on your comment on FE_OVERFLOW. It is important to catch these in unit tests.

NOTE: This change does not enable exceptions for production code, nor for applications, not for vdb_view, vdb_render, etc.

This change only (optionally) affects vdb_test, which does need to catch these cases.

[Idclip]

If you want to make the case that FLT_MAX is special that's fine, but that is not explicitly stated for many of our tools - and saying values are either small or FLT_MAX is a huge generalization.

Yes I'm aware this only impacts the tests. There are lots of places where we check for finite and branch accordingly. So having it fire in these situations will be annoying (assuming our tests actually hit these paths).

[danrbailey]

I think adding just FE_DIVBYZERO and FE_INVALID would be a good start. That's better than what we have right now. By yeah, making it default to OFF for the unit tests would be my vote.

I agree that there are cases where we should allow overflow/underflow to happen. That's something we can guard against so that we can still globally set these flags:

fenv_t env;
feholdexcept(&env);
// operation that can generate overflow
fesetenv(&env);

I'm not sure it's worth maintaining that level of strictness though.

[Idclip]

To be clear, I'm fine leaving this in so long as all our tests currently pass with it enabled. I'm not a fan of arbitrarily treating FLT_MAX as a special value in places where input bounds aren't well defined - but if your change is simply to achieve the prior then for this PR it makes sense

[stolk]

The tests will pass because VDB_TESTS_FPE is off by default.

This is a (very useful) tool for developers that want it. It finds real bugs.

Also, on breaking tests if enabled... I really can't see that as a negative: Breaking unit tests is good. It finds bugs. Having tests pass is not the goal: having correct code is the goal.

It forces a developer to tighten up the use of floating point values. It finds invalid assumptions made by the developer.

Code that produces FP overflows is suspect, and should be flagged: 3.402823466e+38F is a really big number. If your code exceeds it, you probably did something wrong. Just like the case it found, where an 'unbounded' special value was squared, e.g.

[stolk]

I have removed FE_OVERFLOW so that it no longer fires by default. It will only conditionally fire if you toggle FP exceptions on via cmake, and then set the env var OPENVDB_TEST_OVERFLOW at runtime. If not set, it will only fire on FP_INVALID and FP_DIVBYZERO thereby allowing code to overflow float values.

[stolk]

Can someone please approve workflows?

Also, I've addressed the MR feedback, I think this is ready to get merged.

[stolk]

@danrbailey @Idclip @kmuseth

Hello, this MR solves real bugs, and I have revised the MR according to feedback.

Can one of you please approve the MR?

Thanks.

[danrbailey]

I think adding just FE_DIVBYZERO and FE_INVALID would be a good start. That's better than what we have right now. By yeah, making it default to OFF for the unit tests would be my vote.

I agree that there are cases where we should allow overflow/underflow to happen. That's something we can guard against so that we can still globally set these flags:

fenv_t env;
feholdexcept(&env);
// operation that can generate overflow
fesetenv(&env);

I'm not sure it's worth maintaining that level of strictness though.

[stolk]

Ping