Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate or provide a non-minimal SDL_config.h for Unix platforms #229

Open
smcv opened this issue Oct 8, 2024 · 11 comments
Open

Generate or provide a non-minimal SDL_config.h for Unix platforms #229

smcv opened this issue Oct 8, 2024 · 11 comments

Comments

@smcv
Copy link
Contributor

smcv commented Oct 8, 2024

If we want OS distributions (mostly Linux, but also *BSD, Hurd, Haiku, etc.) to eventually replace their SDL2 packages with sdl2-compat, then sdl2-compat's development headers should be as close as is feasible to being a drop-in replacement for "real" SDL2.

Unfortunately, SDL 2 has SDL_config.h as part of its API, defining various symbols that are really implementation details of SDL rather than anything that an application should be relying on, such as HAVE_MEMSET and SDL_AUDIO_DRIVER_ALSA. libsdl-org/sdl12-compat#302, libsdl-org/sdl12-compat#299, libsdl-org/sdl12-compat#297 are examples of places where we needed to make sdl12-compat bug-for-bug compatible with "real" SDL 1.2. I expect that for SDL 2, there should hopefully be less need to do that, but some of it will probably be necessary.

At the moment, in Debian I'm ignoring the SDL_config.h that ships with sdl2-compat, and instead providing a hand-edited header based on the one that Debian's "real SDL2" packaging generates at build-time, which makes some reasonable assumptions about Debian's toolchain, such as:

  • we have libc (glibc, in Debian's case) and all Standard C and POSIX headers and functions
  • we have all features that are available on any reasonable Unix, like SDL_TIMER_UNIX, SDL_VIDEO_DRIVER_X11 and HAVE_DBUS_DBUS_H
  • we have Linux-specific functionality like ALSA if and only if defined(__linux__) (all official Debian ports are Linux, but there are kFreeBSD and Hurd versions maintained by other-kernel enthusiasts)
  • we have Wayland and a modern version of libdecor if and only if defined(__linux__) (in principle Wayland should be implementable anywhere, but in Debian's ports, in practice we only have it on Linux)
  • we have x86 intrinsics if and only if targeting x86
  • we have a basic set of "normal" audio, input, etc. drivers for the platform
  • all dlopen'd libraries have their conventional SONAME for the platform (although we could perhaps simplify this into pretending that all libraries are linked with DT_NEEDED rather than dlopen'd, even if in reality they are dlopen'd)

Can we perhaps generalize that into a SDL_config_unix.h that is analogous to SDL_config_windows.h, but with more #ifdef?

@slouken
Copy link
Collaborator

slouken commented Oct 8, 2024

Sure! Feel free to make a PR for this.

@icculus
Copy link
Collaborator

icculus commented Jan 22, 2025

Fwiw, we opted not to do this in sdl12-compat, where the config file was more crucial and often misused like this...and were surprised that we could fake our way through most of it.

An app could build against SDL2 headers without a custom config script, unlike SDL 1.2, so I was hoping all of this would go away, but we had a report from Fedora already of an app checking for SDL_VIDEO_OPENGL, which is why 340227d got added.

I'm hoping we can get away with adding a few more hacks like that, and most apps will be happy. We'll know more as distros start chewing through their packages with sdl2-compat, though.

@smcv
Copy link
Contributor Author

smcv commented Jan 22, 2025

I'm hoping we can get away with adding a few more hacks like that, and most apps will be happy. We'll know more as distros start chewing through their packages with sdl2-compat, though.

I think the way to test this systematically would be:

  • revert the workaround that I'm currently applying in the Debian packaging;
  • go through a sample of Debian's SDL2 games;
  • build each one as a reference, against "real SDL2";
  • rebuild each one with --add-depends=libsdl2-compat-dev;
  • and use diffoscope to compare the two

(I'm not replacing "real SDL2" with sdl2-compat in Debian any time soon - we're meant to be freezing for Debian 13 somewhat soon, so Debian 14 seems like a better target for switching to sdl2-compat.)

@smcv
Copy link
Contributor Author

smcv commented Jan 22, 2025

Taking Debian packages that depend on SDL2 and start with digits or "A" as my initial sample, it isn't looking great so far.

Obvious build failures like these are actually less of a concern for me, I'm more worried about silent miscompilation resulting in a routine rebuild of a game becoming non-functional (like for example turning off rendering support because sdl2-compat doesn't claim to support a desired graphics backend).

  • 0ad — not tested, fails to build with current Python
  • 1oom — cannot easily tell whether there is a problem, but nothing obvious
  • 7kaa — cannot easily tell whether there is a problem, but nothing obvious
  • adonthell — fails to build with libsdl2-compat-dev error: ‘strcmp’ was not declared in this scope, perhaps SDL2 included <string.h> or defined HAVE_STRING_H and sdl2-compat does not?
  • allure — the diff is huge, I can't tell whether there's a problem or not
  • angband — cannot easily tell whether there is a problem, but nothing obvious
  • antimicro ­— fails to build with libsdl2-compat-dev error: ‘isnan’ was not declared in this scope
  • aranym — error: ‘FILE’ was not declared in this scope, perhaps SDL2 included <stdio.h> or defined HAVE_STDIO_H and sdl2-compat does not?
  • ares — cannot easily tell whether there is a problem, but nothing obvious
  • aseba —not tested, unrelated build failure
  • assaultcube — cannot easily tell whether there is a problem, the diff is too large
  • audacious-plugins — does have differences in the SDL plugin, cannot tell whether they are functionally significant

@icculus
Copy link
Collaborator

icculus commented Jan 22, 2025

I'm more worried about silent miscompilation resulting in a routine rebuild of a game becoming non-functional (like for example turning off rendering support because sdl2-compat doesn't claim to support a desired graphics backend).

I wonder how hard it would be to automate comparing the binaries between two builds...in theory, if the only difference is the SDL2 headers, an identical binary means we did what was expected.

But yeah, we'll probably need to add a #ifdef unix section to SDL_config that just includes some standard headers and HAVE_STRCPY or whatever. We did this for sdl12-compat too. It's not like you're going to find a reasonable Linux system without these functions that also wants to run audacious-plugins.

The good news is that SDL3 removed this nonsense completely, so sdl3-compat won't have this issue in ten years. :)

@smcv
Copy link
Contributor Author

smcv commented Jan 22, 2025

I wonder how hard it would be to automate comparing the binaries between two builds...in theory, if the only difference is the SDL2 headers, an identical binary means we did what was expected.

That's exactly what I was trying to do (diffoscope does content-aware comparisons of binaries). But unfortunately, at a minimum the switch from the classic SDL2 headers to the sdl2-compat headers results in build-IDs changing (because the source code is different), and it seems that in several cases it results in larger and harder-to-ignore differences, like the order of functions being swapped (which perturbs all the offsets in the disassembly), or perhaps code being optimized differently.

Perhaps I can get better comparisons by building with DEB_BUILD_OPTIONS="noopt nostrip"...

@slouken
Copy link
Collaborator

slouken commented Feb 1, 2025

I've added a conservative UNIX platform header in be424f7, does this take care of our needs here?

@smcv
Copy link
Contributor Author

smcv commented Feb 3, 2025

That looks like the sort of thing I had in mind. I'll compare it with what "real SDL2" gives us.

#if defined(__LP64__) || defined(_LP64) || defined(_WIN64)
#define SIZEOF_VOIDP 8
#else
#define SIZEOF_VOIDP 4
#endif

This seems good enough on Linux, but error-prone on other Unixes. we should probably have a

SDL_COMPILE_TIME_ASSERT(SIZEOF_VOIDP, sizeof(void *) == SIZEOF_VOIDP);

in a .c file that includes this header, to make sure we aren't mis-detecting it on some weird platform.

/* Useful headers */

Perhaps a better comment would be: /* Assume that any reasonable Unix platform has Standard C and POSIX headers */ (and ideally we should check that these are headers that are specified by Standard C or POSIX).

#elif defined (__LINUX__) || defined(__FREEBSD__) || defined(__NETBSD__) || defined(__OPENBSD__) || defined(__SOLARIS__)

Can we use #elif defined(__unix__) (which is predefined on Linux with gcc and clang, I don't know about the others), and only add specific Unixes if they are known not to predefine that macro?

Or at least use #elif defined(__unix__) || defined(__LINUX__) || ... to catch "most" Unixes?

@slouken
Copy link
Collaborator

slouken commented Feb 3, 2025

That looks like the sort of thing I had in mind. I'll compare it with what "real SDL2" gives us.

#if defined(__LP64__) || defined(_LP64) || defined(_WIN64)
#define SIZEOF_VOIDP 8
#else
#define SIZEOF_VOIDP 4
#endif

This seems good enough on Linux, but error-prone on other Unixes. we should probably have a

SDL_COMPILE_TIME_ASSERT(SIZEOF_VOIDP, sizeof(void *) == SIZEOF_VOIDP);

in a .c file that includes this header, to make sure we aren't mis-detecting it on some weird platform.

/* Useful headers */

Perhaps a better comment would be: /* Assume that any reasonable Unix platform has Standard C and POSIX headers */ (and ideally we should check that these are headers that are specified by Standard C or POSIX).

#elif defined (__LINUX__) || defined(__FREEBSD__) || defined(__NETBSD__) || defined(__OPENBSD__) || defined(__SOLARIS__)

Can we use #elif defined(__unix__) (which is predefined on Linux with gcc and clang, I don't know about the others), and only add specific Unixes if they are known not to predefine that macro?

Or at least use #elif defined(__unix__) || defined(__LINUX__) || ... to catch "most" Unixes?

I believe __unix__ is defined on all those platforms, and I think we can use that as a stand-in until we get a bug report otherwise.

All these changes sound good, want to make a PR?

@smcv
Copy link
Contributor Author

smcv commented Feb 3, 2025

All these changes sound good, want to make a PR?

I'll add it to my queue (your bug is important to us, please hold)

smcv added a commit to smcv/sdl2-compat that referenced this issue Feb 4, 2025
As discussed on libsdl-org#229, if we're going to try to detect this from compiler
predefined macros like `__LP64__`, we should verify that we've done
it correctly; otherwise, we could end up incorrectly assuming 32-bit
pointers on a 64-bit Unix platform that doesn't define `__LP64__`
(if such a thing exists).

This static assertion succeeds on all Debian architectures where a build
has been attempted, including all release architectures and several
unofficial ports. It might fail on non-Linux kernels or non-GNU libc,
but if it does, our build on those platforms would have been using the
wrong value for `SIZEOF_VOIDP` already, so it'll only be detecting a
pre-existing bug.

Signed-off-by: Simon McVittie <smcv@collabora.com>
@smcv
Copy link
Contributor Author

smcv commented Feb 4, 2025

#314 and #315 make a start on this.

I'll compare it with what "real SDL2" gives us

I think it would still be good to do that, and fill in at least the low-hanging fruit.

slouken pushed a commit that referenced this issue Feb 4, 2025
As discussed on #229, if we're going to try to detect this from compiler
predefined macros like `__LP64__`, we should verify that we've done
it correctly; otherwise, we could end up incorrectly assuming 32-bit
pointers on a 64-bit Unix platform that doesn't define `__LP64__`
(if such a thing exists).

This static assertion succeeds on all Debian architectures where a build
has been attempted, including all release architectures and several
unofficial ports. It might fail on non-Linux kernels or non-GNU libc,
but if it does, our build on those platforms would have been using the
wrong value for `SIZEOF_VOIDP` already, so it'll only be detecting a
pre-existing bug.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants