Attachment 370630 Details for Bug 198229 – Patch for landing

[patch] Patch for landing

bug-198229-20190525095554.patch (text/plain), 4.43 KB, created by Tadeu Zagallo on 2019-05-25 00:55:56 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Tadeu Zagallo

Created: 2019-05-25 00:55:56 PDT

Size: 4.43 KB

patch

obsolete

>Subversion Revision: 245742
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index e9d34b8bafd14c1a66aab6c66f8d603fe17c128d..24d4db626f32294122ae2a55bf7c16fe11cf56de 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,22 @@
>+2019-05-24  Tadeu Zagallo  <tzagallo@apple.com>
>+
>+        JITOperations getByVal should mark negative array indices as out-of-bounds
>+        https://bugs.webkit.org/show_bug.cgi?id=198229
>+
>+        Reviewed by Saam Barati.
>+
>+        get_by_val with an array or string as base value and a negative index causes DFG to OSR exit,
>+        but baseline doesn't mark it as out-of-bounds, since it only considers positive indices. This
>+        leads to discarding DFG code, recompiling it and exiting at the same bytecode.
>+
>+        This is observed in the prepack-wtb subtest of JetStream2. In popContext#CdOhFJ, the last item
>+        of the array popped and the new last value is accessed using `array[array.length - 1]`, which
>+        is -1 when the array is empty. It shows a ~0.5% progression in JetStream2, but it's within the
>+        noise.
>+
>+        * jit/JITOperations.cpp:
>+        (JSC::getByVal):
>+
> 2019-05-23  Devin Rousso  <drousso@apple.com>
> 
>         Web Inspector: Overlay: rulers/guides should be shown whenever element selection is enabled
>diff --git a/Source/JavaScriptCore/jit/JITOperations.cpp b/Source/JavaScriptCore/jit/JITOperations.cpp
>index 12b78aadf4589dd483cc8792d2d6386351990ef4..b5c545b8c0cbf95b25e1c5b676e6214a574410a7 100644
>--- a/Source/JavaScriptCore/jit/JITOperations.cpp
>+++ b/Source/JavaScriptCore/jit/JITOperations.cpp
>@@ -1850,13 +1850,13 @@ static JSValue getByVal(ExecState* exec, JSValue baseValue, JSValue subscript, B
>         }
>     }
> 
>-    if (subscript.isUInt32()) {
>+    if (subscript.isInt32()) {
>         ASSERT(exec->bytecodeOffset());
>         byValInfo->tookSlowPath = true;
> 
>-        uint32_t i = subscript.asUInt32();
>+        int32_t i = subscript.asInt32();
>         if (isJSString(baseValue)) {
>-            if (asString(baseValue)->canGetIndex(i)) {
>+            if (i >= 0 && asString(baseValue)->canGetIndex(i)) {
>                 ctiPatchCallByReturnAddress(returnAddress, operationGetByValString);
>                 RELEASE_AND_RETURN(scope, asString(baseValue)->getIndex(exec, i));
>             }
>@@ -1868,7 +1868,7 @@ static JSValue getByVal(ExecState* exec, JSValue baseValue, JSValue subscript, B
> 
>             bool skipMarkingOutOfBounds = false;
> 
>-            if (object->indexingType() == ArrayWithContiguous && i < object->butterfly()->publicLength()) {
>+            if (object->indexingType() == ArrayWithContiguous && i >= 0 && static_cast<uint32_t>(i) < object->butterfly()->publicLength()) {
>                 // FIXME: expand this to ArrayStorage, Int32, and maybe Double:
>                 // https://bugs.webkit.org/show_bug.cgi?id=182940
>                 auto* globalObject = object->globalObject(vm);
>@@ -1883,7 +1883,8 @@ static JSValue getByVal(ExecState* exec, JSValue baseValue, JSValue subscript, B
>             }
>         }
> 
>-        RELEASE_AND_RETURN(scope, baseValue.get(exec, i));
>+        if (i >= 0)
>+            RELEASE_AND_RETURN(scope, baseValue.get(exec, static_cast<uint32_t>(i)));
>     }
> 
>     baseValue.requireObjectCoercible(exec);
>diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
>index 68de36b936122f781bac003da877ac8209041dfa..a181b9eecc015b10fefcc57dd346e4d147f94516 100644
>--- a/JSTests/ChangeLog
>+++ b/JSTests/ChangeLog
>@@ -1,3 +1,13 @@
>+2019-05-24  Tadeu Zagallo  <tzagallo@apple.com>
>+
>+        JITOperations getByVal should mark negative array indices as out-of-bounds
>+        https://bugs.webkit.org/show_bug.cgi?id=198229
>+
>+        Reviewed by Saam Barati.
>+
>+        * microbenchmarks/get-by-val-negative-array-index.js: Added.
>+        (foo):
>+
> 2019-05-23  Tadeu Zagallo  <tzagallo@apple.com>
> 
>         DFG::OSREntry should not perform arity check
>diff --git a/JSTests/microbenchmarks/get-by-val-negative-array-index.js b/JSTests/microbenchmarks/get-by-val-negative-array-index.js
>new file mode 100644
>index 0000000000000000000000000000000000000000..1a86915f59c47e8793a3ddac87a1ae6ea11819fe
>--- /dev/null
>+++ b/JSTests/microbenchmarks/get-by-val-negative-array-index.js
>@@ -0,0 +1,11 @@
>+function foo(arr, index) {
>+    return arr[index];
>+}
>+noInline(foo);
>+
>+const arr = new Array(1000).fill({});
>+for (let i = 0; i < 1e7; i++) {
>+    foo(arr, i % arr.length);
>+    if (!(i % 1e3))
>+        foo(arr, -1);
>+}

Actions: View | Formatted Diff | Diff

Attachments on bug 198229: 370586 | 370630