n-api: make more symbols local #16234

gabrielschulhof · 2017-10-16T12:46:16Z

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.
allow NAPI_EXTERN to be defined on the compiler's command line.
remove references to EXTERNAL_NAPI.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

n-api

gabrielschulhof · 2017-10-16T12:48:02Z

@addaleax I actually realized that most of the problems can be fixed with static and with an anonymous namespace around uvimpl::Work. We can cherry-pick this commit into node-addon-api without grabbing the one that introduces the new V8 InstanceOf API, and we should be golden, even between 8.0.0 and 8.6.0.

gabrielschulhof · 2017-10-16T13:56:23Z

@addaleax actually, we need one more delta: that we always call node::node_module_register() with the version set to NODE_MODULE_VERSION rather than -1. That is, in Node.js we should always set it to -1, whereas in node-addon-api we should always set it to NODE_MODULE_VERSION. The delta is growing...

mhdawson

LGTM

bnoordhuis · 2017-10-16T15:08:36Z

src/node_api.h

-    #ifdef EXTERNAL_NAPI
-      // Building external N-API, or native module against external N-API
-      #define NAPI_EXTERN /* nothing */
+#ifndef NAPI_EXTERN


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.

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.

... where both node-addon-api and Node.js provide the same symbols, but node-addon-api's symbols must be preferred.

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.

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.

OK, NM. I managed to get -fvisibility=hidden to work for node-addon-api. I guess you re-motivated me to try again :) Thanks!

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 · 2017-10-18T18:23:45Z

CI: https://ci.nodejs.org/job/node-test-pull-request/10820/

gabrielschulhof · 2017-10-18T18:59:10Z

Not sure if https://ci.nodejs.org/job/node-test-commit-aix/9461/ is related. @mhdawson?

gabrielschulhof · 2017-10-18T19:06:35Z

https://ci.nodejs.org/job/node-test-commit-linux/13315/nodes=ubuntu1404-64/ and https://ci.nodejs.org/job/node-test-commit-linux/13315/nodes=ubuntu1604-32/ are almost certainly unrelated.

gabrielschulhof · 2017-10-18T19:08:41Z

The ARM failures are also unrelated.

mhdawson · 2017-10-18T22:02:16Z

@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 <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

gabrielschulhof · 2017-10-19T06:58:52Z

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 <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

* 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 <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

* 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 <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

* 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 <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Oct 16, 2017

gabrielschulhof force-pushed the napi-extern-hidden branch from daa9605 to b3ae886 Compare October 16, 2017 13:34

gabrielschulhof force-pushed the napi-extern-hidden branch 2 times, most recently from 6334156 to 5cbe2ec Compare October 16, 2017 14:01

jasnell approved these changes Oct 16, 2017

View reviewed changes

addaleax approved these changes Oct 16, 2017

View reviewed changes

cjihrig approved these changes Oct 16, 2017

View reviewed changes

mhdawson added the node-api Issues and PRs related to the Node-API. label Oct 16, 2017

mhdawson approved these changes Oct 16, 2017

View reviewed changes

bnoordhuis reviewed Oct 16, 2017

View reviewed changes

gabrielschulhof force-pushed the napi-extern-hidden branch from 5cbe2ec to ad37743 Compare October 16, 2017 20:19

bnoordhuis approved these changes Oct 16, 2017

View reviewed changes

bnoordhuis mentioned this pull request Oct 16, 2017

node-addon-api fails to build against node 4.x nodejs/node-addon-api#142

Closed

gabrielschulhof closed this Oct 19, 2017

gabrielschulhof deleted the napi-extern-hidden branch October 19, 2017 06:58

MylesBorins added the dont-land-on-v6.x label Nov 16, 2017

MylesBorins mentioned this pull request Apr 16, 2018

v6.14.2 proposal #19996

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

n-api: make more symbols local #16234

n-api: make more symbols local #16234

gabrielschulhof commented Oct 16, 2017

gabrielschulhof commented Oct 16, 2017

gabrielschulhof commented Oct 16, 2017 •

edited

mhdawson left a comment

bnoordhuis Oct 16, 2017

gabrielschulhof Oct 16, 2017

gabrielschulhof Oct 16, 2017

bnoordhuis Oct 16, 2017

gabrielschulhof Oct 16, 2017

gabrielschulhof Oct 16, 2017

bnoordhuis Oct 16, 2017

gabrielschulhof commented Oct 18, 2017

gabrielschulhof commented Oct 18, 2017

gabrielschulhof commented Oct 18, 2017

gabrielschulhof commented Oct 18, 2017

mhdawson commented Oct 18, 2017

gabrielschulhof commented Oct 19, 2017

n-api: make more symbols local #16234

n-api: make more symbols local #16234

Conversation

gabrielschulhof commented Oct 16, 2017

Checklist

Affected core subsystem(s)

gabrielschulhof commented Oct 16, 2017

gabrielschulhof commented Oct 16, 2017 • edited

mhdawson left a comment

Choose a reason for hiding this comment

bnoordhuis Oct 16, 2017

Choose a reason for hiding this comment

gabrielschulhof Oct 16, 2017

Choose a reason for hiding this comment

gabrielschulhof Oct 16, 2017

Choose a reason for hiding this comment

bnoordhuis Oct 16, 2017

Choose a reason for hiding this comment

gabrielschulhof Oct 16, 2017

Choose a reason for hiding this comment

gabrielschulhof Oct 16, 2017

Choose a reason for hiding this comment

bnoordhuis Oct 16, 2017

Choose a reason for hiding this comment

gabrielschulhof commented Oct 18, 2017

gabrielschulhof commented Oct 18, 2017

gabrielschulhof commented Oct 18, 2017

gabrielschulhof commented Oct 18, 2017

mhdawson commented Oct 18, 2017

gabrielschulhof commented Oct 19, 2017

gabrielschulhof commented Oct 16, 2017 •

edited