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.

Status:

Platform	Implemented	Tested
Linux GCC amd64	✅️	✅️
Linux GCC i686	✅️	✅️
Linux Clang amd64	✅️	✅️
Windows MinGW amd64	✅️	✅️
Windows MinGW i686	✅️	✅️
Windows MSVC amd64	✅️
Windows MSVC i686	✅️
macOS Clang amd64	✅️	✅️
macOS Clang arm64	✅️
FreeBSD Clang amd64	✅️	✅️

…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

 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

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.

The injectFault floatdiv doesn't work, likely because a volatile doesn't guarantee dead code to be removed.

Not, it's because 0 / 0 is invalid error, not divByZero error. That is 1 / 0 that is divByZero error.

I fixed and extended injectFault.

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

src/engine/qcommon/q_shared.h

+	vec_t length = DotProduct( v, v );

-	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.

What I mean is that it can be fine to purposely ignore one specific hack, instead of making impossible to debug anything else because of one hack.

Anyway, this code raises other questions:

Some questions about VectorNormalizeFast() #1493

So, answers were:

I am not a Quake 3 developer but I will give my best guess.

Were Quake 3 developers correct to consider it was acceptable to get garbage when the length is zero?

Yes for VectorNormalizeFast, no for VectorNormalize.

If they were right to consider it acceptable to get that garbage, is it still correct to get NaN instead of that garbage?

Yes

So if we consider it OK to get NaN there when length is 0, we better make the NaN detector purposely ignore this one.

I just want to see some code comment about why this happens.

I tried to see for myself and apparently some models trigger it in R_TBNtoQtangents.

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

See first post for implementation and test status.

…d, also fix “floatdiv”

illwieckz · 2025-02-06T20:00:20Z

So, this looks ready to me.

I cannot test macOS arm64, neither MSVC, but not only that code is not build by default, but when built, the features are not enabled by default, so that should not prevent the merge: people can test and improve later.

illwieckz · 2025-02-06T21:28:11Z

The Windows code works when built with MinGW and run on Wine, and the code is exactly the same for building with MSVC.

slipher · 2025-02-11T02:49:02Z

src/engine/qcommon/q_shared.h

+	vec_t length = DotProduct( v, v );

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


I just want to see some code comment about why this happens.

src/engine/framework/System.cpp

slipher · 2025-02-11T03:15:34Z

src/engine/framework/System.cpp

+			#endif
+
+			#if defined(DAEMON_USE_ARCH_INTRINSICS_i686_sse)
+				sse_exceptions |= _MM_MASK_INVALID;


amd64 Mac did not work (though arm64 did). I didn't get any effect from the 3 fault injectors.

On my end Mac amd64 still works, you suggested on IRC that maybe Rosetta isn't emulating it.

src/engine/framework/System.cpp

src/common/Command.cpp

cmake/DaemonFlags.cmake

slipher · 2025-02-11T11:25:35Z

src/engine/qcommon/q_shared.h

+	vec_t length = DotProduct( v, v );

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


I tried to see for myself and apparently some models trigger it in R_TBNtoQtangents.

slipher · 2025-02-12T08:02:32Z

LGTM

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

e384d0b

…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

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

42c8c60

…float exceptions

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 5 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

illwieckz force-pushed the illwieckz/catch-0div branch from 0c354c4 to 62ff53c Compare January 7, 2025 01:58

illwieckz mentioned this pull request Jan 8, 2025

Some questions about VectorNormalizeFast() #1493

Closed

illwieckz force-pushed the illwieckz/catch-0div branch from 62ff53c to 0c6e857 Compare January 30, 2025 19:03

illwieckz mentioned this pull request Jan 30, 2025

cmake: add USE_FAST_MATH to enable or disable fast math #1537

Merged

illwieckz force-pushed the illwieckz/catch-0div branch from 0c6e857 to 644d8a8 Compare February 6, 2025 18:54

Command: add “floatinvalid” and “floatoverflow” to injectFault comman…

625a884

…d, also fix “floatdiv”

illwieckz force-pushed the illwieckz/catch-0div branch from e09077b to 60aac27 Compare February 6, 2025 19:58

illwieckz force-pushed the illwieckz/catch-0div branch 4 times, most recently from 9cbb988 to 08b22a0 Compare February 7, 2025 02:08

slipher reviewed Feb 11, 2025

View reviewed changes

illwieckz force-pushed the illwieckz/catch-0div branch from 86b6a78 to 1a41a31 Compare February 11, 2025 03:43

DaemonFlags: silence useless warnings on MSVC when using /fp:strict

6b0062c

slipher reviewed Feb 11, 2025

View reviewed changes

illwieckz force-pushed the illwieckz/catch-0div branch 2 times, most recently from 051b3a1 to 6b0062c Compare February 12, 2025 02:47

VReaperV mentioned this pull request Feb 12, 2025

Broken rendering on disc surface with Clang 14 and later with -march=native #1545

Open

illwieckz merged commit 6c2b388 into master Feb 14, 2025
9 checks passed

illwieckz deleted the illwieckz/catch-0div branch February 14, 2025 16:28

slipher mentioned this pull request Feb 20, 2025

Add a cvar to enable floating point exceptions #770

Closed

Add USE_FLOAT_EXCEPTIONS to enable floating point exceptions #1451

Add USE_FLOAT_EXCEPTIONS to enable floating point exceptions #1451

Uh oh!

Conversation

illwieckz commented Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

illwieckz commented Dec 2, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

illwieckz commented Dec 4, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

illwieckz commented Dec 5, 2024

Uh oh!

illwieckz commented Dec 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

illwieckz commented Dec 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

illwieckz commented Jan 3, 2025

Uh oh!

illwieckz commented Jan 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

illwieckz Jan 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

illwieckz Feb 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

illwieckz Jan 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

illwieckz Jan 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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 Jan 4, 2025 •

edited

Loading

illwieckz Feb 6, 2025 •

edited

Loading

illwieckz Jan 3, 2025 •

edited

Loading

illwieckz Jan 3, 2025 •

edited

Loading

illwieckz commented Jan 4, 2025 •

edited

Loading

illwieckz commented Feb 6, 2025 •

edited

Loading