Skip to content

(libretro-common) Use inline assembly for PowerPC libco #16675

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

Merged
merged 1 commit into from
Jun 25, 2024

Conversation

ashquarky
Copy link
Contributor

@ashquarky ashquarky commented Jun 11, 2024

Guidelines

  1. Rebase before opening a pull request
  2. If you are sending several unrelated fixes or features, use a branch and a separate pull request for each
  3. If possible try squashing everything in a single commit. This is particularly beneficial in the case of feature merges since it allows easy bisecting when a problem arises

Description

Currently libco in libretro-common stashes a bunch of assembly code into a C array, then maps it as RWX and jumps to it. This is incompatible with platforms that can't map arbitrary RWX pages (namely Wii U) and adds RWX pages for questionable reason, which isn't great for security and stability.

Move the assembly into an asm block, which implicitly moves it into .text, making it R_X everywhere as you'd expect for code. I'm using GNU/AT&T assembly syntax here which should be broadly compatible, though I left the old code behind an #if 0 in case the PowerPC Mac people flag it as a problem.

Needed for ScummVM and other libco cores (dosbox-svn?) to work on Wii U. I have no idea how they were ever reported working by users, probably they were using the .elf builds which are fully RWX (cc @Ploggy ?)

Related Issues

[Any issues this pull request may be addressing]

Related Pull Requests

This commit will need to sync out to the libretro-common repository, then ScummVM can have its commit hash updated to pull this code. I don't know what the process is for this.

Reviewers

[If possible @mention all the people that should review your pull request]

@ashquarky
Copy link
Contributor Author

To clarify, this is needed for libco cores on Wii U - not just in general, as the original comment erroneously stated.

@LibretroAdmin
Copy link
Contributor

Hi, will these changes affect other PowerPC systems or do the changes only affect WiiU? Perhaps best to ifdef these changes for WiiU only perhaps if it affects the other PowerPC based systems?

@ashquarky
Copy link
Contributor Author

It does affect all systems in its current version. I think this setup is neater but I can also make it WiiU-specific, yeah.

This puts the code into the binary's .text section, which is needed
for platforms without the ability to map it as RWX (WiiU). Using
GNU/AT&T syntax for the assembly here.
@ashquarky
Copy link
Contributor Author

@LibretroAdmin now it affects WiiU only

@vaguerant
Copy link
Contributor

vaguerant commented Jun 19, 2024

Just dropping a list in here of the cores which use libco on Wii U and will need to be synced after this gets merged:

  • atari800 (libretro-common in-repo at libretro/libretro-common)
  • hatari (libretro-common in-repo at libretro/libretro-common)
  • dosbox-pure (libretro-common in-repo at libretro-common in root)
    • I believe most development on this core occurs upstream in schellingb's repo then gets synced back to libretro's; should be a priority to get a PR out to upstream
  • dosbox-svn (libretro-common as submodule at libretro/deps/common)
  • frodo (libretro-common in-repo at Src/libretro-common)
  • mame2000 (libretro-common in-repo at src/libretro/libretro-common)
  • REminiscence (libco in-repo at 3rdparty/libco)
  • scummvm (I think libretro-common is pulled in by backends/platform/libretro/dependencies.mk, not familiar with build system)

While we're on the subject, this does look like a lot of in-repo duplication of what's supposed to be a bunch of common libraries. It seems like it would be prudent to make more use of submodules to prevent the risk of all of these diverging.

@ashquarky
Copy link
Contributor Author

Is this OK to be merged? We'd like to get it out to all the cores ahead of the Aroma RetroArch PR

@LibretroAdmin LibretroAdmin merged commit 27f337d into libretro:master Jun 25, 2024
27 checks passed
@LibretroAdmin
Copy link
Contributor

@vaguerant I'd rather not use submodules but subtrees instead.

Anyway, I guess I can take a look at syncing most of them.

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

Successfully merging this pull request may close these issues.

3 participants