dolphin-memory-engine
dolphin-memory-engine copied to clipboard
aldelaro5
MacOS support
These changes build off of my previous PR so it might be better to just accept this one if you like the changes.
The majority of the additions were in the MacDolphinProcess files, where the memory scanning functionality for the Mach kernel was implemented. There were some major performance issues at first, mainly because vm_read on Mac is much slower than the equivalent APIs on Windows and Linux (and because I used a 2013 MacBook Air). 500mb of data were being allocated, deallocated, and read from Dolphin each second for the cache, so I removed the cache and it sped up considerably. Not sure if this is a problem, but it also seems to perform better on Linux. It still eats up a bit of CPU, but profiling shows that no function in particular is to blame.
The README has been updated on how to build and run for MacOS.
First sorry for responding so late, but I have been busy with other projects and only come back to handle some stuff for a week on this one.
I don't know if you still want to work on this (if not, I might have to pull your pr and coauthor), but there are couple of major issues preventing me to merge this. Mainly, I can't have the encoding features AND the mac os support in the same PR, you would need to split this pr to ONLY mac os and if needed, update your encoding pr.
The other issue is the cache because I am p sure the reason I ended up HAVING to do it is due to the viewer since the view should only change what parts of the ram is shown while it's already read. I honestly don't like it, but unless I can be convinced it doesn't cause issues with the viewer I can't exactly make this go forward (there's also the fact you seemed to run rather old hardware at the time of the PR while dolphin already expects you to have p good hardware). So I would need to be more convinced that removing the cache is needed.
I also see other issues like some files don't have a line at the end and for some reasons, you seemed to have changed the csproj more than it should be needed.
Do you still want to work on this? As I said, if it's not possible, I could always pull your branch and coauthor the PR to a mergable state myself, but it's up to you.
Thank you for taking a look at my PR.
If I remember correctly, the CPU usage with DME open (and idle) was around 90% on macOS. Most of this time was spent on reading process memory for the cache, so it needed to be removed. The performance hit may be less pronounced on newer systems, but I think the cache probably needs to be made more performant anyways (I removed it completely, thinking that the cache was only an optimization that turned detrimental). I can look into the code again later to see if removing the cache had any unwanted side effects, and if it did, I will fix that.
Also, how do you recommend me to split the PRs? I can fork master again to a separate branch and re-commit the changes to implement macOS support (minus the Unicode support). In that case, I would delete this PR. Does that work?
I will also review the formatting and revert .csproj in both PRs, if that works for you.
If I remember correctly, the CPU usage with DME open (and idle) was around 90% on macOS. Most of this time was spent on reading process memory for the cache, so it needed to be removed. The performance hit may be less pronounced on newer systems, but I think the cache probably needs to be made more performant anyways (I removed it completely, thinking that the cache was only an optimization that turned detrimental). I can look into the code again later to see if removing the cache had any unwanted side effects, and if it did, I will fix that.
I know my memories are very vague, but ik I eventually had to put the cache at least for the viewer to be happy so this is why I am hesitant. If however, there's enough reasons for me to know it could be removed, sure, but I am p sure that the reason I ended up doing it is because it wasn't feasible to read the block of ram needed for the viewer everytime you would scroll. It was WAY more performant to do one big read every once in a while vs many small reads. This is also why I put the timer settings so you can make it update slowly.
So yeah, I am very unsure here.
Also, how do you recommend me to split the PRs? I can fork master again to a separate branch and re-commit the changes to implement macOS support (minus the Unicode support). In that case, I would delete this PR. Does that work?
You can just force push cause you already have a PR opened about string encoding so you could just force push that one with your encoding stuff if needed. For this PR, same thing, you can just force push (and you may as well squash the commits cause it can make the history cleaner).
Point is, you don't NEED to close any PRs for now.
I will also review the formatting and revert .csproj in both PRs, if that works for you.
Yeah, I would have pointed these out in a deeper reivew, but since this had organizational issues, I only glossed the changes and saw these.
Btw I need to ask because I am curious: I tried years ago to make this work on a mac vm and it refused to have the perm to even read a remote process and from what I gathered, you needed to sign the thing, but to do that, the only way I found was to buy a certificate which cost an annual fee. How did you got it to work without doing this?
Sorry for the delay!
I have measured the performance of removing the cache on Linux, Windows, and macOS and there seems to be little overall change other than the speedup on macOS (and a slight improvement on Linux). There is a small increase in CPU usage, though, when scrolling in MemViewer (on Windows mostly), but it is not critical.
For the macOS permissions, the executable needs to be signed with a code signing certificate and it needs to be given debugger entitlements (found in Source/entitlements.xml). Currently, I have updated the README with instructions to do this (and to create a free self-signed code certificate). It is also possible to create a code signing certificate for distribution, but those are usually expensive and need to be purchased from a CA.
Also, I have just separated the PRs, so this PR now only contains changes to implement macOS support. I also tried to address the formatting in both branches, along with fixing a few remaining problems in this PR.
How does everything look at the moment? Let me know if there is anything else I should modify.
So I got a build error with this PR, that happened to occur in Common/CommonUtils.h, but I added another #Elif for mac that uses linux's elif template. I had to add #import <libkern/OSByteOrder.h>, under another #elif for apple, and before the return bSwap16, etc I added #define bswap_16(x) _OSSwapInt16(x) using functions from the import. The Error I ended up getting was a segmentation fault on MacOS Catalina. If anyone wants I could add my edits in a txt to here
Thank you for your report, @Hibyehello. Are you certain you are compiling the correct branch? I already added byte-swap macros in CommonUtils.h guarded with #elif __APPLE__. I don't see any reason for this to not work. In the end, though, were you able to compile? Where did the segmentation fault occur? Did you sign the executable with entitlements?
If you think you are doing everything correctly (as outlined in the new README) and there are still issues, let me know.
Well from I had gotten there was not any Apple specific code in commonutils.h, but let me look, and I will keep testing
What I had pulled didn't have the code but I see the code you have began adding edit: I forgot to switch branches
So after building I was unable to hook it to dolphin after doing all the certificate stuff, it said it saw the process, but couldn't see that the game was running.
This is what happens, and I have signed the app. It just won't hook to dolphin
Not sure where the issue is occurring, but I suspect the code may not be reliably finding the emulator memory region (MacDolphinProcess.cpp). If you want, try running vmmap Dolphin | grep 32.0M before and after starting emulation and post the output here.
Also, what version of Dolphin are you using? It seems to work for me on 5.0-14095.
Before emulation starts: MALLOC_SMALL 00007f902b800000-00007f902d800000 [ 32.0M 7768K 7768K 0K] rw-/rwx SM=PRV MallocHelperZone_0x10c916000
After emulation starts: shared memory 000000012a959000-000000012c959000 [ 32.0M 32.0M 32.0M 0K] rw-/rw- SM=SHM
shared memory 00000001424e7000-00000001444e7000 [ 32.0M 32.0M 32.0M 0K] rw-/rw- SM=SHM
MALLOC_SMALL 00007f902b800000-00007f902d800000 [ 32.0M 14.7M 14.7M 0K] rw-/rwx SM=PRV MallocHelperZone_0x10c916000
Using Dolphin version 5.0-11653
Not sure then. Are you certain the executable has the proper entitlements? Try codesign -d --entitlements :- ./Dolphin-memory-engine and look for <key>com.apple.security.cs.debugger</key> in the output.
Could this be an issue with my version of mac os, since I am running Mac Os 10.12 (Sierra)
I recently got a 2020 MacBook Air on macOS 11.3 and was able to test my changes. Initially, DME was unable to access Dolphin's memory because it seems macOS 11 requires processes to have a certain entitlement (com.apple.security.get-task-allow) to be debugged. However, removing Dolphin's preexisting signature and applying a new self-signed one with the extra entitlement solves this problem. To make this easier, I created the MacSetup.sh script which automatically signs DME's executable and Dolphin's executable, provided a self-signed code certificate.
@Hibyehello, perhaps using the new MacSetup.sh script to sign Dolphin.app will fix your issue. I'm not sure how older versions of macOS handle debugging access.
Sorry I've been busy, but I tried this today and I got
/var/folders/20/rcn061sj6tx02r098cj6b8600000gt/T/tmp.0iVfJViQ: unrecognized blob type (accepting blindly)
/var/folders/20/rcn061sj6tx02r098cj6b8600000gt/T/tmp.0iVfJViQ: invalid length in entitlement blob
Dolphin could not be re-signed!
so DME won't hook
Seems like it's broken on newer Dolphin versions; at least for me on an M1 MBA on Monterey; this worked before but somehow stopped working.
@LapisLazulis, what exactly doesn't work? Everything seems to be fine for me on macOS 12.4, Dolphin 5.0-16380 (most recent beta), and a 2020 M1 MBA.
Also, have you run MacSetup.sh (in Source)? There are instructions in the README for creating a code-signing certificate and running this script. More recent versions of macOS require processes (Dolphin) to be signed with a specific entitlement to be debugged. Dolphin must also be re-signed using this script whenever a new update is installed or the executable is overwritten. Let me know if this doesn't resolve the issue.
Hi @campital, can you please rebase if this is still something you would like in DME?
I have a Mac and will be vetting this for possible release in the next release.
After looking over the code, I don't think it will be a big deal to revisit and rebase after the Qt 6 merge.
This will not be present in the final Qt 5 release. Additionally, I will not be looking into this unless campital returns and/or there is interest in a mac version.
@dreamsyntax I have rebased this PR and removed the commits that affect the cache. CPU usage is still extremely high on all platforms with cache enabled, so I will create another PR that removes the cache unless there are any objections. It does not seem to provide any performance benefit.
@EndangeredNayla @campital I'm not really clear what the state of this is. Which PR are we going with? Is there an advantage to this one over the other?
The GH flows are only in the other one, but break Windows currently.
@dreamsyntax I will create another PR that removes the cache unless there are any objections. It does not seem to provide any performance benefit.
That works for me, then I can do my own profiling.
This PR supports the new ARAM additions and fixes a few bugs (font, additional verification of memory regions), while I believe the other PR currently does not. This PR also includes the automatic signing script and additional instructions in the README (if desired). We could also integrate the GitHub flows from the other PR.
@EndangeredNayla is right that it seems that only the Dolphin executable needs to be signed, so I have removed the signing of the DME executable.
I have copied over the build.yml from the other PR, so this should be ready to merge if everything else looks correct.
I have copied over the
build.ymlfrom the other PR, so this should be ready to merge if everything else looks correct.
Ci:
Invalid workflow file: .github/workflows/build.yml#L106 You have an error in your yaml syntax on line 106
Also, the other PR broke windows (probably because removed the bash line for some reason).