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

Are cores allowed to depend on LC_ALL=C? #21

Open
Alcaro opened this issue Nov 15, 2014 · 3 comments
Open

Are cores allowed to depend on LC_ALL=C? #21

Alcaro opened this issue Nov 15, 2014 · 3 comments

Comments

@Alcaro
Copy link
Collaborator

Alcaro commented Nov 15, 2014

mupen64plus-libretro/glide2gl/src/Glitch64/glitch64_combiner.c, line 304:
sprintf(s, FRAGDEPTH " = dot(texture2D(texture0, vec2(gl_TexCoord[0])), vec4(31*64*32, 63*32, 31, 0))*%g + %g; \n", zscale/2/65535.0, 1-zscale/2);
This looks completely fine, right?

Nope: In the Swedish locale (and probably most of Europe), the result is
gl_FragDepth = dot(texture2D(texture0, vec2(gl_TexCoord[0])), vec4(31*64*32, 63*32, 31, 0))*7,62951e-06 + 0,5;
which is the same thing as gl_FragDepth = dot(...)*7, 62951e-06+0, 5, which equals 5 and is not a useful depth. Locale is the last thing I'd blame for GL rendering errors; I found it only because GL_ARB_debug_output whined about pointless left-hand expressions.

Is the bug that the frontend is setting the locale to something unexpected, or is the bug that the core is using a locale-respecting function as input to something that doesn't support locales? The answer is far from obvious (if it was, I would've pushed a fix somewhere without asking).

If we declare that the core is allowed to depend on LC_ALL=C:

  • The frontend will be unable to format things according to the current locale. For example, a Swedish user will be happier if screenshot dates are "lör 15 nov 2014 22:28:09 CET" than if they're "Sat Nov 15 22:28:09 CET 2014".
  • The locale defaults to C, but Qt and GTK+ set it to whatever LC_ALL/etc says by default, making it easy to forget. Additionally, since the locale defaults to the good one and RetroArch doesn't use GTK+ or Qt, looking for what RetroArch does differently will make you start off in wrong direction.

If we declare that the locale belongs to the frontend:

  • It will be considerably harder to write core code that stringifies a floating point value; I've heard plenty of horror stories about how many dragons are involving in printf("%f"), and floating-point handling in general. We'd have to put a locale-independent sprintf in libretro-sdk (likely copied from FreeBSD), and then somehow force the real one out of the way (preferably with a #define).
  • Since RetroArch uses the good locale, it's easy to accidentally write locale-dependent code that remains hidden for a long time.
  • The frontend may want to use a locale-independent sprintf too. (That can be solved by the above libretro-sdk method.)

A third way would be to create a mutex that must be held in order to do anything locale-dependent, and export it across libretro; the front will also claim it if it wants locale stuff, and if the front supports multiple simultaneous cores, the same mutex will be given to all of them. Claiming this mutex sets locale to C, and releasing it restores it.
The problem with this is that it demands two extra function calls for each printf, and like the other methods, mistakes are hard to find - I think there are locales that don't use 0-9 as their standard numerals, making even printf("%i") dangerous, but I wasn't able to find any examples.

I don't have any strong opinions in either direction; all solutions are messy, and it's hard to find which is the least bad. I'd order them steal printf, mutex, global C, but I'm willing to be convinced of something else.

Does anyone else have any opinions?

@heuripedes
Copy link
Contributor

I think this is a core bug, because it seems like the developer assumed everything would work just like it does on his locale.

The locale defaults to C, but Qt and GTK+ set it to whatever LC_ALL/etc says by default, making it easy to forget. Additionally, since the locale defaults to the good one and RetroArch doesn't use GTK+ or Qt, looking for what RetroArch does differently will make you start off in wrong direction.

In the case of RetroArch, the frontend neither sets nor inherits the locale so it just defaults to C.

I think setting LC_NUMERIC="C" is the better solution, as it preserves the localized messages and doesn't require much logic, but I'm not sure if this is a frontend responsibility.

From printf(3):

   For  some  numeric  conversions  a radix character ("decimal point") or
   thousands' grouping character  is  used.   The  actual  character  used
   depends  on  the  LC_NUMERIC part of the locale.  The POSIX locale uses
   '.' as radix character, and does not have a grouping character.  Thus,

           printf("%'.2f", 1234567.89);

   results in "1234567.89" in the POSIX locale,  in  "1234567,89"  in  the
   nl_NL locale, and in "1.234.567,89" in the da_DK locale.

@heuripedes
Copy link
Contributor

Just gave a second thought about the issue and I think that neither the core nor the frontend should rely on locale settings. Forcing a certain setting all the time when the system expects localized output also seems bad in my view. If either part needs a piece locale-dependent code to behave in a certain way, it should wrap that code within some setlocale() calls so that the rest of the program is unaffected, just like the snippet bellow:

char *previous_value = setlocale(LC_NUMERIC, NULL);
setlocale(LC_NUMERIC, "C");
/* locale dependent code */
setlocale(LC_NUMERIC, previous_value);

@Alcaro
Copy link
Collaborator Author

Alcaro commented Nov 17, 2014

I can swear I mentioned this in the issue (must've gotten lost in a refactoring, I rewrote that post about twenty times), but setlocale is process-global - your suggestion is a race condition if the front (or another core, or another instance of the same core) wants to do locale stuff at the same time on another thread, and I don't want us to add a mandatory race condition to the libretro spec.

It's also an extra piece of code around every printf, and forgetting it will only show up in unusual configurations, so it's fragile and bulky even in single-threaded contexts.

The mutex idea I mentioned solves one of the problems, but it's still a chunk of easy-to-forget code. My favourite is still copying FreeBSD sprintf to libretro-sdk; it's the only one that lets the front format correctly without demanding two extra function calls around every single printf. It doesn't require extending libretro, either, so all frontends will work as soon as we update the cores.

Here's a list of which functions need to be handled if we want to disable locales for the cores. The lack of strtol suggests that the digits are guaranteed to be 0-9A-F, meaning sprintf("%diuoxX") is safe and only %fFeEgG (and maybe %p, but that one doesn't belong outside debug output anyways) are dangerous - unfortunately, there is a %g here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants