Skip to content
Permalink
Browse files
javascript jit bug affecting Google Maps.
https://bugs.webkit.org/show_bug.cgi?id=153431

Reviewed by Filip Pizlo.

Source/JavaScriptCore:

The issue was due to the abstract interpreter wrongly marking the type of the
value read from the Uint3Array as SpecInt52, which precludes it from being an
Int32.  This proves to be false, and the generated code failed to handle the case
where the read value is actually an Int32.

The fix is to have the abstract interpreter use SpecMachineInt instead of
SpecInt52.

* bytecode/SpeculatedType.h:
* dfg/DFGAbstractInterpreterInlines.h:
(JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):

LayoutTests:

* js/regress/bug-153431-expected.txt: Added.
* js/regress/bug-153431.html: Added.
* js/regress/script-tests/bug-153431.js: Added.



Canonical link: https://commits.webkit.org/175047@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@199935 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
Mark Lam committed Apr 22, 2016
1 parent eee35e5 commit a8bde5f5be5596710714c5a80eb6990c1261b511
@@ -1,3 +1,14 @@
2016-04-22 Mark Lam <mark.lam@apple.com>

javascript jit bug affecting Google Maps.
https://bugs.webkit.org/show_bug.cgi?id=153431

Reviewed by Filip Pizlo.

* js/regress/bug-153431-expected.txt: Added.
* js/regress/bug-153431.html: Added.
* js/regress/script-tests/bug-153431.js: Added.

2016-04-22 Geoffrey Garen <ggaren@apple.com>

super should be available in object literals
@@ -0,0 +1,10 @@
JSRegress/bug-153431

On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".


PASS no exception thrown
PASS successfullyParsed is true

TEST COMPLETE

@@ -0,0 +1,12 @@
<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
<html>
<head>
<script src="../../resources/js-test-pre.js"></script>
</head>
<body>
<script src="../../resources/regress-pre.js"></script>
<script src="script-tests/bug-153431.js"></script>
<script src="../../resources/regress-post.js"></script>
<script src="../../resources/js-test-post.js"></script>
</body>
</html>
@@ -0,0 +1,36 @@
//@ runDefault

// Regression test for https://bugs.webkit.org/show_bug.cgi?id=153431.
// Reduced version based on the reproduction case provided by Ryan Sturgell in the bug,
// with some variable renames to read slightly better.

function assert(testedValue) {
if (!testedValue)
throw Error("Failed assertion");
}

function badFunc(arr, operand, resultArr) {
// This re-use of variable "operand" is important - rename it and the bug goes away.
operand = arr[operand];
if (false) {
// If this unreachable block is removed, the bug goes away!!
} else
{
resultArr[0] = operand;
}
}
noInline(badFunc);

function run() {
for (var i = 0; i < 10000; i++) {
var arr = new Uint32Array([0x80000000,1]); // Needs to be an Uint32Array.
var resultArr = [];

badFunc(arr, 0, resultArr);
assert(resultArr[0] == arr[0]);
badFunc(arr, 1, resultArr);
assert(resultArr[0] == arr[1]);
}
}

run();
@@ -1,3 +1,22 @@
2016-04-22 Mark Lam <mark.lam@apple.com>

javascript jit bug affecting Google Maps.
https://bugs.webkit.org/show_bug.cgi?id=153431

Reviewed by Filip Pizlo.

The issue was due to the abstract interpreter wrongly marking the type of the
value read from the Uint3Array as SpecInt52, which precludes it from being an
Int32. This proves to be false, and the generated code failed to handle the case
where the read value is actually an Int32.

The fix is to have the abstract interpreter use SpecMachineInt instead of
SpecInt52.

* bytecode/SpeculatedType.h:
* dfg/DFGAbstractInterpreterInlines.h:
(JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):

2016-04-22 Benjamin Poulain <bpoulain@apple.com>

[JSC] PredictionPropagation should not be in the top 5 heaviest phases
@@ -67,7 +67,7 @@ static const SpeculatedType SpecCell = SpecObject | SpecString | S
static const SpeculatedType SpecBoolInt32 = 1u << 21; // It's definitely an Int32 with value 0 or 1.
static const SpeculatedType SpecNonBoolInt32 = 1u << 22; // It's definitely an Int32 with value other than 0 or 1.
static const SpeculatedType SpecInt32 = SpecBoolInt32 | SpecNonBoolInt32; // It's definitely an Int32.
static const SpeculatedType SpecInt52 = 1u << 23; // It's definitely an Int52 and we intend it to unbox it.
static const SpeculatedType SpecInt52 = 1u << 23; // It's definitely an Int52 and we intend it to unbox it. It's also definitely not an Int32.
static const SpeculatedType SpecMachineInt = SpecInt32 | SpecInt52; // It's something that we can do machine int arithmetic on.
static const SpeculatedType SpecInt52AsDouble = 1u << 24; // It's definitely an Int52 and it's inside a double.
static const SpeculatedType SpecInteger = SpecMachineInt | SpecInt52AsDouble; // It's definitely some kind of integer.
@@ -1562,7 +1562,7 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
if (node->shouldSpeculateInt32())
forNode(node).setType(SpecInt32);
else if (enableInt52() && node->shouldSpeculateMachineInt())
forNode(node).setType(SpecInt52);
forNode(node).setType(SpecMachineInt);
else
forNode(node).setType(SpecInt52AsDouble);
break;

0 comments on commit a8bde5f

Please sign in to comment.