Regression with non-RetroArch frontends

[movahal]

Ever since commit dcb8f97359de3db060100d5114a651143ba9f6c8, frontends that do not implement the newer core options interface are unable to use this core properly, regardless of the content that is loaded. What you get is:

... (many duplicated messages removed)
got env 55 (RETRO_ENVIRONMENT_SET_CORE_OPTIONS_DISPLAY)
got env 55 (RETRO_ENVIRONMENT_SET_CORE_OPTIONS_DISPLAY)
got env 55 (RETRO_ENVIRONMENT_SET_CORE_OPTIONS_DISPLAY)
got env 15 (RETRO_ENVIRONMENT_GET_VARIABLE) with key "reicast_show_vmu_screen_settings"
[error] 00:00:815 libretro/common.cpp:376 E[COMMON]: SIGSEGV @ 7fef7f09807d ... 0x1 -> was not in vram (dyna code 0)
[error] Fatal error : segfault
 in signal_handler -> core/libretro/common.cpp : 383 

[error] 00:00:815 libretro/libretro.cpp:3271 E[COMMON]: DEBUGBREAK!
[error] 00:00:815 libretro/common.cpp:376 E[COMMON]: SIGSEGV @ 7fef7f0a16b8 ... 0x7fef7f0a16b8 -> was not in vram (dyna code 0)
[error] Fatal error : segfault
 in signal_handler -> core/libretro/common.cpp : 383 

[error] 00:00:815 libretro/libretro.cpp:3271 E[COMMON]: DEBUGBREAK!
Illegal instruction

Other cores (GL or not) seem to work fine. For compatibility reasons I think this should not happen.

Environment: Arch Linux x64 gcc 10.2.0

[inactive123]

OK, I will bring this to the attention of @jdgleaver.

[jdgleaver]

This is neither a bug in the core nor the libretro API. This is a bug in the frontend that you are using.

When a core uses the RETRO_ENVIRONMENT_GET_VARIABLE environment callback, it should check whether the variable referenced by the specified key exists. If it does not, it should set the returned variable data to NULL.

The frontend you are using is not doing this. It is trying to access directly a variable that does not exist, and is therefore operating on an invalid memory address, leading to a segfault. You need to raise this issue with the frontend maintainer.

[movahal]

Where is the data variable? Are you referring to value in retro_variable?

libretro.h also says 'key' should be set to a key which has already been set by SET_VARIABLES, but in the case of reicast_show_vmu_screen_settings and many other variables, that has not been done by the core.

[jdgleaver]

The data variable is defined in this struct:

struct retro_variable
{
   /* Variable to query in RETRO_ENVIRONMENT_GET_VARIABLE.
    * If NULL, obtains the complete environment string if more
    * complex parsing is necessary.
    * The environment string is formatted as key-value pairs
    * delimited by semicolons as so:
    * "key1=value1;key2=value2;..."
    */
   const char *key;

   /* Value to be obtained. If key does not exist, it is set to NULL. */
   const char *value;
};

Note the comment: /* Value to be obtained. If key does not exist, it is set to NULL. */

Thus if a RETRO_ENVIRONMENT_GET_VARIABLE call is made for a key that does not exist, the frontend should set value to NULL. What it shouldn't do is try to access an invalid memory location and cause a segfault...

[movahal]

Ok so value is indeed the "data" variable you're referring to then. I tried running under valgrind and AddressSanitizer and that did not pick up any invalid memory access, but the issue seems to have went away on its own somehow. I went ahead and made the change you mentioned and it is still working, so for now I will assume that it is fixed. But is there not still an issue of flycast not giving out all of its variable names when SET_VARIABLES is called?

[jdgleaver]

But is there not still an issue of flycast not giving out all of its variable names when SET_VARIABLES is called?

No, it's not an issue. It's by design. Those omitted variables only have meaning on frontends that support core options v1 (or higher). If they were included on other frontends, then the core options menu would have a number of non-functional entries that would only cause clutter and confusion for the end user.