Title: [245769] trunk
- Revision
- 245769
- Author
- [email protected]
- Date
- 2019-05-25 02:25:25 -0700 (Sat, 25 May 2019)
Log Message
JITOperations getByVal should mark negative array indices as out-of-bounds
https://bugs.webkit.org/show_bug.cgi?id=198229
Reviewed by Saam Barati.
JSTests:
* microbenchmarks/get-by-val-negative-array-index.js: Added.
(foo):
Source/_javascript_Core:
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):
Modified Paths
Added Paths
Diff
Modified: trunk/JSTests/ChangeLog (245768 => 245769)
--- trunk/JSTests/ChangeLog 2019-05-25 01:42:17 UTC (rev 245768)
+++ trunk/JSTests/ChangeLog 2019-05-25 09:25:25 UTC (rev 245769)
@@ -1,3 +1,13 @@
+2019-05-25 Tadeu Zagallo <[email protected]>
+
+ 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-24 Justin Michaud <[email protected]>
[WASM-References] Support Anyref in globals
Added: trunk/JSTests/microbenchmarks/get-by-val-negative-array-index.js (0 => 245769)
--- trunk/JSTests/microbenchmarks/get-by-val-negative-array-index.js (rev 0)
+++ trunk/JSTests/microbenchmarks/get-by-val-negative-array-index.js 2019-05-25 09:25:25 UTC (rev 245769)
@@ -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);
+}
Modified: trunk/Source/_javascript_Core/ChangeLog (245768 => 245769)
--- trunk/Source/_javascript_Core/ChangeLog 2019-05-25 01:42:17 UTC (rev 245768)
+++ trunk/Source/_javascript_Core/ChangeLog 2019-05-25 09:25:25 UTC (rev 245769)
@@ -1,3 +1,22 @@
+2019-05-25 Tadeu Zagallo <[email protected]>
+
+ 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-24 Justin Michaud <[email protected]>
[WASM-References] Support Anyref in globals
Modified: trunk/Source/_javascript_Core/jit/JITOperations.cpp (245768 => 245769)
--- trunk/Source/_javascript_Core/jit/JITOperations.cpp 2019-05-25 01:42:17 UTC (rev 245768)
+++ trunk/Source/_javascript_Core/jit/JITOperations.cpp 2019-05-25 09:25:25 UTC (rev 245769)
@@ -1850,13 +1850,13 @@
}
}
- 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 @@
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 @@
}
}
- 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);
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
