Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hide symbols for mac with -fvisibility=hidden #688

Merged
merged 1 commit into from
Oct 21, 2019
Merged

Conversation

vweevers
Copy link
Member

@vweevers vweevers commented Oct 19, 2019

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 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.

@vweevers vweevers added the semver-patch Bug fixes that are backward compatible label Oct 19, 2019
@vweevers vweevers requested a review from peakji October 19, 2019 14:35
@peakji
Copy link
Member

peakji commented Oct 21, 2019

I tested this and it works, but I don't fully understand why yet.

Maybe worth reading: https://labjack.com/news/simple-cpp-symbol-visibility-demo

@vweevers
Copy link
Member Author

@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.

@vweevers
Copy link
Member Author

Relevant node-gyp PR: nodejs/node-gyp#1891

@vweevers
Copy link
Member Author

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?

@vweevers
Copy link
Member Author

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.

@vweevers vweevers merged commit b263d80 into master Oct 21, 2019
@vweevers vweevers deleted the hide-symbols-for-mac branch October 21, 2019 21:23
@vweevers
Copy link
Member Author

OK, I did a test on mac, loading leveldown with RTLD_LOCAL | RTLD_NOW. No conflicts occur with these flags, so that confirms my suspicions.

A few unixes also default to RTLD_GLOBAL according to The Linux Programming Interface.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch Bug fixes that are backward compatible
Projects
None yet
3 participants