Thread: Re-add <new> include for std::hardware_destructive_interference_size

[akien-mga]

Somehow it would still build fine, but would crash when compiled with GCC 12.2 on Debian 12.

Also re-add wrongly removed Mutex include from thread_safe.h, where it's used in macros.

Add IWYU pragma comments to prevent it from mistakenly flagging those as unused.

• Fixes Godot crash on Linux when compiled with GCC 12 #100768.
• Fixes up Remove unused headers in core #100564 which introduced the regression.

CC @RandomShaper @hpvb
I still don't know why it would compile OK without <new>, but then crash, but I've confirmed that this patch fixes the crash with Debian 12's GCC 12.2.0.

[akien-mga]

CC @Repiteo, I took this format from #100918.

Does that work for clangd too and not just include-what-you-use?

[Repiteo]

It indeed worked for clangd, so much so that it's part of their guidelines:

https://clangd.llvm.org/guides/include-cleaner#scenarios-and-solutions

[akien-mga]

I'm assuming that adding the #if conditions matching where it's used will prevent tools like iwyu from flagging this as unused when run on platforms which don't fulfill that condition (seems to be GCC specific, possibly supported on Clang, but thus not MSVC most likely).

If not, we should add a pragma comment to prevent its removal, but then I wouldn't trust the judgement of those tools.

[akien-mga]

Ok this is what broke my fix, to test __cpp_lib_hardware_interference_size we first need to include <new>.

So pragma comment it will be.

[akien-mga]

I went maybe a tiny bit heavy handed on tweaking code style / #ifdefs in a critical bugfix, so to be clear the minimal fix is just this (i.e. a partial revert of #100564):

diff --git a/core/os/thread.h b/core/os/thread.h
index 066c4befa7..1c442b41f6 100644
--- a/core/os/thread.h
+++ b/core/os/thread.h
@@ -42,6 +42,8 @@
 #include "core/templates/safe_refcount.h"
 #include "core/typedefs.h"
 
+#include <new>
+
 #ifdef MINGW_ENABLED
 #define MINGW_STDTHREAD_REDUNDANCY_WARNING
 #include "thirdparty/mingw-std-threads/mingw.thread.h"
diff --git a/core/os/thread_safe.h b/core/os/thread_safe.h
index 316b0c9687..042a0b7d98 100644
--- a/core/os/thread_safe.h
+++ b/core/os/thread_safe.h
@@ -31,6 +31,8 @@
 #ifndef THREAD_SAFE_H
 #define THREAD_SAFE_H
 
+#include "core/os/mutex.h"
+
 #define _THREAD_SAFE_CLASS_ mutable Mutex _thread_safe_;
 #define _THREAD_SAFE_METHOD_ MutexLock _thread_safe_method_(_thread_safe_);
 #define _THREAD_SAFE_LOCK_ _thread_safe_.lock();

This PR should be equivalent to that diff if I didn't mess up.

[akien-mga]

Taking back as draft for a bit, the latest iteration of my PR doesn't seem to fix the crash anymore... Seems like some of the "harmless" refactoring I've done may have caused harm? Or the problem is deeper than I thought.

[akien-mga]

OK I fixed the issue, the include of <new> can't be made conditional on defined(__cpp_lib_hardware_interference_size) because <new> is needed to check that feature.

Which hints to why everything compiled fine despite the removal of <new>. It meant that (at least occasionally) we were falling back to the #else case here:

#if defined(__cpp_lib_hardware_interference_size) && !defined(ANDROID_ENABLED) // This would be OK with NDK >= 26.
#if defined(__GNUC__) && !defined(__clang__)
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Winterference-size"
#endif
        static constexpr size_t CACHE_LINE_BYTES = std::hardware_destructive_interference_size;
#if defined(__GNUC__) && !defined(__clang__)
#pragma GCC diagnostic pop
#endif
#else
        // At a negligible memory cost, we use a conservatively high value.
        static constexpr size_t CACHE_LINE_BYTES = 128;
#endif

Either it doesn't work properly, or we're mixing both branches depending on whether thread.h is included in code that already includes <new> or not.

I made this quick test to confirm (on master with Debian 12's GCC 12.2.0, i.e. without re-adding <new> in thread.h):

diff --git a/core/os/thread.h b/core/os/thread.h
index 066c4be..6d1ac06 100644
--- a/core/os/thread.h
+++ b/core/os/thread.h
@@ -90,11 +90,13 @@ public:
 #pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-Winterference-size"
 #endif
+       #warning Good branch.
        static constexpr size_t CACHE_LINE_BYTES = std::hardware_destructive_interference_size;
 #if defined(__GNUC__) && !defined(__clang__)
 #pragma GCC diagnostic pop
 #endif
 #else
+       #warning Bad branch.
        // At a negligible memory cost, we use a conservatively high value.
        static constexpr size_t CACHE_LINE_BYTES = 128;
 #endif

Here are the full build logs:
godot-thread-build.log

There's indeed a few "Bad branches." in there:

$ cat ~/godot-thread-build.log | grep "Good branch" | wc -l
2462
$ cat ~/godot-thread-build.log | grep "Bad branch" | wc -l
44

Including <new> explicitly prevents this, but this hints that we might have more such problems in the codebase where a file is wrongly compiled taking diverging code paths, and the consequences can be disastrous, as seen here. We should really look into this and see if we can detect and prevent this.

[fire]

Somehow it would still build fine, but would crash when compiled with GCC 12.2 on Debian 12.

I think the only way is build with Github actions, and then test with a benchmark server with a gpu, but it might to be too expensive.