Changeset 215531 in webkit

Timestamp:

Apr 19, 2017, 2:51:52 PM (8 years ago)

Author:

mark.lam@apple.com

Message:

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.

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:

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/stress/regress-170973.js (added)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/b3/B3CheckSpecial.cpp (modified) (2 diffs)
• Source/JavaScriptCore/b3/B3PatchpointSpecial.cpp (modified) (1 diff)
• Source/JavaScriptCore/b3/B3StackmapSpecial.cpp (modified) (3 diffs)
• Source/JavaScriptCore/b3/B3StackmapSpecial.h (modified) (2 diffs)

trunk/JSTests/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/JavaScriptCore/b3/B3CheckSpecial.cpp

@@ -109 +109 @@
 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);
@@ -119 +124 @@
     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);
 }
 

trunk/Source/JavaScriptCore/b3/B3PatchpointSpecial.cpp

@@ -60 +60 @@
     }
 
-    forEachArgImpl(0, argIndex, inst, SameAsRep, std::nullopt, callback);
+    forEachArgImpl(0, argIndex, inst, SameAsRep, std::nullopt, callback, std::nullopt);
     argIndex += inst.origin->numChildren();
 

trunk/Source/JavaScriptCore/b3/B3StackmapSpecial.cpp

@@ -75 +75 @@
     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>();
@@ -84 +84 @@
     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];
@@ -120 +121 @@
                 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;

trunk/Source/JavaScriptCore/b3/B3StackmapSpecial.h

@@ -1 +1 @@
 /*
- * 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
@@ -61 +61 @@
         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,