Repository navigation
fix(relinker): reject --lazy-binding for macOS output - #10712
adriadam10 merged 1 commit into
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed exact head 9a69e8b against current main. The macOS patcher rejects lazy binding before writing output, matching the documented unsupported Mach-O behavior; the fixture checks exit 2, diagnostic text, and absence of the output file. Local relinker Release build and focused macos_output CTest passed (1/1); git diff --check, current-main conventions and merge-tree pass. Hosted Linux, Windows, macOS x86_64, ASAN and policy checks pass; macOS arm64 remains pending. No blocking source issue found.
|
CI follow-up for reviewed head |
|
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.
|
maximilianfv
left a comment
There was a problem hiding this comment.
Approve: the fix is correct and the new check fails without it and passes with it on macOS. Two optional items below.
Tested on Apple M1 Pro, macOS 27.0.1 (x86_64 build under Rosetta 2)
Applied at 9a69e8b6de to mugurc's macOS series (27e968974) merged with main at 2742004ed2, with other open macOS fixes. I toggled only this PR, and the MacOsMachOPatcher.cpp hunk separately from the test and doc hunks.
macos_output |
the other 17 relinker tests (macos_*, relinker_conversion, cli_args, conversion_diagnostics, *_fixed_base, *_trace_imports, ...) |
relinker --macos --lazy-binding --skip-sce-module on import_fixture() |
|
|---|---|---|---|
| Without this PR | Passed (old test) | 17/17 passed | exit 0, writes the same 77,920-byte Mach-O as --macos alone (sha256 aed941e41b5ff6b0…) |
| PR test, old patcher | Failed: --macos with --lazy-binding: exit 0: Converting conflict.elf |
n/a | n/a |
| With this PR | Passed (3 runs) | 17/17 passed in 3 runs (the C++ test binaries were relinked against the patched core before the third); relinker_game_patch also passed |
exit 2, FAIL: macOS target does not support --lazy-binding, no output; the eager --macos output is byte-identical to the old one |
--lazy-binding --macos and --macos --lazy-binding --macos-fixed-base 0x400000 are also rejected with exit 2 and no output. --lazy-binding without --macos still converts.
- Optional: reject the combination in the CLI as well (
core/relinker/cli/src/CliArgs.cpp:211). The patcher throw atcore/relinker/elfpatcher/src/macos/MacOsMachOPatcher.cpp:62only runs atConversion.cpp:178, afterRelink()(:104) and the guest-module build (:146). On a real title the user waits through the whole relink before getting what is really a usage error. With bundled modules, the user first getsGuest modules require eager binding, and only after dropping the modules learns that macOS has no lazy binding at all. The other target conflicts are already checked inParseArgsatCliArgs.cpp:202-211, before any input is read:--trace-imports cannot be used with --macos,--linux-fixed-base cannot be used with --windows or --macos, and--macos-fixed-base requires --macos. Fix:- add
if (args.lazyBinding && args.toMacos) throw std::runtime_error("--lazy-binding cannot be used with --macos");afterCliArgs.cpp:211. That is below #9408's CliArgs hunk, which removes the--trace-imports/--macoscheck, so the two do not conflict; - add an
expectThrowsnext tocore/relinker/cli/tests/CliArgsTests.cpp:241; - keep the patcher throw as a backstop;
- in
test_macos_output.py:178, match the new message and check for a non-zero exit, as the--windows conflicts with --macoscheck two lines above does, because CLI errors exit 1 (main.cpp:254-256).
- add
- Nit:
docs/dev/debt/relinker.md:42records that--trace-importsexists for Windows and Linux output, not macOS. A matching line for--lazy-bindingwould keep the debt file in step with the newUSAGE.mdwording. It is fine to skip because the flag is deprecated.
AI-assisted: yes (Claude Code applied the PR, ran the tests, read the code and wrote this review).
|
@maximilianfv Thanks for testing it on macOS. 1: This PR keeps the single check where the option is consumed, which covers every macOS conversion, including direct |
|
Tested head 9a69e8b merged onto main aeff26b (clean) on Windows 11 (Ryzen 5 9600X), WinLibs GCC 15.2.0, Release.
AI-assisted: yes (Claude Code); results above were run on this machine. |
MacOsMachOPatcher::Patch discarded its lazyBinding argument, so a module-free `--macos --lazy-binding` conversion exited 0 and wrote the same eagerly bound Mach-O as a run without the flag. The Mach-O writer puts every import in the LC_DYLD_INFO_ONLY bind stream and leaves the lazy_bind stream empty, with no __la_symbol_ptr or stub helper, so the macOS target cannot honour the flag. The patcher now throws "macOS target does not support --lazy-binding", in the form of its --windows-diagnostics check, so such a conversion exits 2 and writes no output. Conversions with bundled modules were already refused by the guest module builder.
9a69e8b to
3123f92
Compare
|
Rebased onto current |
What
MacOsMachOPatcher::Patchdiscarded itslazyBindingargument (static_cast<void>(lazyBinding);), sorelinker --macos --lazy-binding --skip-sce-moduleon theimport_fixture()guest oftest_macos_output.pyexited 0 without an error or warning and wrote the same eagerly bound Mach-O as the run without the flag: 77,920 bytes, byte-identical on amainbuild, while the flag changes the Windows (282,112 to 281,600 bytes) and Linux (25,906 to 25,890 bytes) outputs of the same fixture. Nothing earlier rejects the combination:CliArgs.cpppairs--lazy-bindingonly with--trace-importsand--linux-fixed-base, the target checks inConversion::Convertdo not coverLazyBinding, andGuestModuleBuilderrefuses it only when bundled modules are converted (Guest modules require eager binding; --lazy-binding is incompatible), so every module-free macOS conversion reached the discard.MacOsMachOPatcher::PatchthrowsmacOS target does not support --lazy-bindingwhenlazyBindingis set, in the form of its--windows-diagnosticscheck; the CLI printsFAIL: macOS target does not support --lazy-binding, exits 2 and writes no output. The Mach-O writer puts every import in theLC_DYLD_INFO_ONLYbind stream and leaves its lazy_bind offset and size at 0, with no__la_symbol_ptrsection or stub helper, so the target has no lazy binding to select. Conversions with bundled modules still fail first with theGuestModuleBuildermessage. NoCliArgs.cppconflict is added: the patcher check already covers every macOS conversion with the flag, and feat(relinker): trace function imports on macOS #9408 edits the neighbouring checks. The--lazy-bindingrow ofUSAGE.mdnow saysWindows and Linux output only.check_optionsintest_macos_output.pyruns--macos --lazy-binding --skip-sce-moduleonimport_fixture(), the fixturecheck_importsrelinks eagerly in the same test, and asserts exit 2, the message on stderr and no output file. On untouched code it fails with--macos with --lazy-binding: exit 0: Converting conflict.elf, and the output file is written.--frame-red-zonerow ofdocs/user/USAGE.mddirectly above the--lazy-bindingrow this PR edits; git reports a conflict because the two hunks touch, and the resolution keeps both rows.pr_overlap.pycannot compare this branch with feat(aot): consolidate native ARM64 compiler, runtime and consumers #5021, perf(agc): build the pipelines of a submission ahead on a compile pool #5925, fix(tools): recognize guest module identities in import audit #6171, perf(recompiler): keep runtime-specialized shader modules in the shader disk cache #6924, feat(agc): look up table images at run time through a device image table on the GPU heap (APS5_GPU_PATH) #7642, feat(libSceUserService): take the user name from ANYPS5_USER_NAME #9345, feat(relinker): trace function imports on macOS #9408, perf(agc): GPU descriptor heap on VK_EXT_descriptor_heap (APS5_GPU_HEAP) #9508 and feat(agc): serve lane-keyed, pointer and runtime image-table columns #10193 (docs/user/USAGE.md), because those do not merge withmain, and reports no other shared lines. Checked by hand, none of theirUSAGE.mdhunks touches the--lazy-bindingrow at line 91: feat(aot): consolidate native ARM64 compiler, runtime and consumers #5021 starts at line 33, perf(agc): build the pipelines of a submission ahead on a compile pool #5925 at 165, fix(tools): recognize guest module identities in import audit #6171 at 382, perf(recompiler): keep runtime-specialized shader modules in the shader disk cache #6924 at 280, feat(agc): look up table images at run time through a device image table on the GPU heap (APS5_GPU_PATH) #7642 at 291, feat(libSceUserService): take the user name from ANYPS5_USER_NAME #9345 at 234, feat(relinker): trace function imports on macOS #9408 at 65 and 317, perf(agc): GPU descriptor heap on VK_EXT_descriptor_heap (APS5_GPU_HEAP) #9508 at 317, feat(agc): serve lane-keyed, pointer and runtime image-table columns #10193 at 291. feat(relinker): trace function imports on macOS #9408, which also editsMacOsMachOPatcher.cpp, merges cleanly with this branch.Tested
Windows 11, GCC 15.2.0 (WinLibs UCRT), relinker-only Release build (
-DANYPS5_RELINKER_ONLY=ON -DBUILD_TESTING=ON); synthetic fixtures only, no title or homebrew, relinked outputs not run.macos_outputwith the new case on untouched d0e7fe7 fails with--macos with --lazy-binding: exit 0: Converting conflict.elf; with the change it passes in 0.6 s.import_fixture():--macos --lazy-binding,--lazy-binding --macosand--macos --lazy-binding --macos-fixed-base 0x400000exit 2 with the message and no output; the eager--macosoutput is byte-identical to that of themainbuild (sha256aed941e41b5ff6b0…).ctest -R 'macos|lazy|linux_fixed_base|executable_tls_relocations|trace_imports|conversion|cli_args|windows_fixed_base|linux_gnu_segments': 17 tests passed in 12.7 s,macos_output(0.6 s), the other sevenmacos_*tests,relinker_conversion,conversion_diagnostics,cli_args,linux_fixed_base,linux_gnu_segments,windows_fixed_base,executable_tls_relocationsand the Linux and Windowstrace_importstests.ctest --test-dir build-relinker -j 4: 244 tests, 240 passed,long_pathsskipped; three failures known on this machine onmainas well:select_tests(FileExistsError, Windows symlink fixture),patch_advisor_unit(build path with spaces, fixed in open test(tools): compare quoted and resolved paths in the patch advisor test #10667) andshader_cache_runtime_sources(timeout at 180 s under load; passes alone in 112 s).python tools/check_conventions.py --base upstream/mainwith this title and description passed.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)