Add `USE_FLOAT_EXCEPTIONS` to enable floating point exceptions #1451

illwieckz · 2024-12-02T11:05:51Z

Add USE_FLOAT_EXCEPTIONS to enable floating point exceptions (disabled by default)
Add USE_FAST_MATH to enable or disable fast math (enabled by default)

When USE_FLOAT_EXCEPTIONS is used, nothing is done unless some of those cvars are enabled:

common.floatExceptions.invalid
common.floatExceptions.divByZero
common.floatExceptions.overflow

The USE_FLOAT_EXCEPTIONS option is required to specialize the build to make it possible to enable them.

The common.floatExceptions.test cvar enables some code doing bad floating point operations on purpose to test how they are handled.

…vars to enable floating point exceptions

illwieckz · 2024-12-02T11:08:07Z

I have not tested the Apple and MSVC code.

src/engine/framework/System.cpp

illwieckz · 2024-12-04T08:32:45Z

I renamed the cmake option to USE_FLOAT_EXCEPTIONS.

src/engine/framework/System.cpp

slipher · 2024-12-04T14:16:33Z

src/engine/framework/System.cpp

+static void SetFloatingPointExceptions()
+{
+	// Must be done after Sys::Init() to read cvars from command line.
+	#if defined(DAEMON_USE_FLOAT_EXCEPTIONS_AVAILABLE)


Our code style is to have these on the left side and never indent them based on indentation of non-preprocessor code.

It's bad practice within a block, really.

What? This style is used very consistently in our code and is common in other code bases too. Preprocessor and non-preprocessor lines do not syntactically nest with each other, so it makes a sort of sense that neither one affects the other's indentation.

When the ifdef is just a precompiled replacement for a test that would be doable at run time, it's better to keep them readable the same way, really.

Not indenting some ifdef is only a good solution when something can't be indented properly, like in a single operation, function call with optional parameters, or things like that…

We better use nested ifdef when it's possible, it makes things much more readable.

I modified the code to not indent the first level of ifdef, but keeping the other indentations makes it it much more readable.

This has been done the same way throughout the history of the codebase so we should keep doing it that way instead of worsening the mixture of styles, regardless of some arguments that a different one would be better if starting from scratch.

slipher · 2024-12-04T14:17:15Z

src/engine/framework/System.cpp

+			unsigned int exceptions = 0;
+		#endif
+
+		// Operations with NaN.


Not very accurate, cos(1.001) also does this for example

Are you sure?

Warn: Computing √-1… Warn: Result of √-1: -nan Warn: Computing cos(1.001)… Warn: Result of cos(1.001): 0.539

And the invalid exception isn't caught with cos(1.001), but is caught with sqrt(-1).

Typo, I meant acos (inverse cosine)

cmake/DaemonFlags.cmake

slipher · 2024-12-05T00:11:39Z

cmake/DaemonFlags.cmake

@@ -148,13 +148,20 @@ else()
    set(WARNMODE "no-")
 endif()

+# Compiler options
+option(USE_FLOAT_EXCEPTIONS "Use floating point exceptions" OFF)


The use description of this is not quite accurate. It doesn't turn on exceptions; it alters compiler options in a way that makes the exceptions more likely to be useful. Maybe you could call it USE_FLOAT_DEBUG_MODE or something

Actually I think the name is fine but it would be good to mention the cvars. Enable floating point exceptions with common.floatException.* cvars

illwieckz · 2024-12-05T07:59:05Z

Hmm, my mac doesn't support more than Catalina, so no more than macOS 10.15.7 with Clang 12.0.0, and I get these errors:

src/engine/framework/System.cpp:319:19: error: use of
      undeclared identifier '__fpcr_trap_invalid'
                                exceptions |= __fpcr_trap_invalid;
                                              ^
src/engine/framework/System.cpp:331:19: error: use of
      undeclared identifier '__fpcr_trap_divbyzero'
                                exceptions |= __fpcr_trap_divbyzero;
                                              ^
src/engine/framework/System.cpp:343:19: error: use of
      undeclared identifier '__fpcr_trap_overflow'
                                exceptions |= __fpcr_trap_overflow;
                                              ^
src/engine/framework/System.cpp:356:8: error: no member named
      '__fpcr' in 'fenv_t'
                        env.__fpcr = env.__fpcr | exceptions;
                        ~~~ ^
src/engine/framework/System.cpp:356:21: error: no member named
      '__fpcr' in 'fenv_t'
                        env.__fpcr = env.__fpcr | exceptions;

So I cannot test macOS right now.

illwieckz · 2024-12-06T09:59:23Z

I face a weird cmake bug, if I do that:

	if (USE_FLOAT_EXCEPTIONS)
		message(STATUS "test true")
		# Floating point exceptions requires trapping math
		# to avoid false positives on architectures with SSE.
		set_c_cxx_flag("-ffp-model=strict")
	endif()

The “test true” message is printed, but the -ffp-model=strict flag isn't set.

But if I do:

	set_c_cxx_flag("-ffp-model=strict")
	if (USE_FLOAT_EXCEPTIONS)
		message(STATUS "test true")
		# Floating point exceptions requires trapping math
		# to avoid false positives on architectures with SSE.
	endif()

The flag is set.

So to sum it up:

I know the test is true,
I know the block is executed,
I know the function works,

but the whole combination doesn't work…

illwieckz · 2024-12-06T10:33:11Z

If I do instead:

	if (USE_FLOAT_EXCEPTIONS)
		message(STATUS "test true")
		try_c_cxx_flag(FFP_MODEL_STRICT "-ffp-model=strict")
		# Floating point exceptions requires trapping math
		# to avoid false positives on architectures with SSE.
	endif()

I get this printed:

-- test true
-- Performing Test FLAG_FFP_MODEL_STRICT
-- Performing Test FLAG_FFP_MODEL_STRICT - Success

But the flag is not added to the compiler command line.

On the contrary if I do that:

	try_c_cxx_flag(FFP_MODEL_STRICT "-ffp-model=strict")
	if (USE_FLOAT_EXCEPTIONS)
		message(STATUS "test true")
		# Floating point exceptions requires trapping math
		# to avoid false positives on architectures with SSE.
	endif()

I get this printed:

-- Performing Test FLAG_FFP_MODEL_STRICT
-- Performing Test FLAG_FFP_MODEL_STRICT - Success
-- test true

And the flag is added to the compiler command line.

…float exceptions

…ions

illwieckz · 2025-01-03T18:11:04Z

That looks ready to me. I've added a cvar named common.floatExceptions.test that enables code expected to trigger some floating point exceptions on purpose to test the exception code is working.

illwieckz · 2025-01-03T18:21:57Z

I only tested on Linux.

I can't test on macOS:

my macOS virtual machine is too old for shipping the symbols and I don't want to update it.
my old mac is not allowed by Apple to get a newer macOS so I run into the same problems.

I don't use MSVC.

illwieckz · 2025-01-03T18:24:11Z

Note: people says on the Internet that on macOS this code will only work on amd64, not on arm64, I haven't implemented the macOS arm64 workaround: https://stackoverflow.com/a/71792418

slipher · 2025-01-03T21:42:46Z

src/engine/framework/System.cpp

+		_controlfp_s(&current, exceptions, _MCW_EM);
+	#endif
+
+	if (common_floatExceptions_test.Get())


The injectFault command can already do this. If more types of exception are needed they should be added there

The purpose of this test is not only to test if the exception is caught when raised, but also to make sure that when doing the mistake for real the exception is raised.

As reported on chat, I noticed that doesn't raise an exception:

float f = std::numeric_limits<float>::max(); Log::Warn("Result of 2×%.0f: %.0f", f, 2*f);

But doing that raises an exception:

volatile float f = std::numeric_limits<float>::max(); Log::Warn("Result of 2×%.0f: %.0f", static_cast<float>(f), 2*f);

I want to test that.

We can also do some injectFault() calls, but to me that should only be done after those tests are done (basically to test that the exception catching works even if the error did not happened, so we can investigate while the error did not happened).

An injectFault fault flavor need not be guaranteed to crash in all circumstances. I don't see any reason why this stuff wouldn't fit well there.

slipher · 2025-01-03T21:43:37Z

src/engine/qcommon/q_shared.h


-	VectorScale( v, ilength, v );
+#if DAEMON_USE_FLOAT_EXCEPTIONS


The program should not change its behavior like that when the debugging thing is used. That nullifies the whole point of it.

Not changing this behavior nullifies the whole point of it.

The whole point of it is:

Making possible to debug everything that is not Q_rsqrt_fast() as called by VectorNormalizeFast().

The whole point of it is:

* Making possible to debug everything that is not `Q_rsqrt_fast()` as called by `VectorNormalizeFast()`.

Why? There should be some explanation for this.

illwieckz · 2025-01-04T23:50:27Z

Using MacOS Sequoia… the symbols were still missing, and I discovered the symbols I was missing were arm64-only. 🤦‍♀️️

Now I have a code that works for amd64 on macOS. It probably already works for arm64 on macOS as I actually ported an arm64 code to also support amd64.

illwieckz · 2025-01-04T23:52:35Z

Platform	Implemented	Tested
Linux GCC/Clang	✅️	✅️
MacOS Clang amd64	✅️	✅️
MacOS Clang arm64	✅️
Windows MSVC	✅️

illwieckz added 2 commits December 2, 2024 10:13

cmake: add USE_FAST_MATH to enable or disable fast math

ee49a3f

cmake,System: add USE_FLOAT_EXCEPTIONS and common.floatExceptions.* c…

3ce832a

…vars to enable floating point exceptions

illwieckz force-pushed the illwieckz/catch-0div branch 5 times, most recently from f05b27f to 4993a7f Compare December 3, 2024 09:00

slipher reviewed Dec 4, 2024

View reviewed changes

src/engine/framework/System.cpp Outdated Show resolved Hide resolved

src/engine/framework/System.cpp Outdated Show resolved Hide resolved

src/engine/framework/System.cpp Outdated Show resolved Hide resolved

src/engine/framework/System.cpp Outdated Show resolved Hide resolved

illwieckz changed the title ~~Add USE_DEBUG_FPE to enable floating point exceptions~~ Add USE_FLOAT_EXCEPTIONS to enable floating point exceptions Dec 4, 2024

illwieckz force-pushed the illwieckz/catch-0div branch from 4993a7f to 8687854 Compare December 4, 2024 08:31

illwieckz force-pushed the illwieckz/catch-0div branch 2 times, most recently from b878d3a to 7c60e69 Compare December 4, 2024 08:35

slipher reviewed Dec 5, 2024

View reviewed changes

illwieckz force-pushed the illwieckz/catch-0div branch from 7c60e69 to a65cdc7 Compare December 5, 2024 07:56

illwieckz force-pushed the illwieckz/catch-0div branch 3 times, most recently from 10070cb to 4086988 Compare December 5, 2024 10:12

illwieckz added 2 commits December 20, 2024 01:26

q_shared: avoid rsqrt_fast(0) in VectorNormalizeFast() when trapping …

ae0184b

…float exceptions

System: add common.floatExceptions.test to test floating point except…

0c354c4

…ions

illwieckz force-pushed the illwieckz/catch-0div branch 3 times, most recently from 8417ef8 to 9fa7434 Compare December 20, 2024 04:39

illwieckz force-pushed the illwieckz/catch-0div branch 3 times, most recently from 278fa37 to 7c8ac23 Compare January 3, 2025 18:02

illwieckz force-pushed the illwieckz/catch-0div branch 2 times, most recently from 9e5dd3a to da7a0e5 Compare January 3, 2025 18:10

slipher reviewed Jan 3, 2025

View reviewed changes

illwieckz force-pushed the illwieckz/catch-0div branch from da7a0e5 to 0c354c4 Compare January 4, 2025 23:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `USE_FLOAT_EXCEPTIONS` to enable floating point exceptions #1451

Add `USE_FLOAT_EXCEPTIONS` to enable floating point exceptions #1451

illwieckz commented Dec 2, 2024 •

edited

Loading

illwieckz commented Dec 2, 2024

illwieckz commented Dec 4, 2024

slipher Dec 4, 2024

illwieckz Dec 5, 2024

slipher Dec 6, 2024

illwieckz Jan 3, 2025

slipher Jan 3, 2025

slipher Dec 4, 2024

illwieckz Jan 3, 2025

illwieckz Jan 3, 2025

slipher Jan 3, 2025

slipher Dec 5, 2024

slipher Dec 12, 2024

illwieckz Jan 3, 2025

illwieckz commented Dec 5, 2024

illwieckz commented Dec 6, 2024 •

edited

Loading

illwieckz commented Dec 6, 2024 •

edited

Loading

illwieckz commented Jan 3, 2025 •

edited

Loading

illwieckz commented Jan 3, 2025

illwieckz commented Jan 3, 2025 •

edited

Loading

slipher Jan 3, 2025

illwieckz Jan 4, 2025 •

edited

Loading

slipher Jan 5, 2025

slipher Jan 3, 2025

illwieckz Jan 3, 2025 •

edited

Loading

illwieckz Jan 3, 2025 •

edited

Loading

slipher Jan 5, 2025

illwieckz commented Jan 4, 2025

illwieckz commented Jan 4, 2025


		VectorScale( v, ilength, v );
		#if DAEMON_USE_FLOAT_EXCEPTIONS

Add USE_FLOAT_EXCEPTIONS to enable floating point exceptions #1451

Are you sure you want to change the base?

Add USE_FLOAT_EXCEPTIONS to enable floating point exceptions #1451

Conversation

illwieckz commented Dec 2, 2024 • edited Loading

illwieckz commented Dec 2, 2024

illwieckz commented Dec 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

illwieckz commented Dec 5, 2024

illwieckz commented Dec 6, 2024 • edited Loading

illwieckz commented Dec 6, 2024 • edited Loading

illwieckz commented Jan 3, 2025 • edited Loading

illwieckz commented Jan 3, 2025

illwieckz commented Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

illwieckz Jan 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

illwieckz Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

illwieckz Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

illwieckz commented Jan 4, 2025

illwieckz commented Jan 4, 2025

Add `USE_FLOAT_EXCEPTIONS` to enable floating point exceptions #1451

Add `USE_FLOAT_EXCEPTIONS` to enable floating point exceptions #1451

illwieckz commented Dec 2, 2024 •

edited

Loading

illwieckz commented Dec 6, 2024 •

edited

Loading

illwieckz commented Dec 6, 2024 •

edited

Loading

illwieckz commented Jan 3, 2025 •

edited

Loading

illwieckz commented Jan 3, 2025 •

edited

Loading

illwieckz Jan 4, 2025 •

edited

Loading

illwieckz Jan 3, 2025 •

edited

Loading

illwieckz Jan 3, 2025 •

edited

Loading