B3StackmapSpecial should handle when stackmap values are not recoverable from a Def...
authormark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 19 Apr 2017 21:51:52 +0000 (21:51 +0000)
committermark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 19 Apr 2017 21:51:52 +0000 (21:51 +0000)
https://bugs.webkit.org/show_bug.cgi?id=170973
<rdar://problem/30318657>

Reviewed by Filip Pizlo.

JSTests:

* stress/regress-170973.js: Added.

Source/JavaScriptCore:

In the event of an arithmetic overflow on a binary sub instruction (where the
result register is same as one of the operand registers), the CheckSub FTL
operation will try to recover the original value in the clobbered result register.

This recover is done by adding the other operand value to the result register.
However, this recovery method only works if the width of the original value in
the result register is less or equal to the width of the expected result.  If the
width of the original operand value (e.g. a JSInt32) is wider than the result
(e.g. a machine Int32), then the sub operation would have zero extended the
result and cleared the upper 32-bits of the result register.  Recovery by adding
back the other operand will not restore the JSValue tag in the upper word.

This poses a problem if the stackmap value for the operand relies on that same
clobbered register.

The fix is to detect this potential scenario (i.e. width of the Def's arg < width
of a stackmap value).  If this condition is detected, we'll declare the stackmap
value to be LateColdUse to ensure that the register allocator gives it a
different register if needed so that it's not dependent on the clobbered register.

* b3/B3CheckSpecial.cpp:
(JSC::B3::CheckSpecial::forEachArg):
* b3/B3PatchpointSpecial.cpp:
(JSC::B3::PatchpointSpecial::forEachArg):
* b3/B3StackmapSpecial.cpp:
(JSC::B3::StackmapSpecial::forEachArgImpl):
* b3/B3StackmapSpecial.h:

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

JSTests/ChangeLog
JSTests/stress/regress-170973.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/b3/B3CheckSpecial.cpp
Source/JavaScriptCore/b3/B3PatchpointSpecial.cpp
Source/JavaScriptCore/b3/B3StackmapSpecial.cpp
Source/JavaScriptCore/b3/B3StackmapSpecial.h

index d832238..8955b10 100644 (file)
@@ -1,3 +1,13 @@
+2017-04-19  Mark Lam  <mark.lam@apple.com>
+
+        B3StackmapSpecial should handle when stackmap values are not recoverable from a Def'ed arg.
+        https://bugs.webkit.org/show_bug.cgi?id=170973
+        <rdar://problem/30318657>
+
+        Reviewed by Filip Pizlo.
+
+        * stress/regress-170973.js: Added.
+
 2017-04-19  JF Bastien  <jfbastien@apple.com>
 
         WebAssembly: limit slow memories
diff --git a/JSTests/stress/regress-170973.js b/JSTests/stress/regress-170973.js
new file mode 100644 (file)
index 0000000..f3d5b00
--- /dev/null
@@ -0,0 +1,16 @@
+// This test passes if it does not crash.
+
+function test(i0, i1) {
+    i0 = i0|0;
+    i1 = i1|0;
+    if (i1 & 1)
+        i1 = (((i0 ? i1 : i1)-i0) ? false : 0);
+}
+noInline(test);
+
+for (var k = 0; k < 200; ++k) {
+    if (k < 100)
+        test(0, 0x80000001);
+    else
+        test(0x800, 0x80000001);
+}
index 0e8ebbf..a703e50 100644 (file)
@@ -1,3 +1,39 @@
+2017-04-19  Mark Lam  <mark.lam@apple.com>
+
+        B3StackmapSpecial should handle when stackmap values are not recoverable from a Def'ed arg.
+        https://bugs.webkit.org/show_bug.cgi?id=170973
+        <rdar://problem/30318657>
+
+        Reviewed by Filip Pizlo.
+
+        In the event of an arithmetic overflow on a binary sub instruction (where the
+        result register is same as one of the operand registers), the CheckSub FTL
+        operation will try to recover the original value in the clobbered result register.
+
+        This recover is done by adding the other operand value to the result register.
+        However, this recovery method only works if the width of the original value in
+        the result register is less or equal to the width of the expected result.  If the
+        width of the original operand value (e.g. a JSInt32) is wider than the result
+        (e.g. a machine Int32), then the sub operation would have zero extended the
+        result and cleared the upper 32-bits of the result register.  Recovery by adding
+        back the other operand will not restore the JSValue tag in the upper word.
+
+        This poses a problem if the stackmap value for the operand relies on that same
+        clobbered register.
+
+        The fix is to detect this potential scenario (i.e. width of the Def's arg < width
+        of a stackmap value).  If this condition is detected, we'll declare the stackmap
+        value to be LateColdUse to ensure that the register allocator gives it a
+        different register if needed so that it's not dependent on the clobbered register.
+
+        * b3/B3CheckSpecial.cpp:
+        (JSC::B3::CheckSpecial::forEachArg):
+        * b3/B3PatchpointSpecial.cpp:
+        (JSC::B3::PatchpointSpecial::forEachArg):
+        * b3/B3StackmapSpecial.cpp:
+        (JSC::B3::StackmapSpecial::forEachArgImpl):
+        * b3/B3StackmapSpecial.h:
+
 2017-04-19  JF Bastien  <jfbastien@apple.com>
 
         Unreviewed, rolling out r215520.
index 9f075c4..b6baa8a 100644 (file)
@@ -108,9 +108,14 @@ Inst CheckSpecial::hiddenBranch(const Inst& inst) const
 
 void CheckSpecial::forEachArg(Inst& inst, const ScopedLambda<Inst::EachArgCallback>& callback)
 {
+    std::optional<Width> optionalDefArgWidth;
     Inst hidden = hiddenBranch(inst);
     hidden.forEachArg(
         [&] (Arg& arg, Arg::Role role, Bank bank, Width width) {
+            if (Arg::isAnyDef(role)) {
+                ASSERT(!optionalDefArgWidth); // There can only be one Def'ed arg.
+                optionalDefArgWidth = width;
+            }
             unsigned index = &arg - &hidden.args[0];
             callback(inst.args[1 + index], role, bank, width);
         });
@@ -118,7 +123,7 @@ void CheckSpecial::forEachArg(Inst& inst, const ScopedLambda<Inst::EachArgCallba
     std::optional<unsigned> firstRecoverableIndex;
     if (m_checkKind.opcode == BranchAdd32 || m_checkKind.opcode == BranchAdd64)
         firstRecoverableIndex = 1;
-    forEachArgImpl(numB3Args(inst), m_numCheckArgs + 1, inst, m_stackmapRole, firstRecoverableIndex, callback);
+    forEachArgImpl(numB3Args(inst), m_numCheckArgs + 1, inst, m_stackmapRole, firstRecoverableIndex, callback, optionalDefArgWidth);
 }
 
 bool CheckSpecial::isValid(Inst& inst)
index 7974bce..4ceb1b9 100644 (file)
@@ -59,7 +59,7 @@ void PatchpointSpecial::forEachArg(Inst& inst, const ScopedLambda<Inst::EachArgC
         callback(inst.args[argIndex++], role, inst.origin->resultBank(), inst.origin->resultWidth());
     }
 
-    forEachArgImpl(0, argIndex, inst, SameAsRep, std::nullopt, callback);
+    forEachArgImpl(0, argIndex, inst, SameAsRep, std::nullopt, callback, std::nullopt);
     argIndex += inst.origin->numChildren();
 
     for (unsigned i = patchpoint->numGPScratchRegisters; i--;)
index 30ccde4..87dd951 100644 (file)
@@ -74,7 +74,7 @@ RegisterSet StackmapSpecial::extraEarlyClobberedRegs(Inst& inst)
 void StackmapSpecial::forEachArgImpl(
     unsigned numIgnoredB3Args, unsigned numIgnoredAirArgs,
     Inst& inst, RoleMode roleMode, std::optional<unsigned> firstRecoverableIndex,
-    const ScopedLambda<Inst::EachArgCallback>& callback)
+    const ScopedLambda<Inst::EachArgCallback>& callback, std::optional<Width> optionalDefArgWidth)
 {
     StackmapValue* value = inst.origin->as<StackmapValue>();
     ASSERT(value);
@@ -83,7 +83,8 @@ void StackmapSpecial::forEachArgImpl(
     ASSERT(inst.args.size() >= numIgnoredAirArgs);
     ASSERT(value->children().size() >= numIgnoredB3Args);
     ASSERT(inst.args.size() - numIgnoredAirArgs >= value->children().size() - numIgnoredB3Args);
-    
+    ASSERT(inst.args[0].kind() == Arg::Kind::Special);
+
     for (unsigned i = 0; i < value->children().size() - numIgnoredB3Args; ++i) {
         Arg& arg = inst.args[i + numIgnoredAirArgs];
         ConstrainedValue child = value->constrainedChild(i + numIgnoredB3Args);
@@ -120,6 +121,16 @@ void StackmapSpecial::forEachArgImpl(
                 RELEASE_ASSERT_NOT_REACHED();
                 break;
             }
+
+            // If the Def'ed arg has a smaller width than the a stackmap value, then we may not
+            // be able to recover the stackmap value. So, force LateColdUse to preserve the
+            // original stackmap value across the Special operation.
+            if (!Arg::isLateUse(role) && optionalDefArgWidth && *optionalDefArgWidth < child.value()->resultWidth()) {
+                if (Arg::isWarmUse(role))
+                    role = Arg::LateUse;
+                else
+                    role = Arg::LateColdUse;
+            }
             break;
         case ForceLateUse:
             role = Arg::LateColdUse;
index 97a0813..6742a63 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2015-2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2015-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
@@ -60,8 +60,8 @@ protected:
     void forEachArgImpl(
         unsigned numIgnoredB3Args, unsigned numIgnoredAirArgs,
         Air::Inst&, RoleMode, std::optional<unsigned> firstRecoverableIndex,
-        const ScopedLambda<Air::Inst::EachArgCallback>&);
-    
+        const ScopedLambda<Air::Inst::EachArgCallback>&, std::optional<Width> optionalDefArgWidth);
+
     bool isValidImpl(
         unsigned numIgnoredB3Args, unsigned numIgnoredAirArgs,
         Air::Inst&);