- Revision
- 215531
- Author
- [email protected]
- Date
- 2017-04-19 14:51:52 -0700 (Wed, 19 Apr 2017)
Log 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/_javascript_Core:
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:
Modified Paths
Added Paths
Diff
Modified: trunk/JSTests/ChangeLog (215530 => 215531)
--- trunk/JSTests/ChangeLog 2017-04-19 21:30:05 UTC (rev 215530)
+++ trunk/JSTests/ChangeLog 2017-04-19 21:51:52 UTC (rev 215531)
@@ -1,3 +1,13 @@
+2017-04-19 Mark Lam <[email protected]>
+
+ 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 <[email protected]>
WebAssembly: limit slow memories
Added: trunk/JSTests/stress/regress-170973.js (0 => 215531)
--- trunk/JSTests/stress/regress-170973.js (rev 0)
+++ trunk/JSTests/stress/regress-170973.js 2017-04-19 21:51:52 UTC (rev 215531)
@@ -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);
+}
Modified: trunk/Source/_javascript_Core/ChangeLog (215530 => 215531)
--- trunk/Source/_javascript_Core/ChangeLog 2017-04-19 21:30:05 UTC (rev 215530)
+++ trunk/Source/_javascript_Core/ChangeLog 2017-04-19 21:51:52 UTC (rev 215531)
@@ -1,3 +1,39 @@
+2017-04-19 Mark Lam <[email protected]>
+
+ 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 <[email protected]>
Unreviewed, rolling out r215520.
Modified: trunk/Source/_javascript_Core/b3/B3CheckSpecial.cpp (215530 => 215531)
--- trunk/Source/_javascript_Core/b3/B3CheckSpecial.cpp 2017-04-19 21:30:05 UTC (rev 215530)
+++ trunk/Source/_javascript_Core/b3/B3CheckSpecial.cpp 2017-04-19 21:51:52 UTC (rev 215531)
@@ -108,9 +108,14 @@
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 @@
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)
Modified: trunk/Source/_javascript_Core/b3/B3PatchpointSpecial.cpp (215530 => 215531)
--- trunk/Source/_javascript_Core/b3/B3PatchpointSpecial.cpp 2017-04-19 21:30:05 UTC (rev 215530)
+++ trunk/Source/_javascript_Core/b3/B3PatchpointSpecial.cpp 2017-04-19 21:51:52 UTC (rev 215531)
@@ -59,7 +59,7 @@
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--;)
Modified: trunk/Source/_javascript_Core/b3/B3StackmapSpecial.cpp (215530 => 215531)
--- trunk/Source/_javascript_Core/b3/B3StackmapSpecial.cpp 2017-04-19 21:30:05 UTC (rev 215530)
+++ trunk/Source/_javascript_Core/b3/B3StackmapSpecial.cpp 2017-04-19 21:51:52 UTC (rev 215531)
@@ -74,7 +74,7 @@
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 @@
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 @@
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;
Modified: trunk/Source/_javascript_Core/b3/B3StackmapSpecial.h (215530 => 215531)
--- trunk/Source/_javascript_Core/b3/B3StackmapSpecial.h 2017-04-19 21:30:05 UTC (rev 215530)
+++ trunk/Source/_javascript_Core/b3/B3StackmapSpecial.h 2017-04-19 21:51:52 UTC (rev 215531)
@@ -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 @@
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&);
