-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
JitArm64: Do downcount immediately before jumping to dispatcher #9299
Conversation
Fixes https://bugs.dolphin-emu.org/issues/12327. When we started using fmt in CheckExternalExceptions, JitArm64 mysteriously stopped working even though the code path where fmt was used never was reached. This is because the compiler added a function prologue and epilogue to set up the stack, since the code path that used fmt required the use of the stack. However, the breakage didn't actually have anything to do with the usage of the stack in itself, but rather with the compiler's insertion of a stack canary. In the function epilogue, a cmp instruction was inserted to check that the stack canary had not been overwritten during the execution of the function. This cmp instruction overwriting the status flags ended up having a disastrous side effect once execution returned to code emitted by JitArm64::WriteExceptionExit. JitArm64's dispatcher contains a branch to the "do_timing" code which is intended to be taken if the PPC downcount is negative. However, the dispatcher doesn't update the status flags on its own before this conditional branch, but rather expects the calling code to have set them as a side effect of DoDownCount. The root cause of our bug was that JitArm64::WriteExceptionExit was calling DoDownCount before Check(External)Exceptions instead of after.
JitArm64::DoJit() emits a branch to the dispatcher without setting up the status flags. Should this be replaced with dispatcher_no_check?
|
A more open-ended question: JitArm64BlockCache::WriteLinkBlock() can also emit branches to the dispatcher. Are the status flags guaranteed to be in the right state when it does? |
The flags are not set correctly for a call to the version of the dispatcher which does have a check. Jit64 uses dispatcher_no_check here.
Yes, I believe so. I've added a commit that does this.
I don't think I understand how block linking works well enough to answer this. |
Yeah, I started looking at it and quickly realized I'd need to set aside more time if I wanted to really understand it. :D That said, this change seems like a solid improvement as-is. |
re |
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.
Nice catch.
Fixes https://bugs.dolphin-emu.org/issues/12327.
When we started using fmt in CheckExternalExceptions, JitArm64 mysteriously stopped working even though the code path where fmt was used never was reached. This is because the compiler added a function prologue and epilogue to set up the stack, since the code path that used fmt required the use of the stack.
However, the breakage didn't actually have anything to do with the usage of the stack in itself, but rather with the compiler's insertion of a stack canary. In the function epilogue, a cmp instruction was inserted to check that the stack canary had not been overwritten during the execution of the function. This cmp instruction overwriting the status flags ended up having a disastrous side effect once execution returned to code emitted by JitArm64::WriteExceptionExit.
JitArm64's dispatcher contains a branch to the "do_timing" code which is intended to be taken if the PPC downcount is negative. However, the dispatcher doesn't update the status flags on its own before this conditional branch, but rather expects the calling code to have set them as a side effect of DoDownCount. The root cause of our bug was that JitArm64::WriteExceptionExit was calling DoDownCount before Check(External)Exceptions instead of after.