merryhime/dynarmic

InvalidateCacheRange does not update fast dispatch table

comex opened this issue · 4 comments

comex commented

I'm not sure how easy to reproduce this is, but while running Super Mario Maker 2 in yuzu 6c72b7f01109 / dynarmic 6.0.1 on Linux x86-64, using the new GDB stub, I ran into a situation where, for certain instruction addresses:

  • Consistently, either Yuzu fails to stop on a breakpoint I have set there (even though I know the game is repeatedly executing that address), or Yuzu stops on the breakpoint despite me having previously disabled the breakpoint (causing gdb to print Thread 1 "MainThread" received signal SIGTRAP, Trace/breakpoint trap).
  • Repeatedly enabling and disabling the breakpoint does not help.
  • I verified that Jit::Impl::InvalidateCacheRange is being called properly after I enable or disable the breakpoint.
  • It turns out that I can temporarily fix the situation, using a debugger attached to yuzu, by either:
    • Setting invalidate_entire_cache to true in the Jit::Impl object; or
    • Manually calling ClearFastDispatchTable().
      By "temporarily", I mean that it prevents the misbehavior until the next time I enable or disable the breakpoint, at which point Yuzu again fails to recognize the change.

From a quick code inspection, it seems like the entry in the fast dispatch table for the old JIT block is not being cleared. A64EmitX64::Unpatch would do so, but nothing ever calls that. EmitX64::InvalidateBasicBlocks may call Unpatch, but since Unpatch is not virtual, it only calls the base EmitX64::Unpatch. However, I'm not sure whether Unpatch is the right place to do this anyway, since EmitX64::InvalidateBasicBlocks only calls it if the address has entries in patch_information, which seems unrelated to the fast dispatch table. (However, I could easily be missing something as I haven't spent too much time looking at this code).

cc @liamwhite

Thanks for digging into this.

Unpatch is not virtual

Unpatch is virtual.

since EmitX64::InvalidateBasicBlocks only calls it if the address has entries in patch_information

This is the logic error here.

Will push a fix.

Reproduced in a small test case, unfortunately the bug seems to be more complex than above. Looking into this.

TEST_CASE("ensure fast dispatch entry is cleared even when a block does not have any patching requirements", "[a64]") {
    A64TestEnv env;

    A64::UserConfig conf{&env};
    A64::Jit jit{conf};

    REQUIRE(conf.HasOptimization(OptimizationFlag::FastDispatch));

    env.code_mem_start_address = 100;
    env.code_mem.clear();
    env.code_mem.emplace_back(0xd2800d80);  // MOV X0, 108
    env.code_mem.emplace_back(0xd61f0000);  // BR X0
    env.code_mem.emplace_back(0xd2800540);  // MOV X0, 42
    env.code_mem.emplace_back(0x14000000);  // B .

    jit.SetPC(100);
    env.ticks_left = 4;
    jit.Run();
    REQUIRE(jit.GetRegister(0) == 42);

    jit.SetPC(100);
    env.ticks_left = 4;
    jit.Run();
    REQUIRE(jit.GetRegister(0) == 42);

    jit.InvalidateCacheRange(108, 4);

    jit.SetPC(100);
    env.ticks_left = 4;
    jit.Run();
    REQUIRE(jit.GetRegister(0) == 42);

    env.code_mem[2] = 0xd28008a0;  // MOV X0, 69

    jit.SetPC(100);
    env.ticks_left = 4;
    jit.Run();
    REQUIRE(jit.GetRegister(0) == 42);

    jit.InvalidateCacheRange(108, 4);

    jit.SetPC(100);
    env.ticks_left = 4;
    jit.Run();
    REQUIRE(jit.GetRegister(0) == 69);

    jit.SetPC(100);
    env.ticks_left = 4;
    jit.Run();
    REQUIRE(jit.GetRegister(0) == 69);
}

Found a second long-standing bug in the fast-dispatch lookup code.

Potential fix in cd85b7f, please confirm.