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

Config file are locale dependent. #15756

Open
patlefort opened this issue Oct 1, 2023 · 26 comments
Open

Config file are locale dependent. #15756

patlefort opened this issue Oct 1, 2023 · 26 comments

Comments

@patlefort
Copy link

Description

Parsing and formatting functions used by retroarch are affected by the locale of the user. Floating points for example could be formatted with a ',' instead of a '.'. It might be a separate bug but retroarch is unable to parse floating points numbers with a ',' and they lose their decimal digits. Also, it makes retroarch config non portable.

Using setlocale() with C should fix it before parsing or formatting a file.

Expected behavior

All formatting and parsing of config files should be done with the 'C' locale.

Actual behavior

The locale of the user is used.

Steps to reproduce the bug

Try to launch retroarch with LC_NUMERIC=fr_CA.UTF-8 for example.

Version/Commit

Environment information

  • OS: Arch Linux
  • Compiler: GCC
@cyrilpicard
Copy link

I have a bug similar to #15200 when I connect a wireless Xbox 360 controller because of this config file locale issue.

In my language, the decimal separator is ",". If I start retroarch with Xbox 360 controller connected and retroarch.cfg containing :
input_axis_threshold = "0,500000"
The controller's dpad and keyboard arrows are unresponsive.

Manually editing retroarch.conf to replace "," with "." in the above variable fixes the issue, until retroarch writes back ",".

@patlefort
Copy link
Author

Currently I work around it by launching retroarch with LC_NUMERIC=C. I don't know where the locale is being changed, I don't find a use of setlocale in the code for LC_NUMERIC to anything but "C". It happens before I load any cores so it couldn't be a core unless somehow retroarch still need to load all of them at start.

@zoltanvb
Copy link
Contributor

zoltanvb commented Oct 8, 2023

I took a look as well and could not find an obvious place where it would go wrong.
Can you please send the output of retroarch --features so that it could be narrowed down?

@patlefort
Copy link
Author

Features:
  LibretroDB      - LibretroDB support: yes
  Command         - Command interface support: yes
  Network Command - Network Command interface support: yes
  SDL             - SDL input/audio/video drivers: no
  SDL2            - SDL2 input/audio/video drivers: yes
  X11             - X11 input/video drivers: yes
  UDEV            - UDEV/EVDEV input driver: yes
  Wayland         - Wayland input/video drivers: yes
  Threads         - Threading support: yes
  Vulkan          - Video driver: yes
  Metal           - Video driver: no
  OpenGL          - Video driver: yes
  OpenGLES        - Video driver: no
  XVideo          - Video driver: no
  EGL             - Video context driver: yes
  KMS             - Video context driver: yes
  OpenVG          - Video context driver: no
  CoreAudio       - Audio driver: no
  CoreAudioV3     - Audio driver: no
  ALSA            - Audio driver: yes
  OSS             - Audio driver: no
  Jack            - Audio driver: no
  RSound          - Audio driver: no
  RoarAudio       - Audio driver: no
  PulseAudio      - Audio driver: yes
  DirectSound     - Audio driver: no
  WASAPI          - Audio driver: no
  XAudio2         - Audio driver: no
  OpenAL          - Audio driver: no
  OpenSL          - Audio driver: no
  7zip            - 7zip extraction support: yes
  zlib            - zip extraction support: yes
  External        - External filter and plugin support: yes
  Cg              - Fragment/vertex shader driver: yes
  GLSL            - Fragment/vertex shader driver: yes
  HLSL            - Fragment/vertex shader driver: no
  SDL_image       - SDL_image image loading: no
  rpng            - PNG image loading/encoding: yes
  rjpeg           - JPEG image loading: yes
  Dynamic         - Dynamic run-time loading of libretro library: yes
  FFmpeg          - On-the-fly recording of gameplay with libavcodec: yes
  FreeType        - TTF font rendering driver: yes
  CoreText        - TTF font rendering driver: no
  Netplay         - Peer-to-peer netplay: yes
  Libusb          - Libusb support: no
  Cocoa           - Cocoa UI companion support (for OSX and/or iOS): no
  Qt              - Qt UI companion support: yes
  Video4Linux2    - Camera driver: yes

@nfp0
Copy link
Contributor

nfp0 commented Oct 8, 2023

Looks like I've opened a duplicate here: #15775

I'm also having this issue. The strange thing is that it started happening without me changing RetroArch versions.
It started happening after a Manjaro system update.

Can you please send the output of retroarch --features so that it could be narrowed down?

@zoltanvb Here is mine:

Features:
  LibretroDB      - LibretroDB support: yes
  Command         - Command interface support: yes
  Network Command - Network Command interface support: yes
  SDL             - SDL input/audio/video drivers: no
  SDL2            - SDL2 input/audio/video drivers: yes
  X11             - X11 input/video drivers: yes
  UDEV            - UDEV/EVDEV input driver: yes
  Wayland         - Wayland input/video drivers: yes
  Threads         - Threading support: yes
  Vulkan          - Video driver: yes
  Metal           - Video driver: no
  OpenGL          - Video driver: yes
  OpenGLES        - Video driver: no
  XVideo          - Video driver: yes
  EGL             - Video context driver: yes
  KMS             - Video context driver: yes
  OpenVG          - Video context driver: no
  CoreAudio       - Audio driver: no
  CoreAudioV3     - Audio driver: no
  ALSA            - Audio driver: yes
  OSS             - Audio driver: no
  Jack            - Audio driver: no
  RSound          - Audio driver: no
  RoarAudio       - Audio driver: no
  PulseAudio      - Audio driver: yes
  DirectSound     - Audio driver: no
  WASAPI          - Audio driver: no
  XAudio2         - Audio driver: no
  OpenAL          - Audio driver: no
  OpenSL          - Audio driver: no
  7zip            - 7zip extraction support: yes
  zlib            - zip extraction support: yes
  External        - External filter and plugin support: yes
  Cg              - Fragment/vertex shader driver: no
  GLSL            - Fragment/vertex shader driver: yes
  HLSL            - Fragment/vertex shader driver: no
  SDL_image       - SDL_image image loading: no
  rpng            - PNG image loading/encoding: yes
  rjpeg           - JPEG image loading: yes
  Dynamic         - Dynamic run-time loading of libretro library: yes
  FFmpeg          - On-the-fly recording of gameplay with libavcodec: yes
  FreeType        - TTF font rendering driver: yes
  CoreText        - TTF font rendering driver: no
  Netplay         - Peer-to-peer netplay: yes
  Libusb          - Libusb support: no
  Cocoa           - Cocoa UI companion support (for OSX and/or iOS): no
  Qt              - Qt UI companion support: yes
  Video4Linux2    - Camera driver: yes

@patlefort As you're on Arch and I'm on Manjaro, could this be specific to Arch-based distros?
Did this start happening for you after updating RetroArch, or after updating some other packages?
If this was a recent thing, can you perhaps try and revert some of your most recent package updates to try and pinpoint which one had a change in behavior?

@nfp0
Copy link
Contributor

nfp0 commented Oct 8, 2023

Another strange thing that I think started happening at the same time of this issue, is the fact that now I can't start RetroArch through the VSCode built-in terminal window, while it worked perfectly before.

Now I get this error:

[ERROR] [Wayland]: libdecor Caught error (0): GTK cannot connect to Wayland compositor
Segmentation fault (core dumped)

RetroArch launches fine if I run it through the KDE Konsole terminal.

Please note that VSCode runs under XWayland and not Wayland.
Could this mean that there was a change in how environment variables get exposed to RetroArch? Maybe RetroArch was not being able to read LC_NUMERIC before so it was defaulting to C.

I find it very strange that both these issues showed up at the same time.

EDIT:
Please disregard.
This seems to be a separate VSCode/Electron issue where it's propagating GDK_BACKEND=x11 to subprocesses.
Might have just been a coincidence.

@zoltanvb
Copy link
Contributor

zoltanvb commented Oct 8, 2023

I checked the code again, and did not really find why it would set the locale to C, though it certainly behaves (used to behave) like that.
There are 2 explicit calls, one in Qt UI initialization, but it does not get called unless one invokes the Qt UI with F5 key, the other in SwitchRes, but it is also not touched in my setup (which does not utilize CRTSwitchRes).

Theoretically, the fix is easy, but it would be good to understand what happened. My working assumption is that in one of the included dynamic libraries, there was a setlocale(), which is now removed, or the other way around.

The feature list is largely similar to mine, and it uses following dynamic libraries (you can check your binary with readelf -d retroarch | grep Shared):

Shared library: [librt.so.1]
Shared library: [libQt5Core.so.5]
Shared library: [libQt5Gui.so.5]
Shared library: [libQt5Widgets.so.5]
Shared library: [libQt5Network.so.5]
Shared library: [libasound.so.2]
Shared library: [libjack.so.0]
Shared library: [libpulse.so.0]
Shared library: [libfreetype.so.6]
Shared library: [libfontconfig.so.1]
Shared library: [libpthread.so.0]
Shared library: [libwayland-egl.so.1]
Shared library: [libwayland-client.so.0]
Shared library: [libwayland-cursor.so.0]
Shared library: [libX11.so.6]
Shared library: [libXxf86vm.so.1]
Shared library: [libXinerama.so.1]
Shared library: [libXrandr.so.2]
Shared library: [libX11-xcb.so.1]
Shared library: [libxkbcommon.so.0]
Shared library: [libudev.so.1]
Shared library: [libgbm.so.1]
Shared library: [libdrm.so.2]
Shared library: [libGL.so.1]
Shared library: [libEGL.so.1]
Shared library: [libSDL2-2.0.so.0]
Shared library: [libv4l2.so.0]
Shared library: [libstdc++.so.6]
Shared library: [libdl.so.2]
Shared library: [libm.so.6]
Shared library: [libmvec.so.1]
Shared library: [libgcc_s.so.1]
Shared library: [libc.so.6]

Can you take a look at the package update history, if any of these appear recently, around the time when the problem started? I do not use Arch/Manjaro myself, but the log may be in /var/log/pacman.log .

@nfp0
Copy link
Contributor

nfp0 commented Oct 8, 2023

@zoltanvb I checked the pacman log and, from your list above (and mine), the following libraries were updated on my system when the problem started:

  • qt5-base
  • qt5-imageformats
  • qt5-location
  • qt5-wayland
  • glibc
  • lib32-glibc

At first I thought qt5-location could be related, but it seems it's a library for mapping stuff such as Open Street Maps, and not system location.
Could it be qt5-base?

@zoltanvb
Copy link
Contributor

zoltanvb commented Oct 8, 2023

Thanks for the check! glibc seems to have a few commits related to some problem with "C" locale at end of September, but I guess those are more like the fix, rather than the cause:
https://gitlab.archlinux.org/archlinux/packaging/packages/glibc/-/commits/main
Still reading through it, but there may be a chance that 2.38-6 version will fix this (if this is the problem).

@zoltanvb
Copy link
Contributor

zoltanvb commented Oct 8, 2023

Note to myself:
https://www.gnu.org/software/libc/manual/html_node/Setting-the-Locale.html
ISO C says that all programs start by default in the standard ‘C’ locale.

So, that should work without any setting. Adding an explicit setlocale() at startup may help prevent such errors.

@patlefort
Copy link
Author

The behavior observed will happen if some library somewhere call setlocale(LC_..., ""), an empty locale will set the current locale to the locale of the user for the whole process. As @zoltanvb say, by the default programs start with the 'C' locale. Since it can occur in any libraries unpredictably, all parsing functions that should use a 'C' locale should either set the locale beforehand or use other locale independent functions.

@nfp0
Copy link
Contributor

nfp0 commented Oct 8, 2023

Found the culprit!

It's the libdecor library. It was recently updated to version 0.2.0, which exhibits the problem on RetroArch. Downgrading it to 0.1.1 fixes the problem!

This also fixes the error I was getting on trying to launch RetroArch from the VSCode terminal window, as I described on this post above: #15756 (comment)
It was not a coincidence after all...

So, is this a RetroArch, or a libdecor problem?

@nfp0
Copy link
Contributor

nfp0 commented Oct 8, 2023

Apparently this issue had already been opened on libdecor's side, but it also came from someone using RetroArch.
https://gitlab.freedesktop.org/libdecor/libdecor/-/issues/58

@nfp0
Copy link
Contributor

nfp0 commented Oct 10, 2023

This issue has now been fixed on libdecor with this Merge Request:
https://gitlab.freedesktop.org/libdecor/libdecor/-/merge_requests/132

If no one else has any issues with it, I think we can close this.

@patlefort
Copy link
Author

I still think it's not a good idea to not set the locale to 'C' in retroarch before parsing or formatting. This bug could happen again at any time and will seriously break a user's config.

@zoltanvb
Copy link
Contributor

Would it be possible to build the change (you can pull from my fork: https://github.com/zoltanvb/RetroArch/tree/locale_problem ) and see if it solves the problem when the faulty libdecor version is present? It is a bit hard to decode exactly when the change occurs, and I would prefer submitting an improvement that protects against this particular issue at least.

@patlefort
Copy link
Author

@zoltanvb I tested it and it sadly doesn't resolve the issue. I understand it's just a quick fix but I must stress that the only way to fully guard against this issue is to set the locale to 'C' right before parsing or formatting or using locale independent functions.

@zoltanvb
Copy link
Contributor

Thank you for the check. I will look for a better place, hopefully still avoiding having to call setlocale() on each float read/write to config file.

@nfp0
Copy link
Contributor

nfp0 commented Oct 10, 2023

As @patlefort said, shouldn't we use locale independent functions instead, in this case?

@zoltanvb
Copy link
Contributor

Considering all platforms RA is built for: there is no easy way that I see for that. Codebase should compile for C89.
https://github.com/nothings/stb/blob/master/stb_sprintf.h may provide output, since there are already a few baked-in stb_ dependencies, but reading uses strtod() which is not provided by this library. Maybe it could be an addition to libretro-common like the path manipulation functions.

In the meantime, I pushed small changes for the branch above, in another attempt to have a temporary solution.

@zoltanvb
Copy link
Contributor

Would it be possible to check this last change? I cannot easily reproduce the environment, and since there is no new libdecor version released yet, this bug may bite more users still.

@nfp0
Copy link
Contributor

nfp0 commented Oct 15, 2023

@zoltanvb Sure, I can give it a try later today with the unfixed version of libdecor.
You're talking about testing this branch, correct? #15782

@zoltanvb
Copy link
Contributor

Yes, that branch. Thank you!

@nfp0
Copy link
Contributor

nfp0 commented Oct 15, 2023

@zoltanvb That fixes it when using the broken libdecor version, yes. :)

@nfp0
Copy link
Contributor

nfp0 commented Dec 3, 2023

This seems to have been fixed on both RetroArch and libdecor's side here and here so maybe this can be closed?

@zoltanvb
Copy link
Contributor

zoltanvb commented Dec 3, 2023

Just for reference, the combination of libdecor 0.2.0 (in Wayland environments) and RetroArch 1.16 (or earlier) exhibits the fault, libdecor 0.2.1 contains the fix, RetroArch nightly builds after 2023-10-30 + next stable release (whenever that may be) contain a workaround.

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

4 participants