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

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

Closed
gabrielschulhof opened this issue Sep 25, 2017 · 16 comments
Closed

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

gabrielschulhof opened this issue Sep 25, 2017 · 16 comments

Comments

@gabrielschulhof
Copy link
Contributor

gabrielschulhof commented Sep 25, 2017

$ nvm use 4
$ npm test

Observe build failure.

@gabrielschulhof
Copy link
Contributor Author

I tweaked the node-test-node-addon-api job a bit:

https://ci.nodejs.org/job/node-test-node-addon-api/1/ (nightly - good)
vs.
https://ci.nodejs.org/job/node-test-node-addon-api/2/ (v4 - bad)

@gabrielschulhof
Copy link
Contributor Author

@mhdawson
Copy link
Member

@gabrielschulhof can I assume you are investigating ? It is possible that its related to the async changes that were landed in master/head but I doubt have been backported to 4/6

@gabrielschulhof
Copy link
Contributor Author

Is this #144?

@gabrielschulhof
Copy link
Contributor Author

@mhdawson almost certainly, since it's precisely async that is tripping things up.

@gabrielschulhof
Copy link
Contributor Author

Actually, node-addon-api fails to build against nov 8.4.0 too. So AFAICT it's basically broken against any version of node other than the latest.

@gabrielschulhof
Copy link
Contributor Author

I'm starting to go through the errors, and I've noticed that we've introduced a call to v8::FunctionCallbackInfo's NewTarget() method. This method is not available in previous versions of V8, so can we fall back onto This()? Can NewTarget() be something other than This() when IsConstructCall() is true? @nodejs/addon-api

bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Sep 29, 2017
Add two checks that are there for expository reasons as much as they are
sanity checks.

Refs: nodejs/node-addon-api#142
@bnoordhuis
Copy link
Member

Can NewTarget() be something other than This() when IsConstructCall() is true?

Yes, see nodejs/node#15681. You could back-port nodejs/node#11652 to v4.x.

BridgeAR pushed a commit to nodejs/node that referenced this issue Oct 2, 2017
Add two checks that are there for expository reasons as much as they are
sanity checks.

PR-URL: #15681
Refs: nodejs/node-addon-api#142
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this issue Oct 3, 2017
Add two checks that are there for expository reasons as much as they are
sanity checks.

PR-URL: #15681
Refs: nodejs/node-addon-api#142
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this issue Oct 3, 2017
Add two checks that are there for expository reasons as much as they are
sanity checks.

PR-URL: #15681
Refs: nodejs/node-addon-api#142
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
addaleax pushed a commit to addaleax/ayo that referenced this issue Oct 4, 2017
Add two checks that are there for expository reasons as much as they are
sanity checks.

PR-URL: nodejs/node#15681
Refs: nodejs/node-addon-api#142
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this issue Oct 11, 2017
Add two checks that are there for expository reasons as much as they are
sanity checks.

PR-URL: #15681
Refs: nodejs/node-addon-api#142
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@gabrielschulhof
Copy link
Contributor Author

Hmmm ... on node 8.0.0 it's building our copy of node_api.cc and it's creating node-api.a, but then the linker resolves calls to node's own version, meaning the ABI is mismatched, running into segfaults as a result.

@gabrielschulhof
Copy link
Contributor Author

The problem can be solved with __attribute__((visibility("protected"))) but then you get the error "/usr/bin/ld: Release/obj.target/binding/error.o: relocation R_X86_64_PC32 against protected symbol `napi_create_error' can not be used when making a shared object"

@bnoordhuis
Copy link
Member

on node 8.0.0 it's building our copy of node_api.cc and it's creating node-api.a, but then the linker resolves calls to node's own version

Static linker or dynamic linker? What do nm -C and ldd print for build/Release/*.node?

My first hunch is that it's a link order issue. What does env V=1 node-gyp rebuild print?

@gabrielschulhof
Copy link
Contributor Author

@bnoordhuis
nm -C
ldd:

	linux-vdso.so.1 (0x00007ffd43787000)
	libstdc++.so.6 => /lib64/libstdc++.so.6 (0x00007f2078d42000)
	libm.so.6 => /lib64/libm.so.6 (0x00007f2078a2c000)
	libgcc_s.so.1 => /lib64/libgcc_s.so.1 (0x00007f2078815000)
	libpthread.so.0 => /lib64/libpthread.so.0 (0x00007f20785f6000)
	libc.so.6 => /lib64/libc.so.6 (0x00007f2078225000)
	/lib64/ld-linux-x86-64.so.2 (0x00007f207932b000)

V=1 npm test

I think it's a library load time issue. The symbols are available both locally (via node-api.a) and from Node.js, and the dynamic linker is resolving the needed symbols using Node's symbols rather than those from node-api.a, which is statically linked into binding.node.

Defining NAPI_EXTERN as __attribute__((visibility("hidden"))) causes the symbols in node-api.a to take precedence.

@gabrielschulhof
Copy link
Contributor Author

Wow! Another weird consequence. The Node.js async execute callback gets called, even though the async work item is created inside node-api.a.

@gabrielschulhof
Copy link
Contributor Author

That is, the version of uvimpl::Work::ExecuteCallback available inside node gets called, not the one available inside node-api.a.

@gabrielschulhof
Copy link
Contributor Author

I have to qualify uvimpl::Work::ExecuteCallback and uvimpl::Work::CompleteCallback with NAPI_EXTERN in order for them to get resolved correctly.

@bnoordhuis
Copy link
Member

bnoordhuis commented Oct 16, 2017

I also left a comment on nodejs/node#16234 but if you look at the output from nm, there are a lot of symbols in there that probably have no business being weak:

...
000000000002bafb W napi_value__* Napi::details::WrapCallback<Napi::RegisterModule(napi_env__*, napi_value__*, Napi::Object (*)(Napi::Env, Napi::Object))::{lambda()#1}>(Napi::RegisterModule(napi_env__*, napi_value__*, Napi::Object (*)(Napi::Env, Napi::Object))::{lambda()#1})
0000000000037078 W Napi::details::AccessorCallbackData<Napi::Value (*)(Napi::CallbackInfo const&), void (*)(Napi::CallbackInfo const&)>::GetterWrapper(napi_env__*, napi_callback_info__*)
0000000000037159 W Napi::details::AccessorCallbackData<Napi::Value (*)(Napi::CallbackInfo const&), void (*)(Napi::CallbackInfo const&)>::SetterWrapper(napi_env__*, napi_callback_info__*)
000000000002f638 W Napi::External<int>::New(napi_env__*, int*)
000000000002f302 t Napi::External<int> Napi::External<int>::New<(anonymous namespace)::CreateExternalWithFinalize(Napi::CallbackInfo const&)::{lambda(Napi::Env, int*)#1}>(napi_env__*, int*, (anonymous namespace)::CreateExternalWithFinalize(Napi::CallbackInfo const&)::{lambda(Napi::Env, int*)#1})
000000000002f3e4 t Napi::External<int> Napi::External<int>::New<(anonymous namespace)::CreateExternalWithFinalizeHint(Napi::CallbackInfo const&)::{lambda(Napi::Env, int*, char*)#1}, char>(napi_env__*, int*, (anonymous namespace)::CreateExternalWithFinalizeHint(Napi::CallbackInfo const&)::{lambda(Napi::Env, int*, char*)#1}, char*)
000000000002f7b8 W Napi::External<int>::External(napi_env__*, napi_value__*)
...

gabrielschulhof pushed a commit to gabrielschulhof/node-addon-api that referenced this issue Oct 16, 2017
* Implement node::MakeCallback between 8.0.0 and 8.6.0 to fix
  unresolved symbol
* Fix error message expectation (was: "Invalid pointer", is: "Invalid
  argument")
* Add -fvisibility=hidden to the compilation of sources of node_api.a
  so as to avoid conflicts with Node.js symbols

Fixes: nodejs#142
gabrielschulhof pushed a commit that referenced this issue Oct 17, 2017
* Implement node::MakeCallback between 8.0.0 and 8.6.0 to fix
  unresolved symbol
* Fix error message expectation (was: "Invalid pointer", is: "Invalid
  argument")
* Add -fvisibility=hidden to the compilation of sources of node_api.a
  so as to avoid conflicts with Node.js symbols

Fixes: #142
Fixes: nodejs/abi-stable-node#282
MylesBorins pushed a commit to nodejs/node that referenced this issue Oct 17, 2017
Add two checks that are there for expository reasons as much as they are
sanity checks.

PR-URL: #15681
Refs: nodejs/node-addon-api#142
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this issue Oct 25, 2017
Add two checks that are there for expository reasons as much as they are
sanity checks.

PR-URL: #15681
Refs: nodejs/node-addon-api#142
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this issue Aug 24, 2022
* Implement node::MakeCallback between 8.0.0 and 8.6.0 to fix
  unresolved symbol
* Fix error message expectation (was: "Invalid pointer", is: "Invalid
  argument")
* Add -fvisibility=hidden to the compilation of sources of node_api.a
  so as to avoid conflicts with Node.js symbols

Fixes: nodejs/node-addon-api#142
Fixes: nodejs/abi-stable-node#282
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this issue Aug 26, 2022
* Implement node::MakeCallback between 8.0.0 and 8.6.0 to fix
  unresolved symbol
* Fix error message expectation (was: "Invalid pointer", is: "Invalid
  argument")
* Add -fvisibility=hidden to the compilation of sources of node_api.a
  so as to avoid conflicts with Node.js symbols

Fixes: nodejs/node-addon-api#142
Fixes: nodejs/abi-stable-node#282
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this issue Sep 19, 2022
* Implement node::MakeCallback between 8.0.0 and 8.6.0 to fix
  unresolved symbol
* Fix error message expectation (was: "Invalid pointer", is: "Invalid
  argument")
* Add -fvisibility=hidden to the compilation of sources of node_api.a
  so as to avoid conflicts with Node.js symbols

Fixes: nodejs/node-addon-api#142
Fixes: nodejs/abi-stable-node#282
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this issue Aug 11, 2023
* Implement node::MakeCallback between 8.0.0 and 8.6.0 to fix
  unresolved symbol
* Fix error message expectation (was: "Invalid pointer", is: "Invalid
  argument")
* Add -fvisibility=hidden to the compilation of sources of node_api.a
  so as to avoid conflicts with Node.js symbols

Fixes: nodejs/node-addon-api#142
Fixes: nodejs/abi-stable-node#282
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants