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

Jit64: Properly handle backpatching overflowed address calculations #11445

Merged

Conversation

JosJuice
Copy link
Member

Previously we would only backpatch overflowed address calculations if the overflow was 0x1000 or less. Now we can handle the full 2 GiB of overflow in both directions.

I'm also making equivalent changes to JitArm64's code. This isn't because it needs it – JitArm64 address calculations should never overflow – but because I wanted to get rid of the 0x100001000 inherited from Jit64 that makes even less sense for JitArm64 than for Jit64.

@AdmiralCurtiss
Copy link
Contributor

This looks pretty correct to me. The only thing I'm wondering is whether there's a chance that the current MSR.DR is different from the expected address space -- ie, the code was trying a physical access but is in logical mode or vice-versa. I think our JIT assumes that the MSR.DR at compile time will be the same as at runtime, so this could happen, though I'm not sure if it ever realistically does.

Oh, and you probably have to rebase this because MSR.DR takes a parameter now.

@JosJuice
Copy link
Member Author

JosJuice commented Feb 12, 2023

Rebased.

Yes, the JIT seems to be making some assumptions about MSR.DR. For instance, take a look at this piece of code:

// If the address is known to be RAM, just load it directly.
if (m_jit.jo.fastmem_arena && PowerPC::IsOptimizableRAMAddress(address))
{
UnsafeLoadToReg(reg_value, Imm32(address), accessSize, 0, signExtend);
return;
}

If MSR.DR is 1 at compile time, the address is known at compile time, and the address is valid at compile time, the memory access is implemented using UnsafeLoadToReg. If the jitted code then later runs with MSR.DR set to 0, then... perhaps the address is still valid, or perhaps it isn't. If it isn't, Dolphin crashes without making any attempt to backpatch, at least to the best of my understanding.

But if you use SafeLoadToReg instead of UnsafeLoadToReg (for any memory access, not just the specific case described above), Dolphin does try to backpatch, and triggers the code added by this PR. This code will treat the access as being in bounds of the current memory region (as indicated by the current value of MSR.DR), and will then backpatch it to a call to C++ that works correctly regardless of what MSR.DR is set to. (I have not tested this).

@JosJuice
Copy link
Member Author

Actually nevermind, on Jit64 specifically the backpatched code doesn't work correctly with a changed MSR.DR due to this part:

FixupBranch exit;
const bool dr_set = (flags & SAFE_LOADSTORE_DR_ON) || PowerPC::ppcState.msr.DR;
const bool fast_check_address = !slowmem && dr_set && m_jit.jo.fastmem_arena;
if (fast_check_address)
{
FixupBranch slow = CheckIfSafeAddress(R(reg_value), reg_addr, registersInUse);
UnsafeLoadToReg(reg_value, R(reg_addr), accessSize, 0, signExtend);
if (m_far_code.Enabled())
SwitchToFarCode();
else
exit = J(true);
SetJumpTarget(slow);
}

But either way, the problems with MSR.DR handling are in existing JIT code and not in the backpatching handler.

Previously we would only backpatch overflowed address calculations
if the overflow was 0x1000 or less. Now we can handle the full 2 GiB
of overflow in both directions.

I'm also making equivalent changes to JitArm64's code. This isn't because
it needs it – JitArm64 address calculations should never overflow – but
because I wanted to get rid of the 0x100001000 inherited from Jit64 that
makes even less sense for JitArm64 than for Jit64.
@JosJuice
Copy link
Member Author

Also, just to make it explicit since it's important context for what I wrote above: JitAsm.cpp ensures that RMEM/MEM_REG is updated to match MSR.DR before entering a JIT block.

@AdmiralCurtiss AdmiralCurtiss merged commit 34a459b into dolphin-emu:master Feb 14, 2023
14 checks passed
@JosJuice JosJuice deleted the jit64-wraparound-backpatch branch February 14, 2023 18:20
@JosJuice
Copy link
Member Author

JosJuice commented Apr 7, 2023

Just for reference, I figured out how MSR.DR is handled now. JitCache.cpp ensures that if MSR.DR or MSR.IR has been changed since the block was compiled, the existing block won't run. So there is no problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants