Repository navigation
fix(relinker): keep moved instructions off a function entry after padding - #11413
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
Changes requested only for the branch being behind current main: this head merges from cbaacd2, while current main is 4b1a9a3. CONTRIBUTING requires branches to be kept current; please sync main and rerun the checks on the updated head. The intervening main changes do not touch the three files in this PR, so I found no file-level conflict in that scope.\n\nIndependent validation on exact head b17fb98: the new converterIslandIndirectEntries case passed when run alone in a local Linux relinker-only build, and diff/convention checks against current main passed. The full amd64_only_converter test binary raises SIGILL on this Xeon E5-1620 v2 host because the suite executes CPU-specific instruction tests; it does not establish a failure in this change. The PR reports the complete 303-test suite passing on macOS. No source correctness blocker found in the reviewed window placement logic.
b17fb98 to
d81da62
Compare
|
I manually compared the current #11232 and #11413 patches. Their source changes address separate cases ( |
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed exact refreshed head d81da62fdd6269c591943245ff6b14f8a3dcedcd. The guard treats a candidate instruction after an unconditional transfer and int3/nop padding as a possible function entry; the regression covers int3, 2-byte and 3-byte NOP padding, and no padding, while retaining the control case. The author reports the new case fails on the pre-change converter and the full macOS relinker suite passes 303/303; my local Linux run of the focused case passed on the same source change. Current hosted conventions/labels pass; no hosted build matrix ran on this external branch. Current main is three commits newer than the 5e90e7569 base used for the reported local run, but those commits do not touch the PR files and the merge preview is clean. Manual comparison with #11232 found that both source changes auto-merge; the test driver needs both calls retained when the shared call-list hunk is resolved. AI-assisted review; the macOS suite and title run are author-reported.
|
Thanks for the second pass and the approval! |
MarcZX10
left a comment
There was a problem hiding this comment.
The source review remains positive: the entry-after-padding guard and focused fixture look sound, and the earlier overlap check with #11232 found only the shared test-call-list hunk, with both calls needed. I am updating the review state solely because the branch is no longer current: its merge base is 2db37cf1de, while current main is 0e5d5a54098c. I found no intervening edits to the three files changed here, but CONTRIBUTING requires keeping the branch up to date. Please sync to current main, preserve both #11232 test invocations if the overlap remains, and rerun conventions plus the focused converter test on the resulting head. Existing macOS full-suite/title evidence is author-reported; current hosted checks show only conventions and labels, not a build matrix.
d81da62 to
0b35764
Compare
0b35764 to
0209cdd
Compare
|
Rebased on current main (e0cd125) as 0209cdd, on top of @adriadam10's rebase after #11232 (thanks!). Both test calls stay after |
Dismissing a stale review: the requested changes were addressed after it (new head) and the merge queue blocks on it. Please request changes again if anything is still open.
fb3c5df to
7723ff4
Compare
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — approved
What I checked
- Reviewed exact head
7723ff470f019bd5ad3135c879a06b39f371b9a9against fetchedmain2960e9a6e7. - The guard still prevents moved instruction windows from starting inside padding and overwriting an indirect function entry. The regression covers
int3, two- and three-byte NOP padding, and no-padding control; the shared test driver retains both this case and the #11232 case. - Current-main merge preview is clean;
git diff --checkis clean. The branch is based on319621ba2e, butmainhas no intervening changes to the three files this PR touches. Hosted conventions and labels pass; no hosted build matrix ran for this external branch.
Test evidence
The author reports amd64_only_converter and the 361-test macOS arm64 relinker-only CTest suite passing after the rebase. My earlier local Linux focused test passed on the same source change; I did not rerun locally.
The requested refresh and test report are present, and the intervening main commits do not touch this patch. No blocking issue remains.
XairRrR
left a comment
There was a problem hiding this comment.
Reviewed — changes requested
The fix for the Reanimal case works, and its new test fails with main's converter. However, the new check also rejects windows that lie entirely inside int3 padding. On a layout that main converts correctly, the conversion now fails with "No room for a jump to the stub of 256-bit store within its neighbours or a nearby island".
What I ran
Head 7723ff470f019bd5ad3135c879a06b39f371b9a9, merged with --no-ff into main 2960e9a6e7e1fc99c4ed180c71495b5eb83464c7 (merge base 319621ba2e578bd053b0baba2877fe70520e0737) in a clean worktree. Linux x86-64 (Arch Linux 7.2.11-zen, GCC 16.2.1, Release, Ninja), Intel Core i9-14900KF.
git fetch -q origin main pull/11413/head:refs/review/11413
git checkout -q --detach origin/main
git merge -q --no-ff --no-edit refs/review/11413
git diff --check origin/main...refs/review/11413
python3 tools/check_conventions.py --base origin/main
cmake --build build -j10 --target amd64_only_converter_tests andn_lowering_tests code_data_scanning_tests \
decoder_length_tests guest_intel_trampolines_tests masked_store_lowering_tests mulx_lowering_tests relinker \
relinker_conversion_tests rewrite_boundaries_tests to_intel_linux_stubs_tests
ctest --test-dir build --output-on-failure -j4 -R '^(amd64_only_converter|to_rosetta|to_intel_linux_stubs|guest_intel_trampolines|guest_intel_trampolines_execution|masked_store_lowering|andn_lowering|mulx_lowering|mulx_conversion|relinker_conversion|relinker_executables|rewrite_boundaries|amd64_only_windows|decoder_length|code_data_scanning|call_zero_padding|unused_filter_fallthrough_padding)$'
# negative control
git checkout origin/main -- core/relinker/codegen/include/codegen/x86/X64OpcodeConstants.hpp core/relinker/codegen/src/Amd64OnlyConverter.cpp
cmake --build build -j10 --target amd64_only_converter_tests && ctest --test-dir build -R '^amd64_only_converter$' --output-on-failure
# probe (below), run against main's converter and this PR's converterResults
| Check | Result |
|---|---|
Merge into current main |
Clean |
git diff --check |
Clean |
tools/check_conventions.py --base origin/main |
All conventions checks passed. |
| Build of the relinker and the codegen test executables | OK |
| 16 focused relinker/codegen tests (regex above) | 16/16 passed (0.18 s) |
amd64_only_converter_tests run time |
under 0.01 s |
| Negative control | New case fails on main's converter (see below) |
| Padding-only island probe | Converts on main; throws with this PR (see Blocking) |
Focused ctest tail (merged tree)
15/16 Test #2847: to_rosetta .......................... Passed 0.05 sec
16/16 Test #2846: mulx_conversion ..................... Passed 0.08 sec
100% tests passed out of 16
Negative control
I restored X64OpcodeConstants.hpp and Amd64OnlyConverter.cpp from main, kept the PR's test file, and rebuilt. amd64_only_converter then fails with An island covered the entry of a function that follows a jump and its padding. With the merged files restored, it passes. I also confirm MarcZX10's note that both the #11232 call and this one stay in main().
Findings
Blocking
1. Windows made only of int3 padding are now rejected, so some modules that main converts correctly no longer convert.
_enteredWithin treats every interior instruction for which _mayBeEntered is true as a possible entry. That includes the padding bytes themselves: the second int3 after a ret walks back over the first int3 to the ret and counts as "may be entered". As a result, _islandAt can no longer place an island inside inter-function padding. Padding is the one place where an island disturbs no live code. main uses it whenever the padding is at least 10 bytes, which 16-byte function alignment often leaves.
Repro: the PR's own fixture, with the padding enlarged and the following function replaced by a 5-byte tail jump. I added this case temporarily to Amd64OnlyConverterTests.cpp and ran it against both converters:
const auto converter = Codegen::MakeAmd64OnlyConverter(Codegen::Amd64OnlyTarget::Rosetta);
for (const auto& padding : {Bytes(15, 0xCC), Bytes{0x66, 0x2E, 0x0F, 0x1F, 0x84, 0x00, 0x00, 0x00, 0x00, 0x00, 0x66, 0x2E, 0x0F, 0x1F, 0x84, 0x00, 0x00, 0x00, 0x00, 0x00}}) {
Bytes text = {0xE8, 0x40, 0x00, 0x00, 0x00, 0xC5, 0xFC, 0x11, 0x07, 0xC3, 0xEB, 0xFD};
text.insert(text.end(), padding.begin(), padding.end());
text.insert(text.end(), {0xE9, 0x00, 0x01, 0x00, 0x00});
Bytes file(0x300 + text.size(), 0xCC);
std::copy(text.begin(), text.end(), file.begin() + 0x200);
const auto result = converter->Convert(file, {segmentHeader(text.size())});
}main: 15 x int3 -> trampolines=2 kept=0 [0x20c+5] [0x211+5] (island inside the padding; entry 0x21b untouched)
main: 2 x 10-byte nop -> trampolines=2 kept=0 [0x20c+10] [0x216+5] (inside the padding; entry 0x220 untouched)
PR: 15 x int3 -> throws: No room for a jump to the stub of 256-bit store within its neighbours or a nearby island
PR: 2 x 10-byte nop -> throws: (same)
main's output is correct for both layouts. The PR turns them into hard conversion failures. When the next function's prologue is movable, the PR does not fail, but it moves that prologue instead of using the dead padding. Every call into that function then goes through the extra jump to the stub and back.
Suggested fix: never treat an interior int3 as an entry. A function does not begin with int3, and the function's real first instruction still follows the padding, so _mayBeEntered keeps catching it. I tried this locally:
for (auto index = first + 1; index <= last; ++index) {
const auto bytes = _bytes(index);
if (!(bytes.size() == 1 && bytes[0] == X64OpcodeConstants::OneByteInt3) && _mayBeEntered(index))
return true;
}With it, all of amd64_only_converter passes, including all four variants of converterIslandIndirectEntries, and the int3 probe converts again with the island at 0x20c/0x211, as on main. Keeping multi-byte nop padding conservative, as the PR does, is defensible: with -fpatchable-function-entry, a function can begin with a nop. Please add the int3 padding-only case as a test so the island stays in the padding.
Non-blocking
None.
Not verified
- macOS/arm64 and the Reanimal, Black Ops II and Hollow Knight results in the description. These are author-reported, and I did not run any title.
--to-inteloutput, and Windows hosts.- The full test suite (only the focused relinker/codegen tests above).
To get this to a good state
- Exclude interior
int3instructions from the possible-entry check (or use an equivalent rule), so windows made only ofint3padding stay usable. - Add a test where the island must sit inside
int3padding after aretand the following function cannot be moved, and check that it converts with the island in the padding. - Rerun
amd64_only_converterand the relinker-only suite.
7723ff4 to
3504ccf
Compare
|
@XairRrR thanks for the precise repro, you're right: Updated in
The description is updated accordingly. |
MarcZX10
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 3504ccff8312df00c1564fa7c72c2a1f4953c4cb. The added _int3 exception excludes interior trap bytes from possible function-entry checks while retaining _mayBeEntered protection for the real first instruction after padding; nop handling remains conservative. The new converterIslandInTrapPadding case reproduces the reported failure and checks that the island stays before the next function. The author reports the repro fails on the prior head, the NOP negative control still throws, and amd64_only_converter plus all 484 relinker-only tests pass on macOS arm64. Current-main merge preview is clean and current main has no additional edits to these three paths. I inspected the changed source/test and whitespace; I did not run the suite locally.
…ding An island or relocation window could start in the int3 or nop padding after a jump or return and run into the next function. A function that only a pointer reaches, such as a virtual method, is not a known branch target, so its first bytes became the middle of a jump to a stub. An instruction that follows a jump, a return, a trap or padding after one of them can now only start a window. An int3 inside a window does not count as such an instruction, so a window that lies entirely in int3 padding is still used, as before; nop padding stays excluded, since a function built with -fpatchable-function-entry can begin with a nop.
3504ccf to
96581cb
Compare
Dismissing a stale review: the requested changes were addressed after it (new head) and the merge queue blocks on it. Please request changes again if anything is still open.
|
Checked the entry-after-padding guard against the diff: windows can only start at an instruction that follows a jump, return, trap or padding, an interior |
|
Thanks @adriadam10 for checking the guard against the diff and for the merge, and thanks @MarcZX10 and @XairRrR for the reviews. |
What
Converting a title with
--to-rosettacould overwrite the first bytes of a function that only a pointer reaches. A 256-bit store too short for a 5-byte jump is lowered through a nearby island: the converter moves a run of neighbouring instructions into a stub to make room for the jump. The window search refused only windows that contain a known branch target (direct branches and jump tables), so a window could start in theint3padding after aretand run into the next function's prologue. When that function is entered through a virtual call, the call lands inside the jump written over its entry.An instruction that follows a jump, a return, a trap, or
int3/noppadding after one of them can now only start a window. Anint3inside the window does not count as such an instruction, so a window that lies entirely inint3padding is still used, as onmain;noppadding stays excluded, since a function built with-fpatchable-function-entrycan begin with anop. The check sits in the window placement shared by every lowering that needs room for a jump.Reanimal (PPSA23372, 01.000.006),
eossdk-ps5-shipping.prx, bytes from 0x6ef980 after conversion with--macos --to-rosetta:vmovups [rax], ymm0at 0x6ef981 jumps to its island. Onmainthe island starts at 0x6ef98e, in the padding, so the function at 0x6ef990, which the title calls throughcall [rax+0x10], begins with46 2d 01 90, the tail of a jump, and the title hung while the EOS SDK initialized. With the change the island starts at the function's entry with a jump to its moved prologue, and the padding stays.Tested
macOS 27.0.1 on an Apple M3 Max, relinker-only Release build (AppleClang, arm64) of this branch on
main98ddd9c17:amd64_only_converter: the newconverterIslandIndirectEntriescase (int3, 2-byte and 3-bytenoppadding, and no padding as the control) fails withmain's converter ("An island covered the entry of a function that follows a jump and its padding") and passes with the change.converterIslandInTrapPadding, from XairRrR's review: a 256-bit store before aret, a jump, 15 bytes ofint3and a function that cannot move (a 5-byte tail jump). The island stays in the padding, at the same offsets as onmain(0x20c and 0x211); on the previous head7723ff470the conversion failed with "No room for a jump to the stub of 256-bit store within its neighbours or a nearby island". With two 10-bytenops as the padding, whichmainconverts with the island in the padding, the conversion still fails, sincenoppadding stays excluded.ctest --test-dir build-relinker --parallel 8: 513 of 513 pass. On arm64 the x86 execution cases ofamd64_only_converter(wideStoreExecution,rdseedExecutionand the like) are empty functions; the new cases only check converted bytes.main's, each converting Reanimal'seboot.binwith that module inprx/. The two converted modules differ in 42 bytes.python3 tools/check_conventions.py --base origin/mainpasses, andtools/pr_overlap.pyreports no overlap with open pull requests. fix(relinker): place 256-bit store islands across 2^63 without signed overflow #11232, merged on 2026-10-26, also changed the island placement of a 256-bit store (signed overflow across 2^63). This branch is rebased on it:Amd64OnlyConverter.cppmerged without conflict, andmain()ofAmd64OnlyConverterTests.cppcalls both cases afterconverterWideStoreWindows().--macos --to-rosettaon the local macOS branch described below, with this change since 2026-10-22: Reanimal now gets past EOS initialization, where it hung before; Call of Duty: Black Ops II (PPSA34502) and Hollow Knight: Voidheart Edition (PPSA34971) convert and run as before.Not tested:
--to-inteloutput, Linux and Windows hosts.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)Context: the macOS bring-up this comes from
This is the first change we propose from a bring-up of Call of Duty: Black Ops II and Reanimal on an Apple M3 Max (36 GB, macOS 27.0.1, Rosetta 2, MoltenVK 1.4.2 from Vulkan SDK 1.4.363.0, 1920×1080). This PR does not depend on that work; the rest of this section explains how the change was found and what else the bring-up needed.
Approach. We work on a local branch made of
main, the macOS port (#8658, closed; its author is splitting it: #10419, #11284, #11303 and #12127 are merged, others are open), the open pull requests below integrated before their merge, and our own commits. Each integration is recorded with the head we took, its conflicts and its validation: the same focused tests before and after withVK_LAYER_KHRONOS_validation,ANYPS5_REQUIRE_VULKAN=1andANYPS5_NO_SHADER_CACHE=1, compared per test for status andVUID-counts; a red-first test for each of our own changes; the full suite; and title runs. When one of those PRs is merged we drop our copy at the next rebase onmain. Our own generic fixes go upstream one topic at a time, starting with this one, which needs nothing else.Where the titles are (runs of 2026-10-26 on the local branch rebased on
main4e43321bd):Call of Duty: Black Ops II (PPSA34502). It boots, plays its videos and reached gameplay in Campaign (Angola), Zombies (TranZit) and local Multiplayer (Team Deathmatch on Aftermath); today's rebased branch was checked on the campaign. With warm caches the war-room menu runs at about 10 FPS and the Angola opening at 11.7 FPS averaged over 60 s (10 s windows from 6.8 to 19.2 FPS). The first macOS runs drew that menu at 0.6 to 0.8 FPS and the first campaign runs reached 4.7 to 5.1 FPS in Angola. The draw committer sets the pace: recording draws, MoltenVK's command encoding and its per-barrier resource walk take 75 to 85 % of a frame, so the console's 60 FPS is not reached.
Reanimal (PPSA23372), Unreal Engine 5. It boots and shows the THQ Nordic logo, the photosensitivity warning, the main menu with its 3D scene and the single-player menu, and starts a new game. That load does not finish yet. Before this rebase the title's footprint reached about 13 GB during it and the wired GPU memory neared the limit of this 36 GB Mac. On today's rebased branch the shader cache starts cold (new request format), and the long compiles of the largest compute pipelines trip Unreal's 120 s render-thread watchdog during the load. Getting this far took the 64-bit atomics emulation, the mesh fallback (#9356, #9387), depth bounds without the feature (#9737 and a follow-up of ours), deferred destruction on MoltenVK (#10062) and this relinker fix.
Open pull requests the bring-up carries, and our confidence that each lands soon
Confidence reads the PR's state on 2026-10-28: High when a maintainer has accepted it and it merges cleanly, Medium when it merges cleanly but has no review yet, Low when it conflicts with
mainor has unanswered change requests.11_11_10_FLOATstorage images through the in-shader conversionMerged upstream since we took them, thanks to their authors: #9115, #6951, #6946, #6848, #8612, #7968, #7882, #8034, #8928, #9177, #8756, #8911, #9737, #10062, #9701 and #9702 (the 4,750-block structurizer fix that Unreal titles such as Reanimal need; we dropped our copy at the rebase that followed its merge). Merged on 2026-10-26 to 28, after this PR was opened: #9263, #9193, #8913, #10659, #10885, #11358, #8888 and #8930. Closed: #6539, by a maintainer, for an exit-time
RelocationPoolcrash, which the local fix listed below addresses (we keep it), and #9049, by its author, asmain's memo patching left it no gain (dropped).Our other changes, proposed later one topic at a time
Each has a red-first test or a measurement recorded; the ones that claim speed still need
tools/trace_digest.pysummaries ofmainand of the change, which we can only take oncemainruns titles on macOS or on another host.fix(libSceSystemService): find a converted executable by the main executable's extension. BO2 aborted when Zombies started Multiplayer (codmp.elf) beside an extensionless main output.feat(agc): emulate the depth bounds test beside a depth test with a stencil mask, a follow-up to feat(agc): emulate a one-sided depth bounds test without the depthBounds feature #9737 for Reanimal's deferred lights (GREATER_OR_EQUAL, bounds [0.00287, 1], D32S8).feat(shader): 64-bit image and buffer atomics with device lock words where the device has none (MoltenVK), for Reanimal's Unreal Engine 5 Nanite passes.perf(agc): keep the staging pool's device tier small on an integrated GPU whose heap also has a device-only type; it held 2.5 GiB of idle buffers during Reanimal's load.fix(agc): keep relocation pools alive through outstanding leases, the exit-time crash perf(agc): relocate cached draws across user words that only reach the shader as values #6539 was closed for.fix(libkernel): leave the signal mask alone whenscePthreadExitjumps back; on macOS,longjmprestores the mask throughsigprocmask, which XNU applies to every thread.fix(agc): map a host import window split inside a 16 KiB host page again as one region on macOS; BO2's labels stopped forcing CPU waits (Angola 7.6–8.1 to 10.5–10.7 FPS).