Repository navigation
fix(relinker): resolve BND and NOTRACK call sites - #10647
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed exact head 16eb32f20ef6f9b481a5056d783c67fc2f7c884e. The resolver now skips the BND/REPNE and DS/NOTRACK legacy prefixes while scanning the instruction and clears an earlier REX.W when a legacy prefix follows it; the added cases cover both call/jump forms, combined-prefix orderings, REX placement, pointer loads, and negative controls for FS/GS, address-size override, register operands, and unrelated memory operations. The Linux, Windows, macOS ARM64, and macOS x86_64 relinker jobs and relinker ASan all pass; conventions, eligibility, intake, and labels pass. Product-specific jobs were skipped by eligibility. The author reports extensive direct CLI, CTest, and native instruction checks in the PR body; I did not independently rerun those local checks. No blocking issue found.
roboterspinne
left a comment
There was a problem hiding this comment.
Reviewed head 16eb32f merged onto upstream/main 3ca5b17 (2026-10-24); the merge is clean, check_conventions.py passes and pr_overlap.py reports no overlap.
Environment: Windows 11 Home 10.0.26200, Intel i9-12900H, WinLibs GCC 15.2.0 UCRT, CMake 4.3.1, Ninja 1.13.2, Python 3.13.9, -DANYPS5_RELINKER_ONLY=ON -DBUILD_TESTING=ON, Release.
Results
- Full relinker-only suite on the merged tree: 242 of 243 pass,
long_pathsskipped. The one failure,patch_advisor_unit, ismain's:test_uppercase_digest_and_conversion_optionsasserts--sce-module-path <root>unquoted whilesuggest_patches.pyquotes a path that contains a space (subprocess.list2cmdline), and my temp root does; it fails identically on plainmain3ca5b17 and enteredmainwith #8900 today. Not this PR. - Red/green: the 22 new rows compiled against
main's resolver fail atBND indirect call; against this branchcall_site_resolver,call_site_index,call_registry_writerandcall_registrypass. - End to end with
--registryon thetest_call_registryfixture, Linux and Windows output:mainleavesf2 ff 15,3e ff 15,3e f2 ff 15,f2 3e ff 25,f2 48 ff 15,3e ff 25and3e 48 8b 05with emptycallSitesandcallSitesResolved: false; this branch resolves each to0x200.48 3e 8b 05(narrow load) and3e 67 ff 15stay unresolved on both, and64 f2 ff 15is refused by the Windows output on both (Unsupported Windows guest TLS instruction), so nothing outside the registry changes. - Native check on this CPU (each form in its own process, pointer slot at
[rip+disp]): every BND, NOTRACK, combined and REX-prefixedff 15/ff 25form transfers through the slot;48 3e 8b 05and48 f2 8b 05load 32 bits,3e 48 8b 05,f2 48 8b 05and48 3e 48 8b 05load 64 bits,3e 48 40 8b 05loads 32 bits. That is exactly thehasRexWreset and last-REX rule the resolver implements, and it matches the project's own decoder (X64InstructionDecoder.cpp:42-43,:380) andobjdump's reading of every row. - Scanner length: the rows only reach
0x1100with displacementsf9/f8/f7ifInstructionMatch.Lengthincludes the prefix bytes, soinstrEndatCallSiteResolver.cpp:85is right;X64InstructionDecoder::Decodeconsumes F2, 3E, F3, 66, 67 and segment prefixes before the opcode. - Consumers:
CallSites/CallSitesResolvedare read only byCallRegistryWriter(RelinkerPipeline.cpp:579-581,CallRegistryWriter.cpp:90-96); no patcher reads them.GotAccessIndex(unused-NID filter) already counted these prefixed forms as GOT accesses onmain, so the PR narrows a gap between the filter and the registry rather than opening one. - Body: 22 rows, the CI counts of run 40104543384 (246/243/237/237/33), the base history (
1e740a23a3, patch ide28cdac41e), the binutils thread (both prefix orders) and thepr_overlapinvocation check out.
Nits
CallSiteResolver.cpp:33: of the four segment overrides that 64-bit mode ignores (2E, 26, 36, 3E) only 3E is skipped, so2e ff 15or26 48 8b 05stay unresolved while the same instruction with 3E resolves. Conservative, and no compiler emits them; the set that stays unclassified after this PR (F3, 66, 67, F0, 2E/26/36, FS/GS, and8bwithout REX.W) is wider than the body's "address-size and FS/GS overrides", and only the body records it. A sentence indocs/dev/debt/relinker.mdnext to the registry entry would keep it findable; your call.- Body, Tested: "Both call-site CTests pass on every matrix platform" does not hold for
relinker-asan, which runs--label-regex '^tls_function_coverage$'(33 tests); the two call-site tests are built there but not run. True for the four OS jobs. CallSiteResolverTests.cpp:44: "DS-prefixed pointer load" names the accepted3e 48 8b 05as a DS override while the same inert override via 2E/26/36 is rejected; "NOTRACK-prefixed pointer load" would say why this one is in.
Notes, nothing to change here: ripRelativeDispOffset is a third prefix walker next to X64InstructionDecoder::Decode and DecodeInstruction, and DecodedInstructionInfo already exposes RexPrefix with the same reset rule, SegmentPrefix, AddressSize32 and RipRelativeDispOffset; the loop predates this PR and a swap would have to re-add the rejections the new tests pin, so follow-up material at most. call_site_resolver has no TIMEOUT property where call_site_index has 20 (pre-existing).
Not checked: AMD hardware (the console's Zen 2 and the author's probes were not run here; the probe above is Intel), macOS and Linux execution, the WSL ASan/UBSan runs, the pre-rebase head 00897f4b47.
I found no blocking issue: the two prefixes do not take part in the address match, the REX reset models the documented placement rule and the CPU, the registry is the only consumer, and the counter-check holds. For disclosure: I have open relinker PRs (#10646, #10607, #10564, #10403, #10402, #10401, #10391, #10390); none touches these files.
Written with AI assistance (Claude Code). I ran the builds, tests, registry conversions and native checks above on my Windows 11 machine.
| ++cur; | ||
| continue; | ||
| } | ||
| if (b == PrefixRepne || b == PrefixSegDs) { |
There was a problem hiding this comment.
Nit: 64-bit mode ignores all four segment overrides 2E, 26, 36 and 3E for memory operands (Intel SDM Vol. 2A 2.2.1), but only 3E is skipped here, so 2e ff 15 ... or 26 48 8b 05 ... stay unresolved while the 3E form resolves. Conservative and compiler-irrelevant, so fine; but the forms that remain unclassified after this change (F3, 66, 67, F0, 2E/26/36, FS/GS, 8b without REX.W) are wider than the body's "address-size and FS/GS overrides" and live only in the body. One sentence in docs/dev/debt/relinker.md beside the registry entry would keep that findable; your call.
| check("BND NOTRACK indirect tail jump", {0xf2, 0x3e, 0xff, 0x25, 0xf8, 0, 0, 0}, true); | ||
| check("BND REX indirect call", {0xf2, 0x48, 0xff, 0x15, 0xf8, 0, 0, 0}, true); | ||
| check("NOTRACK REX indirect tail jump", {0x3e, 0x48, 0xff, 0x25, 0xf8, 0, 0, 0}, true); | ||
| check("DS-prefixed pointer load", {0x3e, 0x48, 0x8b, 0x05, 0xf8, 0, 0, 0}, true); |
There was a problem hiding this comment.
Nit: the name presents 3e 48 8b 05 as a DS override, yet the same inert override via 2E, 26 or 36 is rejected by the loop; this row is in because 3E doubles as NOTRACK. "NOTRACK-prefixed pointer load" would say so.
|
Thanks for the detailed independent checks. Addressed the three nits in
Production code is unchanged. Rebuilt both focused test targets on Windows and Linux; each CTest passed five consecutive runs per platform. Conventions and whitespace checks pass. The unrelated patch-advisor test issue is left out of this PR. |
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed the follow-up head 7151c6c0907f7440c724c68652846df29d709e67. Production resolver code is unchanged; the debt entry now lists the remaining unclassified instruction prefixes and narrow loads, and the test label is clearer. I built call_site_resolver_tests and call_site_index_tests with the relinker-only Release configuration and ran both CTests five times (all passed); conventions, diff checks, and the current-main merge tree pass. Hosted Ubuntu and ASan checks pass; Windows and macOS jobs are still running, while product jobs are skipped by policy. The previous approval was on the implementation head, so I’ll recheck the remaining matrix before approving this follow-up head.
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed the follow-up documentation change at 7151c6c. It documents the resolver's unclassified-prefix behavior and narrow-load rule; the implementation remains the previously reviewed call-site resolver. Local focused CTests and passed five repetitions each, as did conventions and ; merge tree against current main is clean. Hosted eligibility, labels, and all four relinker platform/sanitizer jobs passed (a duplicate conventions run was cancelled and the replacement passed). Product builds were skipped by repository policy.
|
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.
|
|
@Bram910 Thanks. Re-checked 7151c6c merged onto Written with AI assistance (Claude Code). I ran the build and the tests above on my Windows 11 machine. |
|
Resolved the main conflict and pushed The only conflict was in Revalidation on the integrated head:
Fresh hosted checks are starting; no new macOS, title, GPU or full-product run is claimed locally. |
|
@Bram910 Thanks, re-checked c265e51 merged onto
Nothing new from my side. Written with AI assistance (Claude Code). I ran the checks above on my Windows 11 machine. |
What
The call-site resolver skips REX prefixes but stops at
F2(BND/REPNE) or3E(NOTRACK/DS). Valid prefixed indirect calls and tail jumps therefore produce emptycallSitesandcallSitesResolved: falsein the import registry, despite referring to the same GOT slots as unprefixed instructions.Skip these two legacy prefixes when locating the opcode and RIP displacement. Clear the accumulated REX.W state when either appears, so an earlier REX cannot incorrectly widen a subsequent pointer load. This changes registry analysis only; it does not rewrite instructions or emulate BND/CET behavior. Instructions with F3, 66, 67, F0, 2E/26/36, FS/GS prefixes, and
8Bloads without REX.W remain unclassified; these limits are recorded beside the registry entry indocs/dev/debt/relinker.md.Add 22 regression scenarios covering individual and combined hints, both prefix orders, REX placement, pointer loads, and negative controls for narrow loads, segment/address overrides, register transfers and unrelated memory operations. GNU binutils' NOTRACK prefix tests also cover combined BND/NOTRACK forms and their prefix ordering.
Tested
mainat1e740a23a3; implementation commit16eb32f20ef6f9b481a5056d783c67fc2f7c884e. The negative control and first round of testing usedd098eca3d6/00897f4b47. The intervening merge changes only unrelated AGC tests; the implementation retains the same stable patch ID (e28cdac41e62502b22c672f67e3ffc8d49ebe720) across that rebase.BND indirect call, then passes with this fix.0x200to the GOT slot at0x300on both the Windows and native Linux relinkers (40 successful fixed conversions). No title was used.call_site_resolverandcall_site_indexCTests pass five consecutive runs each.ANYPS5_RELINKER_ONLY=ON:cmake --build build-relinker --parallel 4succeeds, andctest --test-dir build-relinker --output-on-failure --parallel 4passes 241/241 tests on the final rebased head (169.97 seconds). The initial pre-rebase full run also passed 241/241.-Wall -Wextra -Werror, in Release and ASan/UBSan builds, and pass five consecutive runs each per build (20 successful component runs). Sanitizer options:detect_leaks=1:halt_on_error=1andhalt_on_error=1:print_stacktrace=1.python tools/check_conventions.py --base origin/mainandgit diff --checkpass.python tools/pr_overlap.py --repo boykopovar/AnyPS5 --branch fix/call-site-hint-prefixes --jobs 4reports no overlap with open pull requests, against the final base1e740a23a3.16eb32f20e, merged with the newer main for CI: Ubuntu 246/246, Windows 243/243, macOS arm64 237/237, macOS x86-64 237/237, and ASan 33/33 passed. Both call-site CTests pass in the four OS jobs; the ASan job builds them but runs only the 33 tests labeledtls_function_coverage. Conventions, labels, intake and eligibility also passed. Full-product jobs were skipped, not executed.7151c6c0907f7440c724c68652846df29d709e67only documents remaining registry-analysis limits and renames one test label. The production resolver is byte-identical to16eb32f20e. Both call-site CTests were rebuilt and passed five consecutive runs each on Windows and Linux (20 successful CTest runs), and conventions and whitespace checks pass. The full-suite results above belong to the implementation head; no full local suite was rerun for the documentation/label-only follow-up.Main integration refresh
mainata5a780eaaa7f187fe8f5c7f2ef244f6a6dab2762into headc265e51abf3d747b67d768fec79f6defc9905419on 2026-10-25, preserving published history.docs/dev/debt/relinker.md: retained this PR's resolver-limit note and main's updated Windows preferred-base documentation. The resolver implementation and regression file are byte-identical to reviewed head7151c6c0907f7440c724c68652846df29d709e67; the diff against the refreshed base remains the same three files.-Wall -Wextra -Werror: both resolver and index tests pass five repetitions each in Release and ASan/UBSan builds (20 successful component runs), with leak detection and halt-on-error enabled. Windows registered resolver/index CTests pass five repetitions each; 20 actual Windows relinker registry conversions pass.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)