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

Wii: broken partial SDL2 usage #81

Open
ds-sloth opened this issue Sep 24, 2024 · 16 comments
Open

Wii: broken partial SDL2 usage #81

ds-sloth opened this issue Sep 24, 2024 · 16 comments

Comments

@ds-sloth
Copy link

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
Copy link

Wohlstand commented Sep 24, 2024

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
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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
Copy link

Wohlstand commented Sep 25, 2024

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
Copy link
Author

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
Copy link

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
Copy link
Author

ds-sloth commented Sep 25, 2024

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
Copy link
Member

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

mardy added a commit to mardy/SDL that referenced this issue Sep 26, 2024
This allows building a client application without linking to opengx, if
the client itself does not need OpenGL. The opengx functions used by SDL
are defined as weak, so that they will be used only if a strong symbol
is not found during linking.

    $ nm /opt/devkitpro/portlibs/wii/lib/libSDL2.a | grep ogx
    00000000 W ogx_get_proc_address
    00000000 W ogx_initialize

Partial fix for devkitPro#81
WinterMute pushed a commit that referenced this issue Sep 28, 2024
This allows building a client application without linking to opengx, if
the client itself does not need OpenGL. The opengx functions used by SDL
are defined as weak, so that they will be used only if a strong symbol
is not found during linking.

    $ nm /opt/devkitpro/portlibs/wii/lib/libSDL2.a | grep ogx
    00000000 W ogx_get_proc_address
    00000000 W ogx_initialize

Partial fix for #81
@DacoTaco
Copy link
Member

DacoTaco commented Oct 3, 2024

@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
Copy link
Author

ds-sloth commented Oct 3, 2024

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
Copy link
Member

  • 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
Copy link
Author

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
Copy link

(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
Copy link
Collaborator

mardy commented Oct 15, 2024

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
Copy link
Author

@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?

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

5 participants