libretro-common: get strlcpy declarations on macOS

[atsampson]

Defining _XOPEN_SOURCE here disabled the BSD extended string functions on macOS. Define _DARWIN_C_SOURCE instead on that platform.

(Please let the tests finish on this before merging - I don't have a macOS machine to test locally...)

[atsampson]

That looks happier now.

[rofl0r]

this unbreaks CI, so i guess it should be merged. though usually one uses __APPLE__, not __MACH__ to check for mac os. @rogerman what do you suggest ?

[rogerman]

Use __APPLE__ for modern code.

__MACH__ was used for old code from the 2000s when Apple was transitioning between Mac OS 9 and Mac OS X, where __MACH__ was used to differentiate between the two operating systems. Obviously, we are decades beyond this point.

DeSmuME will never run on any Mac OS that is pre OS X for many reasons technical and maintainable, and so it is proper to use __APPLE__ instead of __MACH__ for DeSmuME.

EDIT: Oh, I see that this is for the libretro library. Does RetroArch even work on Mac OS 9? If it does, then maybe using __MACH__ has some merit. With that said, I do wonder what problem commit 90d0abd actually solved to warrant all these changes in the first place? It almost seems like it was done "just because".

[rofl0r]

implicit function declarations are nasty because they promote all arguments to int. on 64bit archs that leads to silent pointer truncation to 32bits with sign extension and subsequently crashes when dereferencing the pointer. and it seems gcc14 now defaults to erroring out on encountering an implicit declaration. so the commit fixed an actual issue.

[rogerman]

gcc14 now defaults to erroring out on encountering an implicit declaration. so the commit fixed an actual issue.

Ummm.... what?!! Who's compiling using implicit function declarations?

The Xcode projects for macOS and the VS project for Windows certainly don't allow for implicit function declarations. And the last time I compiled for GTK2 Linux using autotools (a few months ago), it certainly didn't allow for implicit function declarations either. AFAIK, all of our builds should throw an error when this happens. If GCC was allowing for them, then this would be considered an error. GCC14 gets this correct, and what should have always been assumed in the first place.

So again -- who is compiling using implicit function declarations?

[rofl0r]

Ummm.... what?!! Who's compiling using implicit function declarations?

basically anyone with a C compiler < gcc 14 who didn't stuff -Werror=implicit-function-declaration into his CFLAGS.
the reason an implicit function declaration happened is because strlcpy() is a non-standard function and exposure from the libc headers require usage of a feature test macro. often these are different for many libcs.
the most future proof fix would probably be to simply not use strlcpy.

[rogerman]

basically anyone with a C compiler < gcc 14 who didn't stuff -Werror=implicit-function-declaration into his CFLAGS.

Yes, not including that flags for your build is the user's fault, and it should be considered a user error if the build fails.

the reason an implicit function declaration happened is because strlcpy() is a non-standard function and exposure from the libc headers require usage of a feature test macro. often these are different for many libcs.
the most future proof fix would probably be to simply not use strlcpy.

We already don't use strlcpy() for platforms that don't support it. I personally do use strlcpy() in the Cocoa port alone, but that's because I know with certainty that the Cocoa port will have it. No other ports use strlcpy() directly. The only way that strlcpy() could be used is indirectly via an internal libretro function call.

Which leads me to my main concern: I see that libretro already has strlcpy() covered. Just include /libretro/compat/compat_strl.c in your build and it should work just fine. The header /libretro/include/compat/strl.h already has the proper function declaration in it for strlcpy(), and so as long as compat_strl.c is included in the build, then libretro's internal calls to strlcpy() should work too.

The more I look into this issue, the more I'm leaning to conclude that implicit function declarations are just a sign of user error, and that commit 90d0abd was completely unnecessary. It's looking more like commit 90d0abd was done 'just because' it didn't seem right at first glance, disregarding the fact that libretro already had compat_strl.c and many other compatibility functions to cover such circumstances.

I am curious to know which exact functions were being reported as implicitly declared prior to commit 90d0abd. There may be a something in libretro/compat/ that may help out with that.

EDIT: Arrgh... I see that the existing way to deal with the lack of strdup() and realpath() is to include /libretro/compat/compat_posix_string.c. The problem is that strdup() can be handled with retro_strdup__() in compat_posix_string.c, but realpath() has no alternative. So commit 90d0abd actually is necessary in this case.

However, I've devised a better way to deal with the Mac build problem while respecting GCC14 issue and also respecting libretro's include pattern. I'll close this PR and fix it through another commit.

[rogerman]

I fixed this a slightly different way in commit 779606e. @atsampson, thanks for the heads up!

[atsampson]

That breaks it again on Linux, though, I'm afraid. You need to #define _XOPEN_SOURCE before the very first #include - doing that with #ifndef __MACH__ wrapped around it should work?

[rogerman]

That breaks it again on Linux, though, I'm afraid.

Our CI disagrees. All builds passed for commit 779606e just fine, including the Linux ones. What do you mean by "breaks it again on Linux"?

[rofl0r]

Yes, not including that flags for your build is the user's fault, and it should be considered a user error if the build fails.

not including the flag leads to a warning somewhere in the build log and then a crash at runtime.
including it leads to a failed build.
i doubt you can expect that the user is familiar enough with C compilation caveats to put the right feature test macro combination into his CPPFLAGS (which in turn could cause other issues - like this followup PR shows, because BSDs disable the default features as soon as one specifies additional FTMs). so it seems reasonable to fix the source, and not burden the user.

However, I've devised a better way to deal with the Mac build problem while respecting GCC14 issue

it's not really a GCC14 issue - unless a build error is an issue, but crashing isn't.
but anyway, thanks for taking a look and please go ahead with your devised fix, so CI goes back to green.

[atsampson]

Your CI is using an older version of GCC, but it produces the same warnings here that GCC 14 treats as fatal errors:

[99/164] Compiling C object 'desmume@sta/.._.._libretro-common_file_file_path.c.o'.
../../../libretro-common/file/file_path.c: In function ‘path_mkdir’:
../../../libretro-common/file/file_path.c:54:24: warning: implicit declaration of function ‘strdup’; did you mean ‘strcmp’? [-Wimplicit-function-declaration]
   54 |    char     *basedir = strdup(dir);
      |                        ^~~~~~
      |                        strcmp
../../../libretro-common/file/file_path.c: In function ‘path_resolve_realpath’:
../../../libretro-common/file/file_path.c:542:9: warning: implicit declaration of function ‘realpath’ [-Wimplicit-function-declaration]
  542 |    if (!realpath(tmp, buf))
      |         ^~~~~~~~

When I build with GCC 14.1, the build fails there.

In general, any of the feature test macros like _POSIX_SOURCE and _BSD_SOURCE need to be defined before you include any headers - the first system header you include will usually pull in the internal header that expands them out.

(I think another way to fix this would be to set c_std=gnu11 rather than c_std=c11 in meson.build, but the way you're currently doing it - enforcing C11 and just turning on the non-C11 features you need explicitly where necessary - means you're more likely to catch non-standard things like this.)

[rofl0r]

btw there's also a catch-all feature test macro called _ALL_SOURCE which can be used instead of the XOPEN.. on newer glibcs and musl, to have all non-standard features enabled. i don't know whether the BSDs and apple adopted it too, but using it seems more sensible than fucking with the C standard in the build infrastructure.

[rogerman]

I would rather that we didn't change the libretro library too much so that we don't get too far away from what's upstream.

With that said, I was very surprised that strdup() failed and took another look at compat_posix_string.c, only to find out that libretro's implementation for strdup() is for WIN32 only. Who knows what other cross-platform flaws may be lurking? I don't want to find out.

So instead, we'll go with @atsampson's solution to keep changes to libretro as minimal as possible. While it looks messy, I don't know of any other solution that doesn't involve more extensive changes to what is a third-party library in the end.

[rogerman]

@rofl0r, do you know if we can configure our CI so that Linux builds treat implicit function declarations as errors instead of warnings?

[zeromus]

if libretro-common is using a nonstandard function strlcpy without its own compat include, then we should just fix that in libretro-common without worrying about upstream

[rofl0r]

do you know if we can configure our CI so that Linux builds treat implicit function declarations as errors instead of warnings?

for meson build you can just put -Werror=implicit-function-declaration into CFLAGS, for autoconf potentially too, but many old configure checks use implicit declarations as a feature and produce incorrect results when the feature is turned off. that was the main reason this wasn't enabled by default until now.