build: re-enable ThinLTO on macOS#51669
Conversation
3bc4823 to
9a99741
Compare
|
Could you trigger a release build from this branch before merging for verification, thanks! |
|
Re-enabling ThinLTO on macOS is surfacing a new issue for header-only classes with vtables. I need to fix this & then I'll mark this as ready for review again. |
9a99741 to
e66dbe9
Compare
Re-enable ThinLTO on macOS. Add a patch to the bundled Node.js source that anchors weak vtables and N-API entry points so they survive clang's -fwhole-program-vtables under ThinLTO. See the patch's commit message for details.
The previous approach (out-of-line member definitions plus explicit template instantiations) still produces weak_odr vtables, which Mach-O ThinLTO localizes during code generation even when other LTO partitions reference them. This breaks the link of executables that link libnode (electron_natives_codecache, node_mksnapshot) with undefined vtable symbol errors. Replace that part of the node patch with explicit specializations of the affected instantiations: - StaticExternalByteResource<uint8_t, char, ExternalOneByteStringResource> - StaticExternalByteResource<uint16_t, uint16_t, ExternalStringResource> - NgRcBufPointer<Http2RcBufferPointerTraits>::External An explicit specialization is an ordinary class that can have a key function (an out-of-line destructor), so its vtable is a strong symbol that LTO must keep external. The primary templates and all user code are unchanged, and no linker or compiler optimizations are disabled. The non-template fixes (MemoryRetainerNode, JSGraphJSNode key functions) and the N-API __attribute__((used)) changes are kept as-is.
e66dbe9 to
e82171a
Compare
|
Rebased onto main and reworked the template vtable part of the node patch - explicit instantiations still produce weak_odr vtables which lld-macho localizes during LTO codegen, so the new libnode-linking executables (electron_natives_codecache, node_mksnapshot) failed to link with the same undefined vtable errors. Replaced them with explicit specializations + out-of-line destructors so the vtables are strong symbols, same trick as your MemoryRetainerNode fix. The napi Full arm64 release build is green locally - framework links, napi symbols all survive, app boots. |
deepak1556
left a comment
There was a problem hiding this comment.
A way to reduce patch churn would be to upstream thin lto support for macOS, Node.js already got windows support via nodejs/node#61964
|
Release Notes Persisted
|
|
I was unable to backport this PR to "42-x-y" cleanly; |
|
I was unable to backport this PR to "41-x-y" cleanly; |
|
I have automatically backported this PR to "43-x-y", please check out #51819 |
|
@MarshallOfSound has manually backported this PR to "42-x-y", please check out #51823 |
* build: re-enable ThinLTO on macOS (#51669) * build: re-enable ThinLTO on macOS Re-enable ThinLTO on macOS. Add a patch to the bundled Node.js source that anchors weak vtables and N-API entry points so they survive clang's -fwhole-program-vtables under ThinLTO. See the patch's commit message for details. * build: anchor node template vtables with explicit specializations The previous approach (out-of-line member definitions plus explicit template instantiations) still produces weak_odr vtables, which Mach-O ThinLTO localizes during code generation even when other LTO partitions reference them. This breaks the link of executables that link libnode (electron_natives_codecache, node_mksnapshot) with undefined vtable symbol errors. Replace that part of the node patch with explicit specializations of the affected instantiations: - StaticExternalByteResource<uint8_t, char, ExternalOneByteStringResource> - StaticExternalByteResource<uint16_t, uint16_t, ExternalStringResource> - NgRcBufPointer<Http2RcBufferPointerTraits>::External An explicit specialization is an ordinary class that can have a key function (an out-of-line destructor), so its vtable is a strong symbol that LTO must keep external. The primary templates and all user code are unchanged, and no linker or compiler optimizations are disabled. The non-template fixes (MemoryRetainerNode, JSGraphJSNode key functions) and the N-API __attribute__((used)) changes are kept as-is. --------- Co-authored-by: Samuel Attard <sam@electronjs.org> * chore: update patches --------- Co-authored-by: Charles Kerr <charles@charleskerr.com> Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
Description of Change
Fixes #44632.
Re-enable ThinLTO on macOS. Add a new patch to ensure N-API entry points aren't stripped by ThinLTO by copying the
__attribute__((used))approach from nodejs/node#49037 .All reviews welcomed; CC @VerteDinde as a stakeholder for opening 44632
Checklist
npm testpassesRelease Notes
Notes: Enabled ThinLTO on macOS builds.