Repository navigation
feat(relinker): trace function imports on macOS - #9408
Conversation
|
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.
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.
|
jotitadev
left a comment
There was a problem hiding this comment.
Validated on Apple Silicon with Rosetta 2, which complements your run (your machine has no Rosetta, so the converted x86-64 Mach-O was not executed there).
- Machine: Apple M2 Max, macOS 27.2, AppleClang, relinker-only Release.
- Head: 989866c, on top of current
maina6dcea9 (merged clean).
macos_trace_imports goes through its full execution path here: it builds the x86-64 dylibs, relinks each input with --macos, and runs the converted Mach-O under Rosetta (/usr/bin/arch -x86_64). All import-tracing exit codes are exercised end to end:
- untraced → 11 (guest
sharedruns), traced → 77 (redirected throughTraceImport_nid_no_patch), --macos-fixed-base 0x400000→ 77 (tracing survives the fixed base),- data import (symbol_info 0x11) → 11 and weak import (0x22) → 11 (neither is traced),
- unsupported libc (no recorder) → nonzero with
TraceImport_nid_no_patchin stderr, --trace-importswithout libc.prx → exit 2--trace-imports requires libc.prx; with--lazy-binding→ conflict error.
The writable-segment stub (store the thunk, no page-protection change) holds up: the traced binaries actually redirect at run time under Rosetta, not just at relink time.
Full relinker-only CTest, both targets on this machine:
- arm64: 230/230 .
- x86-64 under Rosetta (
-DCMAKE_OSX_ARCHITECTURES=x86_64 -DPython3_EXECUTABLE=/usr/bin/python3, Python 3.9.6): 230/230 .
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
|
Windows and Linux non-regression of 989866c merged onto
AI-assisted: yes (Claude Code); results above were run on this machine. |
MarcZX10
left a comment
There was a problem hiding this comment.
The implementation has strong independent macOS/Rosetta runtime reports, and I independently built the relinker-only target from this exact head on Linux. python3 tools/check_conventions.py --base upstream/main passed, and ctest --test-dir build-review -R '^(macos_.*|linux_trace_imports)$' --output-on-failure passed 10/10, including macOS image generation and Linux trace-import regression. On this Linux host, macos_trace_imports exercised conversion only; its x86-64 Mach-O runtime path is platform-gated and was not run here. The relinker build emitted GCC optimizer warnings in unchanged Linux/test sources, with no build errors.
The branch is still based on 0c6a5bd5938f, 423 commits behind current main (da54c652e30b), and GitHub reports DIRTY; a read-only merge-tree check against current main identifies a docs/user/USAGE.md conflict. Please rebase, resolve the current documentation conflicts, rerun conventions and the focused relinker tests, and update the checklist's current-main claim.
There is also a new live overlap with #9439: both branches change CallTrace.cpp, docs/dev/TRACING.md, and docs/user/USAGE.md. A merge-tree check of the two heads reports conflicts in the two docs (the implementation additions in CallTrace.cpp merge together). Please coordinate with #9439's author on ordering, refresh the overlap report after rebasing, and record any prerequisite in Depends on. The existing overlap report also notes older uncomparable/conflicting relinker PRs; re-evaluate those on the rebased branch. No new duplicate implementation was established, but the ordering/doc conflicts are unresolved.
AI assistance was used to prepare this review.
|
@rursache a note on merge order: #10043, an opt-in FSR 1 scaler for the presented image, adds an "Upscaling" section to |
a56c23b to
af74b35
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.
maximilianfv
left a comment
There was a problem hiding this comment.
Request changes: on macOS the new import walk calls TraceImport_nid_no_patch with the stack misaligned by 8 bytes (measured below). Everything else I ran works, and the x86-64 recorder logs a traced guest call at run time.
Tested on Apple M1 Pro, macOS 27.0.1 (x86_64 build under Rosetta 2)
Applied at af74b35f8 to mugurc's macOS series (27e968974) merged with main at 2742004ed2, with other open macOS fixes, then toggled only this PR. The docs/dev/debt/relinker.md hunk was left out in that tree because its context differs there (the series adds an entry after that line); it is documentation only, and the PR merges cleanly with main. Full x86_64 build, so the relinker, libc.prx and every target linking librelinker_core were rebuilt in both states. Each test set ran twice per state.
trace_imports|macos_ tests (2 runs) |
Direct run: guest calls shared in a.prx, which aborts, APS5_TRACE_CALLS=all, real libc.prx |
|
|---|---|---|
| Without this PR | 10/10 passed (macos_trace_imports not registered); libc.prx exports none of the call-trace symbols |
untraced: SIGABRT, no trace; --trace-imports --macos refused: --trace-imports cannot be used with --macos |
| With this PR | 11/11 passed, including macos_trace_imports (its Rosetta execution path ran) |
untraced: SIGABRT, no trace; traced: the abort report lists #1 thread … a.prx shared from … output+0x10006 and 1 a.prx shared; same with --macos-fixed-base 0x400000 (from 0x400006) |
Stack probe: the same fixture with a libc.prx whose TraceImport_nid_no_patch is an -O0 function printing __builtin_frame_address(0) % 16. It reads 8 with --trace-imports, with and without --macos-fixed-base. A correct SysV call reads 0, and the Aps5StartGuest_nid_no_patch stub in the same process reads 0.
Findings
core/relinker/elfpatcher/src/macos/MacOsImage.cpp:105and:128: the walk pushes 5 registers and subtracts 8 (48 bytes) beforecall [rip+slot]at:119.LC_MAINis entered like a called function (rsp ≡ 8 mod 16), so the call goes out with rsp ≡ 8 andTraceImport_nid_no_patchstarts misaligned. The LinuxTraceProloguethis follows (3 pushes +sub rsp,8, 32 bytes) is aligned only because ELF_startbegins with rsp ≡ 0. Under Rosetta the real recorder survived this here. On an Intel Mac, any 16-byte-aligned stack access in the recorder's first call (mapping,new,unordered_map) would fault; I could not run that. The walk also overwritesr12(lea r12at:114) without saving it. Fix: saver12as Linux does, which also restores alignment (6 pushes + 8 = 56 bytes). Add0x41, 0x54after0x53at:105, and0x41, 0x5cbefore0x5bat:128. Intest_macos_trace_imports.py, makeHOOKcheck the alignment, for exampleif (((unsigned long)__builtin_frame_address(0) & 15) != 0) _exit(13);. The test builds its dylibs without-O, so that check is reliable, and it would have caught this.core/libs/prx/libc/CMakeLists.txt:777:guest_call_trace,guest_call_trace_abortandguest_call_trace_abort_racestill run only on Windows and Linux. As a result, the new Apple paths inCallTrace.cpp(mach_vm_remapthunk area, dyld module snapshot, abort report) have no test in the repository. My direct run above is the only run-time check of them. Enabling the test for x86-64 macOS needs one change:core/libs/tests/GuestCallTrace.cppdoes not compile with Apple clang as it is, because__builtin_sysv_va_listis an unknown type there (tried here against this PR'slibc.prx), so the variadic case needs plainva_listoff Windows. If that is out of scope, add a full-build test instead (macos_trace_importsalso runs in relinker-only builds, wherelibc.prxis not built): run one traced case against the builtlibc.prxwithAPS5_TRACE_CALLS=alland a provider that aborts, and matchlast traced system library calls. The run above shows this works on Rosetta hosts.core/libs/prx/libc/src/specifics/x86_64/CallTrace.cpp:186: the macOSSnapshotModulesrecords every segment with nonzeroinitprotinto the 256-entryModuleMap(:155) and stops silently when it is full. An x86-64 C++ program under Rosetta here loads 45 images with 198 such segments before any.prx. A title with a few dozen modules (about 4 segments each) therefore fills the map, and callers in the modules that do not fit print without a module name. Only return addresses are resolved, so record only segments withVM_PROT_EXECUTE(45 in that process), or skip images that haveMH_DYLIB_IN_CACHE.- Nit,
core/relinker/elfpatcher/src/macos/MacOsImage.cpp:530:(input.TraceImports ? traceFunctionSlot : startSlot) + 8istraceFunctionSlot + 8in both cases.
AI-assisted: yes (Claude Code applied the PR, ran the tests and the probes, read the code and wrote this review).
Walk each macOS function import before the guest entry and pass it through TraceImport_nid_no_patch, as Linux and Windows already do. An x86-64 libc build records the call and prints it on SIGABRT.
…utable segments Save r12 in the entry walk so TraceImport_nid_no_patch is called with an aligned stack, record only executable Mach-O segments in the module map, and make the test hook check the alignment. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
af74b35 to
44ea38f
Compare
maximilianfv
left a comment
There was a problem hiding this comment.
Approve: at 44ea38fc2 the macOS import walk calls TraceImport_nid_no_patch with a 16-byte-aligned stack (measured below), and macos_trace_imports now fails when the r12 save is removed. Finding 2 can be a follow-up; please squash the two commits (see 5).
Tested on Apple M1 Pro, macOS 27.0.1 (x86_64 build under Rosetta 2)
Applied at 44ea38fc2 to mugurc's macOS series (27e968974) merged with main at 2742004ed2, with other open macOS fixes and #9439 (this head builds on it; the CallTrace.cpp hunks need it), then toggled only this PR. The docs/dev/debt/relinker.md hunk was left out because its context differs in that tree. Full x86_64 build in both states. Each test set ran twice per state.
trace_imports|macos_ tests (2 runs) |
frame % 16 in TraceImport_nid_no_patch / Aps5StartGuest_nid_no_patch, with and without --macos-fixed-base 0x400000 |
|
|---|---|---|
| Without this PR | 10/10 passed (macos_trace_imports not registered) |
not applicable (no macOS import walk) |
| With this PR | 11/11 passed, including macos_trace_imports |
0 / 0 |
Control: this PR without the r12 push and pop, relinker rebuilt |
macos_trace_imports fails: AssertionError: ('traced', 13, '', '') |
8 / 0 |
Direct run with the built libc.prx, APS5_TRACE_CALLS=all and a provider that aborts: the SIGABRT report lists #1 thread … a.prx shared from 0x0000000140010006 output+0x10006 and 1 a.prx shared. With --macos-fixed-base 0x400000 it lists the same call from 0x0000000000400006 output+0x10006.
Findings from the previous review (at af74b35f8)
- Fixed.
core/relinker/elfpatcher/src/macos/MacOsImage.cpp:105pushesr12afterrbxand:128pops it, so the frame is 6 pushes plus 8 (56 bytes).test_macos_trace_imports.py:18exits 13 on a misaligned frame, and the control above shows the test catches the old layout. - Open, fine as a follow-up.
core/libs/prx/libc/CMakeLists.txt:777still registersguest_call_traceonly on Windows and Linux, andcore/libs/tests/GuestCallTrace.cpp:48still uses__builtin_sysv_va_list. The Apple paths inCallTrace.cpphave no test in the repository; the direct run above is the only run-time check. - Fixed.
core/libs/prx/libc/src/specifics/x86_64/CallTrace.cpp:225records onlyVM_PROT_EXECUTEsegments. - Fixed.
MacOsImage.cpp:530is nowtraceFunctionSlot + 8. - New nit: commit
66ca5c044adds a conflict marker atdocs/dev/TRACING.md:14(>>>>>>> af74b35f84 (feat(relinker): trace function imports on macOS)), and44ea38fc2removes it. Please squash the two commits so the marker does not land inmain's history.
AI-assisted: yes (Claude Code applied the PR, ran the tests, the probe and the control, read the code and wrote this review).
|
Checked the |
What
--trace-importswith--macoswalks each function import before the guest entry and passes the resolved pointer throughTraceImport_nid_no_patch, as Linux and Windows already do. A PLT slot, or a GOT slot whose symbol is a function, is traced. A data import, a weak import, a nonzero addend, a lookup throughsceKernelDlsym, and an import of a module loaded at run time are not. The macOS slot stays in a writable segment, so the stub stores the thunk and does not change the page protection.An x86-64 build of libc records the call and prints the trace on
SIGABRT. An arm64 build of libc does not include the recorder.python3 tools/pr_overlap.py --repo boykopovar/AnyPS5 --base main --branch feat/macos-trace-importsreports shared lines with #8658 (header padding on the same entry-stub placement inMacOsImage.cpp) and with #8973 and #9332 (the usage rows beside--trace-imports). None of them implement import tracing. #7706 also editsMacOsImage.cpp, at the GS-trap loop, and conflicts withmain, so that file was not compared. This change does not build on any of them.Tested
macOS, Apple Silicon, AppleClang 21.0.0, relinker-only Release. Rosetta is not installed, so an x86-64 Mach-O does not run on this machine.
Passed:
windows_trace_imports,linux_trace_imports,macos_trace_imports,macos_output,macos_tls_alu,macos_empty_tls,macos_tls_overlap,macos_gs_traps,macos_fixed_base,macos_executable_exports,macos_guest_weak_imports. Conventions passed.CallTrace.cppcompiled for x86-64.macos_trace_importsrelinks the traced call, the fixed-base call, a data import, a weak import, a libc that does not exportTraceImport_nid_no_patch, and an executable that does not needlibc.prx. The process run is skipped here (Bad CPU type in executable), which is the same skip the other macOS execution tests use when Rosetta is absent. The traced Mach-O was checked from the file:LC_MAINjumps to the walk, the walk callsTraceImport_nid_no_patch, and it jumps back to the existing entry stub.Not tested: a title, Windows, a Linux run of this change beyond the existing Linux relink checks, and execution of the traced Mach-O under Rosetta. The x86-64 recorder was compiled and not linked or run.
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)