src: enable V8's WASM trap handlers #27246

devsnek · 2019-04-15T21:12:27Z

This uses SIGSEGV handlers to catch WASM out of bound (OOB) memory
accesses instead of inserting OOB checks inline, resulting in a 25%-30%
speed increase.

Note that installing a custom SIGSEGV handler will break this, resulting
in potentially scary behaviour. Users should use node::RegisterSignalHandler instead.

Refs: #14927

Closes #14927

Checklist

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

nodejs-github-bot · 2019-04-15T21:12:29Z

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3265

addaleax · 2019-04-15T21:20:06Z

My gut feeling would be that @ofrobots’s comment in #14927 (comment) is a good approach – using our own signal handler, and providing a public C++ API for forwarding that to addons/embedders? That would mean that those would not have to carry the responsibility of integrating with V8’s APIs.

I’d say this is semver-major because of the conflicts with existing published versions of segfault-handler – do you agree? If yes, then I’m happy with any approach, because we should have more than enough time to update that package.

devsnek · 2019-04-15T21:26:10Z

@addaleax we already have node::RegisterSignalHandler, i guess i could set this up with that, although i think this then becomes uh... not platform independent.

we'll need to add a bunch of platform specific switches for calling TryWasmTrapHanderPosix vs TryWasmTrapHandlerWin but if that complexity is okay then we can go for it.

devsnek · 2019-04-15T22:03:42Z

@addaleax set up to use RegisterSignalHandler, unfortunately we only have that for posix atm, so maybe we can expand to win in the future? (although i don't think people on windows are expecting great performance anyway 🤷‍♂️)

addaleax · 2019-04-15T22:54:24Z

@devsnek Yeah, we’d need to find some common API (or expose 2 variants) for both Windows and POSIX; and maybe find some API constraints, e.g. what happens if it is called multiple times, is the registering function threadsafe, etc. and document that

src/node.cc

joyeecheung · 2019-04-16T06:14:36Z

src/node.cc

@@ -452,9 +456,24 @@ void LoadEnvironment(Environment* env) {


 #ifdef __POSIX__
+
+struct sigaction previous_sigsegv_action;


Can you put this into the per_process namespace and make it static?

src/node.cc

bnoordhuis · 2019-04-16T09:07:06Z

Sorry, I forgot to mention something important: you must re-raise the SIGSEGV when V8 doesn't handle it and there's no other handler for it.

bnoordhuis · 2019-04-30T07:20:28Z

src/node.cc

+    } else {
+      uv_tty_reset_mode();
+#ifdef __FreeBSD__
+      // FreeBSD has a nasty bug, see RegisterSignalHandler for details


Not a blocker, just an observation, but I expect that's no longer relevant for the freebsd versions we support.

@nodejs/platform-freebsd can anyone confirm or deny?

@bnoordhuis do you know if there's any more background/detail on the issue?

@kostikbel any insight? The referenced comment is:

// FreeBSD has a nasty bug with SA_RESETHAND reseting the SA_SIGINFO, that is // in turn set for a libthr wrapper. This leads to a crash. // Work around the issue by manually setting SIG_DFL in the signal handler

From my copy of POSIX 1003.1-2017 AKA SUSv7:

SA_RESETHAND If set, the disposition of the signal shall be reset to SIG_DFL and the SA_SIGINFO flag shall be cleared on entry to the signal handler.

And we do exactly that. Am I reading right that this behavior causes some troubles, when combined with libthr ? Can you give a minimal example ?

@emaste @kostikbel The bug report is nodejs/node-v0.x-archive#9326, triage starts in nodejs/node-v0.x-archive#9326 (comment).

I'm reasonably sure that bug was fixed in fbsd 11 but you probably know for sure, @kostikbel, since you authored the fix.

Ok, so should this comment (and possibly some code) removed ? Since I believe all supported FreeBSD versions have the SA_RESETHAND | SA_SIGINFO fix.

That sounds about right (removing code & comment.) I've opened #27515 to track it.

src/node.cc

devsnek · 2019-04-30T13:42:13Z

@bnoordhuis do you know why this compiles fine on macos but has issues finding those atomic symbols on the linux build?

bnoordhuis · 2019-05-01T04:56:35Z

Reproducing here because scrolling trough Travis really is quite horrid:

/home/travis/build/nodejs/node/out/Release/obj.target/libnode/src/node.o: In function `node::TrapWebAssemblyOrContinue(int, siginfo_t*, void*) [clone .part.36]':
node.cc:(.text+0xce): undefined reference to `__atomic_load'
/home/travis/build/nodejs/node/out/Release/obj.target/libnode/src/node.o: In function `node::RegisterSignalHandler(int, void (*)(int, siginfo_t*, void*), bool, sigaction*)':
node.cc:(.text+0x2484): undefined reference to `__atomic_exchange'
collect2: error: ld returned 1 exit status
cctest.target.mk:226: recipe for target '/home/travis/build/nodejs/node/out/Release/cctest' failed
make[1]: *** [/home/travis/build/nodejs/node/out/Release/cctest] Error 1
make[1]: *** Waiting for unfinished jobs....
/home/travis/build/nodejs/node/out/Release/obj.target/libnode/src/node.o: In function `node::TrapWebAssemblyOrContinue(int, siginfo_t*, void*) [clone .part.36]':
node.cc:(.text+0xce): undefined reference to `__atomic_load'
/home/travis/build/nodejs/node/out/Release/obj.target/libnode/src/node.o: In function `node::RegisterSignalHandler(int, void (*)(int, siginfo_t*, void*), bool, sigaction*)':
node.cc:(.text+0x2484): undefined reference to `__atomic_exchange'
collect2: error: ld returned 1 exit status

I'm going to guess that gcc doesn't know how to lower the __atomic_load and __atomic_exchange builtins for compound types (i.e., struct sigaction) and just re-emits them as library calls.

If you switch to function pointers, the problem should hopefully go away.

richardlau · 2019-05-01T07:52:19Z

Or maybe link to libatomic? https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65756#c1

nodejs-github-bot · 2019-05-16T16:19:36Z

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

This uses SIGSEGV handlers to catch WASM out of bound (OOB) memory accesses instead of inserting OOB checks inline, resulting in a 25%-30% speed increase. Note that installing a custom SIGSEGV handler will break this, resulting in potentially scary behaviour. Refs: nodejs#14927 PR-URL: nodejs#27246 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

devsnek · 2019-05-16T17:34:46Z

landed in 70c7686

This uses SIGSEGV handlers to catch WASM out of bound (OOB) memory accesses instead of inserting OOB checks inline, resulting in a 25%-30% speed increase. Note that installing a custom SIGSEGV handler will break this, resulting in potentially scary behaviour. Refs: nodejs#14927 PR-URL: nodejs#27246 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

This workaround should no longer be necessary with supported versions of FreeSBD. Originall introduced in commit b64983d ("src: reset signal handler to SIG_DFL on FreeBSD") from March 2015. Fixes: nodejs#27515 Refs: nodejs#27246 (comment)

BridgeAR · 2019-05-21T09:43:57Z

@devsnek @addaleax is there a way to backport this PR with a compatibility patch to remove the breaking change? It would already be helpful if e.g. 50% of the code would be backported.

addaleax · 2019-05-21T10:52:52Z

@BridgeAR I guess it would be possible to backport this in a non-breaking manner, but it should not be released without #27775 as well

BridgeAR · 2019-05-21T11:00:31Z

@addaleax sounds good 👍. Would you be willing to open the backport after #27775 landed?

This workaround should no longer be necessary with supported versions of FreeSBD. Originally introduced in commit b64983d ("src: reset signal handler to SIG_DFL on FreeBSD") from March 2015. Fixes: nodejs#27515 Refs: nodejs#27246 (comment) Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

This workaround should no longer be necessary with supported versions of FreeSBD. Originally introduced in commit b64983d ("src: reset signal handler to SIG_DFL on FreeBSD") from March 2015. Fixes: nodejs#27515 PR-URL: nodejs#27780 Refs: nodejs#27246 (comment) Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

This macro is only defined when building Node.js, so addons cannot use it as a way of detecting feature availability. PR-URL: #27775 Refs: #27246 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Without this, we would re-enter the signal handler immediately after re-raising the signal, leading to an infinite loop. PR-URL: #27775 Refs: #27246 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

This is not easily implementable, and should be explicitly disallowed. PR-URL: #27775 Refs: #27246 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Ensure coverage for the different combinations of arguments. PR-URL: #27775 Refs: #27246 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

devsnek · 2019-09-09T17:37:35Z

Notable change:

This speeds up memory operations for WASM by around 25-30%. However, native code must now install the SIGSEGV signal handlers using node::RegisterSignalHandler.

Notable changes: - **assert**: - do not repeat .throws() code (Ruben Bridgewater) [#28263](#28263) - wrap validation function errors (Ruben Bridgewater) [#28263](#28263) - fix generatedMessage property (Ruben Bridgewater) [#28263](#28263) - improve class instance errors (Ruben Bridgewater) [#28263](#28263) - **benchmark**: - use test/common/tmpdir consistently (João Reis) [#28858](#28858) - **build**: - make full-icu the default for releases (Richard Lau) [#29887](#29887) - update minimum Xcode version for macOS (Michael Dawson) [#29622](#29622) - **child_process**: - runtime deprecate \_channel (cjihrig) [#27949](#27949) - simplify spawn argument parsing (cjihrig) [#27854](#27854) - **console**: - display timeEnd with suitable time unit (Xavier Stouder) [#29251](#29251) - **deps**: - patch V8 to 7.8.279.14 (Myles Borins) [#29694](#29694) - patch V8 to 7.8.279.12 (Myles Borins) [#29694](#29694) - patch V8 to 7.8.279.10 (Myles Borins) [#29694](#29694) - update V8's postmortem script (cjihrig) [#29694](#29694) - V8: cherry-pick 716875d (Myles Borins) [#29694](#29694) - update V8 to 7.8.279.9 (Myles Borins) [#29694](#29694) - V8: cherry-pick b33af60 (Michaël Zasso) [#28016](#28016) - update V8 to 7.6.303.28 (Michaël Zasso) [#28016](#28016) - **domain**: - error handler runs outside of its domain (Julien Gilli) [#26211](#26211) - **fs**: - make FSWatcher.start private (Lucas Holmquist) [#29905](#29905) - add runtime deprecate for file stream open() (Robert Nagy) [#29061](#29061) - allow int64 offset in fs.write/writeSync/fd.write (Zach Bjornson) [#26572](#26572) - use IsSafeJsInt instead of IsNumber for ftruncate (Zach Bjornson) [#26572](#26572) - allow int64 offset in fs.read/readSync/fd.read (Zach Bjornson) [#26572](#26572) - close file descriptor of promisified truncate (João Reis) [#28858](#28858) - **http**: - do not emit end after aborted (Robert Nagy) [#27984](#27984) - don't emit 'data' after 'error' (Robert Nagy) [#28711](#28711) - remove legacy parser (Anna Henningsen) [#29589](#29589) - throw if 'host' agent header is not a string value (Giorgos Ntemiris) [#29568](#29568) - replace superfluous connection property with getter/setter (Robert Nagy) [#29015](#29015) - fix test where aborted should not be emitted (Robert Nagy) [#20077](#20077) - remove default 'timeout' listener on upgrade (Luigi Pinca) [#26030](#26030) - **http, http2**: - remove default server timeout (Ali Ijaz Sheikh) [#27558](#27558) - **http2**: - remove security revert flags (Anna Henningsen) [#29141](#29141) - remove callback-based padding (Anna Henningsen) [#29144](#29144) - **lib**: - rename validateInteger to validateSafeInteger (Zach Bjornson) [#26572](#26572) - correct error.errno to always be numeric (Joyee Cheung) [#28140](#28140) - no need to strip BOM or shebang for scripts (Refael Ackermann) [#27375](#27375) - rework logic of stripping BOM+Shebang from commonjs (Gus Caplan) [#27768](#27768) - **module**: - runtime deprecate createRequireFromPath() (cjihrig) [#27951](#27951) - **readline**: - error on falsy values for callback (Sam Roberts) [#28109](#28109) - **repl**: - close file descriptor of history file (João Reis) [#28858](#28858) - **src**: - bring 425 status code name into accordance with RFC 8470 (Sergei Osipov) [#29880](#29880) - update NODE\_MODULE\_VERSION to 79 (Myles Borins) [#29694](#29694) - update NODE\_MODULE\_VERSION to 78 (Michaël Zasso) [#28918](#28918) - add error codes to errors thrown in C++ (Yaniv Friedensohn) [#27700](#27700) - use non-deprecated overload of V8::SetFlagsFromString (Michaël Zasso) [#28016](#28016) - update NODE\_MODULE\_VERSION to 77 (Michaël Zasso) [#28016](#28016) - update NODE\_MODULE\_VERSION to 74 (Refael Ackermann) [#27375](#27375) - make process.env.TZ setter clear tz cache (Ben Noordhuis) [#20026](#20026) - enable V8's WASM trap handlers (Gus Caplan) [#27246](#27246) - **stream**: - throw unhandled error for readable with autoDestroy (Robert Nagy) [#29806](#29806) - always invoke callback before emitting error (Robert Nagy) [#29293](#29293) - invoke callback before emitting error always (Robert Nagy) [#29293](#29293) - do not flush destroyed writable (Robert Nagy) [#29028](#29028) - don't emit finish on error (Robert Nagy) [#28979](#28979) - disallow stream methods on finished stream (Robert Nagy) [#28687](#28687) - do not emit after 'error' (Robert Nagy) [#28708](#28708) - fix destroy() behavior (Robert Nagy) [#29058](#29058) - simplify `.pipe()` and `.unpipe()` in Readable (Weijia Wang) [#28583](#28583) - **tools**: - patch V8 to run on older XCode versions (Ujjwal Sharma) [#29694](#29694) - update V8 gypfiles (Michaël Zasso) [#29694](#29694) - support full-icu by default (Steven R. Loomis) [#29522](#29522) - **util**: validate formatWithOptions inspectOptions (Ruben Bridgewater) [#29824](#29824) PR-URL: #29504

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 15, 2019

devsnek requested review from addaleax, ofrobots and joyeecheung April 15, 2019 21:12

devsnek force-pushed the enable-wasm-trap-handler branch from 2aa0fb8 to 5402f28 Compare April 15, 2019 21:18

devsnek added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 15, 2019

devsnek force-pushed the enable-wasm-trap-handler branch 3 times, most recently from ea0ade9 to ab9a5b7 Compare April 15, 2019 22:00

devsnek force-pushed the enable-wasm-trap-handler branch 2 times, most recently from 7c3fee8 to 0865e93 Compare April 15, 2019 22:18

devsnek added v8 engine Issues and PRs related to the V8 dependency. wasm Issues and PRs related to WebAssembly. labels Apr 16, 2019

devsnek commented Apr 16, 2019

View reviewed changes

src/node.cc Outdated Show resolved Hide resolved

devsnek force-pushed the enable-wasm-trap-handler branch from 0865e93 to a244828 Compare April 16, 2019 03:39

joyeecheung reviewed Apr 16, 2019

View reviewed changes

bnoordhuis reviewed Apr 16, 2019

View reviewed changes

src/node.cc Outdated Show resolved Hide resolved

src/node.cc Outdated Show resolved Hide resolved

src/node.cc Show resolved Hide resolved

src/node.cc Outdated Show resolved Hide resolved

src/node.cc Outdated Show resolved Hide resolved

devsnek force-pushed the enable-wasm-trap-handler branch 2 times, most recently from 94fbe09 to d05e688 Compare April 20, 2019 17:56

devsnek force-pushed the enable-wasm-trap-handler branch from d05e688 to c7d11fe Compare April 30, 2019 05:29

bnoordhuis reviewed Apr 30, 2019

View reviewed changes

devsnek added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed tsc-agenda Issues and PRs to discuss during the meetings of the TSC. labels May 16, 2019

devsnek force-pushed the enable-wasm-trap-handler branch from 3cdac98 to 70c7686 Compare May 16, 2019 17:33

devsnek merged commit 70c7686 into nodejs:master May 16, 2019

devsnek deleted the enable-wasm-trap-handler branch May 16, 2019 17:35

addaleax mentioned this pull request May 20, 2019

src: fix segfault handling/RegisterSignalHandler #27775

Closed

4 tasks

bnoordhuis mentioned this pull request May 20, 2019

src: remove freebsd SA_RESETHAND workaround #27780

Closed

BridgeAR added the dont-land-on-v12.x label May 21, 2019

addaleax mentioned this pull request Jun 14, 2019

test: skip test-worker-prof on all platforms. #28175

Closed

BethGriggs mentioned this pull request Sep 9, 2019

2019-10-22, Version 13.0.0 (Current) #29504

Merged

devsnek added the notable-change PRs with changes that should be highlighted in changelogs. label Sep 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: enable V8's WASM trap handlers #27246

src: enable V8's WASM trap handlers #27246

devsnek commented Apr 15, 2019 •

edited

nodejs-github-bot commented Apr 15, 2019

addaleax commented Apr 15, 2019

devsnek commented Apr 15, 2019 •

edited

devsnek commented Apr 15, 2019

addaleax commented Apr 15, 2019

joyeecheung Apr 16, 2019

bnoordhuis commented Apr 16, 2019

bnoordhuis Apr 30, 2019

devsnek Apr 30, 2019

emaste Apr 30, 2019

kostikbel Apr 30, 2019 •

edited

bnoordhuis May 1, 2019 •

edited

kostikbel May 1, 2019

bnoordhuis May 1, 2019

devsnek commented Apr 30, 2019 •

edited

bnoordhuis commented May 1, 2019

richardlau commented May 1, 2019

nodejs-github-bot commented May 16, 2019

devsnek commented May 16, 2019

BridgeAR commented May 21, 2019

addaleax commented May 21, 2019

BridgeAR commented May 21, 2019

devsnek commented Sep 9, 2019

		@@ -452,9 +456,24 @@ void LoadEnvironment(Environment* env) {


		#ifdef __POSIX__

		struct sigaction previous_sigsegv_action;

src: enable V8's WASM trap handlers #27246

src: enable V8's WASM trap handlers #27246

Conversation

devsnek commented Apr 15, 2019 • edited

Checklist

nodejs-github-bot commented Apr 15, 2019

addaleax commented Apr 15, 2019

devsnek commented Apr 15, 2019 • edited

devsnek commented Apr 15, 2019

addaleax commented Apr 15, 2019

joyeecheung Apr 16, 2019

Choose a reason for hiding this comment

bnoordhuis commented Apr 16, 2019

bnoordhuis Apr 30, 2019

Choose a reason for hiding this comment

devsnek Apr 30, 2019

Choose a reason for hiding this comment

emaste Apr 30, 2019

Choose a reason for hiding this comment

kostikbel Apr 30, 2019 • edited

Choose a reason for hiding this comment

bnoordhuis May 1, 2019 • edited

Choose a reason for hiding this comment

kostikbel May 1, 2019

Choose a reason for hiding this comment

bnoordhuis May 1, 2019

Choose a reason for hiding this comment

devsnek commented Apr 30, 2019 • edited

bnoordhuis commented May 1, 2019

richardlau commented May 1, 2019

nodejs-github-bot commented May 16, 2019

devsnek commented May 16, 2019

BridgeAR commented May 21, 2019

addaleax commented May 21, 2019

BridgeAR commented May 21, 2019

devsnek commented Sep 9, 2019

devsnek commented Apr 15, 2019 •

edited

devsnek commented Apr 15, 2019 •

edited

kostikbel Apr 30, 2019 •

edited

bnoordhuis May 1, 2019 •

edited

devsnek commented Apr 30, 2019 •

edited