chore: Add unit tests for arm64 disassembler - #3245
Conversation
AI review
Accumulator bugs
Tests that can't fail
Minor
It looks like it found some possible bugs in the accumulator (I did not verify myself). Fine if you want to fix them here, or defer for out-of-scope. |
7aa8d2b to
52dc08c
Compare
This PR intended to add tests to verify existing arm64 disassembler behaviors.
|
|
Findings are all in the new test code; the production changes look good (the formatter padding is a real fix — a mnemonic reaching
Reviewed with Claude Code. |
84d7a22 to
a01d20d
Compare
a01d20d to
2fdfa41
Compare
2fdfa41 to
025c00a
Compare
timcassell
left a comment
There was a problem hiding this comment.
Follow-up on the updated head. The fixes from the last round all look right — the macOS-arm64 address, WithStrictOrdering, the duplicate MOVZ row, DmbIshLdInstr, and the Decode.cs comments. A few items are still open, plus one new one and one follow-up note.
Reviewed with Claude Code.
| protected static IDataReader CreateMockDataReader(uint[] rawInstructions, Func<ulong, ulong> getPointer) | ||
| { | ||
| return new MockDataReader( | ||
| read: (address, buffer) => |
There was a problem hiding this comment.
Still open from the last round: the read delegate ignores its address parameter and always serves rawInstructions from index 0, so the address the disassembler reads from is never asserted for TryReadStubHead, TryFollowJumpTrampoline, or the multi-hop loop. Mutating the trampoline read to dataReader.Read(address + 0x1000, head) leaves the whole suite green.
Keying the delegate off address — serve the bytes at a registered base, return 0 elsewhere — would close it.
| internal const ulong DummyBaseAddress = 0x0000_F000_0000_0000UL; | ||
|
|
||
| internal static readonly Version DummyTargetFrameworkVersion = new Version(10, 0); | ||
| internal static readonly MockClrMethod DummyMethodNotUsed = default!; |
There was a problem hiding this comment.
Still open: DummyMethodNotUsed = default! is a null IClrMethod, passed as the non-nullable currentMethod argument in TryTranslateAddressToName_GetJitHelperFunctionName_ReturnsNonEmptyValue and _NoAlignedAddress. Those pass only because production returns before reaching method.NativeCode == currentMethod.NativeCode (ClrMdDisassembler.cs:343). If that early-return ordering ever changes, they fail with a NullReferenceException instead of a readable assertion. A real MockClrMethod instance would be safer.
| }; | ||
| PrintInstructions(rawInstructions); | ||
|
|
||
| var clrRuntime = CreateMockClrRuntime(rawInstructions, address => |
There was a problem hiding this comment.
Raised in the earlier review and still present (also :102 and :131): the getPointer callback returns ExpectedResultAddress for any address, so the slot arithmetic these paths exist for is never asserted. Changing dataReader.ReadPointer(countSlot + 8, ...) to countSlot in the CallCountingStub branch, or following off1/offB instead of off0/offA, keeps all three tests green.
The sibling TryResolvePrecode_* tests already assert address.Should().Be(mdSlot) inside the callback — same assertion belongs here.
| private readonly TryReadPointerDelegate _tryReadPointer = (_, out _) => throw new InvalidOperationException($"{nameof(_tryReadPointer)} field is not set."); | ||
|
|
||
|
|
||
| public MockDataReader(ulong dummyValue = 0) |
There was a problem hiding this comment.
This overload sets only _tryReadPointer, leaving _read as the throwing default. It backs DummyClrRuntime and Arm64InstructionFormatterTests.GetArm64Asms via CreateMockClrRuntime(ulong), and GetArm64Asms runs the full Decode loop — so the first test case added there containing an indirect branch (e.g. movz/br) will hit FlushCachedDataIfNeeded -> Read and die with InvalidOperationException: _read field is not set. rather than an assertion failure.
Delegating to CreateMockDataReader([], _ => dummyValue) would give it a working Read.
| var rawInstructions = new[] | ||
| { | ||
| Arm64InstructionFactory.DMB(Arm64BarrierOperationLimitKind.ISHLD), // dmb ishld | ||
| Arm64InstructionFactory.LDR(X10, 0x10000), // ldr x11, #0x10000 |
There was a problem hiding this comment.
Register comments still mismatch the code here and on :69 (BR(X10) commented br x11) and :125 (LDR(X9, ...) commented ldr x11), plus Arm64DisassemblerTests.TryResolvePreCode.cs:20,22. Decode.cs was fixed in the last push; these were missed.
Worth getting right because the register number is exactly what distinguishes StubPrecode (x10/x12) from FixupPrecode (x11/x12) — someone "fixing" code to match a comment here would break stub recognition.
| target.Should().Be(ExpectedResultAddress); | ||
| } | ||
|
|
||
| // TryFollowJumTrampoline seems not support FixupPrecode with pre-backpatch form. |
There was a problem hiding this comment.
Follow-up rather than something to fix here, in the same bucket as the other pre-existing issues you deferred: this test locks in a real asymmetry — TryResolvePrecode handles the pre-backpatch FixupPrecodeCode_Fixup shape (Arm64Disassembler.cs:233) but TryFollowJumpTrampoline does not. A direct BL onto a fixup precode that has never been backpatched therefore fails the trampoline chase, GetMethodByHandle(stubAddress) then fails because the address is the stub rather than the MethodDesc, and the call target is left untranslated in the disassembly.
Pre-existing (from #3208) and out of scope, but a TODO or issue link would read better than the bare "seems not support" comment, which makes it look intentional. (Also a typo there: TryFollowJumTrampoline.)
This PR contains following changes.
1. Cleanup arm64 disassembler related code to preparing to add unit tests
See following PR comment for details.
2. Add AsmArm64 package reference
AsmArm64 package to unit test project.
Currently it's used for test purpose.
It's expected existing arm64 disassembler is replaced to
AsmArm64based implementation. (#3246)3. Add arm64 disassembler related unit tests.
To ensure existing arm64 disassembler behavior.
Unit test codes are added for major code paths. (It can confirm code coverage results with
Analyze Code Coverageon VS)Note:
Almost of unit tests on .NET Framework are excluded by
#if NETdirective.UnsafeAccessor(It requires .NET 8 or later)Libraryproject)