Wii: broken partial SDL2 usage

ds-sloth commented 1 month ago

Bug Report

Thanks for maintaining this project!

devkitPro's SDL2 port has recently become much more complete, which is a very good thing, but it has resulted in certain unintended consequences for applications like https://github.com/TheXTech/TheXTech which use some, but not all, SDL2 features. For context, TheXTech on homebrew uses SDL2's RWops, thread, and audio APIs, but does not use any other major SDL2 features. We directly use wiiuse and libogc/video/gx to interface with Wiimotes and the Wii's video/rendering system.

What's the issue you encountered?

When I updated my SDL2 package from release 2.28.5-12 to release 2.28.5-13 last month, I noticed the following issues:

Build now requires opengx to link correctly
pacman package does not list opengx as a dependency
Codesize increased by 100kb, static memory increased by 200kb (via opengx, even though it is never initialized)
Game crashes due to SDL2 initializing wiiuse even when not requested by the game, resulting in double-init of the library

I'm happy to split this into multiple issues if you prefer.

How can the issue be reproduced?

I'm not asking you to do this, but by building TheXTech (using the Wii cmake toolchain) after installing devkitPro's sdl2-wii.

Environment?

I'm using Fedora, but I don't think that's relevant here. This didn't affect our CI builds because the devkitPPC docker image hadn't been updated yet when I caught the issue locally.

Additional context

For now we are using our own, much more minimal build of SDL2 for Wii (https://github.com/Wohlstand/SDL, branch wii-support). This works fine for our CI, but is inconvenient for users who might want to build our engine locally.

Wohlstand commented 1 month ago

more minimal build of SDL2 for Wii

It's not "more minimal", it's "more incompleted" version that I started a while ago before devkitPro did their own version. Actually, it's possible to build SDL2 itself with disabling several sub-systems via SDL2's CMake options (like, disable the whole Video subsystem or disabling Joysticks subsystem completely).

ds-sloth commented 1 month ago

It's certainly possible to make a build that disables some subsystems, but I think an ideal solution that allows people to use the mainline build would look like:

(1) Add public facing per-subsystem init functions to allow better unused symbol stripping (2) Make sure that wiiuse is not initialized unless the joystick, event, or cursor subsystems are initialized

DacoTaco commented 1 month ago

for now i can not comment about SDL or SDL2 stuff, as thats not part of my expertise, but as for the wiiuse stuff, i think in general wiiuse should not crash if WPAD_Init() is called twice. be it from SDL or a coding mistake. that is an issue i can easily fix in libogc though :)

ds-sloth commented 1 month ago

DacoTaco, thanks for the note. You should be able to reproduce this issue easily by initializing SDL2 in any example that currently uses wiiuse, then connecting (or reconnecting) a new Wiimote. Stack traces can lie, but if I recall correctly, there was a hardware exception triggered in wiiuse code that was called on a Bluetooth event.

Wohlstand commented 1 month ago

It's certainly possible to make a build that disables some subsystems, but I think an ideal solution that allows people to use the mainline build would look like:

(1) Add public facing per-subsystem init functions to allow better unused symbol stripping (2) Make sure that wiiuse is not initialized unless the joystick, event, or cursor subsystems are initialized

SDL2 already has options to initialize certain sub-systems by flags of SDL_Init(). So, it's just need to init only subsystems are declared at SDL_Init()'s argument as flags.

ds-sloth commented 1 month ago

That's not enough to help with code size and static memory, because almost no compilers will do the relevant cross-function control flow analysis to strip symbols pertaining to the unused subsystems based on a flags bitmask, but most compilers will strip symbols based on a function reachability graph.

I suppose another solution could be to find some way to force-inline the SDL_Init function. It's much more likely that a within-function control flow analysis would avoid adding the unused symbols.

Wohlstand commented 1 month ago

With the static memory the custom build with disabling of unnecessary sub-systems should work. Or splitting the library to separated components, but that's more idea for SDL3.

ds-sloth commented 1 month ago

Sounds like a great plan, thanks a lot for the update. [Edit: this comment was in response to the note which is now below it.]

DacoTaco commented 1 month ago

just to have some public info about this, we might be splitting this up in 3 issues. one is the missing dependency in pacman, the other is the WPAD_Init issue and another is the SDL code size issue. we have some ideas to tackle the latter (as it is a bigger technical challenge than the others) and we have a few ideas that @mardy can try if he has time. we want to, optimally only have 1 library so there can be made no issues there. what we were thinking is basically having stub functions in SDL that would be overwritten if opengx is linked in in the project. ill be trying to reproduce the WPAD_Init later this week

DacoTaco commented 1 month ago

@ds-sloth : got a minimal project i can build/test the wiimote thing with? been trying to reproduce it but can't get it to crash haha

ds-sloth commented 1 month ago

Are you trying to disconnect and reconnect Wiimotes while your test app is running? That's the specific thing that causes crashes for me. If you can't get your app to crash by doing that, then let me try to strip down our usage of the wiiuse API to the minimum and make you a test app.

WinterMute commented 4 weeks ago

Build now requires opengx to link correctly

pacman package does not list opengx as a dependency

Codesize increased by 100kb, static memory increased by 200kb (via opengx, even though it is never initialized)

These parts should have been fixed by the recent changes to SDL2 and opengx to use weak linking. I also updated the SDL2 package with the opengx dependency. If those work for you then I think we can close this and open another for the wiimote issue.

ds-sloth commented 4 weeks ago

Thanks a lot for making these changes. I just got the time to test them out now.

I ran into a hiccup at first: we use CMake and your shipped SDL2staticTargets.cmake config file currently links opengx without any question. That seems like it'd definitely be the right thing to do for "just works" ports of SDL2/CMake games to Wii, but it causes us to miss the benefits of your recent changes. I'm not familiar enough with SDL2's build process to know how to adjust that, but I imagine it shouldn't be too hard to add an off-by-default option such as SDL2_NO_OPENGX that allows developers with Wii-specific rendering code to avoid linking it in.

After I manually removed opengx from the linker invocation, things look very promising. The codesize no longer increases, and the BSS increases by a much more modest 50kb (from the fact that you use aesnd and we use asnd -- do you know the rationale for that on your side?). I think this is a success!

Lastly, the system crash when disconnecting and reconnecting a Wiimote still exists, so I will file a new issue for that whenever we close this one.

Wohlstand commented 4 weeks ago

(from the fact that you use aesnd and we use asnd -- do you know the rationale for that on your side?)

The aesnd is a newer library than asnd, and has better API for the audio processing as I remember.

mardy commented 3 weeks ago

I ran into a hiccup at first: we use CMake and your shipped SDL2staticTargets.cmake config file currently links opengx without any question. That seems like it'd definitely be the right thing to do for "just works" ports of SDL2/CMake games to Wii, but it causes us to miss the benefits of your recent changes. I'm not familiar enough with SDL2's build process to know how to adjust that, but I imagine it shouldn't be too hard to add an off-by-default option such as SDL2_NO_OPENGX that allows developers with Wii-specific rendering code to avoid linking it in.

Can you please try a build with the -lopengx option on if the size actually increases? I'm asking this because, in theory, that option should harmless, as long as no symbols from that library are being pulled in.

ds-sloth commented 3 weeks ago

@mardy, you're right, sorry for the confusion. My understanding was that the goal of #82 was to make the behavior depend on whether the -lopengx flag was included, but I ran some clean builds with and without the flag, and I found that the resulting size was the same whether or not it was included (though, strangely, 100kb larger than my builds from yesterday), and I confirmed that no opengx symbols were included. So, how is someone meant to enable opengx when using SDL2 with CMake?

devkitPro / SDL