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

Exception handler - catch bad memory accesses by the JIT #11795

Merged
merged 17 commits into from
Jul 14, 2020

Conversation

hrydgard
Copy link
Owner

@hrydgard hrydgard commented Feb 12, 2019

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:

  • Does not seem to be catching exceptions on ARM 32-bit?? or my bad test write accidentally went through and overwrote something in the crowded memory space.
  • Bad reads should return 0. Or maybe some other value, not sure - right now they effectively just return whatever was in the destination register, as the instruction is simply skipped.
  • SIMD instructions are not handled in the x86 analyze function. It also is very ugly and buggy in general, needs a rewrite, or maybe we can use udis.

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.

@hrydgard hrydgard added armjit Occurs with JIT on 32-bit ARM devices, but not another CPU backend. arm64jit Occurs with JIT on 64-bit ARM devices, but not another CPU backend. x86jit x86/x64 JIT bugs labels Feb 12, 2019
@hrydgard hrydgard added this to the v1.8.0 milestone Feb 12, 2019
Copy link
Collaborator

@unknownbrackets unknownbrackets left a 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);
Copy link
Collaborator

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]

Copy link
Owner Author

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.

Copy link
Collaborator

@unknownbrackets unknownbrackets Feb 17, 2019

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]

Common/ExceptionHandlerSetup.cpp Show resolved Hide resolved
UI/EmuScreen.cpp Outdated
@@ -1347,6 +1376,11 @@ void EmuScreen::renderUI() {
DrawProfile(*ctx);
}
#endif

if (coreState == CORE_ERROR) {
Copy link
Collaborator

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]

Copy link
Owner Author

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?

Copy link
Collaborator

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]

Copy link
Owner Author

@hrydgard hrydgard Feb 18, 2019

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.

UI/EmuScreen.cpp Show resolved Hide resolved
Common/MachineContext.h Outdated Show resolved Hide resolved
Core/MemMap.cpp Outdated Show resolved Hide resolved
Core/MemMap.cpp Outdated Show resolved Hide resolved
@unknownbrackets
Copy link
Collaborator

Looks like it's still gregs on Linux/x86_32. I guess iOS should use the USE_SIGACTION_ON_APPLE path?

-[Unknown]

@hrydgard
Copy link
Owner Author

hrydgard commented Feb 25, 2019

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.

@hrydgard hrydgard modified the milestones: v1.8.0, v1.9.0 Feb 25, 2019
@hrydgard hrydgard modified the milestones: v1.9.0, v1.10.0 Aug 6, 2019
@Panderner
Copy link
Contributor

Does Tony Hawk's Underground 2 and MTX Mototrax helps this issue?

@hrydgard
Copy link
Owner Author

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...

@hrydgard hrydgard modified the milestones: v1.10.0, v1.11.0 May 10, 2020
@hrydgard
Copy link
Owner Author

hrydgard commented Jul 4, 2020

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.

@hrydgard
Copy link
Owner Author

hrydgard commented Jul 12, 2020

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 bIgnoreBadMemAccess . Pretty sure its current form is a bad idea.

Exactly what kind of reports we will send to report.ppsspp.org remains to be designed. But there sure is plenty of information we could collect.

@hrydgard hrydgard changed the title WIP: Exception handler - catch bad memory access by the JIT Exception handler - catch bad memory accesses by the JIT Jul 13, 2020
@hrydgard
Copy link
Owner Author

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.

@hrydgard hrydgard merged commit fe91f62 into master Jul 14, 2020
@hrydgard hrydgard deleted the exception-handler branch July 14, 2020 11:35
int sicode = info->si_code;
if (sicode != SEGV_MAPERR && sicode != SEGV_ACCERR) {
// Huh? Return.
return;
Copy link
Collaborator

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.
Copy link
Collaborator

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]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arm64jit Occurs with JIT on 64-bit ARM devices, but not another CPU backend. armjit Occurs with JIT on 32-bit ARM devices, but not another CPU backend. x86jit x86/x64 JIT bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants