mirror of
https://github.com/ARMSX2/ARMSX2.git
synced 2026-08-24 16:50:16 -07:00
Fix: the recompiler-test GIF Path 1 sink missed a wrapped XGKICK's head
`mVU_XGKICK_` splits a packet that runs past the top of VU1 memory in two: the pre-wrap head goes to `Gif_Path::CopyGSPacketData`, and only the post-wrap tail goes to `Gif_Unit::TransferGSPacketData`. The PCSX2_RECOMPILER_TESTS sink hooked only the second one, so a wrapped kick was captured as its tail alone — no GIFtag at all. Measured while landing the console XGKICK cases: 64 captured bytes from the recompiler against 112 from the interpreter, which loops through TransferGSPacketData and never calls CopyGSPacketData. That reads exactly like a serious microVU miscompile and is entirely our instrumentation; both engines emit the same correct stream. CopyGSPacketData now feeds the same sink and skips the ring, since nothing drains it while the sink is installed. The gif_test_hooks declaration moves above Gif_Path so the member function can see it. Consequence, which is larger than the bug: before this, no test could observe a wrapped XGKICK's GIFtag. Every XGKICK test used packets that fit inside VU1 memory, so the blind spot never showed. Test-build only — the whole block is inside #ifdef PCSX2_RECOMPILER_TESTS. vu1_xgkick_drain_tests.cpp moves with it. That file is ours and postdates the branch this came from, and its wrap case was written against the blind spot: it asserted the JIT capture was the 32-byte tail, with a header comment explaining that the head was unreachable. Both halves now arrive, so it asserts the whole 48 bytes — head carrying the GIFtag, tail resuming at offset 0 — which is the stronger property and the one that was never testable before. The XgKickHack wrap case already asserted that shape and only loses a stale "unlike the non-hack path" aside. Idea by pstef.
This commit is contained in:
committed by
Brian Degenhardt
parent
63e7904e55
commit
9eb45cf590
+31
-15
@@ -193,6 +193,22 @@ static __fi void incTag(u32& offset, u32& size, u32 incAmount)
|
||||
offset += incAmount;
|
||||
}
|
||||
|
||||
#ifdef PCSX2_RECOMPILER_TESTS
|
||||
namespace gif_test_hooks
|
||||
{
|
||||
// When non-null, the GIF Path 1 byte stream is appended to *g_path1_sink
|
||||
// instead of entering the path-1 ring buffer + MTGS::WaitGS(), which
|
||||
// asserts when no GS thread is running. VuTestHarness installs/clears
|
||||
// this pointer. Both entry points feed it — see Gif_Path::CopyGSPacketData
|
||||
// and Gif_Unit::TransferGSPacketData.
|
||||
extern std::vector<u8>* g_path1_sink;
|
||||
|
||||
// When true, Gif_Unit::checkPaths(p1=true, ...) reports path 1 as busy.
|
||||
// Used by EeVu1Vif's Mscalf-stall test to force the GIF-busy code path.
|
||||
extern bool g_force_path1_busy;
|
||||
}
|
||||
#endif
|
||||
|
||||
struct Gif_Path_MTVU
|
||||
{
|
||||
u32 fakePackets; // Fake packets pending to be sent to MTGS
|
||||
@@ -314,6 +330,21 @@ struct Gif_Path
|
||||
|
||||
void CopyGSPacketData(u8* pMem, u32 size, bool aligned = false)
|
||||
{
|
||||
#ifdef PCSX2_RECOMPILER_TESTS
|
||||
if (gif_test_hooks::g_path1_sink && idx == GIF_PATH_1)
|
||||
{
|
||||
// mVU's XGKICK wrap path (mVU_XGKICK_) copies the pre-wrap head
|
||||
// of the packet straight into this buffer and hands only the
|
||||
// post-wrap tail to TransferGSPacketData. Capturing just the
|
||||
// latter made a wrapped kick look like a headless packet with no
|
||||
// GIFtag — a harness blind spot, not a divergence. Feed the sink
|
||||
// here too, and skip the ring entirely: with the sink installed
|
||||
// nothing ever drains it.
|
||||
gif_test_hooks::g_path1_sink->insert(
|
||||
gif_test_hooks::g_path1_sink->end(), pMem, pMem + size);
|
||||
return;
|
||||
}
|
||||
#endif
|
||||
if (curSize + size > buffSize)
|
||||
{ // Move gsPack to front of buffer
|
||||
GUNIT_LOG("CopyGSPacketData: Realigning packet!");
|
||||
@@ -526,21 +557,6 @@ struct Gif_Path
|
||||
}
|
||||
};
|
||||
|
||||
#ifdef PCSX2_RECOMPILER_TESTS
|
||||
namespace gif_test_hooks
|
||||
{
|
||||
// When non-null, Gif_Unit::TransferGSPacketData(GIF_TRANS_XGKICK, ...)
|
||||
// appends the packet bytes to *g_path1_sink and returns size — bypassing
|
||||
// the path-1 ring buffer + MTGS::WaitGS() that asserts when no GS thread
|
||||
// is running. VuTestHarness installs/clears this pointer.
|
||||
extern std::vector<u8>* g_path1_sink;
|
||||
|
||||
// When true, Gif_Unit::checkPaths(p1=true, ...) reports path 1 as busy.
|
||||
// Used by EeVu1Vif's Mscalf-stall test to force the GIF-busy code path.
|
||||
extern bool g_force_path1_busy;
|
||||
}
|
||||
#endif
|
||||
|
||||
struct Gif_Unit
|
||||
{
|
||||
Gif_Path gifPath[3];
|
||||
|
||||
@@ -76,11 +76,11 @@ void WriteEopOnlyTag(VuTestHarness& h, u32 addr)
|
||||
//
|
||||
// Note on what the capture can see: the split's FIRST half goes out through
|
||||
// Gif_Path::CopyGSPacketData and the second through
|
||||
// Gif_Unit::TransferGSPacketData. Only the latter is hooked by the test
|
||||
// sink, so the JIT's captured stream is the tail alone. That is a property
|
||||
// of the capture, not of the transfer — and the tail is precisely what pins
|
||||
// the seam arithmetic, since it must be exactly `size - diff` bytes taken
|
||||
// from offset 0.
|
||||
// Gif_Unit::TransferGSPacketData. Both feed the test sink, so the JIT's
|
||||
// captured stream is the whole packet — head, then tail resuming at offset 0.
|
||||
// It used to hook only the latter, which made a wrapped kick look like a
|
||||
// headless packet carrying no GIFtag at all; the seam arithmetic was then the
|
||||
// only thing this could pin, and the tag half was invisible.
|
||||
// =========================================================================
|
||||
|
||||
TEST(Vu1XgkickDrain, PacketWrappingTheTopOfVu1MemorySplitsAtTheSeam)
|
||||
@@ -114,16 +114,22 @@ TEST(Vu1XgkickDrain, PacketWrappingTheTopOfVu1MemorySplitsAtTheSeam)
|
||||
h.Run();
|
||||
|
||||
const std::vector<u8>& jit = h.Path1PacketBytesJit();
|
||||
ASSERT_EQ(jit.size(), kTailBytes)
|
||||
<< "the wrapped half must be exactly (packet size - distance to the top "
|
||||
"of VU1 memory) bytes";
|
||||
ASSERT_EQ(jit.size(), kPacketBytes)
|
||||
<< "both halves of the split must reach the sink: the head up to the top "
|
||||
"of VU1 memory, then the wrapped remainder";
|
||||
|
||||
std::vector<u8> expected(kTailBytes);
|
||||
std::memcpy(expected.data(), &vuRegs[1].Mem[0], kTailBytes);
|
||||
EXPECT_EQ(jit, expected) << "the wrapped half must resume at VU1 memory offset 0";
|
||||
// Head is `kDiff` bytes read from the tag address; the tail is the rest,
|
||||
// taken from offset 0. A tail lifted from the wrong offset shows up as
|
||||
// content, not merely as a length.
|
||||
std::vector<u8> expected(kPacketBytes);
|
||||
std::memcpy(expected.data(), &vuRegs[1].Mem[kTagAddr], kDiff);
|
||||
std::memcpy(expected.data() + kDiff, &vuRegs[1].Mem[0], kTailBytes);
|
||||
EXPECT_EQ(jit, expected)
|
||||
<< "the head must carry the GIFtag and the wrapped half must resume at "
|
||||
"VU1 memory offset 0";
|
||||
|
||||
// The interpreter drains the same packet in two metered steps and pushes
|
||||
// both through the hooked entry point, so it sees the whole 48 bytes.
|
||||
// The interpreter drains the same packet in two metered steps, so it
|
||||
// reaches the same 48 bytes by a different route.
|
||||
EXPECT_EQ(h.Path1PacketBytesInterp().size(), kPacketBytes);
|
||||
}
|
||||
|
||||
@@ -192,9 +198,10 @@ TEST(Vu1XgkickDrain, XgKickHackDrainsAWrappingPacketInTwoChunks)
|
||||
});
|
||||
h.Run();
|
||||
|
||||
// Unlike the non-hack path, both halves go out through the hooked entry
|
||||
// point here, so the capture is the whole packet: the tag from the top of
|
||||
// memory followed by the two payload qwords from offset 0.
|
||||
// The capture is the whole packet: the tag from the top of memory followed
|
||||
// by the two payload qwords from offset 0. The hack reaches that through
|
||||
// the metered loop rather than through the split above, which is the point
|
||||
// of running the wrap under it as well.
|
||||
const std::vector<u8>& jit = h.Path1PacketBytesJit();
|
||||
ASSERT_EQ(jit.size(), kPacketBytes);
|
||||
|
||||
|
||||
Reference in New Issue
Block a user