macOS port: Fix SDL initialization, race conditions, and shutdown deadlock #58

jhurliman · 2025-12-28T02:08:25Z

Summary

This PR ports GLSMAC to macOS, fixing threading issues, race conditions, and shutdown problems discovered during the port.

Key Changes

macOS-Specific Fixes

SDL Initialization on Main Thread (macOS requirement)
- SDL video, audio, and event subsystems must be initialized on the main thread due to AppKit requirements
- Added InitSDLOnMainThread() methods to graphics, audio, and input modules
- Created cross-platform MainThreadDispatch system to queue operations for the main thread
OpenGL 3.3 Core Profile Support
- Requested OpenGL 3.3 Core Profile for GLSL 3.30 compatibility
- Fixed shader code: replaced deprecated texture2D() with texture()
- Added VAO (Vertex Array Object) binding as required by Core Profile
- Changed texture format from GL_RGBA to GL_RGBA8 for Core Profile compatibility
OpenGL Context Thread Safety
- OpenGL contexts are thread-local on macOS
- Added SDL_GL_MakeCurrent() calls before OpenGL operations
- Fixed race condition where main thread could make context current while worker thread was using it
- Release context from worker thread before dispatching swap to main thread

Race Condition Fixes (Detected by ThreadSanitizer)

Logging Race Condition
- Fixed data race in common::Class::Log() where last_time global variable was accessed without synchronization
- Added mutex to protect last_time access
UnitManager Race Condition
- Fixed data race in UnitManager::m_unit_updates unordered_map
- QueueUnitUpdate() and PushUpdates() called from different threads
- Added mutex to protect all access to m_unit_updates

Other Improvements

Improved Crash Diagnostics
- Enhanced crash handler with async-signal-safe stack trace printing
- Added alternate signal stack for more reliable crash handling
- Improved error messages and diagnostics
Path Resolution
- Fixed script path resolution for relative datapath arguments
Build System
- Added CMAKE_EXPORT_COMPILE_COMMANDS for LSP support
- Fixed vendored ossp-uuid library configuration

Testing

Tested on macOS with ThreadSanitizer enabled
All race conditions detected by TSAN in basic startup have been fixed
Application runs and a new game can be started
Game functionality verified with GoG SMAC installation

- Enable compile_commands.json generation in CMakeLists.txt - Add CMAKE_EXPORT_COMPILE_COMMANDS to CMakePresets.json base preset - This enables C++ LSP (clangd) to work properly in IDEs

- Add InitSDLOnMainThread() method to initialize SDL on main thread - Call InitSDLOnMainThread() from main() before starting worker threads - Modify OpenGL::Start() to verify SDL is initialized on macOS - Fixes NSInternalInconsistencyException crash on macOS On macOS, SDL initialization (SDL_VideoInit and SDL_CreateWindow) must happen on the main thread due to AppKit requirements. Previously, these calls were happening in OpenGL::Start() which runs on a worker thread, causing a crash.

…thread On macOS, all SDL subsystem initialization must happen on the main thread due to AppKit requirements. Previously, SDL_Init(SDL_INIT_AUDIO) and SDL_Init(SDL_INIT_EVENTS) were being called from worker threads, causing trace traps. - Add InitSDLOnMainThread() methods to audio::sdl2::SDL2 and input::sdl2::SDL2 - Call these methods from main() before starting worker threads (similar to graphics) - Modify Start() methods to assert SDL is already initialized on macOS - Add debug instrumentation to track thread IDs and SDL initialization state

Remove #ifdef __APPLE__ guards from the dispatch queue system to make it available on all platforms. This provides consistent SDL operation routing through the main thread across Linux, Windows, and macOS. - Remove guards from MainThreadDispatch.h and .cpp - Always include MainThreadDispatch.cpp in CMakeLists.txt - Remove guards from Engine.cpp ProcessQueue() call - Remove guards from SDL2 event polling and clipboard operations - Remove guards from OpenGL window operations (SetFullscreen/SetWindowed) - Keep platform-specific pthread_main_np() checks conditional (macOS-only) The dispatch queue now routes all SDL operations through the main thread on all platforms, providing consistent behavior and easier maintenance.

- Request OpenGL 3.3 Core Profile context for GLSL 3.30 support on macOS - Update shaders to use texture() instead of deprecated texture2D() - Add VAO binding for Core Profile requirements (shader validation, texture ops) - Fix GL thread tracking: update MemoryWatcher GL thread ID when context switches - Add stack trace printing on crashes (backtrace) for better diagnostics - Ensure OpenGL context is current on worker threads before GL calls - Add GL_RGBA internal format support to MemoryWatcher - Improve shader compilation/linking error messages

- Add fallback path resolution in GSE::RunScript() to handle paths starting with ../ - When a path starts with ../, also try the same path without the ../ prefix - This allows both --datapath ../GLSMAC_data and --datapath GLSMAC_data to work - Use std::filesystem::weakly_canonical() to properly resolve .. components

- Remove the fallback code that tried paths without ../ prefix - Keep proper path normalization using std::filesystem::weakly_canonical() - Users should pass the correct datapath argument relative to current working directory

- Fix data race in common::Class::Log() by adding mutex to protect last_time global variable accessed by multiple threads - Fix OpenGL context race condition on macOS by releasing context from worker thread before dispatching swap to main thread, and using synchronous dispatch to ensure swap completes before continuing - Remove all debug logging code added during debugging session - Clean up unnecessary includes (<fstream>, <chrono>) added for debugging

- Add mutex to protect m_unit_updates unordered_map from concurrent access - QueueUnitUpdate() and PushUpdates() are called from different threads - Protect all access to m_unit_updates (find, insert, erase, empty, iteration, clear)

…ng for threads - MAIN thread waits for SDL_GL_SwapWindow() dispatched to main thread - Main thread was waiting for MAIN thread to stop without processing dispatch queue - Continue calling ProcessQueue() while waiting for threads to stop - This allows pending swap operations to complete and unblock MAIN thread

- Revert m_accumulations and m_accumulated_objects back to unordered_set - The mutex fixes added elsewhere likely resolved the underlying race conditions - Tree-based containers were a workaround; hash-based containers should work with proper synchronization

misclick

afwbkbc

To sum up:

do not fix what is not broken (even if AI thinks otherwise)
keep only changes related to Apple (MainThreadDispatch can stay but I would make it a module and include in t_main in Engine.cpp, instead of completely independent piece of code)
OpenGL is broken on Linux. When I removed those 3 lines, it works, however, moving map causes terrible framerates ( see example https://www.youtube.com/watch?v=-NLW8bq8-kg ), this was caused by changes in this MR. Zooming in/out also got small but noticeable (maybe 100ms) response lag, was near-instant before this MR.
Thanks.

afwbkbc · 2025-12-30T15:44:18Z

src/error_handler/Stdout.cpp

+	#define STRLEN_LITERAL(s) (sizeof(s) - 1)
+
+	// Alternate signal stack for crash handler (needed for SA_ONSTACK)
+	static char g_sigstack[SIGSTKSZ];


error: size of array 'g_sigstack' is not an integral constant-expression 20 | static char g_sigstack[SIGSTKSZ];

this is strange because SIGSTKSZ is a define with value 8192, but if I replace SIGSTKSZ with 8192 then it compiles. Using hardcoded 8192 is not right solution tho.

afwbkbc · 2025-12-30T15:56:07Z

src/audio/sdl2/SDL2.cpp

+#ifdef __APPLE__
+void SDL2::InitSDLOnMainThread() {


Is there a reason to NOT use initialization in main thread for all platforms?
I don't see any.
Maybe rename such methods to just Init() and remove #ifdef APPLE

afwbkbc · 2025-12-30T15:57:37Z

src/audio/sdl2/SDL2.cpp

+#ifdef __APPLE__
+	// On macOS, SDL initialization must happen on the main thread due to AppKit requirements
+	// SDL should already be initialized via InitSDLOnMainThread() called from main()


Same - I'd make such behavior default on all platforms

afwbkbc · 2025-12-30T15:59:08Z

src/common/Common.cpp


 #ifdef DEBUG
 static uint64_t last_time = 0;
+static std::mutex last_time_mutex;


I can see a disadvantage of lowering log throughput. In case of heavy logging it make slow things down.
I myself never encountered any issue with logs without that mutex.
We can keep it for now but if logs get too slow I'll remove it.

afwbkbc · 2025-12-30T16:05:33Z

src/common/MainThreadDispatch.cpp

@@ -0,0 +1,54 @@
+#include "MainThreadDispatch.h"


See t_main in Engine.cpp.
Also, this class belongs in util::, not common::

afwbkbc · 2025-12-30T16:43:35Z

src/gse/GSE.cpp

+			// Normalize path to resolve .. components
+			std::string normalized_path = path;
+			if ( !util::FS::IsAbsolutePath( path, PATH_SEPARATOR ) ) {
+				// Convert to absolute path first, then normalize
+				normalized_path = util::FS::GetAbsolutePath( path, PATH_SEPARATOR );
+			}
+			// Use std::filesystem to resolve .. components (weakly_canonical doesn't require path to exist)
+			try {
+				std::filesystem::path fs_path( normalized_path );
+				normalized_path = std::filesystem::weakly_canonical( fs_path ).string();
+			}
+			catch ( const std::exception& ) {
+				// If canonicalization fails, try lexically_normal as fallback
+				try {
+					std::filesystem::path fs_path( normalized_path );
+					normalized_path = fs_path.lexically_normal().string();
+				}
+				catch ( const std::exception& ) {
+					// If normalization fails, use original path
+					normalized_path = path;
+				}
+			}
+


This definetely does not belong here.
There is already util::FS::NormalizePath(), why not use it? Update with apple-specific logic if needed.

afwbkbc · 2025-12-30T16:48:07Z

src/gse/runner/Interpreter.cpp

 	}
 }

 gse::Value* const Interpreter::EvaluateExpression( context::Context* ctx, ExecutionPointer& ep, const Expression* expression, bool* returnflag ) {


Is there actual problem that is being fixed here? Or it's just AI thinks 'there might be a bug'.
Scripting engine is thoroughly and carefully tested and any change must have a very good reason, especially if it involves memory operations.
And how is it even related to Apple?

afwbkbc · 2025-12-30T16:50:38Z

src/input/sdl2/SDL2.cpp

-	}
-	return result;
+	// SDL_GetClipboardText() dispatched to main thread for consistency
+	auto future = common::MainThreadDispatch::GetInstance()->Dispatch<std::string>([]() {


If it's used often I'd make a macro like MAIN_THREAD_CALL( ... )

afwbkbc · 2025-12-30T16:51:17Z

src/main.cpp

 		graphics::opengl::OpenGL graphics( title, window_size.x, window_size.y, vsync, start_fullscreen );
 		audio::sdl2::SDL2 audio;
+		input::sdl2::SDL2 input;
+#ifdef __APPLE__


Maybe would make sense to use this for all platforms

afwbkbc · 2025-12-30T16:51:33Z

src/scheduler/Simple.cpp

-		Log( "Starting task [" + ( *it )->GetName() + "]" );
-		( *it )->Start();
+		try {
+			Log( "Starting task [" + ( *it )->GetName() + "]" );
+			( *it )->Start();
+		}
+		catch ( const std::exception& e ) {
+			throw;
+		}
+		catch ( ... ) {
+			throw;
+		}


What is this?

jhurliman added 13 commits December 27, 2025 13:46

Add CMAKE_EXPORT_COMPILE_COMMANDS for LSP support

7718120

- Enable compile_commands.json generation in CMakeLists.txt - Add CMAKE_EXPORT_COMPILE_COMMANDS to CMakePresets.json base preset - This enables C++ LSP (clangd) to work properly in IDEs

Add .cache/ and .cursor/ to .gitignore

cbb2eb0

Remove fallback path resolution hack

e2ff348

- Remove the fallback code that tried paths without ../ prefix - Keep proper path normalization using std::filesystem::weakly_canonical() - Users should pass the correct datapath argument relative to current working directory

debug cleanup

3c8adb5

Fix data race in UnitManager::m_unit_updates

be0c956

- Add mutex to protect m_unit_updates unordered_map from concurrent access - QueueUnitUpdate() and PushUpdates() are called from different threads - Protect all access to m_unit_updates (find, insert, erase, empty, iteration, clear)

afwbkbc previously approved these changes Dec 30, 2025

View reviewed changes

afwbkbc reviewed Dec 30, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

macOS port: Fix SDL initialization, race conditions, and shutdown deadlock #58

macOS port: Fix SDL initialization, race conditions, and shutdown deadlock #58

Uh oh!

jhurliman commented Dec 28, 2025 •

edited

Loading

Uh oh!

afwbkbc left a comment

Uh oh!

afwbkbc Dec 30, 2025

Uh oh!

afwbkbc Dec 30, 2025

Uh oh!

afwbkbc Dec 30, 2025

Uh oh!

afwbkbc Dec 30, 2025

Uh oh!

afwbkbc Dec 30, 2025

Uh oh!

afwbkbc Dec 30, 2025

Uh oh!

afwbkbc Dec 30, 2025

Uh oh!

afwbkbc Dec 30, 2025

Uh oh!

afwbkbc Dec 30, 2025

Uh oh!

afwbkbc Dec 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

macOS port: Fix SDL initialization, race conditions, and shutdown deadlock #58

Are you sure you want to change the base?

macOS port: Fix SDL initialization, race conditions, and shutdown deadlock #58

Uh oh!

Conversation

jhurliman commented Dec 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Key Changes

macOS-Specific Fixes

Race Condition Fixes (Detected by ThreadSanitizer)

Other Improvements

Testing

Uh oh!

afwbkbc left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jhurliman commented Dec 28, 2025 •

edited

Loading