fix: export V8 internals used by public headers - #2048
edusperoni wants to merge 2 commits into
Conversation
Inline code in V8's public headers calls out-of-line helpers that live in v8::internal:: and cppgc::internal::, which the version script hid: a plugin compiled against those headers failed to link on, among others, v8::internal::Internals::GetCurrentIsolate(), reached from v8::Object::GetInternalField(). Lists the referenced names individually rather than re-admitting the private namespaces wholesale. Adds 58 symbols to the arm64 dynamic symbol table (1683 -> 1741) and 16 KiB to libNativeScript.so.
src/main/cpp/include/libc++ has no tracked files and nothing populates it: download_v8.sh installs only the V8 include tree, which has no libc++. The comment above it described a custom STL that v8-buildscripts stopped producing when it moved to use_custom_libcxx=false against the stock NDK libc++; the constraint that replaces it is recorded where the monolith is linked.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe runtime CMake configuration removes the custom libc++ include path and moves the V8 NDK ABI compatibility comment. The ChangesCMake ABI configuration
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~5 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The configuration change follows the documented V8 NDK compatibility contract without changing library linkage. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit hops through CMake lines Comment |
1 participant
Description
Current behavior
Release flavors of
libNativeScript.soare linked withexported-symbols.map, which exports the public V8 API by case (v8::[A-Z]*,cppgc::[A-Z]*, …) and hides everything inv8::internal::/cppgc::internal::. But inline code in V8's public headers calls a handful ofV8_EXPORThelpers that live in those private namespaces, so a plugin compiled against the public headers cannot link against the runtime. The concrete case:v8::Object::GetInternalField()'s inline fast path callsv8::internal::Internals::GetCurrentIsolate(), which is absent from the dynamic symbol table of all three AAR flavors of 9.1.1:Plugins currently have to ship their own definition of that symbol to work around it.
Separately,
CMakeLists.txtstill addssrc/main/cpp/include/libc++to the include path, with a comment about V8 using a custom STL built withLIBCPP_ABI_UNSTABLE. That directory has no tracked files and nothing populates it (download_v8.shinstalls only V8's include tree), and v8-buildscripts now builds V8 withuse_custom_libcxx=falseagainst the stock NDK libc++.New behavior
include/references — 9 entries underv8::internal::and 20 undercppgc::internal::, static data members included (the write-barrier and pointer-compression fast paths read them directly). They are listed individually instead of re-admittingv8::internal::*/cppgc::internal::*, which would put tens of thousands of symbols back. Entries match a whole class or a bare function name followed by*, so they survive signature changes across V8 bumps; the header comment records the rule for adding to the list, and that an unquoted pattern cannot contain a parenthesis (an overload set can only be narrowed to its name).v8::internal, 40cppgc::internal; nothing removed, no otherv8::internal::symbol exposed),libNativeScript.so+16 KiB (+0.06 %).libv8_monolith.acarries the NDK libc++ it was compiled with, so v8-buildscripts must build V8 with the same NDK the runtime uses. The resulting.sois identical in symbol set and size to a build without that change.Two things worth a reviewer's opinion:
cppgc::internalpatterns currently match nothing (StrongPersistentPolicy,WeakPersistentPolicy, bothCrossThreadPersistentPolicys,BaseObjectSizeTrait,ExplicitManagementImpl,TraceTraitFromInnerAddressImpl,NameTraitBase,DCheckImpl,SameThreadEnabledCheckingPolicyBase): a version script controls visibility, it does not make lld extract archive members nothing references. So a plugin using e.g.cppgc::Persistent<T>would still fail to link. Forcing them in (-Wl,-uper symbol or--whole-archive) cuts against the size goal, so this PR does not; the patterns are left as statements of intent. Dropping allcppgc::internallines instead would take the delta from +58 to +18 symbols, if cppgc is considered outside the plugin surface.cppgc::internal::Fatal*also exportsFatalOutOfMemoryHandler::operator()and::SetCustomHandler(2 symbols with no inline caller), because the pattern cannot be narrowed past the name.Out of scope, noted for completeness: the case rule also hides public API in lower-case namespaces (
v8_inspector::,cppgc::subtle::,v8::metrics::). None is referenced by inline header code, so none fails to link merely from an#include, but a plugin calling them directly would.Related Pull Requests
edusperoni/nativescript-plugins#8 — the SQLite plugin's raw-V8 backend, which carries the
Internals::GetCurrentIsolateworkaround this makes unnecessary (v8_internals_shim.cpp).Does your pull request have unit tests?
No. The change is to the linker version script and a CMake include path; nothing in the repo inspects the exported surface today. It was verified manually:
:runtime:assembleRelease -Poptimized -Pabis=arm64-v8abefore and after and comparedllvm-nm -D --defined-only -Cof the stripped library: 58 names added, 0 removed, each added name traced to the inline body ininclude/that references it (checked both by reading everyV8_EXPORTin the public headers and by compiling a translation unit that includes every public V8/cppgc header and listing its undefined symbols).GetInternalField()links with--no-undefinedagainst the rebuiltlibNativeScript.sowith its workaround removed, and fails with the error above against both the published 9.1.1 library and a pre-change build.A cheap guard, if wanted as a follow-up: a CI step after the package build that compiles such a "surface" translation unit against
src/main/cpp/includeand asserts each of its undefinedv8::/cppgc::symbols is inllvm-nm -D --defined-onlyof the built library.Summary by CodeRabbit