minor endian bits / issues?

[sezero]

Noticed these in a morphos fork by @BeWorld2018, this commit in particular: https://github.com/BeWorld2018/SDL/commit/6f259e4f44dd4e0ea6518ba82e5107df798e06c1 If you guys review the following, we can pick the necessary bits for us here in mainstream.

• In SDL_atomic.h, the morphos fork uses sync instead of lwsync for SDL_MemoryBarrierAcquire and SDL_MemoryBarrierRelease. Don't know the difference / significance of it.

• Missing ppc checks for ppc: Our current ppc preprocessor checks reflect Apple's old gcc macros, and other macros, i.e. __PPC__ and __powerpc__ are missing. On the other hand, the _M_PPC macro feels like microsoft's and if it really is so then it is wrong because windows is always little endian.

  
  diff --git a/include/SDL_endian.h b/include/SDL_endian.h
  index e1c6b5e..256855e 100644
  --- a/include/SDL_endian.h
  +++ b/include/SDL_endian.h
  @@ -78,7 +78,7 @@
  #if defined(__hppa__) || \
   defined(__m68k__) || defined(mc68000) || defined(_M_M68K) || \
   (defined(__MIPS__) && defined(__MIPSEB__)) || \

• defined(ppc) || defined(POWERPC) || defined(_M_PPC) || \

• defined(ppc) || defined(POWERPC) || defined(powerpc) || defined(_M_PPC) || \ defined(sparc)

  define SDL_BYTEORDER SDL_BIG_ENDIAN

  else

• The vendor, product and version assignments in SDL_GetJoystickGUIDInfo need byte-swapping? The commit I referenced above swaps bytes for vendor and product, seems to have missed version.

  
  diff --git a/src/joystick/SDL_joystick.c b/src/joystick/SDL_joystick.c
  index 5ce191f..6fc32f7 100644
  --- a/src/joystick/SDL_joystick.c
  +++ b/src/joystick/SDL_joystick.c
  @@ -1759,15 +1759,15 @@ void SDL_GetJoystickGUIDInfo(SDL_JoystickGUID guid, Uint16 *vendor, Uint16 *prod
       /* guid16[4] is product ID */
       guid16[5] == 0x0000
       /* guid16[6] is product version */

• ) {

• ) { if (vendor) {

• *vendor = guid16[2];

• *vendor = SDL_SwapLE16(guid16[2]); } if (product) {

• *product = guid16[4];

• *product = SDL_SwapLE16(guid16[4]); } if (version) {

• *version = guid16[6];

• *version = SDL_SwapLE16(guid16[6]); } } else { if (vendor) {

[slouken]

These are fine to apply...

[sezero]

OK then, will push shortly.

Should I remove the _M_PPC check (and maybe replace it with __PPC__ instead)?

[sezero]

OK, pushed b8f30c021bd4a96765b84744939eece23c432478 and 4fa2653394150140c4d69cf66a78cd83e1175f99, where I also removed that _M_PPC thing.

The only remaining item in this ticket is that ppc sync vs lwsync thing: If we aren't going to do anything about it we can close this ticket.

[slouken]

The only remaining item in this ticket is that ppc sync vs lwsync thing: If we aren't going to do anything about it we can close this ticket.

We intentionally used lwsync, but if there was a reason they used the heavy-weight primitive instead that would be good to know.

[sezero]

We intentionally used lwsync, but if there was a reason they used the heavy-weight primitive instead that would be good to know.

@BeWorld2018 ?

[cgutman]

Maybe this? https://github.com/concurrencykit/ck/issues/121

[sezero]

Maybe this? concurrencykit/ck#121

@slouken?

[slouken]

Let's leave it as-is unless we get a PR with a specific test case.

[sezero]

These are fine to apply...

Even though you approved, I have a feeling that https://github.com/libsdl-org/SDL/commit/4fa2653394150140c4d69cf66a78cd83e1175f99 should be actually tested on a big-endian host. Do we have anyone who could do that?

[slouken]

If we write an automated test using the virtual joystick, we might be able to have @smcv run it on a big endian CI system.

[sezero]

I don't know how to do that though, can't help with it.

[smcv]

I have access to big-endian remote systems (Debian mips and s390x, which are 32-bit and 64-bit BE respectively), but not to real hardware; so yes, any test that you want me to run on those systems would have to be "headless" using a virtual game controller or something.

Assuming that defined(__ppc__) || defined(__POWERPC__) || defined(__powerpc__) || defined(__PPC__) implies big-endian is not really 100% correct, because more recent PowerPC systems run in little-endian mode (the Debian ppc64el architecture is like this). However, modern gcc/clang versions have __BYTE_ORDER__, and Linux/*BSD have <endian.h> or similar, both of which SDL tests earlier; so the only systems where those architecture macros are used would be non-Linux, non-BSD systems with non-gcc, non-clang compilers, which is pretty obscure these days.

[sezero]

Assuming that defined(__ppc__) || defined(__POWERPC__) || defined(__powerpc__) || defined(__PPC__) implies big-endian is not really 100% correct,

Newer versions are already covered by __ORDER_xxx_ENDIAN__ and __BYTE_ORDER__ above.

[slouken]

I added a test to cover this in https://github.com/libsdl-org/SDL/commit/7e2a99695858e5c6c744ae3075513958ee6eebf6 Just build and run testautomation --filter Joystick

[sezero]

I have a feeling that 4fa2653 should be actually tested on a big-endian host.

I added a test to cover this in 7e2a996 Just build and run testautomation --filter Joystick

@smcv: is this doable for you? If not, I can just revert commit 4fa2653

[slouken]

Please don't revert the commit, I'm sure @smcv will look at it as he has time. In the meantime it works correctly on little-endian platforms.