Log Message
DoubleRep fails to convert SpecBoolean values. https://bugs.webkit.org/show_bug.cgi?id=150313
Reviewed by Geoffrey Garen. This was uncovered by the op_sub stress test on 32-bit builds. On 32-bit builds, DoubleRep will erroneously convert 'true' to a 'NaN' instead of a double 1. On 64-bit, the same issue exists but is masked by another bug in DoubleRep where boolean values will always erroneously trigger a BadType OSR exit. The erroneous conversion of 'true' to 'NaN' is because the 'true' case in compileDoubleRep() is missing a jump to the "done" destination. Instead, it fall through to the "isUndefined" case where it produces a NaN. The 64-bit erroneous BadType OSR exit is due to the boolean type check being implemented incorrectly. It was checking if any bits other than bit 0 were set. However, boolean JS values always have TagBitBool (the 3rd bit) set. Hence, the check will always fail if we have a boolean value. This patch fixes both of these issues. No new test is needed because these issues are already covered by scenarios in the op_sub.js stress test. This patch also fixes the op_sub.js test to throw an exception if any failures are encountered (as expected by the stress test harness). This patch also re-worked the test code to provide more accurate descriptions of each test scenario for error reporting. * dfg/DFGSpeculativeJIT.cpp: (JSC::DFG::SpeculativeJIT::compileDoubleRep): * tests/stress/op_sub.js: (generateScenarios): (func): (initializeTestCases): (runTest): (stringify): Deleted.
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (191289 => 191290)
--- trunk/Source/_javascript_Core/ChangeLog 2015-10-19 16:10:16 UTC (rev 191289)
+++ trunk/Source/_javascript_Core/ChangeLog 2015-10-19 16:13:59 UTC (rev 191290)
@@ -1,3 +1,42 @@
+2015-10-19 Mark Lam <mark....@apple.com>
+
+ DoubleRep fails to convert SpecBoolean values.
+ https://bugs.webkit.org/show_bug.cgi?id=150313
+
+ Reviewed by Geoffrey Garen.
+
+ This was uncovered by the op_sub stress test on 32-bit builds. On 32-bit builds,
+ DoubleRep will erroneously convert 'true' to a 'NaN' instead of a double 1.
+ On 64-bit, the same issue exists but is masked by another bug in DoubleRep where
+ boolean values will always erroneously trigger a BadType OSR exit.
+
+ The erroneous conversion of 'true' to 'NaN' is because the 'true' case in
+ compileDoubleRep() is missing a jump to the "done" destination. Instead, it
+ fall through to the "isUndefined" case where it produces a NaN.
+
+ The 64-bit erroneous BadType OSR exit is due to the boolean type check being
+ implemented incorrectly. It was checking if any bits other than bit 0 were set.
+ However, boolean JS values always have TagBitBool (the 3rd bit) set. Hence, the
+ check will always fail if we have a boolean value.
+
+ This patch fixes both of these issues.
+
+ No new test is needed because these issues are already covered by scenarios in
+ the op_sub.js stress test. This patch also fixes the op_sub.js test to throw an
+ exception if any failures are encountered (as expected by the stress test
+ harness). This patch also re-worked the test code to provide more accurate
+ descriptions of each test scenario for error reporting.
+
+ * dfg/DFGSpeculativeJIT.cpp:
+ (JSC::DFG::SpeculativeJIT::compileDoubleRep):
+
+ * tests/stress/op_sub.js:
+ (generateScenarios):
+ (func):
+ (initializeTestCases):
+ (runTest):
+ (stringify): Deleted.
+
2015-10-19 Yusuke Suzuki <utatane....@gmail.com>
Drop !newTarget check since it always becomes true
Modified: trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp (191289 => 191290)
--- trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp 2015-10-19 16:10:16 UTC (rev 191289)
+++ trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp 2015-10-19 16:13:59 UTC (rev 191290)
@@ -2196,11 +2196,12 @@
done.append(isNull);
DFG_TYPE_CHECK(JSValueRegs(op1GPR), node->child1(), ~SpecCell,
- m_jit.branchTest64(JITCompiler::NonZero, op1GPR, TrustedImm32(static_cast<int32_t>(~1))));
+ m_jit.branchTest64(JITCompiler::Zero, op1GPR, TrustedImm32(static_cast<int32_t>(TagBitBool))));
JITCompiler::Jump isFalse = m_jit.branch64(JITCompiler::Equal, op1GPR, TrustedImm64(ValueFalse));
static const double _one_ = 1;
m_jit.loadDouble(MacroAssembler::TrustedImmPtr(&one), resultFPR);
+ done.append(m_jit.jump());
done.append(isFalse);
isUndefined.link(&m_jit);
@@ -2249,6 +2250,7 @@
JITCompiler::Jump isFalse = m_jit.branchTest32(JITCompiler::Zero, op1PayloadGPR, TrustedImm32(1));
static const double _one_ = 1;
m_jit.loadDouble(MacroAssembler::TrustedImmPtr(&one), resultFPR);
+ done.append(m_jit.jump());
done.append(isFalse);
isUndefined.link(&m_jit);
Modified: trunk/Source/_javascript_Core/tests/stress/op_sub.js (191289 => 191290)
--- trunk/Source/_javascript_Core/tests/stress/op_sub.js 2015-10-19 16:10:16 UTC (rev 191289)
+++ trunk/Source/_javascript_Core/tests/stress/op_sub.js 2015-10-19 16:13:59 UTC (rev 191290)
@@ -24,19 +24,24 @@
};
var set1 = [
- o1,
- null,
- undefined,
- NaN,
- "abc",
+ 'o1',
+ 'null',
+ 'undefined',
+ 'NaN',
+ '"abc"',
];
var set2 = [
- 10, -10,
- 2147483647, -2147483647,
- 4294967296, -4294967296,
- 100.2, -100.2,
- true, false
+ '10',
+ '-10',
+ '2147483647',
+ '-2147483647',
+ '4294967296',
+ '-4294967296',
+ '100.2',
+ '-100.2',
+ 'true',
+ 'false',
];
// Assemble the values that we'll be testing with:
@@ -46,22 +51,18 @@
for (var i = 0; i < set2.length; i++)
values.push(set2[i]);
for (var i = 0; i < set2.length; i++)
- values.push("" + set2[i]);
+ values.push('"' + set2[i] + '"');
-function stringify(value) {
- if (typeof value == "string")
- return '"' + value + '"';
- return "" + value;
-}
-
function generateScenarios(xvalues, yvalues) {
var scenarios = [];
for (var i = 0; i < xvalues.length; i++) {
for (var j = 0; j < yvalues.length; j++) {
- var name = "(" + xvalues[i] + " - " + yvalues[j] + ")";
- var x = xvalues[i];
- var y = yvalues[j];
- var expected = eval(stringify(x) + " - " + stringify(y));
+ var xStr = xvalues[i];
+ var yStr = yvalues[j];
+ var x = eval(xStr);
+ var y = eval(yStr);
+ var name = "(" + xStr + " - " + yStr + ")";
+ var expected = eval("" + xStr + " - " + yStr);
var scenario = { name: name, x: x, y: y, expected: expected };
scenarios.push(scenario);
@@ -87,172 +88,172 @@
{
name: "subI32V",
func: function(x, y) { return 10 - y; },
- xvalues: [ 10 ],
+ xvalues: [ '10' ],
yvalues: values
},
{
name: "subVI32",
func: function(x, y) { return x - 10; },
xvalues: values,
- yvalues: [ 10 ]
+ yvalues: [ '10' ]
},
{
name: "subI32oV",
func: function(x, y) { return -2147483647 - y; },
- xvalues: [ -2147483647 ],
+ xvalues: [ '-2147483647' ],
yvalues: values
},
{
name: "subVI32o",
func: function(x, y) { return x - 2147483647; },
xvalues: values,
- yvalues: [ 2147483647 ]
+ yvalues: [ '2147483647' ]
},
{
name: "subI52V",
func: function(x, y) { return 4294967296 - y; },
- xvalues: [ 4294967296 ],
+ xvalues: [ '4294967296' ],
yvalues: values
},
{
name: "subVI52",
func: function(x, y) { return x - 4294967296; },
xvalues: values,
- yvalues: [ 4294967296 ]
+ yvalues: [ '4294967296' ]
},
{
name: "subDV",
func: function(x, y) { return 100.2 - y; },
- xvalues: [ 100.2 ],
+ xvalues: [ '100.2' ],
yvalues: values
},
{
name: "subVD",
func: function(x, y) { return x - 100.2; },
xvalues: values,
- yvalues: [ 100.2 ]
+ yvalues: [ '100.2' ]
},
{
name: "subBV",
func: function(x, y) { return true - y; },
- xvalues: [ true ],
+ xvalues: [ 'true' ],
yvalues: values
},
{
name: "subVB",
func: function(x, y) { return x - true; },
xvalues: values,
- yvalues: [ true ]
+ yvalues: [ 'true' ]
},
{
name: "subSi32V",
func: function(x, y) { return "10" - y; },
- xvalues: [ "10" ],
+ xvalues: [ '"10"' ],
yvalues: values
},
{
name: "subVSi32",
func: function(x, y) { return x - "10"; },
xvalues: values,
- yvalues: [ "10" ]
+ yvalues: [ '"10"' ]
},
{
name: "subSi32oV",
func: function(x, y) { return "-2147483647" - y; },
- xvalues: [ "-2147483647" ],
+ xvalues: [ '"-2147483647"' ],
yvalues: values
},
{
name: "subVSi32o",
func: function(x, y) { return x - "2147483647"; },
xvalues: values,
- yvalues: [ "2147483647" ]
+ yvalues: [ '"2147483647"' ]
},
{
name: "subSi52V",
func: function(x, y) { return "4294967296" - y; },
- xvalues: [ "4294967296" ],
+ xvalues: [ '"4294967296"' ],
yvalues: values
},
{
name: "subVSi52",
func: function(x, y) { return x - "4294967296"; },
xvalues: values,
- yvalues: [ "4294967296" ]
+ yvalues: [ '"4294967296"' ]
},
{
name: "subSdV",
func: function(x, y) { return "100.2" - y; },
- xvalues: [ "100.2" ],
+ xvalues: [ '"100.2"' ],
yvalues: values
},
{
name: "subVSd",
func: function(x, y) { return x - "100.2"; },
xvalues: values,
- yvalues: [ "100.2" ]
+ yvalues: [ '"100.2"' ]
},
{
name: "subSbV",
func: function(x, y) { return "true" - y; },
- xvalues: [ "true" ],
+ xvalues: [ '"true"' ],
yvalues: values
},
{
name: "subVSb",
func: function(x, y) { return x - "true"; },
xvalues: values,
- yvalues: [ "true" ]
+ yvalues: [ '"true"' ]
},
{
name: "subSV",
func: function(x, y) { return "abc" - y; },
- xvalues: [ "abc" ],
+ xvalues: [ '"abc"' ],
yvalues: values
},
{
name: "subVS",
func: function(x, y) { return x - "abc"; },
xvalues: values,
- yvalues: [ "abc" ]
+ yvalues: [ '"abc"' ]
},
{
name: "subNV",
func: function(x, y) { return null - y; },
- xvalues: [ null ],
+ xvalues: [ 'null' ],
yvalues: values
},
{
name: "subVN",
func: function(x, y) { return x - null; },
xvalues: values,
- yvalues: [ null ]
+ yvalues: [ 'null' ]
},
{
name: "subOV",
func: function(x, y) { return o1 - y; },
- xvalues: [ o1 ],
+ xvalues: [ 'o1' ],
yvalues: values
},
{
name: "subVO",
func: function(x, y) { return x - o1; },
xvalues: values,
- yvalues: [ o1 ]
+ yvalues: [ 'o1' ]
},
{
name: "subNaNV",
func: function(x, y) { return NaN - y; },
- xvalues: [ NaN ],
+ xvalues: [ 'NaN' ],
yvalues: values
},
{
name: "subVNaN",
func: function(x, y) { return x - NaN; },
xvalues: values,
- yvalues: [ NaN ]
+ yvalues: [ 'NaN' ]
},
];
@@ -264,6 +265,8 @@
}
initializeTestCases();
+var errorReport = "";
+
function runTest(test) {
var failedScenario = [];
var scenarios = test.scenarios;
@@ -278,16 +281,18 @@
if (Number.isNaN(result) && Number.isNaN(scenario.expected))
continue;
if (!failedScenario[scenarioID]) {
- print("FAIL: " + test.name + ":" + scenario.name + " started failing on iteration " + i + ": expected " + scenario.expected + ", actual " + result);
+ errorReport += "FAIL: " + test.name + ":" + scenario.name + " started failing on iteration " + i + ": expected " + scenario.expected + ", actual " + result + "\n";
failedScenario[scenarioID] = scenario;
}
}
}
} catch(e) {
- print("Unexpected exception: " + e);
+ errorReport += "Unexpected exception: " + e + "\n";
}
}
for (var test of testCases)
runTest(test);
+if (errorReport !== "")
+ throw "Error: bad result:\n" + errorReport;
_______________________________________________ webkit-changes mailing list webkit-changes@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-changes
