Use of infinity() in code compiled with -ffast-math ( durationutiltest.cpp & frametest.cpp )

[JoergAtGithub]

Bug Description

If I compile with clang-tidy 18, I get the following errors, because we use infinity() in code that is compiled with option -ffast-math. And -ffast-math sets -ffinite-math-only.

 Error: /home/runner/work/mixxx/mixxx/src/test/durationutiltest.cpp:118:21: error: use of infinity is undefined behavior due to the currently enabled floating-point options [-Werror,-Wnan-infinity-disabled]
  118 |     formatTime("?", std::numeric_limits<double>::infinity());

Error: /home/runner/work/mixxx/mixxx/src/test/frametest.cpp:21:41: error: use of infinity is undefined behavior due to the currently enabled floating-point options [-Werror,-Wnan-infinity-disabled]
   21 |     EXPECT_FALSE(mixxx::audio::FramePos(std::numeric_limits<double>::infinity()).isValid());

 Error: /home/runner/work/mixxx/mixxx/src/test/frametest.cpp:57:36: error: use of infinity is undefined behavior due to the currently enabled floating-point options [-Werror,-Wnan-infinity-disabled]
   57 |             mixxx::audio::FramePos(std::numeric_limits<
      |                                    ^~~~~~~~~~~~~~~~~~~~
   58 |                     mixxx::audio::FramePos::value_t>::infinity()));

Error: /home/runner/work/mixxx/mixxx/src/test/frametest.cpp:61:22: error: use of infinity is undefined behavior due to the currently enabled floating-point options [-Werror,-Wnan-infinity-disabled]
   61 |                     -std::numeric_limits<
      |                      ^~~~~~~~~~~~~~~~~~~~
   62 |                             mixxx::audio::FramePos::value_t>::infinity()));

 Error: /home/runner/work/mixxx/mixxx/src/test/frametest.cpp:65:36: error: use of infinity is undefined behavior due to the currently enabled floating-point options [-Werror,-Wnan-infinity-disabled]
   65 |             mixxx::audio::FramePos(std::numeric_limits<
      |                                    ^~~~~~~~~~~~~~~~~~~~
   66 |                     mixxx::audio::FramePos::value_t>::infinity()));

Version

main

OS

Clang 18

[Swiftb0y]

likely fairly straightforward to fix using ::max() instead of ::infinity().

[Swiftb0y]

Do you have an idea on how to ensure we don't create an Inf anyway and construct a FramePos from it?

[JoergAtGithub]

For the durationutiltest.cpp it's pretty easy to use max() instead. But for the FramePos I'm unsure. I would've fixed it, if I would understand this.

[daschuer]

We have already a hack implemented for this in FpClassify

mixxx/CMakeLists.txt

Line 2546 in a716202

target_compile_options(FpClassify PRIVATE -ffp-contract=on -fno-fast-math)

So you may use infinity though that library to rely on a defined representation in the targets memory.

[JoergAtGithub]

Compiling this without -ffast-math as for FpClassify would remove the errors of cause. But I don't think this is a proper fix, as there seems to be no need to use slow math here.

[daschuer]

The solution is to add a wrapper for infinity() to pfclassyfy.cpp. We have already is_infinity() there which need to be used for the checks.

[Swiftb0y]

I don't think that works. Its UB the moment Inf enters some -ffast-math code. putting a wrapper into fpclassify essentially silences the error, because the compiler can't look through it anymore, but its just as unsafe!! At least thats my interpretation of it. Technically, the is_infinity thing we have is probably unnecessary at least (if is_infinity(v) is true for some v in -ffast-math code we probably caused undefined behavior already). So the entire thing concept of -ffast-math code interacting with non-fast-math code is fundamentally flawed unless we have non-fast-math code that ensures we never pass infinity to -ffast-math code. Checking for infinity in -ffast-math code is too late.

[daschuer]

Think of CPU Registers and memory. Just because the CPU executed code compiled with fast-math there is no effect to thesese even if there are infinity values.
The UB happens when the code interpreted the value it finds. The UB allows the compiler to just use the fastest path ignoring that a infinity value may exist. The result is whatever the hardware does in that case.
This does not happen if code with defined behavior in to fplassify.cpp does it and the Boolean result is than handled in the fast code. This is verified for all our targets with a unit test.

This "trick" has been introduced to hand elexactly the case you described, when we receive infinity from libraries not compiled with fast-math we can handle them via declassify.

[Swiftb0y]

We had this discussion before. If you program in C++, you're not programming against the CPU, you're programming against the C++ abstract machine. The entire point is that the compiler is free to change the behavior in cases which (from the viewpoint of the compiler) can't happen (such as Inf in `-ffast-math code). As such, the result is unfortunately not only whatever the hardware does but potentially completely different because the compiler may have emitted extra optimization code for this (under defined circumstances impossible) case.

The usecase with external libraries is the only reasonable use of fpclassify. Any operation on infinity in -ffast-math code is still UB regardless of whether the underlying hardware may be able to handle it. Your proposal of introducing Inf into fpclassify in order to use it in -ffast-math code does exactly the opposite. Instead of catching potential Infs, it creates them explicitly for usage in -ffast-math. That is why I consider that solution fundamentally flawed.

To conclude. See this code example where -ffast-math results in different code generation and fundamentally different behavior because of that. You can even test it out yourself where without fast-math, it will call nonUB() while with -ffast-math it can't do anything but call UB().

[daschuer]

If you program in C++, you're not programming against the CPU, you're programming against the C++ abstract machine.

Confirmed.

The entire point is that the compiler is free to change the behavior in cases which (from the viewpoint of the compiler) can't happen (such as Inf in `-ffast-math code).

Confirmed

As such, the result is unfortunately not only whatever the hardware does but potentially completely different because the compiler may have emitted extra optimization code for this (under defined circumstances impossible) case.

Confirmed.

The usecase with external libraries is the only reasonable use of fpclassify. Any operation on infinity in -ffast-math code is still UB regardless of whether the underlying hardware may be able to handle it.

Confirmed

Your proposal of introducing Inf into fpclassify in order to use it in -ffast-math code does exactly the opposite. Instead of catching potential Infs, it creates them explicitly for usage in -ffast-math.

No, it is not intended/possible to deal with inf in that code. All operations dealing with inf() need to be done via in the fpcallsify.cpp wrapper.

That is why I consider that solution fundamentally flawed.

Not really, we use anyway library functions for that, the additional wrapper "just" prevents inlining and optimizing away inf handling as shown in the compiler explorer. The fpcallsify.cpp wrapper makes sure the left asm representation in your compiler explorer of the C++ code is used.

[JoergAtGithub]

I agree with @Swiftb0y here, moving the code to fpcallsify.cpp would be an ugly workaround and no proper fix. It's our own code, that uses INF - therefore we can and should fix it, instead of moving it into the slow-math entity.

[Swiftb0y]

I see how @daschuer's proposal could work, but its really easy to accidentally make mistakes with it IMO. If we get an Inf and only non-fast-math code interacts with in fast-math code, its probably safe. However, we still need to ensure that no fast-math code does any FPU operation on it, which is really hard. So I still don't consider that solution particular viable.

[daschuer]

I don't mind which solution we pick. It is finally a question how easy and risky a logic change is.

For my understanding the issue is only to fix a new more strict and valid warning in code that still works because the UB is like we want. Wrapping it by a library with defined behaviour is therefore a valid fix. And any math operation on inf() fails anyway in any code.