Repository navigation
fix(relinker): honor prefixes in the syscall number byte walk - #10537
oneandonlydean merged 2 commits into
Conversation
The raw-byte walk behind the refusal message ignored the prefixes of the constant rax loads it recognizes. mov r8d, imm32 (41 B8) and xor r8d, eax (41 31 C0) reported the immediate or zero as the syscall number, xor ax, ax (66 31 C0) reported a zeroed rax, and mov r8b, imm8 (41 B0) misaligned the walk so an established number was not reported. An active REX prefix is only the byte immediately before the opcode, and a REX byte earlier in the prefix run is dead, so 41 66 B8 is a mov ax, imm16 with one dead byte in front. The B8 arm accepts a REX without W or B, since rex.R and rex.X do not change the register, and rejects an operand-size override; the xor arm rejects a 16-bit pair and the REX bit that extends the destination, B for the 31 form and R for the 33 form; the B0 arm treats REX.B as a different register and walks past the three bytes; the 66 B8 arm keeps the dead REX byte in the walked span. Test the reported and skipped byte patterns on every target. Generated by Mistral Vibe. Co-Authored-By: Mistral Vibe <vibe@mistral.ai>
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed the exact head 79fef78d01c7b5da0ae71e4f9bb45e8709915734. The prefix-aware backward walk correctly distinguishes the reported REX and operand-size cases, including REX.B byte writes and the dead REX before 66 B8. The relinker-only Release build completed with GCC 12.2 (cmake -S . -B /tmp/anyps5-review-10537/build -G Ninja -DCMAKE_BUILD_TYPE=Release -DBUILD_TESTING=ON -DANYPS5_RELINKER_ONLY=ON, then target relinker), and ctest -R sce_code_gaps passed (1/1). git diff --check upstream/main...HEAD is clean. Hosted conventions, intake, and label checks pass. I did not run the full project build or non-Linux suites locally.
|
Reading this alongside #10439: Written with AI assistance (Claude Code). I ran the checks above on my Windows 11 machine. |
MarcZX10
left a comment
There was a problem hiding this comment.
I am revising my earlier approval after the new review discussion and an independent check of the REX/ModRM fields.
At the current head 79fef78d01c7b5da0ae71e4f9bb45e8709915734, the 31 C0 / 33 C0 branch still accepts 44 31 C0 and 41 33 C0 as zeroing RAX. Both encode xor eax, r8d: opcode 31 /r uses ModRM r/m as destination and reg as source; 33 /r uses reg as destination and r/m as source. REX.R extends the reg field and REX.B extends r/m, so these prefixes select R8D as the source, not EAX as both operands. The result depends on R8D and cannot be reported as syscall number 0. Intel's SDM lists these operand directions and REX field extensions.
Please reject these two cases in ConstantRaxSetEndingAt, move their fixtures from the reported-number group to the no-number/skipped group, and correct the PR body. #10439's decoded path already declines both encodings, so this also restores agreement between the two paths. The current tests explicitly expect the wrong number, making this a correctness issue rather than only a documentation discrepancy.
|
Thanks for the careful re-check. You're right. I've prepared a correction to the XOR recognizer and added regression coverage for both invalid encodings, including a case where the XOR invalidates a previously loaded EAX value. The focused tests and convention checks pass with the correction. Since the fix depends on the prefix-aware scanner changes in this PR, and the contribution guide requires follow-up work in a new PR, would you prefer that I submit it as a declared dependent PR now, or wait until #10537 merges and then open a separate PR against updated |
|
Thanks. The fix belongs in this PR rather than a follow-up: the two test rows that assert the wrong value are introduced here, and MarcZX10's change request asks for the same correction, so the branch should not merge with them in place. The "follow-ups go in a new PR" rule is for new work, not for correcting what a PR itself adds. Please push the correction to this branch (a second commit or an amend, whichever you prefer), including the regression where the XOR invalidates a previously loaded EAX, and update the body sentence about |
Generated by Mistral Vibe. Co-Authored-By: Mistral Vibe <vibe@mistral.ai>
|
Fixed and pushed in |
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed exact head 1f6c936b36446eeadc0c1e0f8bd37a4de8c65aa0. The extended-operand correction rejects both REX.R and REX.B forms, moves all four xor eax,r8d/xor r8d,eax encodings to the skipped fixtures, and covers the case that clobbers a preceding EAX load. On the exact head, a Release relinker-only build with GCC 12.2 completed and ctest -R sce_code_gaps passed (1/1); git diff --check upstream/main...HEAD and conventions passed. I manually checked the overlap warning for #8042: it only changes SyscallScanner to opt into undecodable-byte resynchronization; the constant-number helper changes are separate. The focused gap test passed with that scanner behavior included. No full project or Windows/macOS local build was run; hosted build jobs are not present on this current head.
|
Thanks for the thorough review and for checking the overlap with #8042 :) |
|
Re-ran on 1f6c936 merged with #10439 onto Written with AI assistance (Claude Code). I ran the checks above on my Windows 11 machine. |
|
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
What
The refusal message names the FreeBSD syscall number that a constant load before the pattern establishes. When the
syscallpattern sits outside the unwind functions,ConstantSyscallNumberInBytescomputes that number from the raw bytes, and its recognizers matched byte patterns without looking at their prefixes:41 B8 imm32ismov r8d, imm32, but theB8arm reported the immediate as the syscall number.41 31 C0isxor r8d, eax, but the xor arm reported 0.66 31 C0and66 33 C0clear only AX, but the xor arm reported 0.41 B0 imm8ismov r8b, imm8; theB0arm consumed two of its three bytes asmov al, misaligning the walk so that a number a previous load established was not reported.ActivePrefixesBeforenow reads the prefixes of each recognized form: the active REX is only the byte immediately before the opcode,66anywhere earlier in the prefix run sets the operand size, and a REX before another prefix is a dead byte, so41 66 B8ismov ax, imm16with a dead byte in front. Per form:B8: a REX without W or B does not change the register (rex.Randrex.Xare ignored forB8+rd), so the immediate is still accepted; W, B and an operand-size override are not.31 C0/33 C0: a 16-bit pair is rejected, and so is any REX bit that extends an operand away from EAX: withModRM C0the two operands are the same register only while neither REX.R nor REX.B is set, so both bits (0x05) reject the pattern.41 31 C0and44 33 C0writer8d, and41 33 C0and44 31 C0arexor eax, r8d, so all four leave the number unknown and the site is skipped. REX.W (48 31 C0) still zeroes RAX, and REX.X (42 31 C0) extends no register without a SIB byte, so both are accepted as zero.B0: REX.B makes it a write tor8b-r11b, so the walk passes the three bytes without recording an AL write; without it the AL write is recorded with the prefix bytes in its length.66 B8: the immediate is recorded as a partial write of the low word of RAX, whose upper bits stay unchanged, so it never establishes a number on its own; a dead REX byte in front is kept in the walked span only, so the walk stays on instruction boundaries and a preceding full write is still found.The encodings were verified against GNU objdump (x86-64) before the change. A number is reported only when the walked instructions genuinely establish the value in RAX; where they do not, the site is now counted as skipped, the scanner's existing outcome for a pattern without a number, instead of naming a wrong one.
test_sce_code_gaps.pygains seven reported-number cases, each run on the Linux, Windows and macOS targets:48 31 C0and42 31 C0as zero,44 B8asmov eax,B8 3B 00 00 00; 41 B0 05and31 C0; 41 B0 05where themov r8bno longer stops the walk,31 C0; 41 66 B8where the dead REX byte is walked and the partial AX write is merged over the zeroed RAX, andB8 3B 00 00 00; 47 31 C0wherexor r8d, r8ddoes not touch EAX and the loaded 59 is kept. Eight cases, on the Linux target like the existing skipped cases, assert that no number is reported for41 B8,41 31 C0,44 33 C0,41 33 C0,44 31 C0,66 31 C0and66 33 C0, and forB8 3B 00 00 00; 41 33 C0, wherexor eax, r8dwrites an unknown value into EAX so the earlier load no longer counts. The existing cases, including the plain forms and the low-byte and low-word zeroing chains, are unchanged and pass.#10439 fixes the decoded path (
ConstantRaxLoad,ConstantSyscallNumber), and its body leaves the byte window's66 31 C0alone, noting that excluding it there would flip patterns from refused to skipped. This PR takes exactly that up, for the whole prefix family rather than the66forms alone, and makes the flip explicit and tested: the byte window now rejects the same prefixed zeroing the decoded path rejects there and recognizes the same plain forms. The functions are disjoint, andtools/pr_overlap.pyreports no overlap with open pull requests.Tested
Relinker-only build on Windows 10 Pro (build 19045), MSYS2 MinGW-w64 GCC 15.2.0 (mingw64, MSVCRT), CMake 4.2.3, Ninja, Python 3.14.3,
mainaeeaefad9against this branch.Focused
ctest:syscall_number,sce_code_gaps(with the new cases),sdk_mman_syscall,guest_syscall_trap,input_magic- all pass. The three wrong numbers were reproduced before the change by runningmain's relinker on the same gap fixtures the new cases use (41 B8names 5,41 31 C0and66 31 C0name 0); the new cases pass on the branch.Full relinker suite: 236 of 238 pass.
shader_cache_file_readandcheck_compilefail, reproduced identically on the unmodified base commitaeeaefad9before this change: with this MSVCRT toolchainstd::filesystem::create_directoriesrefuses the\\?\-prefixed paths the cache test writes, and theclangfound onPATHcannot start (exit0xC0000139); the failing subtests invoke it. Both are environment-specific and untouched by this change.syscall_scanneris registered only on Linux x86-64 (core/relinker/CMakeLists.txt), so it runs in the CI Linux relinker leg, not in this Windows run.python3 tools/check_conventions.py --base origin/main- passes.python3 tools/pr_overlap.py --branch fix/relinker-syscall-number-rex-prefixes- no overlap with open pull requests.No title or homebrew run: the change affects conversion diagnostics only.
Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)