libsdl-org / SDL

Simple Directmedia Layer
https://libsdl.org
zlib License
9.79k stars 1.82k forks source link

inclusion of intrinsics headers in SDL_cpuinfo.h makes it impossible to use SDL2 with alternative compilers #4729

Closed rofl0r closed 1 year ago

rofl0r commented 3 years ago

i'd like to compile my SDL2-using projects with alternative C compilers, such as tcc or cproc for faster compilation speed (or to check for GNUisms) while developing, however all of them bail out failing to parse GCC's intrinsics headers which are unconditionally included.

after some discussion with fellow compiler authors and linux distribution packagers we all agreed that SDL shouldn't include the intrinsics headers from a public, but only from a private header for internal use (i.e. during compile of SDL2 itself).

alternatively, the inclusion of all of the intrinsics headers could be shielded by some preprocessor macro such as SDL_INTERNAL which is defined during the build using the build system but not in SDL's headers themselves.

InfoTeddy commented 3 years ago

SDL shouldn't include the intrinsics headers from a public, but only from a private header for internal use (i.e. during compile of SDL2 itself).

Sounds like src/SDL_internal.h is the perfect place to move these.

alternatively, the inclusion of all of the intrinsics headers could be shielded by some preprocessor macro such as SDL_INTERNAL which is defined during the build using the build system but not in SDL's headers themselves.

Taking a look at SDL_cpuinfo.h, I see several checks for SDL_DISABLE_* macros around the intrinsics includes. Have you tried using those?

rofl0r commented 3 years ago

Sounds like src/SDL_internal.h is the perfect place to move these.

indeed.

Taking a look at SDL_cpuinfo.h, I see several checks for SDLDISABLE* macros around the intrinsics includes. Have you tried using those?

right, my project compiles with current SDL2 release if i add -DSDL_DISABLE_ARM_NEON_H -DSDL_DISABLE_MM3DNOW_H -DSDL_DISABLE_IMMINTRIN_H -DSDL_DISABLE_MMINTRIN_H -DSDL_DISABLE_XMMINTRIN_H -DSDL_DISABLE_EMMINTRIN_H -DSDL_DISABLE_PMMINTRIN_H to my CPPFLAGS. problem is it's opt-out instead of opt-in, and a user will be forced to constantly revise this stanza with every SDL release as it's likely more platform-specific intrinsics headers will be added over time.

slouken commented 2 years ago

The intention is for the application to be able to make use of intrinsics by including SDL_cpuinfo.h. If we move these headers out, we should probably create a separate header that includes them so people relying on this functionality can easily fix their build.

slouken commented 2 years ago

This might be important to consider with https://github.com/libsdl-org/SDL/issues/4531

icculus commented 2 years ago

So I took a moment to try building SDL with cproc, and while this thing is cool, it'd take more work to get this to build everything, but here's a patch to get around the inline assembly issue...just dumping this in here because I don't want to find out this breaks a bunch of targets this close to 2.0.22 shipping, but I think it's a reasonable change over all.

commit 416abc625042f94b86c1c7932b14fb7cbd240cf9 (HEAD -> main)
Author: Ryan C. Gordon <icculus@icculus.org>
Date:   Wed Mar 30 10:05:03 2022 -0400

    byteswap: Only do GNU-style inline assembly on things that support it.

diff --git a/include/SDL_endian.h b/include/SDL_endian.h
index 2866f4bea..9e30ca8b9 100644
--- a/include/SDL_endian.h
+++ b/include/SDL_endian.h
@@ -121,21 +121,21 @@ extern "C" {
 #elif defined(_MSC_VER) && (_MSC_VER >= 1400)
 #pragma intrinsic(_byteswap_ushort)
 #define SDL_Swap16(x) _byteswap_ushort(x)
-#elif defined(__i386__) && !HAS_BROKEN_BSWAP
+#elif (defined(__GNUC__) || defined(__clang__)) && defined(__i386__) && !HAS_BROKEN_BSWAP
 SDL_FORCE_INLINE Uint16
 SDL_Swap16(Uint16 x)
 {
   __asm__("xchgb %b0,%h0": "=q"(x):"0"(x));
     return x;
 }
-#elif defined(__x86_64__)
+#elif (defined(__GNUC__) || defined(__clang__)) && defined(__x86_64__)
 SDL_FORCE_INLINE Uint16
 SDL_Swap16(Uint16 x)
 {
   __asm__("xchgb %b0,%h0": "=Q"(x):"0"(x));
     return x;
 }
-#elif (defined(__powerpc__) || defined(__ppc__))
+#elif (defined(__GNUC__) || defined(__clang__)) && (defined(__powerpc__) || defined(__ppc__))
 SDL_FORCE_INLINE Uint16
 SDL_Swap16(Uint16 x)
 {
@@ -144,7 +144,7 @@ SDL_Swap16(Uint16 x)
   __asm__("rlwimi %0,%2,8,16,23": "=&r"(result):"0"(x >> 8), "r"(x));
     return (Uint16)result;
 }
-#elif (defined(__m68k__) && !defined(__mcoldfire__))
+#elif (defined(__GNUC__) || defined(__clang__)) && (defined(__m68k__) && !defined(__mcoldfire__))
 SDL_FORCE_INLINE Uint16
 SDL_Swap16(Uint16 x)
 {
@@ -170,21 +170,21 @@ SDL_Swap16(Uint16 x)
 #elif defined(_MSC_VER) && (_MSC_VER >= 1400)
 #pragma intrinsic(_byteswap_ulong)
 #define SDL_Swap32(x) _byteswap_ulong(x)
-#elif defined(__i386__) && !HAS_BROKEN_BSWAP
+#elif (defined(__GNUC__) || defined(__clang__)) && defined(__i386__) && !HAS_BROKEN_BSWAP
 SDL_FORCE_INLINE Uint32
 SDL_Swap32(Uint32 x)
 {
   __asm__("bswap %0": "=r"(x):"0"(x));
     return x;
 }
-#elif defined(__x86_64__)
+#elif (defined(__GNUC__) || defined(__clang__)) && defined(__x86_64__)
 SDL_FORCE_INLINE Uint32
 SDL_Swap32(Uint32 x)
 {
   __asm__("bswapl %0": "=r"(x):"0"(x));
     return x;
 }
-#elif (defined(__powerpc__) || defined(__ppc__))
+#elif (defined(__GNUC__) || defined(__clang__)) && (defined(__powerpc__) || defined(__ppc__))
 SDL_FORCE_INLINE Uint32
 SDL_Swap32(Uint32 x)
 {
@@ -195,7 +195,7 @@ SDL_Swap32(Uint32 x)
   __asm__("rlwimi %0,%2,24,0,7"  : "=&r"(result): "0" (result), "r"(x));
     return result;
 }
-#elif (defined(__m68k__) && !defined(__mcoldfire__))
+#elif (defined(__GNUC__) || defined(__clang__)) && (defined(__m68k__) && !defined(__mcoldfire__))
 SDL_FORCE_INLINE Uint32
 SDL_Swap32(Uint32 x)
 {
@@ -222,7 +222,7 @@ SDL_Swap32(Uint32 x)
 #elif defined(_MSC_VER) && (_MSC_VER >= 1400)
 #pragma intrinsic(_byteswap_uint64)
 #define SDL_Swap64(x) _byteswap_uint64(x)
-#elif defined(__i386__) && !HAS_BROKEN_BSWAP
+#elif (defined(__GNUC__) || defined(__clang__)) && defined(__i386__) && !HAS_BROKEN_BSWAP
 SDL_FORCE_INLINE Uint64
 SDL_Swap64(Uint64 x)
 {
@@ -238,14 +238,14 @@ SDL_Swap64(Uint64 x)
           : "0" (v.s.a),  "1"(v.s.b));
     return v.u;
 }
-#elif defined(__x86_64__)
+#elif (defined(__GNUC__) || defined(__clang__)) && defined(__x86_64__)
 SDL_FORCE_INLINE Uint64
 SDL_Swap64(Uint64 x)
 {
   __asm__("bswapq %0": "=r"(x):"0"(x));
     return x;
 }
-#elif defined(__WATCOMC__) && defined(__386__)
+#elif (defined(__GNUC__) || defined(__clang__)) && defined(__WATCOMC__) && defined(__386__)
 extern __inline Uint64 SDL_Swap64(Uint64);
 #pragma aux SDL_Swap64 = \
   "bswap eax"     \
icculus commented 2 years ago

As for just using the headers with cproc, this is made more difficult by the fact it doesn't appear to define a preprocessor macro to alert us that we're building with that specific compiler.

Other tools may or may not have similar problems, but this isn't looking like a quick fix anymore, so this is bumping out of the milestone for when we rewrite this code.

sezero commented 2 years ago
-#elif defined(__WATCOMC__) && defined(__386__)
+#elif (defined(__GNUC__) || defined(__clang__)) && defined(__WATCOMC__) && defined(__386__)

This part at least is wrong

icculus commented 2 years ago

And that's why I didn't commit it...! 🤪

rofl0r commented 1 year ago

the PR closing this issue uses #include <SDL3/...> - does this mean there won't ever be an SDL2 version released that can be used with a compiler other than gcc and clang ?

slouken commented 1 year ago

Yes, we're not planning to make this change for SDL2.