Skip to content

fix(relinker): preserve EIP-relative jump-table destinations - #15639

Open
PolkadotDom wants to merge 1 commit into
boykopovar:mainfrom
PolkadotDom:fix/eip-jump-table-targets
Open

PolkadotDom wants to merge 1 commit into
boykopovar:mainfrom
PolkadotDom:fix/eip-jump-table-targets

Conversation

@PolkadotDom

Copy link
Copy Markdown
Contributor

What

The AMD-only converter misses relative jump tables addressed by 67 LEA when the effective address wraps to 32 bits. It then treats a table destination as an instruction it can move into a conversion stub. In the regression, Rosetta conversion of a four-byte vector store consumes the following instruction even though the jump table enters it.

For example, an eight-byte LEA at 0x1000 with displacement bits 0x80000008 forms 0x80001010. Main looks for the table at 0xffffffff80001010 instead. The table's second entry targets 0x1019, which main moves into the store's stub. With this fix the converter takes the preceding instruction into the stub and leaves the destination at 0x1019 intact.

Truncate the computed table address to 32 bits when the decoder reports the address-size override. Normal RIP-relative table addresses retain their existing behavior. The architecture rule is Intel SDM Volume 2A, §2.2.1.6, reproduced in the Intel documentation changes PDF: the address-size prefix truncates and zero-extends the effective address.

Tested

Base 1c8c0efc8c; candidate 8a0ce96cba.

  • Windows 11 x86-64, GCC 12.2, Release relinker-only configuration: built relinker, amd64_only_converter_tests, amd64_only_windows_tests, relinker_conversion_tests and relinker_game_patch_tests.
  • Seven focused CTests passed: amd64_only_converter, amd64_only_windows, relinker_conversion, relinker_game_patch, relinker_executables, mulx_conversion, and to_rosetta.
  • Fresh Ubuntu/WSL build with GCC 11.4: amd64_only_converter_tests built and its CTest passed.
  • The added converter matrix checks 12 combinations: RIP/EIP addressing, normal/wrapping displacement, and no segment prefix/FS/GS. It checks the placement and the unchanged destination bytes. The RIP controls prevent incorrectly truncating all table addresses. The new regression fails on unchanged main with A jump table's address size did not protect the instruction after a 256-bit store.
  • Independent native x86-64 Linux probe: an EIP-relative LEA, signed table-entry load, add and indirect jump reached both destinations (returning 41 and 42), with no segment prefix, FS and GS. Six executions passed. Source and command below.
  • Conventions and whitespace checks passed; clean merge preview against current main.
  • The initial overlap scan covered 1,802 open PRs. Moved the new test/function call to avoid fix(relinker): keep embedded data tables out of --to-intel and --to-rosetta conversion #11525's insertion points, then reran against 1,804 open PRs after verifying 1,799 cached head hashes and fetching five changed/missing heads. The final scan has no line/export collision. Its exit status is 2 because fix(relinker): lower scalar RCPSS and RSQRTSS #11192 and fix(relinker): match AppContent utility import to provider #13486 already conflict with main; their shared-file diffs were reviewed manually. fix(relinker): lower scalar RCPSS and RSQRTSS #11192 changes reciprocal tests, and fix(relinker): match AppContent utility import to provider #13486's converter changes concern entry protection around padding; neither implements the EIP table-address fix or conflicts with the relocated test. The combined merge preview with fix(relinker): keep embedded data tables out of --to-intel and --to-rosetta conversion #11525 is clean. No dependency is needed.

Synthetic code/conversion validation. No PS5 title, Apple/Rosetta execution, GPU testing or full product build was performed.

Native x86-64 jump-table probe

Compile with g++ -std=c++20 -O2 probe.cpp -o probe, then run ./probe on Linux x86-64. The mappings use MAP_FIXED_NOREPLACE and are private to the probe. It verifies the input instruction/addressing semantics; it does not execute the converter's Rosetta output.

#include <sys/mman.h>
#include <cstdint>
#include <cstdio>
#include <cstring>
#include <vector>

int main() {
    constexpr std::uintptr_t codeAddress = 0x10000000;
    constexpr std::uintptr_t dataAddress = 0x90000000;
    constexpr std::uintptr_t tableAddress = dataAddress + 0x10;
    auto* code = static_cast<std::uint8_t*>(mmap(reinterpret_cast<void*>(codeAddress), 4096, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED_NOREPLACE, -1, 0));
    auto* data = static_cast<std::uint8_t*>(mmap(reinterpret_cast<void*>(dataAddress), 4096, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED_NOREPLACE, -1, 0));
    if (code == MAP_FAILED || data == MAP_FAILED) {
        std::perror("mmap");
        return 1;
    }
    const auto put32 = [](std::uint8_t* at, std::uint32_t value) { std::memcpy(at, &value, 4); };
    for (const std::uint8_t prefix : {0x00, 0x64, 0x65}) {
        if (mprotect(code, 4096, PROT_READ | PROT_WRITE)) return 2;
        std::vector<std::uint8_t> bytes{0x89, 0xf8};
        if (prefix) bytes.push_back(prefix);
        bytes.insert(bytes.end(), {0x67, 0x48, 0x8d, 0x0d, 0, 0, 0, 0});
        put32(bytes.data() + bytes.size() - 4, static_cast<std::uint32_t>(tableAddress - (codeAddress + bytes.size())));
        bytes.insert(bytes.end(), {0x48, 0x63, 0x04, 0x81, 0x48, 0x01, 0xc8, 0xff, 0xe0});
        std::memcpy(code, bytes.data(), bytes.size());
        for (unsigned index = 0; index < 2; ++index) {
            const auto offset = 0x20 + index * 0x10;
            code[offset] = 0xb8;
            put32(code + offset + 1, 41 + index);
            code[offset + 5] = 0xc3;
            put32(data + 0x10 + 4 * index, static_cast<std::uint32_t>(codeAddress + offset - tableAddress));
        }
        if (mprotect(code, 4096, PROT_READ | PROT_EXEC)) return 3;
        auto function = reinterpret_cast<int (*)(int)>(code);
        for (int index = 0; index < 2; ++index) {
            const auto result = function(index);
            std::printf("prefix=%02x index=%d result=%d\n", prefix, index, result);
            if (result != 41 + index) return 4;
        }
    }
    munmap(code, 4096);
    munmap(data, 4096);
}
prefix=00 index=0 result=41
prefix=00 index=1 result=42
prefix=64 index=0 result=41
prefix=64 index=1 result=42
prefix=65 index=0 result=41
prefix=65 index=1 result=42

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file
  • No notes, investigation .md files or images added to the repository (attach them to this PR)
  • New third-party code is a submodule built from source
  • Generic behaviour, not specific to one title
  • Depends on: none
  • Fixes: none
  • Unblocks: none
  • GPUs tested: n/a
  • AI-assisted: yes

@BunnyFl BunnyFl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PolkadotDom thanks, the fix looks right to me. With a 67 prefix in 64-bit mode, the RIP-relative form becomes EIP-relative: next-instruction address plus disp32, truncated to 32 bits (Intel SDM Vol. 2A, §2.2.1.6). Casting the sum to std::uint32_t (Amd64OnlyConverter.cpp:327) does that and leaves the plain RIP case unchanged. lea computes only the offset and ignores the segment (SDM Vol. 2A, LEA), and the FS and GS rows in the new test (Amd64OnlyConverterTests.cpp:117) expect the same placement as the unprefixed ones. The 0x3000 rows check that RIP-relative tables still resolve, and the 0x80001010 row without 67 checks that wrapping isn't applied to them.

What I ran: head 8a0ce96cba merged onto main 1c8c0efc8c, Windows 11, WinLibs GCC, relinker-only Release build:

  • amd64_only_converter, amd64_only_windows, relinker_conversion, mulx_conversion and to_rosetta pass (5 of 5);
  • negative control, Amd64OnlyConverter.cpp taken from main with the new test kept: amd64_only_converter fails with "A jump table's address size did not protect the instruction after a 256-bit store";
  • tools/check_conventions.py --base upstream/main on the head: all checks passed;
  • tools/pr_overlap.py --branch: no shared lines or exports. The tool reported #11192 and #13486 as conflicting with main and left them uncompared. #13486's diff changes no lines of src/Amd64OnlyConverter.cpp, and #11192 touches only the test file, as you found.

Approving.

@BunnyFl

BunnyFl commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

Correction to my review above: the FS and GS rows are at Amd64OnlyConverterTests.cpp:1163 (the segment loop), not :117. I took the number from the diff hunk by mistake. Nothing else changes.

@TechLevelZero

Copy link
Copy Markdown
Contributor

✅ AMD GPU test: passed

PR head 8a0ce96 merged onto main 1c8c0ef · tested 2026-11-04 02:32 UTC · 19 min

GPU AMD Radeon RX 6300 (RADV NAVI24), driver Mesa 25.2.8-0ubuntu0.24.04.4, Vulkan 1.4.318
Host Ubuntu 26.04.1 LTS, kernel 7.0.0-42-generic, container ubuntu:24.04
Logs build and test logs for this run
Build ✅ passed (Release, -DANYPS5_ENABLE_SPIRV_TOOLS=ON)
Tests ✅ 1,069 passed · 4 failed (0 new, 3 also fail on main, 1 unstable on main) · 8 skipped (ANYPS5_REQUIRE_VULKAN=1)
Test setup CPU-only tests without a GPU; GPU tests (and any CPU-only test that failed or skipped without one) once on the GPU with VK_LAYER_KHRONOS_validation and ANYPS5_NO_SHADER_CACHE=1; main ran the same way
Test selection 1,081 of 3,842 tests run: the ones this change can reach, chosen by tools/select_tests.py from main's build graph
Vulkan validation ✅ no new validation IDs vs main (523 tests run on the GPU)
Slow tests (>30 s, GPU tests timed with validation layers) ⚠️ agc_driver_pipeline_journal 120.3 s, ⚠️ public_headers 72.0 s, ⚠️ agc_driver_image_sample_unnormalized 36.4 s
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)
agc_driver_pipeline_journal: failed in 8 of the last 10 main runs
Failing on main too (3, not counted against this PR)
agc_driver_image_table
agc_driver_native_depth_bda_clear
agc_driver_native_multisample_depth
Validation IDs already present on main (8 IDs in 446 tests)
VUID-VkDeviceCreateInfo-pNext-pNext (446 tests): agc_driver_address_passes, agc_driver_alias_copy_label, agc_driver_alpha_to_coverage, agc_driver_array_layer_target, agc_driver_bc_block_copy, agc_driver_bda_anchor_reads, agc_driver_bda_span_reads, agc_driver_bindless_images, and 438 more
VUID-vkCmdCopyImage-srcImage-01548 (4 tests): agc_driver_depth_clear_hle, agc_driver_depth_storage_dispatch, agc_driver_depth_surface_from_storage, agc_driver_dispatch_move_depth
VUID-vkCmdCopyImage-srcImage-01551 (4 tests): agc_driver_depth_clear_hle, agc_driver_depth_storage_dispatch, agc_driver_depth_surface_from_storage, agc_driver_dispatch_move_depth
VUID-VkPipelineColorBlendStateCreateInfo-pAttachments-00605 (1 test): agc_driver_srgb8_color_target
VUID-VkSamplerCreateInfo-addressModeU-01079 (1 test): agc_driver_image_gather_volume
VUID-VkSamplerCreateInfo-addressModeU-parameter (1 test): agc_driver_image_gather_volume
VUID-VkSamplerCreateInfo-addressModeV-parameter (1 test): agc_driver_image_gather_volume
VUID-VkSamplerCreateInfo-addressModeW-parameter (1 test): agc_driver_image_gather_volume
Skipped (8)
agc_driver_copied_writer_label_order: skipped, the device reports a minStorageBufferOffsetAlignment of 4, so the dispatch binds its record at +0x1004 in place and nothing is written back; the test needs an alignment above 4
agc_driver_float16_unused_nan_payloads: skipped, the device has no native f16 conversions with RTE rounding and kept f16 denormals
agc_driver_gcn_display_present: skipped, no display: Vulkan support is either not configured in SDL or not available in current SDL video driver (offscreen) or platform
agc_driver_linear_target_direct_write_completion: skipped, the device does not keep host imports write-watched, so no completion stamp over the target is tracked
agc_driver_ngg_vertex_wave_reduction: skipped, s3 describes a 32-lane wave only on 32-lane subgroups, and the device has 64 lanes
agc_driver_scattered_runs: skipped, host imports are unavailable: /dev/udmabuf is not accessible
agc_driver_sparse_dma_buf_import: skipped, host imports are unavailable: /dev/udmabuf is not accessible
Reproduce
git fetch origin 8a0ce96cbac09056620155ce47e8af78cfd01e85 1c8c0efc8c41314402e6966da5bef6ea3d639d10
git checkout 1c8c0efc8c41314402e6966da5bef6ea3d639d10 && git merge --no-ff 8a0ce96cbac09056620155ce47e8af78cfd01e85
git submodule update --init --recursive
cmake -S . -B build -G Ninja -DCMAKE_BUILD_TYPE=Release -DBUILD_TESTING=ON -DANYPS5_ENABLE_SPIRV_TOOLS=ON
cmake --build build && cmake --build build --target libs
VK_DRIVER_FILES=/nonexistent/icd.json ctest --test-dir build -E '^(agc|shader|recompiler|videoout|video_out|guest_video_out|vulkan)|gpu' --timeout 120 --parallel 12 --output-on-failure
ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation \
  ctest --test-dir build -R '^(agc|shader|recompiler|videoout|video_out|guest_video_out|vulkan)|gpu' --timeout 120 --parallel 16 --output-on-failure
# failures are then re-run alone: --timeout 360 --parallel 2; new validation IDs: the test alone, twice, --parallel 1

Automated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs.

@ycbuzi

ycbuzi commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

Tested on an Intel Mac (i5-2415M, AVX1 only, macOS 13.7.8, x86-64 native, no Rosetta), LLVM clang 19.1.7, relinker-only build; PR head 8a0ce96 merged into main ab21370.
python3 tools/select_tests.py selected 668 tests; 3 runs: 664 passed each time, and the 4 failures are the same as on main (2 check_compile_modes from Apple clang 14 on PATH, masked_store_lowering and amd64_only_converter SIGILL on this pre-AVX2 CPU).
Because amd64_only_converter stops at that SIGILL before the new case, I also ran it with #16236 merged: 3/3 passed on main and 3/3 with this PR, so converterAddressSizeJumpTableTargets passes here.
AI-assisted: yes (Claude Code)

@github-actions

github-actions Bot commented Nov 7, 2026 •

Copy link
Copy Markdown

Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.

Pull request Conflicting files (lines on main) Same exports Same new files
PR 15903 core/relinker/codegen/src/Amd64OnlyConverter.cpp L326

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.

Pull request Shared files not compared
PR 11525 core/relinker/codegen/src/Amd64OnlyConverter.cpp
core/relinker/codegen/tests/Amd64OnlyConverterTests.cpp
PR 12678 core/relinker/codegen/src/Amd64OnlyConverter.cpp
PR 14214 core/relinker/codegen/tests/Amd64OnlyConverterTests.cpp

@oneandonlydean oneandonlydean self-assigned this Nov 7, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants