Segmentation Fault #8310
Comments
|
|
Also, could you try running this with a debug build of node? |
Tested with
Arch Linux
Got rid of
Do I need to build it myself or is there an official debug build of node somewhere I can download or a docker image? |
I think you’d have to build it yourself, |
|
I just had a question on IRC wrt missing |
|
6.5.0 has not been promoted yet, which is why the headers are not available On Sun, Aug 28, 2016, 4:03 PM Johan Bergström [email protected]
|
|
@thealphanerd yeah, I know. I just emailed the maintainer in Archlinux about this and queried if he could align the releases with ours. |
|
Looks like I believe that makes this an Arch Linux issue, probably one of the shared libraries. I'll try playing with it a little bit more. |
Sounds quite possible… somebody else is experience very similar difficulties on Arch (almost identical stack trace) in npm/npm#13782 |
|
Experienced problems after upgrading node to 6.5 on arch as well, rolling back to 6.4 solves the issue tho. |
|
fwiw, I’ve set up Arch Linux in a VM, but couldn’t reproduce any problems so far… do you have something like reproducible test cases you could share (even if they are large) or something in that direction? |
|
@addaleax Apricity OS Arch distro update to node with packman from 6.4 to 6.5 leads to problem, rollback solves issue that's all i've got. Point me in some direction, how can i help work this out. |
|
You could checkout the node source from git and do a |
|
Or it could just be how Arch is building node... Either way building node manually from source will answer those questions. |
|
Building node from the tag with a normal |
|
@furkanmustafa What was/is your compiler version? |
|
Okay, so, I think I have an idea of what is going on here (big thanks to @leafi for providing access to an Arch VM for investigating!). Node v6.1.0: 000000000091d530 <_ZN2v88internal9FieldType7ConvertEPNS0_4ZoneE@@Base>:
91d530: 48 b8 00 00 00 00 01 movabs $0x100000000,%rax
91d537: 00 00 00
91d53a: 48 39 c7 cmp %rax,%rdi # IsAny()
91d53d: 74 61 je 91d5a0 <_ZN2v88internal9FieldType7ConvertEPNS0_4ZoneE@@Base+0x70>
91d53f: 48 85 ff test %rdi,%rdi # IsNone()
91d542: b8 01 00 00 00 mov $0x1,%eax
91d547: 74 4d je 91d596 <_ZN2v88internal9FieldType7ConvertEPNS0_4ZoneE@@Base+0x66>
91d549: 55 push %rbp
91d54a: 48 89 e5 mov %rsp,%rbp
...Node v6.2.0 (v6.5.0 looks the same): 0000000000920780 <_ZN2v88internal9FieldType7ConvertEPNS0_4ZoneE@@Base>:
920780: 48 b8 00 00 00 00 01 movabs $0x100000000,%rax
920787: 00 00 00
92078a: 48 39 c7 cmp %rax,%rdi # IsAny()
92078d: 74 51 je 9207e0 <_ZN2v88internal9FieldType7ConvertEPNS0_4ZoneE@@Base+0x60>
92078f: 55 push %rbp
920790: 48 89 e5 mov %rsp,%rbp
...This also explains why @NullDivision has been encountering npm/npm#13782 even prior to Node 6.5.0. I can’t compile anything for Arch with GCC 6 right now, but this completely naïve fix doesn’t break the Node.js tests for me, so it might be an option: diff --git a/deps/v8/src/field-type.cc b/deps/v8/src/field-type.cc
index 76d694c13262..5d84f3f6450b 100644
--- a/deps/v8/src/field-type.cc
+++ b/deps/v8/src/field-type.cc
@@ -13,7 +13,7 @@ namespace internal {
// static
FieldType* FieldType::None() {
- return reinterpret_cast<FieldType*>(Smi::FromInt(0));
+ return reinterpret_cast<FieldType*>(Smi::FromInt(2));
}
// static/cc @nodejs/v8 |
|
amazing work! /cc @nodejs/v8 On Mon, Aug 29, 2016, 7:40 AM Anna Henningsen [email protected]
|
|
That is some Rainman level work. I tip my hat to you sir. |
Not exactly a sir. :) |
|
Oops. :)) Well, imagine my embarrassment. Sorry. Handles are hard to follow. 😆 |
|
Also maybe /cc @felixonmars fyi |
|
Encountered the same issue. (node v6.5.0 on ArchLinux) |
|
@addaleax I am not sure if it is safe to change the value from None 0 to 2. I don't have access to a gcc-6.2 machine. Could you try something like this on the arch linux VM to see if the compiler is still agreeable: diff --git a/deps/v8/src/field-type.cc b/deps/v8/src/field-type.cc
index f1cb962..663f87f 100644
--- a/deps/v8/src/field-type.cc
+++ b/deps/v8/src/field-type.cc
@@ -11,14 +11,25 @@
namespace v8 {
namespace internal {
+static const int kNone = 0;
+static const int kAny = 1;
+
// static
FieldType* FieldType::None() {
- return reinterpret_cast<FieldType*>(Smi::FromInt(0));
+ return reinterpret_cast<FieldType*>(Smi::FromInt(kNone));
+}
+
+bool FieldType::IsNone() {
+ return reinterpret_cast<intptr_t>(this) == kNone;
}
// static
FieldType* FieldType::Any() {
- return reinterpret_cast<FieldType*>(Smi::FromInt(1));
+ return reinterpret_cast<FieldType*>(Smi::FromInt(kAny));
+}
+
+bool FieldType::IsAny() {
+ return reinterpret_cast<intptr_t>(this) == kAny;
}
// static
diff --git a/deps/v8/src/field-type.h b/deps/v8/src/field-type.h
index eb7ffca..4eebf82 100644
--- a/deps/v8/src/field-type.h
+++ b/deps/v8/src/field-type.h
@@ -33,8 +33,8 @@ class FieldType : public Object {
bool IsClass();
Handle<i::Map> AsClass();
- bool IsNone() { return this == None(); }
- bool IsAny() { return this == Any(); }
+ bool IsNone();
+ bool IsAny();
bool NowStable();
bool NowIs(FieldType* other);
bool NowIs(Handle<FieldType> other); |
|
/cc @jeisinger on whether it is safe to change the None |
I don't think that makes it any less UB. A conforming compiler can still optimize it to |
|
@ofrobots Thanks, will do! If you see anything obviously wrong with the patch, please say so :)
Done, nodejs/build#480 |
|
It is pretty bad that V8 relies on a variety of undefined/implementation defined behaviors in C++ compilers, and this seems to be only the tip of the iceberg. But as sad as it is, there's not much we can do about this short-term, except trying to work-around issues as in this case. Maybe we (as in V8 + Node.js) can raise awareness of this problem to the GCC (and clang) folks, and somehow buy us some time to first address all the undefined behavior (or at least the serious stuff) before the compilers start to exploit UB for optimizations (although in my opinion this is already a pretty, pretty bad idea anyways, esp. in a language like C++). |
When FieldType::None() returns a cast Smi::FromInt(0), which translates as nullptr, the FieldType::IsNone() check becomes equivalent to `this == nullptr` which is not allowed by the standard and therefore optimized away as a false constant by GCC 6. This has lead to crashes when invoking methods on FieldType::None(). Using a different Smi constant for FieldType::None() makes the compiler always include a comparison against that value. The choice of these constants has no effect as they are effectively arbitrary. BUG=nodejs/node#8310 Review-Url: https://codereview.chromium.org/2292953002 Cr-Commit-Position: refs/heads/master@{#39023}
|
@bmeurer Perhaps you can add |
|
@bnoordhuis I guess that would be doable. Does that only affect GCC 6 behavior? Is there anything to be aware of? |
|
@bmeurer I turned on the flag in v0.10 and v0.12 and I couldn't find a measurable difference but that's with V8 3.14 and 3.28 (and many more moving parts, of course.) |
|
@bnoordhuis Do you need this on the V8 side? If so, can you file a ticket and assign to to [email protected]? |
|
@bmeurer I don't need it for node.js, we can control that through our own build flags. I was thinking that disabling it in V8 might save smaller embedders like plv8 some head scratching. |
|
Ok, I leave that decision to @jeisinger. |
|
To make the decision tangible: https://codereview.chromium.org/2310513002. |
Original commit message:
Make FieldType::None() non-nullptr value to avoid undefined behaviour
When FieldType::None() returns a cast Smi::FromInt(0), which translates
as nullptr, the FieldType::IsNone() check becomes equivalent to
`this == nullptr` which is not allowed by the standard and
therefore optimized away as a false constant by GCC 6.
This has lead to crashes when invoking methods on FieldType::None().
Using a different Smi constant for FieldType::None() makes the compiler
always include a comparison against that value. The choice of these
constants has no effect as they are effectively arbitrary.
BUG=nodejs#8310
Review-Url: https://codereview.chromium.org/2292953002
Cr-Commit-Position: refs/heads/master@{nodejs#39023}
Fixes: nodejs#8310
Original commit message:
Make FieldType::None() non-nullptr value to avoid undefined behaviour
When FieldType::None() returns a cast Smi::FromInt(0), which translates
as nullptr, the FieldType::IsNone() check becomes equivalent to
`this == nullptr` which is not allowed by the standard and
therefore optimized away as a false constant by GCC 6.
This has lead to crashes when invoking methods on FieldType::None().
Using a different Smi constant for FieldType::None() makes the compiler
always include a comparison against that value. The choice of these
constants has no effect as they are effectively arbitrary.
BUG=#8310
Review-Url: https://codereview.chromium.org/2292953002
Cr-Commit-Position: refs/heads/master@{#39023}
Fixes: #8310
PR-URL: #8411
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
I think the most serious stuff is casting generated code into a function pointer and calling it, which is pretty much unavoidable. |
|
Clang doesn't support |
|
Clang does know about |
|
Does anyone have enough knowledge of Clang and/or LLVM to implement On Sep 16, 2016 4:20 AM, "Ben Noordhuis" [email protected] wrote: CodeGenFunction::ShouldNullCheckClassCastValue() in clang 3.8 suggests that Clang does know about -fno-delete-null-pointer-checks but it's a no-op. — |
|
It should be easy enough to add but I don't have time at the moment. You are welcome to run with it. |
|
Patch against clang 3.9.0. Still need to update the test suite but I thought I'd post it here in case I forget; someone is sure to remind me in due time. :-) |
|
I'm on ArchLinux. I use node v6.9.1, and I experience ghcjs/ghcjs#542 |
|
@crocket Then it’s a different bug, this one was fixed in v6.6.0. If you think it’s reasonable to believe you are experiencing a bug with Node core, I’d suggest that you try to write down a full reproduction of that problem and open a new issue here. A stack trace or a full core dump for the segfault would also be very useful information, if that’s possible. |
|
How do I provide a stack trace of a full core dump? Can you give me links? |
|
https://gist.github.com/crocket/0e68d3d6f27c280daa3b7449337a8f94 is the output of |
|
@crocket I’m not sure why this might look like a bug in Node – fwiw, the file listed under |
|
This was fixed in f829660, closing. |
6.5.0Linux thehostname 4.7.2 #21 SMP Fri Aug 26 03:23:44 UTC 2016 x86_64 GNU/LinuxI'm not exacly sure what's the main reason for this crash is, therefore I don't have a way to replicate. My application is quite simple and doesn't have any native modules. General outline is, the app connects to Mongo & Redis, uses babel, and is a simple background worker for kue tasks. Application works well for a while, and then segfaults.
Here is the coredumps;
The text was updated successfully, but these errors were encountered: