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

[RFC / WIP] iOS support with working JIT #8492

Closed
wants to merge 81 commits into from

Conversation

OatmealDome
Copy link
Member

@OatmealDome OatmealDome commented Nov 24, 2019

This PR adds experimental support for iOS devices with a working JIT. It is by no means complete, and is only intended to initiate a discussion as to whether iOS is a platform that Dolphin should support.

READ THIS FIRST

You probably shouldn't ask for technical support in the comments here.

Please remember that this isn't ready for general use yet. I cannot stress this enough. If you only know enough to press the run button in Xcode, you should probably wait for some sort of release. If you still really want to try it out, be sure you read the PR fully to understand the current issues and limitations.

If you need support, or are a developer who would like to contribute, contact me on Twitter or ask in #dolphin-ios on my Discord server (I may take a while to respond since I'm currently busy with other responsibilities - I'll ping you when I get a chance).

PR details

First off, here's a quick demo video and some screenshots.

Let's get into the details.

Application

A new Xcode project has been placed in Source/iOS/DolphiniOS. The build system for iOS takes inspiration from the Android version of Dolphin.

Xcode has been configured to run some shell scripts on every build. The first one runs cmake using an iOS toolchain to create static libraries, which are then linked into the application. The second shell script copies image assets from the Android port.

The Sys folder is embedded into the bundle.

A "jni" equivalent can be found in Source/iOS/Interface. Its only purpose is to hold the MainiOS source file.

User interface

The UI is written in Objective-C and Swift. It is extremely barebones at the moment - while there is a working touchscreen controller, pretty much everything else needs work.

The code is 50/50 Objective-C and Swift, but I believe it would be best to write future UI code in Swift and transition what we can away from Objective-C. (Given the necessity of using C++ to access Dolphin code, some Objective-C++ will likely need to stay if this port is given a green light.)

All Dolphin user files are stored in the app container's Documents directory. This folder is then exposed to Files.app, allowing users to modify files as they wish.

A Software folder is automatically created in the Documents directory. The UI will automatically scan this folder for software and show found files in a list on startup.

Target devices

The minimum supported devices should be ones with an A9 processor.

Building

You need the following:

  • Mac
  • Xcode 11 (older Xcode versions may work, not tested)
  • cmake
  • Jailbroken iOS / iPadOS device running iOS 12 and above

Then, you can build:

  1. Install AppSync Unified on your device from cydia.angelxwind.net.
  2. Clone the repo.
  3. Open Source/iOS/DolphiniOS/DolphiniOS.xcodeproj.
  4. Initiate a build in Xcode.

Problems

Limitations

  • BoundingBox is not supported, since MoltenVK sets fragmentStoresAndAtomics to true only on macOS. However, forcing this feature to be enabled shows that it works fine on iOS (albeit with minor glitches). I'm not sure what I should be looking for on the Metal feature set table to see if iOS GPU drivers really support this feature.
  • Metal does not support geometry shaders or setting line width.
  • OpenGLES, like OpenGL on macOS, has been abandoned for a very long time. EXT_buffer_storage support is nowhere to be found. As expected, the OpenGL backend is unusable.

Known issues

  • Core::IsCPUThread() does not work for some reason. The call in JitArm64::HandleFault() needs to be commented out when testing for now.
  • Controller INIs from the Android version needs to be copied manually. GCPadNew.ini and WiimoteNew.ini go in Config and WiimoteProfile.ini in Config/Profiles/Wiimote.
  • To stop emulation, you have to quit the application.
  • You have to manually switch the controller type (GameCube controller or Wiimote with Nunchuk) in the source code.
  • There is no D-Pad on any controller type.
  • There is no build system that generates a deb for Cydia.
  • There are likely code style compliance issues. Swift code is entirely unchecked by lint.
  • Many other things.

Regarding the jailbroken device requirement

The iOS kernel places an artificial restriction on the amount of address space that each process can access, depending on how much RAM the device has. This is the core issue that plagued Dolphin support for a very, very long time. Fastmem requires a large amount of virtual memory to play with, which unfortunately is greater than the limit of every publicly released iOS device.

Thankfully, Apple created an entitlement called dynamic-codesigning that allows the process to create executable memory. It was added for applications that need JIT, like Safari. An additional perk of dynamic-codesigning is that it also allows the process to access a large amount of address space. However, it cannot be set on third-party apps as it is an Apple-internal entitlement. This is why a jailbroken device is needed.

There is a way, however, to run Dolphin under a jailed device. (In fact, I am doing the majority of my testing on a jailed device.) It requires modifying the iOS bootloader and setting the boot argument arm64_maxoffset. Because this method is quite complicated, I don't think this is a viable option for end-users. It is significantly easier to just require a jailbroken device than to support users who try this method.

The future

As we all know, Dolphin support for macOS doesn't get much love because of a lack of maintainers. While I don't plan to abandon this project, I think it would be best to see if any others are willing to contribute. I am just one person, after all.

Changes to Dolphin

This is a non-exhaustive list.

  • The define IPHONEOS has been introduced. IOS is not used to avoid confusion and conflicts with the Wii's IOS.
  • The iOS cmake toolchain from here has been added. CMakeLists use IOS because it is set from the toolchain.
  • Mach's vm functions are used wherever possible (one, two).
  • (This only applies to jailed devices.) When attached to a debugger, iOS allows memory to be marked as executable. However, memory cannot be writable and executable at the same time, so code has been introduced into the ARM64 JIT to support W^X exclusivity.
  • A new "EAGL" GLInterface was introduced. Also, because iOS doesn't create a default framebuffer, the OpenGL backend calls the EAGL GLInterface to create one.
  • The iOS MoltenVK dylib has been added to Externals.
  • The CoreAudio backend has been restored, because Cubeb support for iOS is old and broken.
  • Since ButtonManager and Touchscreen are useful on iOS too, they have been moved into a new "Touch" folder in InputCommon.
  • The required clang-format version has been updated to 9.0. Some Objective-C code was not being formatted correctly on 7.0 for whatever reason.

Credits

This PR is the culmination of many attempts by several people, including myself. It could not be done without them and support from others.

Previous PRs:

Thanks to:

  • @Simonx22 for testing
  • leetal's iOS toolchain for cmake
  • ppsspp for mach vm functions and W^X exclusivity example code
  • Apple for releasing their ARM XNU sources

@WilliamLCobb
Copy link

WilliamLCobb commented Nov 26, 2019

This is unbelievably cool, but you might run into the same problem as last time where the need to modify your device was just too much to swallow to officially support this.

However, #3941 shows we can run JIT code on iOS without any hardware or OS modifications. We just need to mprotect() the page holding JIT code to be READ, EXECUTE instead of READ, WRITE, EXECUTE. This brings a small overhead and adds some complexity to the codebase, but allows JIT on jailed phones.

That plus your MoltenVK fixes means we can probably get good performance on unmodified iOS devices.

@phire
Copy link
Member

phire commented Nov 26, 2019

Yeah, this annoying falls into the category of "Super Cool, but do we want to merge it"

@phire
Copy link
Member

phire commented Nov 26, 2019

There is a way, however, to run Dolphin under a jailed device.....It requires modifying the iOS bootloader and setting the boot argument arm64_maxoffset

Can this modification be done without a bootloader exploit?

@aydenp
Copy link

aydenp commented Nov 26, 2019

I know this isn’t necessarily in the area this mostly would need to bring it to official release, but I’d be very willing to contribute to and maintain the UI, packaging, and even hosting (I run a default repo) wherever needed, if the team decides that they want to go forward with it

@OatmealDome
Copy link
Member Author

@WilliamLCobb As you mentioned, the JIT is essentially no issue on a jailed device. My concern is the limited address space imposed by the kernel, so jailed devices can't use fastmem. That would definitely degrade performance.

@phire Without jailbreaking, not that I know of.

@tokfrans03

This comment has been minimized.

@OatmealDome
Copy link
Member Author

@tokfrans03 This script adds the special dynamic-codesigning entitlement, and it can't find the entitlements plist. It is located at that path when I build with Xcode 11, so I think it might be because you are using Xcode 10.

@degasus
Copy link
Member

degasus commented Nov 26, 2019

Thanks for lot for bringing this up again. It seems like it improved a lot.

I have one question about unmodified devices:

I got that you can't map more than ~1GB or memory. It requires some modification to our memory management, but that is fine. The performance loss will be ~10% if implemented fine.

But I haven't got if you actually support the JIT itself. The mprotect trick is fine. Requiring a jailbreak IMO not.

@Ichicoro
Copy link

I tried building it for my jailbroken iPhone 6S (iOS 13.2.2), but it crashes with EXC_BAD_ACCESS. It hangs in the CPU thread. The settings view also crashes, don't know if it's intended behavior.

@OatmealDome
Copy link
Member Author

OatmealDome commented Nov 26, 2019

@degasus JIT can work on a jailed device. I've already implemented support for W^X exclusivity like how @WilliamLCobb suggests.

The process does need to be debugged before the kernel will allow setting memory as executable. There's a trick that ppsspp uses where if you call ptrace(PTRACE_TRACEME) the kernel will think that it is being debugged. It causes the process to hang on exit, though, since it is still waiting for the debugger. ppsspp has a fix for this, but it only works on jailbroken devices because fork() is disallowed in the sandbox.

Shrinking the memory map to fit within 1GB would be amazing. I made some attempts but haven't had success with it yet. Might need help with it.

@Ichicoro

  1. See point 1 on known issues if you haven't already.
  2. Yeah, the settings view has been left unimplemented. I decided to open this PR and get comments first before starting on anything else.

@degasus
Copy link
Member

degasus commented Nov 26, 2019

@OatmealDome If they only missing requirement is the 1 GB virtual memory size for unmodified devices, I will do so. Please ask me on IRC for the current state, I assume I will forget about it a few times :(

@OatmealDome
Copy link
Member Author

@degasus Yes, it is the only blocking issue. Thank you for working on it!

I'm primarily concerned with the poor UX on jailed devices if Dolphin is quit in the task switcher. (Might also happen if iOS kills it to reclaim memory, but I've never tested it.) Once that happens, the device needs to be rebooted to launch Dolphin again.

@WilliamLCobb
Copy link

WilliamLCobb commented Nov 26, 2019

@OatmealDome From what I remember, the weird behavior caused by the app waiting for something to debug it can be fixed by manually terminating the app when the user tries to background it. We could save the game state and then close the app on jailed devices to prevent any freezes or bugs.

@OatmealDome
Copy link
Member Author

@WilliamLCobb Excellent, that sounds like a good workaround. Hopefully it works like you remember.

@Helios747
Copy link
Contributor

As cool as this is, this probably belongs in a fork.

One/two people owning this entire change set, ifdefs littered throughout parts of core and rendering. Requires a jailbroken device, which Apple makes an active effort to make harder, causing that landscape to be fairly turbulent.

We're already bad enough about supporting a mobile platform that doesn't mind dolphin existing. I can't see upstreaming support for a platform that doesn't want dolphin there going well.

Also I appreciate that you moved some input stuff to common and cleaned up Android a bit, but can you extract that to a separate PR? This PR is already big enough.

@Ichicoro
Copy link

@OatmealDome sorry, totally missed the first point.

I've gotten it to build, but it will still crash with EXC_BAD_INSTRUCTION at a crc32w instruction. Forcing it to use GetMurmurHash3 stops it from crashing but there's no video at all. Am I missing something else?

@OatmealDome
Copy link
Member Author

OatmealDome commented Nov 26, 2019

Given the suggestion above that this should be a fork, I think I should make my position clear. (I have already said this on IRC but it should be said here too.)

I'm not willing to maintain a fork - it would be too much of a burden on myself. If this doesn't get merged upstream I don't think I will continue maintaining the code.

@Helios747 I don't really see the ifdefs to be more or less than any other platform, with exception to the W^X exclusivity code. We can probably also get rid of the jailbroken device requirement.

About input cleanup: will open a PR when I get access to a computer.

@Ichicoro Hm, I've only had the CRC32X issue on A7. Weird that it is happening on A9.

To fix no video, set Dolphin to use the Vulkan backend. The UI code is hard-coded to assume that right now.

@Helios747
Copy link
Contributor

Right, but it's just More Ifdefs for a platform that supporting is already an uphill battle. We Ifdef stuff for various platform specific bits, but the idea of adding even more isn't fun.

@Ichicoro
Copy link

Ichicoro commented Nov 26, 2019

Each platform might have their own ifdefs... but I assume @OatmealDome will maintain the code once it's merged in upstream, or won't they?

Besides, jaibreaking is entering a new golden age so it probably the effort won't be wasted on a dead platform.

@OatmealDome Sorry, I couldn't quite catch that. Do I have to change it in-source or via config files?

@OatmealDome
Copy link
Member Author

@Ichicoro In Dolphin.ini.

And yes, I'm committing to maintaining the port if it gets merged.

Copy link
Member

@lioncash lioncash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made a cursory general review in case this moves forwards, and I've placed the bulk of my thoughts in one of the comments near the end, which can probably be summarized as: "It's a cool idea, but I think it needs a bit more polish".

Source/Core/AudioCommon/CoreAudioSoundStream.cpp Outdated Show resolved Hide resolved
Source/Core/AudioCommon/CoreAudioSoundStream.cpp Outdated Show resolved Hide resolved
static bool isValid() { return true; }

private:
AudioUnit audioUnit;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
AudioUnit audioUnit;
AudioUnit m_audio_unit;

Source/Core/AudioCommon/CubebStream.cpp Outdated Show resolved Hide resolved
@@ -64,13 +73,24 @@ void CPUInfo::Detect()
num_cores = sysconf(_SC_NPROCESSORS_CONF);
strncpy(cpu_string, GetCPUString().c_str(), sizeof(cpu_string));

#ifdef IPHONEOS
// TODO(OatmealDome): Figure out how to get this information
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should likely be figured out before and not after the fact.

Source/Core/Core/PowerPC/JitArm64/JitArm64_BackPatch.cpp Outdated Show resolved Hide resolved
Source/Core/Core/PowerPC/JitArm64/JitArm64_BackPatch.cpp Outdated Show resolved Hide resolved
Source/Core/Core/PowerPC/JitArm64/JitAsm.cpp Outdated Show resolved Hide resolved
@degasus
Copy link
Member

degasus commented Nov 27, 2019

https://github.com/degasus/dolphin/commits/fastmem should disable the fastmem area. It is not complete (jits are broken), but the interpreter boots fine. JitArm64 might boot as well if you disable fastmem (but with a big performance hit).

@mouksx

This comment has been minimized.

@OatmealDome
Copy link
Member Author

Minor changes.

  • CoreAudioSoundStream has been cleaned up to comply with the code-style.
  • OpenGL ES warnings have been silenced with a special define. The APIs have been marked deprecated since iOS 12, but OpenGL ES is still useful for Dolphin's purposes (software renderer, OpenGL backend).

@Ichicoro
Copy link

I know this isn't very important right now, but I've taken the time to add rudimentary MFi controller support to the emulator (just so I could comfortably play WW :P). Here's a patch with my implementation. FYI: this sets the TouchControls view's alpha to 0 when a controller is connected.

@OatmealDome
Copy link
Member Author

@Ichicoro Thanks for the patch. I'm holding off on implementing anything past basic emulation until it becomes clear if there is a chance the PR will be merged or not. I'll have a look at it when that time comes.

@OatmealDome OatmealDome requested a review from lioncash November 30, 2019 23:47
@stenzek
Copy link
Contributor

stenzek commented Dec 1, 2019

A function which checks if a page is set as executable has been added to MemoryUtil.
There is no implementation for platforms other than Windows, macOS, and iOS, unfortunately. There seems to be no POSIX API which lets you check a region's permissions. The commonly suggested workaround for Linux is to read /proc/self/maps, but I'm not sure if that would be the best for performance.

FWIW, VirtualQuery: https://docs.microsoft.com/en-us/windows/win32/api/memoryapi/nf-memoryapi-virtualquery

I'm not sure why this is needed, but it'd probably easier to just have an array/bitset for each page in the code range with the executable bit.

@OatmealDome
Copy link
Member Author

@stenzek Yeah, I use VirtualQuery on Windows.

it'd probably easier to just have an array/bitset for each page in the code range with the executable bit.

I agree. I was looking into making a vector of executable regions, but it didn't work out during the time I had available, so I just went with what I had for now (VirtualQuery on Windows and vm_region_64 on Apple).

@stenzek
Copy link
Contributor

stenzek commented Dec 1, 2019

I think degasus may have asked this before, but is it possible to allocate a shared memory arena and create multiple aliases in iOS? Similar to what we do for fastmem, except for the JIT code space. This way one of the mappings could be RW and the other RX, which means we wouldn't have to worry about changing or tracking per-page permissions.

It would require modifications in the emitter for dealing with absolute addresses, but addresses relative to the code space would be the same.

@OatmealDome
Copy link
Member Author

@stenzek I whipped up a quick test program. It does seem to be possible. (Just going to ping @degasus since they will probably be interested.)

Source code here.

DolphiniOS[39050:16582746] vm_mem 0x10396c000
DolphiniOS[39050:16582746] OK, remap at 0x1039bc000
DolphiniOS[39050:16582746] OK, remap at 0x1039c0000
(lldb) m region 0x10396c000
[0x000000010396c000-0x0000000103970000) rw-
(lldb) m region 0x1039bc000
[0x00000001039bc000-0x00000001039c0000) rw-
(lldb) m region 0x1039c0000
[0x00000001039c0000-0x00000001039c4000) r-x
(lldb) m w 0x1039bc000 0xAA 0xBB 0xCC 0xDD
(lldb) m read 0x1039bc000
0x1039bc000: aa bb cc dd 00 00 00 00 00 00 00 00 00 00 00 00  ................
0x1039bc010: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
(lldb) m read 0x1039c0000
0x1039c0000: aa bb cc dd 00 00 00 00 00 00 00 00 00 00 00 00  ................
0x1039c0010: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
(lldb) 

@ghost

This comment has been minimized.

@degasus
Copy link
Member

degasus commented Dec 5, 2019

@SGamer33 It is in early development right now. If you can't compile it on your own, you won't be able to use it.

@ghost

This comment has been minimized.

@Nuxx5

This comment has been minimized.

@OatmealDome
Copy link
Member Author

Hi everyone,

Thanks for your interest. Out of respect for the Dolphin devs, I'd rather that this PR not become a technical support thread.

Please remember that this isn't ready for general use yet. I cannot stress this enough. If you only know enough to press the run button in Xcode, you should probably wait for some sort of release.

If you need support, or are a developer who would like to contribute, contact me on Twitter or ask in #dolphin-ios on my Discord server (I may take a while to respond since I'm currently busy with other responsibilities - I'll ping you when I get a chance).

@SGamer33 Clone my fork and checkout the ios branch.

@JohnG210 Read "Known Issues".

@Nuxx5 Set the video backend in Config/Dolphin.ini to Vulkan.

@ghost

This comment has been minimized.

@saagarjha
Copy link

If you do end up going down the route of shipping ptrace(PT_TRACE_ME, …) I'd suggest setting up a Mach exception handler to catch any signals and terminating (in addition to the resignation detection that @WilliamLCobb suggested) because they'll bork the device too if you don't handle them.

@Brandon-T
Copy link

To get around the issue of allocating large amounts of memory, have you guys tried using mmap or NSData.dataWithContentsOfMappedFile(path: NSString)?

FastImageCache uses it to get around memory limitations. I have used it as well to get around the limitation iOS places on allocation size.

I wonder if it will work in this case as well OR if the allocation will be too large.

@saagarjha
Copy link

mmap would have the same issue; all the virtual memory functions end up going through the same code path in the kernel.

@Notjailbrokenguy
Copy link

Is it possible to have interpreter please? Because you don’t need JB for interpreter. I hope you can reply

Greetings

@stenzek
Copy link
Contributor

stenzek commented Jan 13, 2020

@Notjailbrokenguy the interpreter isn't fast enough to be usable on desktops let alone phones.

Closing because the author abandoned the PR and is offering it as a paid patreon release instead

@stenzek stenzek closed this Jan 13, 2020
@OatmealDome
Copy link
Member Author

@stenzek Are you really closing this PR because I'm offering Patreon releases? I get that you don't like what I'm doing, but I don't think you should bring your personal opinions into this.

I'm not "offering [this PR] as a paid Patreon release". While the convenience of a pre-built version is provided for people who donate, DolphiniOS is free at my Cydia repo (https://cydia.oatmealdome.me). I've told you this before on IRC. In addition, the source for the absolute latest version (public or Patreon) is provided at https://github.com/oatmealdome/dolphin/tree/ios-jb for those who want to build it themselves.

@JosJuice
Copy link
Member

The problem here isn't really the Patreon, it's that there's not much point in keeping the PR open if it's no longer being updated. If you want to continue updating this PR, I personally don't see any problem with re-opening it.

@stenzek
Copy link
Contributor

stenzek commented Jan 13, 2020

You're contradicting yourself a little there. Saying "it's free" yet you don't get the same builds if you're not a patron. How is that free?

Even if we don't support iOS, I think it's a shame that the work you're doing isn't available to the whole community and may eventually diverge so far from upstream that it's not usable in the future (see: other dolphin forks). But that's your choice.

Edit: I am referring to any non iOS specific changes, e.g. W^X, optimisations, etc which can be upstreamed. If that was your intention, I apologise, I wasn't trying to discourage you from contributing.

Anyway, regardless of my thoughts, there isn't any point in keeping the pull request open if you are not going to continue work on it. It's still here if somebody else wants to pick it up, and please feel free to re-open it in the future if you change your mind about upstreaming your changes.

@OatmealDome
Copy link
Member Author

@JosJuice If @stenzek just said he was closing the PR because of abandonment, I would not have any issues. There is no point in mentioning the Patreon, especially since his comment implies DolphiniOS is completely locked behind a paywall.

@stenzek You can get the source code for free and build it. It's as simple as clicking the "download as ZIP" button on GitHub and then clicking the "Run" button in Xcode.

The work I'm doing is always available to the entire community. The absolute latest source (yes, even the Patreon builds) is always available for free at https://github.com/oatmealdome/dolphin/tree/ios-jb. As soon as a Patreon build is released, anyone can download the source and build it themselves, regardless if they are donating to me or not.

I'm not modifying the Dolphin core any more than I already have in this PR, so it can't really diverge from upstream any further. The vast majority of my work is on the iOS application, which is basically a "reimplementation" of DolphinQt in iOS's UIKit, which can't be merged into upstream as discussed earlier. I plan to eventually PR what I can (for example WX) but I haven't really found the time so far.

@stenzek
Copy link
Contributor

stenzek commented Jan 13, 2020

My initial response should have been more professional. By 'releases' I was referring to the binary builds. Regardless of my opinion on the matter you are complying with the license terms and that's all we can ask.

I still hope that some day we can find a way to support the platform upstream without the maintenance burden falling on the core team.

@Notjailbrokenguy
Copy link

Notjailbrokenguy commented Jan 14, 2020

Hey sorry to disturb you guys. This may be a bit off topic. But I may have a solution for non-jail broken devices. I heard it’s possible to acces JIT by connecting your device to you Mac. Is this true? If so how can I do it?

@OatmealDome
Copy link
Member Author

@Notjailbrokenguy By running an app using Xcode, iOS will let the app run a JIT. However, while this "solves" one part of Dolphin's issues, the other part (limited memory space) isn't solved, so Dolphin wouldn't be able to run with good performance.


Could someone lock this PR's comments to collaborators only?

I don't want it to become a support thread or a place for people to ask questions. I'll edit the OP to include my contact information so they can ask me directly instead of bothering all of you.

Sorry for the inconvenience.

@dolphin-emu dolphin-emu locked as off-topic and limited conversation to collaborators Jan 15, 2020
@degasus
Copy link
Member

degasus commented Jan 15, 2020

@OatmealDome My fastmem PR was merged, so once you rebase, the 16 GB memory region won't be allocated any more if you disable fastmem. Not having fastmem is ofc a performance hit, but by far not as much as without jit support.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.