Fix undefined behaviour of infinity()

[daschuer]

This fixes the undefined behviour of of std::numeric_limits<double>::infinity()) in -ffastmath (-ffinite-math-only) code. By moving it to fclassify.cpp which is compiled without this flags leading to defined behavior. It makes sure that the IEEE infinity is returned which is 0x7FF0000000000000.

Clang 18 warns about this as use of infinity is undefined behavior due to the currently enabled floating-point options. This warning may indicate a real issue in case of aggressive optimization.

This solution not only mutes this warning as -Wnan-infinity-disabled. We have argued that we shall not use infinity at all, but we can't entirely prevent it because all UB does not mean infinity does not happen especially when we interact with non -ffastmath libraries.

Fixes: #13780

The discussion around using inf() as invalid value in this is orthogonal. We have test in place that verify that it is working for now. I consider a refactoring of it as unnecessary regression risk, but will not block an attempt. Just let's merge this as a fix for UB.

[daschuer]

Let me explain my view a bit more: https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html#index-ffinite-math-only

Allow optimizations for floating-point arithmetic that assume that arguments and results are not NaNs or +-Infs.

For my understanding that means, that all results from floating-point arithmetic where NaNs or +-Infs or ifs are involved are undefined behavior.

The compiler warns that using std::numeric_limits<double>::infinity()) is also undefined behavior. The movement to non -ffastmath code fixes this issue effectively without any doubts.

Copying float values around is not floating-point arithmetic, in addition there is no gain for optimization. So we can be sure that the result is well defined even in ffastMath code. It is just copying 0x7FF0000000000000 like any other data type.

The unittest helps to confirm that. If we follow you suggestion to not set the -ffastMath flag, the unittest doe no longer represents the Mixxx code where the fclassify functions are used in that way.

Changing the test falgs only affets the tests, where we see immediately any issues in our CI. So I would not worry that much.

Considering, this we may add a reinterprete cast test, checking the IEEE infinity raw value.

[Swiftb0y]

Right, I see that. My primary problem is that adding util_*_infinity to produce Inf in finite-math-only code makes it too easy to produce these values in contexts where they're not expected and thus produce UB. I agree that copying these values is probably safe, but its too easy to accidentally do something unsafe if you can synthesize that value anywhere. util_*_infinity is a prime example of what is commonly referred to as a "footgun" because its very hard to not do something stupid with it.

IMO the proper solution would be to create a separate opaque type, compiled without finite-math-only and if you want to use infinity, you can only do it through that type. Conversion from that type to double would then do that check. I would offer to create this type if you agree that this is a better solution.

[daschuer]

Right, I see that. My primary problem is that adding util_*_infinity to produce Inf in finite-math-only code makes it too easy to produce these values in contexts where they're not expected and thus produce UB.

I can confirm you concerns. This shall however not block this PR, because it just fixes an UB risk that already exists. Without the newly introduced clang-18 warning it is even more easy to do it but I can not really imagine how anyone wants to do math with infinity. The only case I can imagine is:

    double a; 
    double b;
    double c = a / b;
   if (c == infinity()) {
   ...
   }

However in case b is 0, the value of c is already UB before the comparison.

[Swiftb0y]

I can not really imagine how anyone wants to do math with infinity.

Right, it doesn't make sense when looking at it from a microscale, but once infinity passes function boundaries (which it does) its not as clear anymore. In the context of finite-math-only code, infinity needs to be treated just as careful as nullptr when doing operations on pointers. Only this time, checking for it is even more expensive...

[Swiftb0y]

in practice instead of messing with this hack, we can simply replace the usage of infinity as a sentinel value in FramePos with a different (safe) sentinel value and we're good. mixxx::Duration is basically deprecated and from what I can tell, there are no other uses anyway...

[daschuer]

We are here here in a stable branch. The work can be done in the main branch if you wish.

However I cannot follow your reasoning. Because division by zero is always there in ffastmath it is UB and without infinity. This patch does not relate to your concerns.

In the context of finite-math-only code, infinity needs to be treated just as careful as nullptr when doing operations on pointers. Only this time, checking for it is even more expensive...

That is not true. Independent from finite-math-only we must not do calculations that result in infinity, because it is a dead end. It is reasonable to check devisors for zero and similar before doing the calculation and go the safe branch.

The difference is that the compiler does not check for it in such code. It assumes inf does not exist and the result is whatever the CPU does with it.

You see. There is no issue here and all expectations are verified by unittest.

[JoergAtGithub]

util_*_infinity is a prime example of what is commonly referred to as a "footgun" because its very hard to not do something stupid with it.

As we call it only from the unit test cases, why not use 0x7FF0000000000000 as expected value there directly? Than we don't need the dangling "footgun" in the code of the application.

[Swiftb0y]

just document that its only supposed to be used in test code. Then I'm fine with it.

[daschuer]

The CI Failure is fixed here: https://github.com/mixxxdj/mixxx/pull/13904

[Swiftb0y]

yeah, should be good regardless.