Repository navigation
fix(relinker): preserve weak guest exports on macOS - #8118
Conversation
|
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.
|
|
Verdict: confirm. These are results from Windows and Linux hosts, plus the effect on real guest modules. Tested at
AI-assisted: yes (Claude Code); results above were run on this machine. |
Tested on Apple M1 Pro, macOS 27.0.1 (x86_64 build under Rosetta 2)Applied at
The other macOS and weak-symbol relinker tests ( I also ran the full test suite with this PR ( AI-assisted: yes (Claude Code applied the PR, ran the tests and wrote this report). |
thepoppfish
left a comment
There was a problem hiding this comment.
Source-focused review of exact head 50bc39a50db7b91298dbac24963d1925a1f9845e. No blocking findings.
The new binding check sets ExportWeakDefinition only for STB_WEAK and carries the flag through both ordinary and TLS exports. This matches the existing executable-export path in MacOsMachOPatcher.cpp. MacOsImage.cpp carries the TLS descriptor's flags into the final export entries before deriving MH_WEAK_DEFINES, so TLS-only weakness reaches the header too. The existing filtering of local, undefined, absolute, hidden and internal symbols remains in place; strong exports keep zero flags.
I reviewed all three changed files and the structural/native-control test paths. The test covers strong/weak function, data and TLS metadata on every host, and on supported macOS hosts compares function/data coalescing with native weak and strong providers. Its CI guard prevents missing Rosetta from silently skipping the native probe there. TLS runtime coalescing is not covered or claimed.
My local checks: tools/check_conventions.py --base origin/main --head refs/review/8118, whitespace checks, a clean merge-tree against main 2742004ed28f765c7147db0e0269ff5868e9eb6f, and Python syntax compilation of the new regression. I did not build or execute this PR's tests or run macOS/dyld locally.
The existing Windows/Linux report and macOS/Rosetta report already provide independent runtime/structural results and negative controls at this head; those are their reported results, not new runs by me. This approval adds a source review. No new title, TLS-runtime or full-integration claim is made. AI-assisted review: yes (OpenAI Codex).
What
A converted macOS guest module currently emits every definition as strong, even when its ELF symbol has
STB_WEAK. dyld therefore does not include those definitions in its weak-definition coalescing. A native consumer with the same weak function and global keeps its own copies instead of sharing the converted provider's definitions.Preserve
STB_WEAKon ordinary and TLS exports using the existing Mach-O export flags. The writer already derivesMH_WEAK_DEFINESfrom those flags. Strong definitions remain strong. This changes only the macOS guest writer; the executable-export implementation is already onmainthrough #7361.The regression test compares the converted provider with a native x86-64 dylib built by AppleClang, loading both through the real macOS loader in separate probe processes. The native weak provider's function/global give 18; before the fix the converted provider gives 165, and after the fix it gives 18. A strong-provider control matches its native counterpart as well. Structural checks cover weak and strong function, data and TLS exports, including the Mach-O header flag, on every test host.
No title-specific workaround or claim of a newly playable game is included.
Tested
macOS 27.2, Apple Silicon, AppleClang 21.0.0.21000334, Python 3.9.6, CMake 4.4.4 and Ninja 1.13.2, Release. Based on upstream
0d8c51a54e04c165f108b7bedbba3867e503118a.PATH; the first runs exposed a missing Ninja in a nested CMake fixture, which passed after correcting that environment.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)