Jit: Don't use a second stack #11399
Conversation
JosJuice
force-pushed
the
jit-one-stack
branch
2 times, most recently
from
b155dd7
to
1824a57
Compare
|
IIRC macOS uses 512k stacks |
JosJuice
force-pushed
the
jit-one-stack
branch
from
1824a57
to
d75b496
Compare
|
got this:
It still runs but I get that warning before the game starts. |
JosJuice
force-pushed
the
jit-one-stack
branch
from
d75b496
to
1af0783
Compare
|
On IRC, we discussed setting the stack size manually. It is possible to do this using pthreads, but std::thread doesn't let you get a pthread handle until the thread has already started running, so it seems like if we want to do this we would have to forgo using std::thread for the CPU thread. Instead, I've changed the constants so we now accept macOS's 512 KiB stacks. It's probably fine... Assuming there was no deeper reason for 2 MiB being picked in the past. |
|
warning dialog with this now when starting: Game plays normally like before. |
|
That's weird... The call to ReadProtectMemory is basically unchanged from before. Or is this some kind of alignment shenanigans? @TellowKrinkle, would you have the ability to debug this on macOS? |
|
Alignment seems like a good guess, the address argument must be page boundary aligned. |
|
(And don't forget that M1 uses 16K pages) |
|
I find it a bit odd that the stack wouldn't be page aligned, but... Alright, here's an updated version of the code that should ensure that the alignment is correct. |
JosJuice
force-pushed
the
jit-one-stack
branch
from
1af0783
to
90253b9
Compare
|
Sorry to annoy you with this but it's now pointing to the same line as my first comment. Maybe wait until TellowKrinkle has time to spare. |
JosJuice
force-pushed
the
jit-one-stack
branch
from
90253b9
to
cf65980
Compare
|
So, that the line number is the same is just a coincidence. What's actually happened here is that a check that's done fairly early on (before any of the other checks you've triggered) actually was broken before but is fixed now, and macOS is failing the check. I guess it might be because macOS returns the top of the stack instead of the base of the stack when asked for the stack address. I've uploaded a new version of the commit, if you want to check if that hypothesis was correct. |
JosJuice
force-pushed
the
jit-one-stack
branch
from
cf65980
to
dcb3a79
Compare
Nice! Looks like you were right since it’s no longer complaining. |
|
This also explains why it seemed like the stack wasn't page aligned before: In fact it was the top of the stack that wasn't page aligned. (Well, not that we know for sure if the bottom is aligned or not, since the current code does handle the case of it not being aligned.) |
Seems like this was broken all along. The safe zone is at the lower addresses of the stack, not the higher addresses.
This second stack leads to JNI problems on Android, because ART fetches the address and size of the original stack using pthread functions (see GetThreadStack in art/runtime/thread.cc), and (presumably) treats stack addresses outside of the original stack as invalid. (What I don't understand is why some JNI operations on the CPU thread work fine despite this but others don't.) Instead of creating a second stack, let's borrow the approach ART uses: Use pthread functions to find out the stack's address and size, then install guard pages at an appropriate location. This lets us get rid of a workaround we had in the MsgAlert function. Because we're no longer choosing the stack size ourselves, I've made some tweaks to where the put the guard pages. Previously we had a stack of 2 MiB and a safe zone of 512 KiB. We now accept stacks as small as 512 KiB (used on macOS) and use a safe zone of 256 KiB. I feel like this should be fine, but haven't done much testing beyond "it seems to work". By the way, on Windows it was already the case that we didn't create a second stack... But there was a bug in the implementation! The code for protecting the stack has to run on the CPU thread, since it's the CPU thread's stack we want to protect, but it was actually running on EmuThread. This commit fixes that, since now this bug matters on other operating systems too.
Deduplication between Jit64 and JitArm64.
JosJuice
force-pushed
the
jit-one-stack
branch
from
dcb3a79
to
b6256a5
Compare
|
Can this be merged now? |
This second stack leads to JNI problems on Android, because ART fetches the address and size of the original stack using pthread functions (see GetThreadStack in art/runtime/thread.cc), and (presumably) treats stack addresses outside of the original stack as invalid. (What I don't understand is why some JNI operations on the CPU thread work fine despite this but others don't.)
Instead of creating a second stack, let's borrow the approach ART uses: Use pthread functions to find out the stack's address and size, then install guard pages at an appropriate location. This lets us get rid of a workaround we had in the MsgAlert function.
By the way, on Windows it was already the case that we didn't create a second stack... But there was a bug in the implementation! The code for protecting the stack has to run on the CPU thread, since it's the CPU thread's stack we want to protect, but it was actually running on EmuThread. This commit fixes that, since now this bug matters on other operating systems too.
An important question that will require some testing is: Is the stack size we get actually big enough on all platforms? On Android I'm getting a size just below 1 MiB. If that's the smallest we're dealing with, I'd be fine simply with dropping the requirement to that (which is what I've done for now), but if there are platforms that give us an even smaller stack, we should probably request a bigger stack in some way.