Check Jolt with TSAN in CI (#1278)

This commit adds a new GitHub Actions workflow that checks Jolt with ThreadSanitizer under Ubuntu & Clang.

* Replaces usage of atomic_thread_fences in Reference.h and JobSystem.h with regular atomic ops when
building under TSAN, as it does not support fences and unlikely ever will do so.
* Limits the max number of mutexes to use under TSAN to work around a TSAN limitation, see: google/sanitizers#950.
* Replaces Semaphore::mCount with an atomic int that's used in relaxed mode.

Co-authored-by: Jorrit Rouwe <jrouwe@gmail.com>

.github/workflows/build.yml

@@ -39,6 +39,22 @@ jobs:
       working-directory: ${{github.workspace}}/Build/Linux_${{matrix.build_type}}
       run: ctest --output-on-failure --verbose
 
+  linux_clang_tsan:
+    runs-on: ubuntu-latest
+    name: Linux Clang TSAN
+
+    steps:
+    - name: Checkout Code
+      uses: actions/checkout@v4
+    - name: Configure CMake
+      working-directory: ${{github.workspace}}/Build
+      run: ./cmake_linux_clang_gcc.sh ReleaseTSAN ${{env.UBUNTU_CLANG_VERSION}} -DTARGET_VIEWER=OFF -DTARGET_SAMPLES=OFF -DTARGET_HELLO_WORLD=OFF -DTARGET_UNIT_TESTS=ON -DTARGET_PERFORMANCE_TEST=ON
+    - name: Build
+      run: cmake --build ${{github.workspace}}/Build/Linux_ReleaseTSAN -j $(nproc)
+    - name: Unit Tests
+      working-directory: ${{github.workspace}}/Build/Linux_ReleaseTSAN
+      run: ctest --output-on-failure --verbose
+
   linux-clang-so:
     runs-on: ubuntu-latest
     name: Linux Clang Shared Library

Build/CMakeLists.txt

@@ -105,7 +105,7 @@ if (CMAKE_CURRENT_SOURCE_DIR STREQUAL CMAKE_SOURCE_DIR) # Only do this when we'r
 	if ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "MSVC" OR "${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU")
 		set(CMAKE_CONFIGURATION_TYPES "Debug;Release;Distribution")
 	elseif ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang" OR "${CMAKE_CXX_COMPILER_ID}" STREQUAL "AppleClang")
-		set(CMAKE_CONFIGURATION_TYPES "Debug;Release;ReleaseASAN;ReleaseUBSAN;ReleaseCoverage;Distribution")
+		set(CMAKE_CONFIGURATION_TYPES "Debug;Release;ReleaseASAN;ReleaseUBSAN;ReleaseTSAN;ReleaseCoverage;Distribution")
 	endif()
 endif()
 
@@ -161,6 +161,7 @@ if (MSVC)
 	set(CMAKE_CXX_FLAGS_DISTRIBUTION "${CMAKE_CXX_FLAGS_RELEASE}")
 	set(CMAKE_CXX_FLAGS_RELEASEASAN "-fsanitize=address /Od")
 	set(CMAKE_CXX_FLAGS_RELEASEUBSAN "-fsanitize=undefined,implicit-conversion,float-divide-by-zero,local-bounds -fno-sanitize-recover=all")
+	set(CMAKE_CXX_FLAGS_RELEASETSAN "${CMAKE_CXX_FLAGS_RELEASE} -fsanitize=thread")
 	set(CMAKE_CXX_FLAGS_RELEASECOVERAGE "-fprofile-instr-generate -fcoverage-mapping")
 
 	# Set linker flags
@@ -179,6 +180,7 @@ if (MSVC)
 		set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Qunused-arguments") # Clang emits warnings about unused arguments such as /MP and /GL
 		set(CMAKE_EXE_LINKER_FLAGS_RELEASEASAN "/SUBSYSTEM:CONSOLE /LIBPATH:${CLANG_LIB_PATH} clang_rt.asan-x86_64.lib -wholearchive:clang_rt.asan-x86_64.lib clang_rt.asan_cxx-x86_64.lib -wholearchive:clang_rt.asan_cxx-x86_64.lib")
 		set(CMAKE_EXE_LINKER_FLAGS_RELEASEUBSAN "${CMAKE_EXE_LINKER_FLAGS_RELEASE} /LIBPATH:${CLANG_LIB_PATH}")
+		set(CMAKE_EXE_LINKER_FLAGS_RELEASETSAN "${CMAKE_EXE_LINKER_FLAGS_RELEASE} /LIBPATH:${CLANG_LIB_PATH}")
 		set(CMAKE_EXE_LINKER_FLAGS_RELEASECOVERAGE "${CMAKE_EXE_LINKER_FLAGS_RELEASE} /LIBPATH:${CLANG_LIB_PATH}")
 	endif()
 else()
@@ -240,6 +242,7 @@ else()
 	set(CMAKE_CXX_FLAGS_DISTRIBUTION "${CMAKE_CXX_FLAGS_RELEASE}")
 	set(CMAKE_CXX_FLAGS_RELEASEASAN "-fsanitize=address")
 	set(CMAKE_CXX_FLAGS_RELEASEUBSAN "-fsanitize=undefined,implicit-conversion,float-divide-by-zero,local-bounds -fno-sanitize-recover=all")
+	set(CMAKE_CXX_FLAGS_RELEASETSAN "${CMAKE_CXX_FLAGS_RELEASE} -fsanitize=thread")
 	set(CMAKE_CXX_FLAGS_RELEASECOVERAGE "-O0 -DJPH_NO_FORCE_INLINE -fprofile-instr-generate -fcoverage-mapping")
 endif()
 

Build/cmake_linux_clang_gcc.sh

@@ -19,7 +19,7 @@ fi
 BUILD_DIR=Linux_$BUILD_TYPE
 
 echo Usage: ./cmake_linux_clang_gcc.sh [Configuration] [Compiler]
-echo "Possible configurations: Debug (default), Release, Distribution, ReleaseUBSAN, ReleaseASAN, ReleaseCoverage"
+echo "Possible configurations: Debug (default), Release, Distribution, ReleaseUBSAN, ReleaseASAN, ReleaseTSAN, ReleaseCoverage"
 echo "Possible compilers: clang++, clang++-XX, g++, g++-XX where XX is the version"
 echo Generating Makefile for build type \"$BUILD_TYPE\" and compiler \"$COMPILER\" in folder \"$BUILD_DIR\"
 

Jolt/Core/Core.h

@@ -578,4 +578,15 @@ static_assert(sizeof(void *) == (JPH_CPU_ADDRESS_BITS == 64? 8 : 4), "Invalid si
 	#error Undefined
 #endif
 
+// Check if Thread Sanitizer is enabled
+#ifdef __has_feature
+	#if __has_feature(thread_sanitizer)
+		#define JPH_TSAN_ENABLED
+	#endif
+#else
+	#ifdef __SANITIZE_THREAD__
+		#define JPH_TSAN_ENABLED
+	#endif
+#endif
+
 JPH_NAMESPACE_END

Jolt/Core/JobSystem.h

@@ -195,13 +195,19 @@ class JPH_EXPORT JobSystem : public NonCopyable
 		}
 		inline void			Release()
 		{
+		#ifndef JPH_TSAN_ENABLED
 			// Releasing a reference must use release semantics...
 			if (mReferenceCount.fetch_sub(1, memory_order_release) == 1)
 			{
 				// ... so that we can use acquire to ensure that we see any updates from other threads that released a ref before freeing the job
 				atomic_thread_fence(memory_order_acquire);
 				mJobSystem->FreeJob(this);
 			}
+		#else
+			// But under TSAN, we cannot use atomic_thread_fence, so we use an acq_rel operation unconditionally instead
+			if (mReferenceCount.fetch_sub(1, memory_order_acq_rel) == 1)
+				mJobSystem->FreeJob(this);
+		#endif
 		}
 
 		/// Add to the dependency counter.

Jolt/Core/JobSystemWithBarrier.cpp

@@ -146,8 +146,11 @@ void JobSystemWithBarrier::BarrierImpl::Wait()
 			} while (has_executed);
 		}
 
-		// Wait for another thread to wake us when either there is more work to do or when all jobs have completed
-		int num_to_acquire = max(1, mSemaphore.GetValue()); // When there have been multiple releases, we acquire them all at the same time to avoid needlessly spinning on executing jobs
+		// Wait for another thread to wake us when either there is more work to do or when all jobs have completed.
+		// When there have been multiple releases, we acquire them all at the same time to avoid needlessly spinning on executing jobs.
+		// Note that using GetValue is inherently unsafe since we can read a stale value, but this is not an issue here as this is the only
+		// place where we acquire the semaphore. Other threads only release it, so we can only read a value that is lower or equal to the actual value.
+		int num_to_acquire = max(1, mSemaphore.GetValue());
 		mSemaphore.Acquire(num_to_acquire);
 		mNumToAcquire -= num_to_acquire;
 	}

Jolt/Core/Reference.h

@@ -59,13 +59,19 @@ class RefTarget
 
 	inline void				Release() const
 	{
+	#ifndef JPH_TSAN_ENABLED
 		// Releasing a reference must use release semantics...
 		if (mRefCount.fetch_sub(1, memory_order_release) == 1)
 		{
 			// ... so that we can use acquire to ensure that we see any updates from other threads that released a ref before deleting the object
 			atomic_thread_fence(memory_order_acquire);
 			delete static_cast<const T *>(this);
 		}
+	#else
+		// But under TSAN, we cannot use atomic_thread_fence, so we use an acq_rel operation unconditionally instead
+		if (mRefCount.fetch_sub(1, memory_order_acq_rel) == 1)
+			delete static_cast<const T *>(this);
+	#endif
 	}
 
 	/// INTERNAL HELPER FUNCTION USED BY SERIALIZATION

Jolt/Core/Semaphore.cpp

@@ -51,7 +51,7 @@ void Semaphore::Release(uint inNumber)
 	}
 #else
 	std::lock_guard lock(mLock);
-	mCount += (int)inNumber;
+	mCount.fetch_add(inNumber, std::memory_order_relaxed);
 	if (inNumber > 1)
 		mWaitVariable.notify_all();
 	else
@@ -74,7 +74,7 @@ void Semaphore::Acquire(uint inNumber)
 	}
 #else
 	std::unique_lock lock(mLock);
-	mCount -= (int)inNumber;
+	mCount.fetch_sub(inNumber, std::memory_order_relaxed);
 	mWaitVariable.wait(lock, [this]() { return mCount >= 0; });
 #endif
 }

Jolt/Core/Semaphore.h

@@ -33,7 +33,7 @@ class JPH_EXPORT Semaphore
 	void				Acquire(uint inNumber = 1);
 
 	/// Get the current value of the semaphore
-	inline int			GetValue() const								{ return mCount; }
+	inline int			GetValue() const								{ return mCount.load(std::memory_order_relaxed); }
 
 private:
 #ifdef JPH_PLATFORM_WINDOWS
@@ -44,7 +44,7 @@ class JPH_EXPORT Semaphore
 	// Other platforms: Emulate a semaphore using a mutex, condition variable and count
 	mutex				mLock;
 	condition_variable	mWaitVariable;
-	int					mCount = 0;
+	atomic<int>			mCount { 0 };
 #endif
 };
 

Jolt/Jolt.cmake

@@ -487,6 +487,7 @@ if (BUILD_SHARED_LIBS)
 	# Set linker flags for other build types to be the same as release
 	set(CMAKE_SHARED_LINKER_FLAGS_RELEASEASAN "${CMAKE_SHARED_LINKER_FLAGS_RELEASE}")
 	set(CMAKE_SHARED_LINKER_FLAGS_RELEASEUBSAN "${CMAKE_SHARED_LINKER_FLAGS_RELEASE}")
+	set(CMAKE_SHARED_LINKER_FLAGS_RELEASETSAN "${CMAKE_SHARED_LINKER_FLAGS_RELEASE}")
 	set(CMAKE_SHARED_LINKER_FLAGS_RELEASECOVERAGE "${CMAKE_SHARED_LINKER_FLAGS_RELEASE}")
 	set(CMAKE_SHARED_LINKER_FLAGS_DISTRIBUTION "${CMAKE_SHARED_LINKER_FLAGS_RELEASE}")
 
@@ -518,7 +519,7 @@ endif()
 
 # Set the debug/non-debug build flags
 target_compile_definitions(Jolt PUBLIC "$<$<CONFIG:Debug>:_DEBUG>")
-target_compile_definitions(Jolt PUBLIC "$<$<CONFIG:Release,Distribution,ReleaseASAN,ReleaseUBSAN,ReleaseCoverage>:NDEBUG>")
+target_compile_definitions(Jolt PUBLIC "$<$<CONFIG:Release,Distribution,ReleaseASAN,ReleaseUBSAN,ReleaseTSAN,ReleaseCoverage>:NDEBUG>")
 
 # ASAN should use the default allocators
 target_compile_definitions(Jolt PUBLIC "$<$<CONFIG:ReleaseASAN>:JPH_DISABLE_TEMP_ALLOCATOR;JPH_DISABLE_CUSTOM_ALLOCATOR>")
@@ -571,14 +572,14 @@ endif()
 if (DEBUG_RENDERER_IN_DISTRIBUTION)
 	target_compile_definitions(Jolt PUBLIC "JPH_DEBUG_RENDERER")
 elseif (DEBUG_RENDERER_IN_DEBUG_AND_RELEASE)
-	target_compile_definitions(Jolt PUBLIC "$<$<CONFIG:Debug,Release,ReleaseASAN,ReleaseUBSAN>:JPH_DEBUG_RENDERER>")
+	target_compile_definitions(Jolt PUBLIC "$<$<CONFIG:Debug,Release,ReleaseASAN,ReleaseUBSAN,ReleaseTSAN>:JPH_DEBUG_RENDERER>")
 endif()
 
 # Enable the profiler
 if (PROFILER_IN_DISTRIBUTION)
 	target_compile_definitions(Jolt PUBLIC "JPH_PROFILE_ENABLED")
 elseif (PROFILER_IN_DEBUG_AND_RELEASE)
-	target_compile_definitions(Jolt PUBLIC "$<$<CONFIG:Debug,Release,ReleaseASAN,ReleaseUBSAN>:JPH_PROFILE_ENABLED>")
+	target_compile_definitions(Jolt PUBLIC "$<$<CONFIG:Debug,Release,ReleaseASAN,ReleaseUBSAN,ReleaseTSAN>:JPH_PROFILE_ENABLED>")
 endif()
 
 # Compile the ObjectStream class and RTTI attribute information

Jolt/Physics/Body/BodyManager.cpp

@@ -105,6 +105,9 @@ void BodyManager::Init(uint inMaxBodies, uint inNumBodyMutexes, const BroadPhase
 
 	// Num body mutexes must be a power of two and not bigger than our MutexMask
 	uint num_body_mutexes = Clamp<uint>(GetNextPowerOf2(inNumBodyMutexes == 0? 2 * thread::hardware_concurrency() : inNumBodyMutexes), 1, sizeof(MutexMask) * 8);
+#ifdef JPH_TSAN_ENABLED
+	num_body_mutexes = min(num_body_mutexes, 32U); // TSAN errors out when locking too many mutexes on the same thread, see: https://github.com/google/sanitizers/issues/950
+#endif
 
 	// Allocate the body mutexes
 	mBodyMutexes.Init(num_body_mutexes);