n-api: make more symbols local #16234
Conversation
nodejs-github-bot
added
c++
|
@addaleax I actually realized that most of the problems can be fixed with |
gabrielschulhof
force-pushed
the
napi-extern-hidden
branch
from
daa9605 to
b3ae886
Compare
|
@addaleax actually, we need one more delta: that we always call |
gabrielschulhof
force-pushed
the
napi-extern-hidden
branch
2 times, most recently
from
6334156 to
5cbe2ec
Compare
src/node_api.h
Outdated
| #ifdef EXTERNAL_NAPI | ||
| // Building external N-API, or native module against external N-API | ||
| #define NAPI_EXTERN /* nothing */ | ||
| #ifndef NAPI_EXTERN |
There was a problem hiding this comment.
I don't see the point of letting users override it. The definition needs to reflect how node_api.cc is compiled, otherwise linking won't work. You can't override NODE_EXTERN, UV_EXTERN or V8_EXPORT either.
There was a problem hiding this comment.
Well, it helps us out in node-addon-api by letting us do this:
'conditions': [
[ "OS=='win'", {
'defines': ['NAPI_EXTERN=']
}, {
'defines': ['NAPI_EXTERN=__attribute__((visibility("hidden")))']
} ]
]to prevent conflicts with Node.js between versions >= 8.0.0 and < 8.6.0.
There was a problem hiding this comment.
... where both node-addon-api and Node.js provide the same symbols, but node-addon-api's symbols must be preferred.
There was a problem hiding this comment.
But that's the wrong way to go about it. The right way is to fix the node-addon-api build so that the symbols from the static library are searched before symbols from the node binary.
This is in the context of nodejs/node-addon-api#142, right? If you look at the nm -C gist you posted, there are a lot of n-api symbols in there that are weak (W) when they shouldn't be.
There was a problem hiding this comment.
Actually, those symbols belong to the C++ wrapper, and none of them overlap with Node.js. None of the symbols which overlap with Node.js are weak.
I could find no way to cause the symbols from the static library to be searched before the symbols from Node.js, because without this change they are both declared with default visibility. In fact, visibility hidden is explicitly designed to indicate that, although the symbols are declared global because they have to be in different compilation units, they are not intended for consumption outside the .so and are thus to be treated, in a way, as static.
There was a problem hiding this comment.
OK, NM. I managed to get -fvisibility=hidden to work for node-addon-api. I guess you re-motivated me to try again :) Thanks!
There was a problem hiding this comment.
those symbols belong to the C++ wrapper, and none of them overlap with Node.js
Still, they probably shouldn't be weak. Different versions might clash and, if nothing else, it means the dynamic linker has to do more work when the module is loaded. Might be worth adding a regression test for in node-addon-api.
* namespaced functions such as v8impl::JsHandleScopeFromV8HandleScope become part of Node's public symbols unless they are declared static. * the class uvimpl::Work needs to be enclosed in an anonymous namespace else it, too becomes part of Node's public symbols. * remove references to EXTERNAL_NAPI.
gabrielschulhof
force-pushed
the
napi-extern-hidden
branch
from
5cbe2ec to
ad37743
Compare
|
Not sure if https://ci.nodejs.org/job/node-test-commit-aix/9461/ is related. @mhdawson? |
|
The ARM failures are also unrelated. |
|
@gabrielschulhof the AIX failure is not related, I had modified the config and temporarily broken the job. |
* namespaced functions such as v8impl::JsHandleScopeFromV8HandleScope become part of Node's public symbols unless they are declared static. * the class uvimpl::Work needs to be enclosed in an anonymous namespace else it, too becomes part of Node's public symbols. * remove references to EXTERNAL_NAPI. PR-URL: #16234 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
|
Landed in 7be4a84. |
* namespaced functions such as v8impl::JsHandleScopeFromV8HandleScope become part of Node's public symbols unless they are declared static. * the class uvimpl::Work needs to be enclosed in an anonymous namespace else it, too becomes part of Node's public symbols. * remove references to EXTERNAL_NAPI. PR-URL: #16234 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
* namespaced functions such as v8impl::JsHandleScopeFromV8HandleScope become part of Node's public symbols unless they are declared static. * the class uvimpl::Work needs to be enclosed in an anonymous namespace else it, too becomes part of Node's public symbols. * remove references to EXTERNAL_NAPI. PR-URL: nodejs/node#16234 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
* namespaced functions such as v8impl::JsHandleScopeFromV8HandleScope become part of Node's public symbols unless they are declared static. * the class uvimpl::Work needs to be enclosed in an anonymous namespace else it, too becomes part of Node's public symbols. * remove references to EXTERNAL_NAPI. PR-URL: nodejs/node#16234 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
* namespaced functions such as v8impl::JsHandleScopeFromV8HandleScope become part of Node's public symbols unless they are declared static. * the class uvimpl::Work needs to be enclosed in an anonymous namespace else it, too becomes part of Node's public symbols. * remove references to EXTERNAL_NAPI. PR-URL: nodejs#16234 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
* namespaced functions such as v8impl::JsHandleScopeFromV8HandleScope become part of Node's public symbols unless they are declared static. * the class uvimpl::Work needs to be enclosed in an anonymous namespace else it, too becomes part of Node's public symbols. * remove references to EXTERNAL_NAPI. Backport-PR-URL: #19447 PR-URL: #16234 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
become part of Node's public symbols unless they are declared static.
else it, too becomes part of Node's public symbols.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
n-api