-
Notifications
You must be signed in to change notification settings - Fork 465
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
[WIP][do not merge] Illustrate osx crash #456
Closed
gabrielschulhof
wants to merge
4
commits into
nodejs:master
from
gabrielschulhof:illustrate-osx-crash
Closed
[WIP][do not merge] Illustrate osx crash #456
gabrielschulhof
wants to merge
4
commits into
nodejs:master
from
gabrielschulhof:illustrate-osx-crash
Conversation
This file contains 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
Add method `SuppressDestruct()` to `AsyncWorker`, which will cause an instance of the class to remain allocated even after the `OnOK` callback fires. Such an instance must be explicitly `delete`-ed from user code. Re: nodejs#231 Re: nodejs/abi-stable-node#353
gabrielschulhof
pushed a commit
to gabrielschulhof/node-gyp
that referenced
this pull request
Mar 11, 2019
On OSX symbols are exported by default, and they overlap if two different copies of the same symbol appear in a process. This change ensures that, on OSX, symbols are hidden by default. Re: nodejs/node-addon-api#456
3 tasks
gabrielschulhof
pushed a commit
to gabrielschulhof/node-gyp
that referenced
this pull request
Mar 11, 2019
On OSX symbols are exported by default, and they overlap if two different copies of the same symbol appear in a process. This change ensures that, on OSX, symbols are hidden by default. Re: nodejs/node-addon-api#456
gabrielschulhof
pushed a commit
to gabrielschulhof/node-gyp
that referenced
this pull request
Mar 12, 2019
On OSX symbols are exported by default, and they overlap if two different copies of the same symbol appear in a process. This change ensures that, on OSX, symbols are hidden by default. Re: nodejs/node-addon-api#456
gabrielschulhof
pushed a commit
to gabrielschulhof/node-gyp
that referenced
this pull request
Mar 19, 2019
On OSX symbols are exported by default, and they overlap if two different copies of the same symbol appear in a process. This change ensures that, on OSX, symbols are hidden by default. Re: nodejs/node-addon-api#456 Fixes: nodejs/node#26765
I think this can be closed now, as a PR went in to address the issue. |
I think we've established the issue with OSX and arrived at a solution. |
gabrielschulhof
pushed a commit
to gabrielschulhof/node-gyp
that referenced
this pull request
Jun 4, 2019
On OSX symbols are exported by default, and they overlap if two different copies of the same symbol appear in a process. This change ensures that, on OSX, symbols are hidden by default. Re: nodejs/node-addon-api#456 Fixes: nodejs/node#26765
rvagg
pushed a commit
to nodejs/node-gyp
that referenced
this pull request
Jun 20, 2019
On OSX symbols are exported by default, and they overlap if two different copies of the same symbol appear in a process. This change ensures that, on OSX, symbols are hidden by default. Re: nodejs/node-addon-api#456 Fixes: nodejs/node#26765 PR-URL: #1689 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Refael Ackermann <refack@gmail.com>
rvagg
pushed a commit
to nodejs/node-gyp
that referenced
this pull request
Jun 20, 2019
On OSX symbols are exported by default, and they overlap if two different copies of the same symbol appear in a process. This change ensures that, on OSX, symbols are hidden by default. Re: nodejs/node-addon-api#456 Fixes: nodejs/node#26765 PR-URL: #1689 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Refael Ackermann <refack@gmail.com>
rvagg
pushed a commit
to nodejs/node-gyp
that referenced
this pull request
Jun 21, 2019
Use `Nan::Set()` and `Nan::GetFunction()` instead of their V8 equivalents to avoid OSX test failures with Node.js v12.x. Thanks @rvagg! Re: nodejs/node-addon-api#456 Fixes: nodejs/node#26765 PR-URL: #1689 Reviewed-By: Rod Vagg <rod@vagg.org>
rvagg
pushed a commit
to nodejs/node-gyp
that referenced
this pull request
Jun 21, 2019
On OSX symbols are exported by default, and they overlap if two different copies of the same symbol appear in a process. This change ensures that, on OSX, symbols are hidden by default. Re: nodejs/node-addon-api#456 Fixes: nodejs/node#26765 PR-URL: #1689 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Refael Ackermann <refack@gmail.com>
rvagg
pushed a commit
to nodejs/node-gyp
that referenced
this pull request
Jun 21, 2019
Use `Nan::Set()` and `Nan::GetFunction()` instead of their V8 equivalents to avoid OSX test failures with Node.js v12.x. Thanks @rvagg! Re: nodejs/node-addon-api#456 Fixes: nodejs/node#26765 PR-URL: #1689 Reviewed-By: Rod Vagg <rod@vagg.org>
rvagg
pushed a commit
to nodejs/node-gyp
that referenced
this pull request
Sep 26, 2019
On OSX symbols are exported by default, and they overlap if two different copies of the same symbol appear in a process. This change ensures that, on OSX, symbols are hidden by default. Re: nodejs/node-addon-api#456 Fixes: nodejs/node#26765 PR-URL: #1689 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Refael Ackermann <refack@gmail.com>
rvagg
pushed a commit
to nodejs/node-gyp
that referenced
this pull request
Sep 26, 2019
Use `Nan::Set()` and `Nan::GetFunction()` instead of their V8 equivalents to avoid OSX test failures with Node.js v12.x. Thanks @rvagg! Re: nodejs/node-addon-api#456 Fixes: nodejs/node#26765 PR-URL: #1689 Reviewed-By: Rod Vagg <rod@vagg.org>
rvagg
pushed a commit
to nodejs/node-gyp
that referenced
this pull request
Oct 3, 2019
On OSX symbols are exported by default, and they overlap if two different copies of the same symbol appear in a process. This change ensures that, on OSX, symbols are hidden by default. Re: nodejs/node-addon-api#456 Fixes: nodejs/node#26765 PR-URL: #1689 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Refael Ackermann <refack@gmail.com>
rvagg
pushed a commit
to nodejs/node-gyp
that referenced
this pull request
Oct 3, 2019
Use `Nan::Set()` and `Nan::GetFunction()` instead of their V8 equivalents to avoid OSX test failures with Node.js v12.x. Thanks @rvagg! Re: nodejs/node-addon-api#456 Fixes: nodejs/node#26765 PR-URL: #1689 Reviewed-By: Rod Vagg <rod@vagg.org>
This was referenced Oct 19, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This branch illustrates a crash I've seen on OSX ever since I introduced the
suppressDestruct()
method forAsyncWorker
. To reproduce:${ADDON_API_DIR}
${NODEDIR}
export npm_config_nodedir=${NODEDIR}
export PATH=${NODEDIR}:${PATH}
alias npm=${NODEDIR}/deps/npm/bin/npm-cli.js
npm run-script pretest
node test/asyncworker-persistent.js
At this point it will segfault. The backtrace is as follows:
Note how
frame #12
is in a different native addon than the rest of the frames. This is strange. Different instances of native addons should not be able to unintentionally reach each other's functions.So, assuming that there's something fishy about the way OSX loads two different addons which contain identical symbols, but where one is compiled with C++ exceptions and the other is not, we can add
-fvisibility=hidden
to the build flags intest/binding.gyp
:This does indeed prevent the segfault from occurring. It also prevents execution from jumping across native addon boundaries:
Another experimental change we can make is to compile
bindings.node
andbindings_noexcept.node
identically, without C++ exceptions. That will also result in a successful run.Another possibility, that of adding
__attribute__((visibility("hidden")))
to individual functions, especiallyOnWorkComplete()
andOnExecute()
results in a successful run on Node.js 10.15.0, but not on the latest master.Yet another experimental change also fixes the segfault. We can disambiguate the class names at the top of
napi.h
:As an additional step we can identify which functions end up affected by the above preprocessor directives by running
nm -Ca test/build/Debug/binding_noexcept.node | grep No_Except | grep ' T '
on the resulting native addon, and mark them as hidden using__attribute__((visibility("hidden")))
. After that, we can remove the above preprocessor definitions. And, indeed, with the used symbols now hidden, there is no more segfault:This obviously addresses the use case of running the test, however, in general, we should mark all symbols as hidden.