Repository navigation
Object.is(-Number.MIN_VALUE, -0) broken on ≥ v10.0.0 #25268
Description
- 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
Activity
ryzokuken commentedon Dec 29, 2018
@TimothyGu I guess all we need to do is identify which patch fixed this and backport it to all affected release lines?
TimothyGu commentedon Dec 29, 2018
@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 commentedon Dec 29, 2018
The latest version of V8 seems to be just better at optimizing this sequence of calls
Disassembly
Notice how the sequence of eight instructions operating on xmm's that was in the original disassembly after
vmovsdwas 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 commentedon Dec 29, 2018
@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 commentedon Dec 29, 2018
Done.
Work around V8 Object.is() bug
deps: cherry-pick 56f6a76 from upstream V8