Node: Object.is(-Number.MIN_VALUE, -0) broken on ≥ v10.0.0

[TimothyGu]

• Version: v10.0.0–master
• Platform: Linux 4.9.0-8-amd64 SMP Debian 4.9.130-2 (2018-10-27) x86_64 GNU/Linux
• Subsystem: v8 (TurboFan)

This seems to be fixed as of upstream V8 7.2.502.13 (in Google Chrome 72.0.3626.28 beta) but is in all Node.js releases after 10.0.0 and the current master. It is distinct from #25221.

// test-1.js
function a() {
  if (Object.is(-Number.MIN_VALUE, -0)) {
    throw 'BAD';
  }
}

a();
a();
%OptimizeFunctionOnNextCall(a);
a();

throws "BAD" after TurboFan runs. --print-opt-code reveals that the expression Object.is(-Number.MIN_VALUE, -0) was constant-folded to true, which is not good.

---

// test-2.js
function a(value) {
  if (Object.is(value, -0)) {
    throw 'BAD';
  }
}

a(0.1);
a(0.1);
%OptimizeFunctionOnNextCall(a);
a(-Number.MIN_VALUE);

Like before, this throws "BAD", but avoids constant folding. --print-opt-code reveals a complex set of floating point instructions emitted by TurboFan, that eventually lead to a bad result.

Disassembly

0x1603c60c1a83    43  c5fb104007     vmovsd xmm0,[rax+0x7]
0x1603c60c1a88    48  c5f176c9       vpcmpeqd xmm1,xmm1,xmm1
0x1603c60c1a8c    4c  c5f173f136     vpsllq xmm1,xmm1,54
0x1603c60c1a91    51  c5f173d102     vpsrlq xmm1,xmm1,2
0x1603c60c1a96    56  c5f35ec0       vdivsd xmm0,xmm1,xmm0
0x1603c60c1a9a    5a  c5f928c0       vmovapd xmm0,xmm0
0x1603c60c1a9e    5e  c5f176c9       vpcmpeqd xmm1,xmm1,xmm1
0x1603c60c1aa2    62  c5f173f134     vpsllq xmm1,xmm1,52
0x1603c60c1aa7    67  c5f92ec8       vucomisd xmm1,xmm0
0x1603c60c1aab    6b  0f8a3a000000   jpe 0x1603c60c1aeb  <+0xab> (tg note: = return undefined)
0x1603c60c1ab1    71  0f8534000000   jnz 0x1603c60c1aeb  <+0xab> (tg note: = return undefined)
0x1603c60c1ab7    77  48bb3963e644e83b0000 REX.W movq rbx,0x3be844e66339    ;; object: 0x3be844e66339 <String[3]: BAD>
0x1603c60c1ac1    81  53             push rbx
0x1603c60c1ac2    82  48bb80eb070100000000 REX.W movq rbx,0x107eb80
0x1603c60c1acc    8c  488bd0         REX.W movq rdx,rax
0x1603c60c1acf    8f  48be413718f6c93d0000 REX.W movq rsi,0x3dc9f6183741    ;; object: 0x3dc9f6183741 <NativeContext[249]>
0x1603c60c1ad9    99  b801000000     movl rax,0x1
0x1603c60c1ade    9e  49ba4033690100000000 REX.W movq r10,0x1693340  (CEntry_Return1_DontSaveFPRegs_ArgvOnStack_NoBuiltinExit)
0x1603c60c1ae8    a8  41ffd2         call r10
0x1603c60c1aeb    ab  498b45a0       REX.W movq rax,[r13-0x60] (root (0x12d54a0825a1 <undefined>))
0x1603c60c1aef    af  488be5         REX.W movq rsp,rbp
0x1603c60c1af2    b2  5d             pop rbp
0x1603c60c1af3    b3  c21000         ret 0x10

---

/cc @devsnek @nodejs/v8

[ryzokuken]

@TimothyGu I guess all we need to do is identify which patch fixed this and backport it to all affected release lines?

[TimothyGu]

@ryzokuken Yeah I think so, though I don't have the resources to do that at this moment. Even additional confirmation that V8 7.2 fixes is would be nice as I don't trust that I tested things correctly with Chrome. Tested with latest node-v8 canary; is indeed fixed there.

[TimothyGu]

The latest version of V8 seems to be just better at optimizing this sequence of calls

Disassembly

0x2e68d363f80f    4f  c5fb104007     vmovsd xmm0,[rax+0x7]
0x2e68d363f814    54  c4e1f97ec3     vmovq rbx,xmm0
0x2e68d363f819    59  48ba0000000000000080 REX.W movq rdx,0x8000000000000000
0x2e68d363f823    63  483bd3         REX.W cmpq rdx,rbx
0x2e68d363f826    66  0f8534000000   jnz 0x2e68d363f860  <+0xa0>
0x2e68d363f82c    6c  48bb41996d980f250000 REX.W movq rbx,0x250f986d9941    ;; object: 0x250f986d9941 <String[3]: BAD>
0x2e68d363f836    76  53             push rbx
0x2e68d363f837    77  48bb20eb220100000000 REX.W movq rbx,0x122eb20
0x2e68d363f841    81  488bd0         REX.W movq rdx,rax
0x2e68d363f844    84  48bea1169864c50a0000 REX.W movq rsi,0xac5649816a1    ;; object: 0x0ac5649816a1 <NativeContext[248]>
0x2e68d363f84e    8e  b801000000     movl rax,0x1
0x2e68d363f853    93  49baa073c90100000000 REX.W movq r10,0x1c973a0  (CEntry_Return1_DontSaveFPRegs_ArgvOnStack_NoBuiltinExit)
0x2e68d363f85d    9d  41ffd2         call r10
0x2e68d363f860    a0  498b45d8       REX.W movq rax,[r13-0x28] (root (undefined_value))
0x2e68d363f864    a4  488be5         REX.W movq rsp,rbp
0x2e68d363f867    a7  5d             pop rbp
0x2e68d363f868    a8  c21000         ret 0x10

Notice how the sequence of eight instructions operating on xmm's that was in the original disassembly after vmovsd was replaced with merely "vmovq + movq + cmpq". This makes me think that this bug was fixed as a side effect of another bigger issue, which would make backporting difficult…

[BridgeAR]

@TimothyGu I think I got the right commit that has to be backported. I am compiling right now to check if it indeed fixes the issue.

I'll open a backport if it's correct.

[BridgeAR]

Done.