You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
This repository was archived by the owner on Dec 1, 2024. It is now read-only.
I tested this and it works, but I don't fully understand why yet. Here's the difference between the symbol tables of a leveldown master build (as node_modules/ldmaster) and this PR (as node_modules/ldhidden):
Click to expand
$ objdump --syms node_modules/ldmaster/build/Release/leveldown.node | grep -i baseworker
0000000000000000 l d *UND* __ZTV10BaseWorker
0000000000004732 lw F __TEXT,__text __ZN10BaseWorkerC2EP10napi_env__P8DatabaseP12napi_value__PKc
0000000000004898 lw F __TEXT,__text __ZN10BaseWorker9DoFinallyEv
000000000000489e lw F __TEXT,__text __ZN10BaseWorker16HandleOKCallbackEv
0000000000004956 lw F __TEXT,__text __ZN10BaseWorkerD1Ev
0000000000004956 l F __TEXT,__text __ZN10BaseWorkerD0Ev
000000000000495c lw F __TEXT,__text __ZN10BaseWorker10DoCompleteEv
0000000000004a2a lw F __TEXT,__text __ZN10BaseWorkerD2Ev
0000000000004a80 lw F __TEXT,__text __ZN10BaseWorker9SetStatusEN7leveldb6StatusE
0000000000004b66 lw F __TEXT,__text __ZN10BaseWorker15SetErrorMessageEPKc
0000000000004912 gw F __TEXT,__text __ZN10BaseWorker7ExecuteEP10napi_env__Pv
0000000000004920 gw F __TEXT,__text __ZN10BaseWorker8CompleteEP10napi_env__11napi_statusPv
000000000002e4a0 lw O __DATA,__const __ZTV10BaseWorker
$ objdump --syms node_modules/ldhidden/build/Release/leveldown.node | grep -i baseworker
0000000000000000 l d *UND* __ZTV10BaseWorker
000000000000468a lw F __TEXT,__text __ZN10BaseWorkerC2EP10napi_env__P8DatabaseP12napi_value__PKc
00000000000047ec lw F __TEXT,__text __ZN10BaseWorker9DoFinallyEv
00000000000047f2 lw F __TEXT,__text __ZN10BaseWorker16HandleOKCallbackEv
00000000000048aa lw F __TEXT,__text __ZN10BaseWorkerD1Ev
00000000000048b0 lw F __TEXT,__text __ZN10BaseWorkerD0Ev
00000000000048b6 lw F __TEXT,__text __ZN10BaseWorker10DoCompleteEv
0000000000004980 lw F __TEXT,__text __ZN10BaseWorkerD2Ev
00000000000049d2 lw F __TEXT,__text __ZN10BaseWorker9SetStatusEN7leveldb6StatusE
0000000000004ab8 lw F __TEXT,__text __ZN10BaseWorker15SetErrorMessageEPKc
0000000000004866 lw F __TEXT,__text __ZN10BaseWorker7ExecuteEP10napi_env__Pv
0000000000004874 lw F __TEXT,__text __ZN10BaseWorker8CompleteEP10napi_env__11napi_statusPv
000000000002e418 lw O __DATA,__const __ZTV10BaseWorker
Note: I reordered the output for easier comparison.
Notice how for example the __ZN10BaseWorker7ExecuteEP10napi_env__Pv symbol is global (g) in the master build, but local (l) in the -fvisibility=hidden build. I'm guessing when two builds have global symbols with the same name (which happens on any two 5.x builds on mac), that's when conflicts like #686 arise.
@peakji thanks! There's one section that confuses me:
-fvisibility=hidden is not needed to fix this on macOS. From the GNU documentation:
On Darwin, default visibility means that the declaration is visible to other modules.
This is in contrast to ELF format (i.e. Linux), which allows declarations to also be overridden:
On ELF, default visibility means that the declaration is visible to other modules and, in shared libraries, means that the declared entity may be overridden.
What we're seeing is the opposite: we do need -fvisibility=hidden for mac, but not for linux.
The linux binary does contain global symbols - but not of BaseWorker, so it's possible the tests I've been doing don't cover it. Is there some other mechanism that prevents conflicts, or should I expand the tests to be sure?
I think the difference between linux and mac can be explained by their default mode of dlopen(). Linux defaults to RTLD_LOCAL ("Symbols defined in this library are not made available to resolve references in subsequently loaded libraries") while mac defaults to RTLD_GLOBAL.
Still, even when I manually do process.dlopen(.., dlopen.RTLD_GLOBAL | dlopen.RTLD_NOW) I can't get Linux to trip. Oh well, that's good and I'm in way over my head. Time to move on.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Labels
semver-patchBug fixes that are backward compatible
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #686. Alternative to #687 - see #687 (comment). I'm hoping for some feedback on nodejs/node-addon-api#460 (comment).
I tested this and it works, but I don't fully understand why yet. Here's the difference between the symbol tables of a
leveldownmaster build (asnode_modules/ldmaster) and this PR (asnode_modules/ldhidden):Click to expand
Note: I reordered the output for easier comparison.
Notice how for example the
__ZN10BaseWorker7ExecuteEP10napi_env__Pvsymbol is global (g) in the master build, but local (l) in the-fvisibility=hiddenbuild. I'm guessing when two builds have global symbols with the same name (which happens on any two 5.x builds on mac), that's when conflicts like #686 arise.