SIGILL when using abort-on-uncaught-exception

[AndreasMadsen]

• Version: master (8b2c61c)
• Platform: posix
• Subsystem: NA

test case:

'use strict';
const assert = require('assert');
const { spawnSync } = require('child_process');

const child = spawnSync(process.execPath, ['--abort-on-uncaught-exception', '-e', 'throw new Error();']);
assert.strictEqual(child.signal, 'SIGABRT'); // bug, the return signal is SIGILL
assert.strictEqual(child.code, null);

When using --abort-on-uncaught-exception and an error throws, the process should exit with SIGABRT instead it exists with SIGILL (illegal instruction). When using process.abort() directly the process exits with SIGABRT as it should.

This is the abort message:

Uncaught Error

FROM
[eval]:1:1
ContextifyScript.Script.runInThisContext (vm.js:1:1)
Object.runInThisContext (vm.js:1:1)
Object.<anonymous> ([eval]-wrapper:1:1)
Module._compile (module.js:1:1)
evalScript (bootstrap_node.js:1:1)
startup (bootstrap_node.js:1:1)
bootstrap_node.js:1:1
Received signal 4 <unknown> 000100c40b92

==== C stack trace ===============================

 [0x000100c3f9c4]
 [0x7fffc7388bba]
 [0x7fff5fbfd7e0]
 [0x00010092034b]
 [0x3c3dde7043fd]
 [0x3c3dde807c7a]
 [0x3c3dde7bbe7c]
[end of stack trace]

[addaleax]

Has this been fixed by #13985? It might still be nice if you could find out the root cause of the SIGILL, because it points to a bug in V8, but if #13985 fixes it then Node won’t have to worry.

[AndreasMadsen]

I tested on 8b2c61c, that is after #13985 is applied. I just tested the very latest (fe730d3), same issue.

[addaleax]

Ah, right. This isn’t a new thing then, is it? It seems V8 has aborted like this since at least Node 0.12.

[AndreasMadsen]

Ah, right. This isn’t a new thing then, is it? It seems V8 has aborted like this since at least Node 0.12.

I think so. I checked 200 commits back and it happened before that.

[AndreasMadsen]

/cc @nodejs/v8

[targos]

what happens if you run it with --no-hard-abort?

[AndreasMadsen]

@targos then it exits with SIGABRT.

[targos]

This is setup here:

node/deps/v8/src/v8.cc

Line 75 in bec3877

base::OS::Initialize(FLAG_random_seed, FLAG_hard_abort, FLAG_gc_fake_mmap);

We can use Isolate::SetAbortOnUncaughtExceptionCallback to change the behavior.
The default is here:

node/deps/v8/src/isolate.cc

Lines 1114 to 1121 in 2db2857

	// Prevent endless recursion.
	FLAG_abort_on_uncaught_exception = false;
	// This flag is intended for use by JavaScript developers, so
	// print a user-friendly stack trace (not an internal one).
	PrintF(stderr, "%s\n\nFROM\n",
	MessageHandler::GetLocalizedMessage(this, message_obj).get());
	PrintCurrentStackTrace(stderr);
	base::OS::Abort();

[bnoordhuis]

Does anyone care enough about this issue to make the necessary changes or should we just close it out?

[bnoordhuis]

Since no one replied I'll go ahead and close out the issue, reopen if that's a mistake. :-)

(But note that to reopen is to volunteer.)

[addaleax]

Sorry, this has actually been fixed in #13985, I think