[libretro] retroarch crashes when ppsspp_libretro.so is compiled with clang

[amverweij]

Game or games this happens in

None

What area of the game / PPSSPP

Building ppsspp_libretro.so on Linux using clang. The issue can be reproduced on a x86_64 laptop under X11 and on a RPi4 using KMS/DRM.

After setting the compiler to clang, to build I use:
CMAKE_ARGS="-DUSING_GLES2=OFF -DUSE_FFMPEG=ON -DUSE_SYSTEM_FFMPEG=ON -DUSE_SYSTEM_LIBZIP=ON -DVULKAN=ON -DUSE_DISCORD=OFF -DUSE_MINIUPNPC=OFF -DUSING_X11_VULKAN=ON" ./b.sh
(This is in case of X11).

Retroarch is configured to use the Vulkan driver.

The crash is reproduced by starting a game using ppsspp_libretro (either interactively or from the commandline). I use
retroarch -L /usr/lib64/libretro/ppsspp_libretro.so Soulcalibur\ -\ Broken\ Destiny\ \(USA\)\ \(En\,Ja\,Fr\,De\,Es\,It\,Ru\).iso

Here is a gdb session illustrating the crash:
`verweij@freddie ~ $ gdb --args /usr/bin/retroarch -L /usr/lib64/libretro/ppsspp_libretro.so Soulcalibur\ -\ Broken\ Destiny\ (USA)\ (En,Ja,Fr,De,Es,It,Ru).iso
GNU gdb (Gentoo 10.2 vanilla) 10.2
Copyright (C) 2021 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later http://gnu.org/licenses/gpl.html
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
Type "show copying" and "show warranty" for details.
This GDB was configured as "x86_64-pc-linux-gnu".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
https://bugs.gentoo.org/.
Find the GDB manual and other documentation resources online at:
http://www.gnu.org/software/gdb/documentation/.

For help, type "help".
Type "apropos word" to search for commands related to "word"...
Reading symbols from /usr/bin/retroarch...
Reading symbols from /usr/lib/debug//usr/bin/retroarch.debug...
(gdb) run
Starting program: /usr/bin/retroarch -L /usr/lib64/libretro/ppsspp_libretro.so Soulcalibur\ -\ Broken\ Destiny\ (USA)\ (En,Ja,Fr,De,Es,It,Ru).iso
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".
[New Thread 0x7ffff2efe640 (LWP 138406)]
OpenCFile(): PathType not yet supported
01:10:100 Thread/ThreadManager.cpp:109 I[SYSTEM]: ThreadManager::Init(compute threads: 4, all: 8)
[New Thread 0x7fffed766640 (LWP 138407)]
[New Thread 0x7fffecf65640 (LWP 138408)]
[New Thread 0x7fffec764640 (LWP 138409)]
[New Thread 0x7fffebf63640 (LWP 138410)]
[New Thread 0x7fffeb762640 (LWP 138411)]
[New Thread 0x7fffeaf61640 (LWP 138412)]
[New Thread 0x7fffea760640 (LWP 138413)]
[New Thread 0x7fffe9f5f640 (LWP 138414)]
[New Thread 0x7fffe855d640 (LWP 138415)]
[New Thread 0x7fffe7d5c640 (LWP 138416)]

Thread 1 "retroarch" received signal SIGSEGV, Segmentation fault.
0x00007ffff26ee290 in vkEnumerateInstanceExtensionProperties () from /usr/lib64/libretro/ppsspp_libretro.so
(gdb) bt
#0 0x00007ffff26ee290 in vkEnumerateInstanceExtensionProperties () at /usr/lib64/libretro/ppsspp_libretro.so
#1 0x00005555558a7c14 in vulkan_find_instance_extensions (num_exts=2, exts=0x7fffffffc0d0) at gfx/common/vulkan_common.c:1441
#2 vulkan_context_init (vk=vk@entry=0x5555566c0e18, type=type@entry=VULKAN_WSI_XCB) at gfx/common/vulkan_common.c:1947
#3 0x00005555558871ae in gfx_ctx_x_vk_init (data=) at gfx/drivers_context/x_vk_ctx.c:222
#4 0x000055555561d16b in video_context_driver_init
(settings=settings@entry=0x7ffff2eff010, data=data@entry=0x5555566bee90, ctx=0x555555feb040 <gfx_ctx_vk_x>, api=api@entry=GFX_CTX_VULKAN_API, major=major@entry=1, minor=minor@entry=0, hw_render_ctx=false, ctx_data=0x7fffffffc320, ident=0x7ffff2f00de9 "", p_rarch=0x555556041ee0 <rarch_st>)
at retroarch.c:32713
#5 0x000055555562e7ee in vk_context_driver_init_first
(p_rarch=0x555556041ee0 <rarch_st>, ctx_data=0x7fffffffc320, hw_render_ctx=false, minor=0, major=1, api=GFX_CTX_VULKAN_API, ident=0x7ffff2f00de9 "", data=0x5555566bee90, settings=0x7ffff2eff010) at retroarch.c:32764
#6 video_context_driver_init_first
(data=data@entry=0x5555566bee90, ident=0x7ffff2f00de9 "", api=api@entry=GFX_CTX_VULKAN_API, major=major@entry=1, minor=minor@entry=0, hw_render_ctx=hw_render_ctx@entry=false, ctx_data=0x7fffffffc320) at retroarch.c:32863
#7 0x00005555558a139e in vulkan_get_context (vk=0x5555566bee90) at gfx/drivers/vulkan.c:76
#8 vulkan_init (video=0x7fffffffc460, input=0x55555605ffd8 <rarch_st+123128>, input_data=0x55555605ffe0 <rarch_st+123136>) at gfx/drivers/vulkan.c:1192
#9 0x000055555564f024 in video_driver_init_internal
(p_rarch=0x555556041ee0 <rarch_st>, verbosity_enabled=false, video_is_threaded=, settings=0x7ffff2eff010) at retroarch.c:30866
#10 drivers_init
(settings=settings@entry=0x7ffff2eff010, flags=flags@entry=2047, verbosity_enabled=verbosity_enabled@entry=false, p_rarch=0x555556041ee0 <rarch_st>)
at retroarch.c:33615
#11 0x0000555555655508 in retroarch_main_init (argc=4, argv=argv@entry=0x7fffffffda08) at retroarch.c:36085
#12 0x00005555556666a7 in content_load (info=info@entry=0x7fffffffd8b0, p_content=p_content@entry=0x555556062538 <rarch_st+132696>)
at tasks/task_content.c:1475
#13 0x00005555556672ed in task_load_content_internal
(content_info=content_info@entry=0x7fffffffd8b0, loading_from_cli=loading_from_cli@entry=true, loading_from_companion_ui=loading_from_companion_ui@entry=false, loading_from_menu=true) at tasks/task_content.c:2342
#14 0x00005555556676af in task_push_load_content_from_cli
(core_path=core_path@entry=0x0, fullpath=fullpath@entry=0x0, content_info=content_info@entry=0x7fffffffd8b0, type=type@entry=CORE_TYPE_PLAIN, cb=cb@entry=0x0, user_data=user_data@entry=0x0) at tasks/task_content.c:2436
#15 0x000055555564d5fa in rarch_main (argc=4, argv=0x7fffffffda08, data=0x0) at retroarch.c:15677
#16 0x00007ffff5cee7fd in __libc_start_main () at /lib64/libc.so.6
#17 0x0000555555616a7a in _start ()
`

The problem seems to be that retroarch is trying to use vkEnumerateInstanceExtensionProperties, which is declared extern in Common/GPU/Vulkan/VulkanLoader.h, and at this stage still nullptr.

No clue why this isn't a problem when using gcc.

What should happen

It shouldn't crash.

Logs

No response

Platform

libretro / Retroarch

Mobile phone model or graphics card

V3D (on the RPi4) and something intel on my laptop

PPSSPP version affected

git master 3c62fbc

Last working version

No response

Graphics backend (3D API)

Vulkan

Checklist

• Test in the latest git build in case it's already fixed.
• Search for other reports of the same issue.
• Try resetting settings or older versions and include if the issue is related.
• Try without any cheats and without loading any save states.
• Include logs or screenshots of issue.

[amverweij]

After prefixing the Vulkan externals with ppsspp_ the core works quite alright again. In case you're interested, here is a patch: https://github.com/amverweij/ppsspp/commit/12155d07431104e42bf5cb55b34c41845643be4d.patch

Using clang seems to be worth it: Soulcalibur runs at 4x PSP resolution without audio stutter on the RPi4. With gcc, I only get 2x.

[unknownbrackets]

Prefixing every function call to Vulkan with ppsspp_ just because of some issue in libretro seems awful. I really hope we don't go that route just to support the fraction of users using retroarch.

-[Unknown]

[amverweij]

With libretro_ppsspp.so that includes the above patch, we can see what should happen by setting a breakpoint at gfx/common/vulkan_common.c:1441 and single-stepping:

`

Thread 1 "retroarch" hit Breakpoint 1, vulkan_find_instance_extensions (num_exts=2, exts=0x7fffffffc0d0) at gfx/common/vulkan_common.c:1441
1441 if (vkEnumerateInstanceExtensionProperties(NULL, &property_count, NULL) != VK_SUCCESS)
(gdb) step
[Thread 0x7fffe855d640 (LWP 169602) exited]
0x00007fffeddce044 in vkEnumerateInstanceExtensionProperties (pLayerName=0x0, pPropertyCount=0x7fffffffc05c, pProperties=0x0)
at /usr/src/debug/media-libs/vulkan-loader-1.2.170/Vulkan-Loader-1.2.170/loader/trampoline.c:100
100 VkExtensionProperties *pProperties) {
(gdb) bt
#0 0x00007fffeddce044 in vkEnumerateInstanceExtensionProperties (pLayerName=0x0, pPropertyCount=0x7fffffffc05c, pProperties=0x0)
at /usr/src/debug/media-libs/vulkan-loader-1.2.170/Vulkan-Loader-1.2.170/loader/trampoline.c:100
#1 0x00005555558a7c14 in vulkan_find_instance_extensions (num_exts=2, exts=0x7fffffffc0d0) at gfx/common/vulkan_common.c:1441
#2 vulkan_context_init (vk=vk@entry=0x5555566c0dd8, type=type@entry=VULKAN_WSI_XCB) at gfx/common/vulkan_common.c:1947
#3 0x00005555558871ae in gfx_ctx_x_vk_init (data=) at gfx/drivers_context/x_vk_ctx.c:222
#4 0x000055555561d16b in video_context_driver_init
(settings=settings@entry=0x7ffff2eff010, data=data@entry=0x5555566bee50, ctx=0x555555feb040 <gfx_ctx_vk_x>, api=api@entry=GFX_CTX_VULKAN_API, major=major@entry=1, minor=minor@entry=0, hw_render_ctx=false, ctx_data=0x7fffffffc320, ident=0x7ffff2f00de9 "", p_rarch=0x555556041ee0 <rarch_st>)
at retroarch.c:32713
#5 0x000055555562e7ee in vk_context_driver_init_first
(p_rarch=0x555556041ee0 <rarch_st>, ctx_data=0x7fffffffc320, hw_render_ctx=false, minor=0, major=1, api=GFX_CTX_VULKAN_API, ident=0x7ffff2f00de9 "", data=0x5555566bee50, settings=0x7ffff2eff010) at retroarch.c:32764
#6 video_context_driver_init_first
(data=data@entry=0x5555566bee50, ident=0x7ffff2f00de9 "", api=api@entry=GFX_CTX_VULKAN_API, major=major@entry=1, minor=minor@entry=0, hw_render_ctx=hw_render_ctx@entry=false, ctx_data=0x7fffffffc320) at retroarch.c:32863
#7 0x00005555558a139e in vulkan_get_context (vk=0x5555566bee50) at gfx/drivers/vulkan.c:76
#8 vulkan_init (video=0x7fffffffc460, input=0x55555605ffd8 <rarch_st+123128>, input_data=0x55555605ffe0 <rarch_st+123136>) at gfx/drivers/vulkan.c:1192
#9 0x000055555564f024 in video_driver_init_internal
(p_rarch=0x555556041ee0 <rarch_st>, verbosity_enabled=false, video_is_threaded=, settings=0x7ffff2eff010) at retroarch.c:30866
#10 drivers_init
(settings=settings@entry=0x7ffff2eff010, flags=flags@entry=2047, verbosity_enabled=verbosity_enabled@entry=false, p_rarch=0x555556041ee0 <rarch_st>)
at retroarch.c:33615
#11 0x0000555555655508 in retroarch_main_init (argc=4, argv=argv@entry=0x7fffffffda08) at retroarch.c:36085
#12 0x00005555556666a7 in content_load (info=info@entry=0x7fffffffd8b0, p_content=p_content@entry=0x555556062538 <rarch_st+132696>)
at tasks/task_content.c:1475
#13 0x00005555556672ed in task_load_content_internal
(content_info=content_info@entry=0x7fffffffd8b0, loading_from_cli=loading_from_cli@entry=true, loading_from_companion_ui=loading_from_companion_ui@entry=false, loading_from_menu=true) at tasks/task_content.c:2342
#14 0x00005555556676af in task_push_load_content_from_cli
(core_path=core_path@entry=0x0, fullpath=fullpath@entry=0x0, content_info=content_info@entry=0x7fffffffd8b0, type=type@entry=CORE_TYPE_PLAIN, cb=cb@entry=0x0, user_data=user_data@entry=0x0) at tasks/task_content.c:2436
#15 0x000055555564d5fa in rarch_main (argc=4, argv=0x7fffffffda08, data=0x0) at retroarch.c:15677
#16 0x00007ffff5cee7fd in __libc_start_main () at /lib64/libc.so.6
#17 0x0000555555616a7a in _start ()

`

We now go into the vulkan-loader shared library where we stepped into libretro_ppsspp.so (and crashed) before.

When compiled with clang, libretro_ppsspp.so the externals of the vulkan-loader shared library. The crash is because after loading libretro_ppsspp.so, retroarch is trying to call the vulkan-loader shared library, but it ends up in libretro_ppsspp.so.

[amverweij]

@unknownbrackets : I understand your point of view, I guess there will be better solutions as well.

It is not my intention to fix the problem. My intention is to make you aware that libretro_ppsspp.so is hijacking the public API of the vulkan loader.

[amverweij]

With all due respect, the fact remains that you should either implement the vulkan loader itself or not export its public API. Not declaring these symbols external is not an option, since they are used across different cpp files. So renaming (prefixing) might be your only option. This makes one wonder how other projects solve this issue.

From reading the code, Flycast seems to prefix using "vulkan_symbol_wrapper_". However they do so only in their vulkan_symbol_wrapper.h header, together with some clever C pre-processor macros that take care of the rest if their code base (see https://github.com/libretro/flycast/blob/master/core/libretro-common/include/vulkan/vulkan_symbol_wrapper.h).

A similar strategy is used by RetroArch (https://github.com/libretro/RetroArch/blob/master/libretro-common/include/vulkan/vulkan_symbol_wrapper.h).

Actually I tried this as well for PPSSPP, but got stuck on the WARP macros in libretro/libretro_vulkan.cpp. (I lack understanding of this code and couldn't get it to work.) When successful this would clearly reduce the footprint.

Just my two cents...

[hrydgard]

It's true that ideally one should not export symbols with the same name as the vulkan symbols. On Windows though, symbols are private in a different way so it doesn't matter.

Intuitively I would have thought that our vk func pointers would get mangled enough by the C++ symbol mangler to not clash with the c names, or is something preventing that? Could we maybe make use of that since we have no C code that calls into Vulkan, only C++?

Anyway, I am open to any solution that does not require us to use different vk function names in the main ppsspp code, and has minimal impact on it.

[amverweij]

I had another go at the Flycast approach. The result can be found here: amverweij@9dfa120. The only files that needed changing were VulkanLoader.h and libretro/libretro_vulkan.cpp.

@hrydgard : could you please have a look at the diff and tell me if such a solution would be acceptable for you?

This was tested using PPSSPPSDL and ppsspp_libretro.so on x86_64 (X11), and with ppsspp_libretro.so on arm64 (KMS/DRM).

[hrydgard]

Since the impact is limited to those two files, I'm leaning towards acceptable.

However I'd still like to understand why the C++ name mangling isn't enough to solve the automatically. Is it because the typedefs (like typedef void (VKAPI_PTR *PFN_vkCmdTraceRaysKHR)(...);) are inside extern "C", and that infects the variables you declare with the typedef? I guess in that case ideally we'd want identical typedefs but without extern "C" so we can declare our variables without them colliding... but that's more of a mess than what you're doing, so maybe not practical...

[amverweij]

Extern "C" is definitely intended to make the code compatible with C compilers.

[amverweij]

I guess type correctness requires it as well. After all, the functions exported by the Vulkan loader will be assigned to these function pointers.

[amverweij]

Actually there could be a cleaner solution using C++ namespaces. Prefixing is basically a poor man's solution to not having namespaces.

[hrydgard]

See if you can do something with that, otherwise I guess we'll just have to go with your Flycast-style solution.

[amverweij]

Here you can find a solution using a C++ namespace: amverweij@cc00a41 .

Coding wise this is a lot cleaner than the C preprocessor macro approach: the casual reader of the source code actually sees the same thing as what the C++ compiler compiles.

It does mean that when one wants to use one of the function pointers, the compiler must know you intend to use the one from the PPSSPP namespace. This can be accomplished by prefixing by PPSSPP:: or by putting using namespace PPSSPP; at the top of the file. I opted for the second approach in the existing code, given that this minimizes the number of lines affected.

The most changed files are VulkanLoader.h and VulkanLoader.cpp. Most of these changes are only for indentation.

[hrydgard]

We generally don't indent namespaces in particular (there's a setting for that in VS' auto indenter), so that would cut down the diff a lot.

That one is not too bad actually, I think I slightly prefer it to the preprocessor one. Maybe rename the namespace to PPSSPP_VK or something. What do you think @unknownbrackets ?

[unknownbrackets]

I think using a namespace seems fine, agree it shouldn't just be PPSSPP. We could probably even put using namespace PPSSPP_VK; inside VulkanLoader.h and not change any other files.

-[Unknown]

[amverweij]

@unknownbrackets , are you aware that putting a using directive in a header file is considered a bad coding practice? See https://docs.microsoft.com/en-us/cpp/cpp/namespaces-cpp?view=msvc-160#using_directives, or https://www.google.com/search?client=firefox-b-e&q=why+not+put+using+namespace+in+header.

I would feel bad about introducing that into your code base in case you wouldn't be aware of the associated issues.

[amverweij]

I created a pull request from the namespace PPSSPP_VK solution.

Please do add further code review remarks for any issue that is still remaining.

(And most of all, thank you for making PPSSPP, and also supporting the libretro core. I've been having lots of fun using it!)

[unknownbrackets]

@unknownbrackets , are you aware that putting a using directive in a header file is considered a bad coding practice? See https://docs.microsoft.com/en-us/cpp/cpp/namespaces-cpp?view=msvc-160#using_directives, or https://www.google.com/search?client=firefox-b-e&q=why+not+put+using+namespace+in+header.

I would feel bad about introducing that into your code base in case you wouldn't be aware of the associated issues.

Using Assembly code is also considered a bad practice, but we do it where it makes sense. Using Direct3D 9 is a bad practice, and we still support that. Bad practices are guidelines.

Obviously, it "defeats the purpose" of a namespace to just using it in the header. But we're practically using it as a hack here: we actually want these to just be accessible without special effort, and we're working around the fact that libretro is accidentally importing our symbols. There's no semantic benefit to adding the using in individual places, and the Vulkan functions are already prefixed with "vk" which is, also, a "bad practice" for C++. So there's no actual downside to the using in the header.

-[Unknown]