libsdl-org/sdl2-compat

windows SDL_CreateThread hack correctness

sezero opened this issue · 9 comments

See: https://github.com/libsdl-org/sdl2-compat/blob/main/src/sdl3_include_wrapper.h#L871-L877
and: https://github.com/libsdl-org/sdl2-compat/blob/main/src/sdl3_include_wrapper.h#L892-L916

For normal windows it's correct. But I don't know about WinRT and GDK:
At the least, is SDL_PASSED_BEGINTHREAD_ENDTHREAD machinery used with
WinRT and GDK? If not, we have to exclude them there.

I think we use them on winrt, I'll check over here today.

No idea about GDK, though.

@slouken, @icculus: Any news about this one?

The SDL_PASSED_BEGINTHREAD_ENDTHREAD magic seems to be only for WIN32
desktop and GDK: We specifically not include __WINRT__ for it:
https://github.com/libsdl-org/SDL/blob/main/include/SDL3/SDL_thread.h#L84-L105

Now if we only had a good way of early-detecting that at compile time..

@icculus, @slouken: Is the following correct and good-enough?

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 8038f75..c727026 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -204,6 +204,11 @@ if(CMAKE_C_COMPILER_ID MATCHES "Clang|GNU")
   endif()
 endif()
 
+check_include_file("winapifamily.h" HAVE_WINAPIFAMILY_H)
+if(HAVE_WINAPIFAMILY_H)
+  set(EXTRA_CFLAGS "${EXTRA_CFLAGS} -DSDL_HAVE_WINAPIFAMILY_H")
+endif()
+
 # This isn't needed for compat libSDL2 who use SDL3 headers.
 # SDL2_test and SDL2_main (and the tes programs) still need
 # this, because they use SDL2 headers.
diff --git a/src/sdl3_include_wrapper.h b/src/sdl3_include_wrapper.h
index 592e4e6..95c341d 100644
--- a/src/sdl3_include_wrapper.h
+++ b/src/sdl3_include_wrapper.h
@@ -868,8 +868,15 @@
 #define SDL_LogSetOutputFunction IGNORE_THIS_VERSION_OF_SDL_LogSetOutputFunction
 
 /* *** HACK HACK HACK:
- * *** Avoid including SDL_thread.h: it defines SDL_CreateThread() as a macro */
+ * *** Avoid including SDL_thread.h: it defines SDL_CreateThread() as a macro
+ * *** for Win32 Desktop and GDK, but not for WinRT. */
+#ifdef SDL_HAVE_WINAPIFAMILY_H
+#include <winapifamily.h>
+#if (!WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_DESKTOP) && WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_APP))
+#define SDL2COMPAT_WINRT
+#endif
+#endif
-#ifdef _WIN32
+#if defined(_WIN32) && !defined(SDL2COMPAT_WINRT)
 #define _SDL_thread_h
 #define SDL_thread_h_
 #define SDL_PASSED_BEGINTHREAD_ENDTHREAD
@@ -890,7 +897,7 @@
 #include <SDL3/SDL_vulkan.h>
 
 /* Missing SDL_thread.h stuff (see above) */
-#ifdef _WIN32
+#if defined(_WIN32) && !defined(SDL2COMPAT_WINRT)
 #ifndef WIN32_LEAN_AND_MEAN
 #define WIN32_LEAN_AND_MEAN 1
 #endif
diff --git a/src/sdl3_syms.h b/src/sdl3_syms.h
index a619778..5a7fb12 100644
--- a/src/sdl3_syms.h
+++ b/src/sdl3_syms.h
@@ -659,7 +659,7 @@ SDL3_SYM_PASSTHROUGH(SDL_bool,IsTablet,(void),(),return)
 SDL3_SYM(SDL_DisplayOrientation,GetDisplayOrientation,(SDL_DisplayID a),(a),return)
 SDL3_SYM_RENAMED(SDL_bool,HasColorKey,SurfaceHasColorKey,(SDL_Surface *a),(a),return)
 
-#if defined(__WIN32__) || defined(__GDK__)
+#if (defined(__WIN32__) || defined(__GDK__)) && !defined(__WINRT__)
 SDL3_SYM_PASSTHROUGH(SDL_Thread*,CreateThreadWithStackSize,(SDL_ThreadFunction a, const char *b, const size_t c, void *d, pfnSDL_CurrentBeginThread e, pfnSDL_CurrentEndThread f),(a,b,c,d,e,f),return)
 #else
 SDL3_SYM_PASSTHROUGH(SDL_Thread*,CreateThreadWithStackSize,(SDL_ThreadFunction a, const char *b, const size_t c, void *d),(a,b,c,d),return)

P.S.: If sdl2-compat is going to support WinRT, it would need some more changes,
e.g. changing LoadLibrary to LoadPackagedLibrary and possibly more.

This seems reasonable to me.

I don't think sdl2-compat on Windows RT is a big priority. I think new development will use SDL3 and old projects will have SDL2 bundled with them.

Go ahead and apply this.

We should support WinRT, but I'm inclined to leave it alone until someone files a bug about it.

We should support WinRT, but I'm inclined to leave it alone until someone files a bug about it.

As of commit 3f84910, any attempt to build sdl2-compat for WinRT fails
because of unsupported calls to MessageBoxA, lstrcpyA and wsprintfA, and
possibly LoadLibraryA.