On ARM64, DFG::SpeculativeJIT::compileArithMod() failed to ensure result is of DataFo...

https://bugs.webkit.org/show_bug.cgi?id=170473
<rdar://problem/29912391>

Reviewed by Saam Barati.

JSTests:

* stress/regress-170473.js: Added.

Source/JavaScriptCore:

In Unchecked mode, when DFG::SpeculativeJIT::compileArithMod() detects that the
divisor is 0, we want it to return 0.  The result is expected to be of
DataFormatIn32.

The ARM implementation just returns the value in the divisor register.  However,
the divisor in this case can be of DataFormatJSInt32.  On ARM64, returning the
divisor register yields the wrong result format because the same register also
holds the upper 32-bit of the JSValue encoding.  The fix is to return an
immediate 0 instead.

Also turned on the assertion in jitAssertIsInt32 for ARM64.  This assertion being
disabled may have contributed to this bug going unnoticed all this time.

* dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::SpeculativeJIT::compileArithMod):
* jit/AssemblyHelpers.cpp:
(JSC::AssemblyHelpers::jitAssertIsInt32):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@214927 268f45cc-cd09-0410-ab3c-d52691b4dbfc

diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
index e832ead..eb9bb71 100644 (file)
--- a/JSTests/ChangeLog
+++ b/JSTests/ChangeLog
@@ -1,3 +1,13 @@
+2017-04-04  Mark Lam  <mark.lam@apple.com>
+
+        On ARM64, DFG::SpeculativeJIT::compileArithMod() failed to ensure result is of DataFormatInt32.
+        https://bugs.webkit.org/show_bug.cgi?id=170473
+        <rdar://problem/29912391>
+
+        Reviewed by Saam Barati.
+
+        * stress/regress-170473.js: Added.
+
 2017-04-03  Mark Lam  <mark.lam@apple.com>
 
         Fix incorrect capacity delta calculation reported in SparseArrayValueMap::add().

diff --git a/JSTests/stress/regress-170473.js b/JSTests/stress/regress-170473.js
new file mode 100644 (file)
index 0000000..16e16d7
--- /dev/null
+++ b/JSTests/stress/regress-170473.js
@@ -0,0 +1,14 @@
+var heap = new SharedArrayBuffer(4096);
+var Uint8ArrayView = new Uint8Array(heap);
+
+function test(k)  {
+    var d = new Float32Array();
+    var c = d | 0;
+    var b = 1 % c;
+    var a = b | 0;
+    Uint8ArrayView[a] = 0;
+}
+noInline(test);
+
+for (var k = 0; k < 200; ++k)
+    test(k);

diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
index 814b621..744735e 100644 (file)
--- a/Source/JavaScriptCore/ChangeLog
+++ b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,29 @@
+2017-04-04  Mark Lam  <mark.lam@apple.com>
+
+        On ARM64, DFG::SpeculativeJIT::compileArithMod() failed to ensure result is of DataFormatInt32.
+        https://bugs.webkit.org/show_bug.cgi?id=170473
+        <rdar://problem/29912391>
+
+        Reviewed by Saam Barati.
+
+        In Unchecked mode, when DFG::SpeculativeJIT::compileArithMod() detects that the
+        divisor is 0, we want it to return 0.  The result is expected to be of
+        DataFormatIn32.
+
+        The ARM implementation just returns the value in the divisor register.  However,
+        the divisor in this case can be of DataFormatJSInt32.  On ARM64, returning the
+        divisor register yields the wrong result format because the same register also
+        holds the upper 32-bit of the JSValue encoding.  The fix is to return an
+        immediate 0 instead.
+
+        Also turned on the assertion in jitAssertIsInt32 for ARM64.  This assertion being
+        disabled may have contributed to this bug going unnoticed all this time.
+
+        * dfg/DFGSpeculativeJIT.cpp:
+        (JSC::DFG::SpeculativeJIT::compileArithMod):
+        * jit/AssemblyHelpers.cpp:
+        (JSC::AssemblyHelpers::jitAssertIsInt32):
+
 2017-04-04  Filip Pizlo  <fpizlo@apple.com>
 
         Air::eliminateDeadCode should not repeatedly process the same live instructions

diff --git a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
index 78a277e..2b34ce3 100644 (file)
--- a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
+++ b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
@@ -5101,7 +5101,10 @@ void SpeculativeJIT::compileArithMod(Node* node)
             speculationCheck(Overflow, JSValueRegs(), 0, m_jit.branchTest32(JITCompiler::Zero, divisorGPR));
         else {
             JITCompiler::Jump denominatorNotZero = m_jit.branchTest32(JITCompiler::NonZero, divisorGPR);
-            m_jit.move(divisorGPR, quotientThenRemainderGPR);
+            // We know that the low 32-bit of divisorGPR is 0, but we don't know if the high bits are.
+            // So, use TrustedImm32(0) on ARM instead because done expects the result to be in DataFormatInt32.
+            // Using an immediate 0 doesn't cost anything extra on ARM.
+            m_jit.move(TrustedImm32(0), quotientThenRemainderGPR);
             done.append(m_jit.jump());
             denominatorNotZero.link(&m_jit);
         }

diff --git a/Source/JavaScriptCore/jit/AssemblyHelpers.cpp b/Source/JavaScriptCore/jit/AssemblyHelpers.cpp
index 316fd99..f89e1a0 100644 (file)
--- a/Source/JavaScriptCore/jit/AssemblyHelpers.cpp
+++ b/Source/JavaScriptCore/jit/AssemblyHelpers.cpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2011, 2013-2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2011-2017 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -192,7 +192,7 @@ void AssemblyHelpers::clearSamplingFlag(int32_t flag)
 #if USE(JSVALUE64)
 void AssemblyHelpers::jitAssertIsInt32(GPRReg gpr)
 {
-#if CPU(X86_64)
+#if CPU(X86_64) || CPU(ARM64)
     Jump checkInt32 = branch64(BelowOrEqual, gpr, TrustedImm64(static_cast<uintptr_t>(0xFFFFFFFFu)));
     abortWithReason(AHIsNotInt32);
     checkInt32.link(this);