-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Exception handler - catch bad memory accesses by the JIT #11795
Conversation
6bd2a06
to
276a0de
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bad reads returning 0 at least matches fast memory off, so I think it is a good plan.
Travis seems to have still failed with actual failures, like error: unknown type name 'kern_return_t'
on iOS, No context definition for architecture
on x86 Android?, and a link error on x86_64 (doesn't have the x86 disassembler - would be the old mk file.)
-[Unknown]
if (coreParameter.updateRecent) { | ||
g_Config.AddRecent(filename); | ||
} | ||
|
||
InstallExceptionHandler(&Memory::HandleFault); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it okay that this may occur on a different thread than the CPU thread (depending on Vulkan vs OpenGL, I think?)
-[Unknown]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe all these methods are process-global, I know the Windows path is, but hm, maybe a concern indeed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it's probably only a concern for thread_set_exception_ports. I think for Mac it'll always be on the CPU thread (I guess it's safe on Vulkan too anyway.)
Checked and the threaded CPU startup is after this line.
-[Unknown]
UI/EmuScreen.cpp
Outdated
@@ -1347,6 +1376,11 @@ void EmuScreen::renderUI() { | |||
DrawProfile(*ctx); | |||
} | |||
#endif | |||
|
|||
if (coreState == CORE_ERROR) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, we currently also use CORE_ERROR for LoadExec changes (some games switch executable) that fail, and also game load fail. We should probably make sure these still do what they're supposed to do...
-[Unknown]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively we should assign a new coreState? CORE_RUNTIME_ERROR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. Or maybe even CORE_ERROR_CRASHDUMP or something to indicate plainly that it's the sort of error a crashdump makes sense for?
I guess that would be all runtime errors, though, except maybe LoadExec swaps of the binary mid-game.
-[Unknown]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think CORE_ERROR_RUNTIME would be specific enough for now. I'll fix it up in a bit. And if we want to split it further in the future we can.
Looks like it's still gregs on Linux/x86_32. I guess iOS should use the USE_SIGACTION_ON_APPLE path? -[Unknown] |
Once I've gotten 1.8.0 out and stable, that's definitely one of the first things I want to look at. Got some ideas for a hyper-low-latency, so-so quality time stretcher that would probably be a better option for emulators than the usual libraries. But please don't post such requests on completely different issues - post on the original one instead, I see them all. Regarding this particular one, I think it may have to wait, too much work required to get it good enough to make 1.8.0. |
Does Tony Hawk's Underground 2 and MTX Mototrax helps this issue? |
No. This is so that we can catch game crashes gracefully and automatically report them, with as many details as we can get, in the future. However it is super complicated to get right, I'm gonna need to sit down a week or so someday and finish this... |
Rebasing this, but haven't started digging again yet. Note to self: I've been thinking more about this, and the case where we simply stop executing and don't offer a way to continue (but do for example offer approximate crash reporting and prevent the whole app from exiting) is actually rather easy. We can just set CoreState = ERROR and set PC directly to a suitable location in the dispatcher before returning from the handler, instead of carefully trying to disassemble and skip the offending instruction. We can thus fall out of execution safely (the state of the registers will be trash but that's just the way it has to be). This part is implemented. This way we don't need to fully implement x64Analyzer etc support to get this to a useful state - we'll simply not offer a recovery attempt option for cases when a game crashes with an instruction it doesn't understand... To get this super neat and clean though and to know exactly which MIPS instruction we crashed on, much more advanced register state handling will be necessary. |
942f8c6
to
9341d6c
Compare
9341d6c
to
d3db7cc
Compare
Alright, little more testing and it will finally be time to get this merged. Rebased and ready. I've scaled back the plans a little - for now this will always kill the running game instead of trying to recover by returning 0 or something. It would be cool to keep stepping from a crash - but if you really need that, turn off Fast Memory and it will work. Related to that, I want to kill or at least do something about Exactly what kind of reports we will send to |
… the PSP memory space.
…BadMemAccess is false). Add setting for ignore bad memory accesses
55adff0
to
f6cc45a
Compare
Finally a green build. Here we go. I'm gonna fix it up on Windows/ARM in a separate PR, just realized that it's probably broken. |
int sicode = info->si_code; | ||
if (sicode != SEGV_MAPERR && sicode != SEGV_ACCERR) { | ||
// Huh? Return. | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be calling the old handler in this case?
-[Unknown]
@@ -533,19 +533,16 @@ bool HandleFault(uintptr_t hostAddress, void *ctx) { | |||
// It was a read. Fill the destination register with 0. | |||
// TODO | |||
} | |||
// Move on to the next instruction. | |||
// Move on to the next instruction. Note that handling bad accesses like this is pretty slow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could set a flag to rebuild jit with slow memory, either for this block or all blocks. Might be faster overall.
-[Unknown]
NOTE: See 2020 Update below.
Installs an exception handler that filters out exceptions from the JIT and let the emulated executable continue running (or bail out cleanly, if set not to ignore bad memory accesses).
The main idea is to prevent game crashes from crashing the emulator, even with fast-memory enabled. This will hopefully get rid of a lot of bad callstacks from the Google Play crash reporting, and improve the user experience with bad ISOs and whatever.
This is the last new major feature that will make the cut for 1.8.0. Other than this, it's just bugfixes until its out.A bit of the exception handling code is taken from Dolphin. It's also GPL though, and I originally wrote that code anyway so it's fine :)
A few things remain:
Executable that does a bad write:
crash.zip
2020 update
Rebased this on master, now it has the lovely bluescreens from #13092.
Not yet tested on ARM32.
Note that it's a little confusing when running from VS' debugger - first the debugger will catch the exception. Just press F5 and it will continue to the handler.