Title: [230143] trunk
- Revision
- 230143
- Author
- fpi...@apple.com
- Date
- 2018-04-01 10:08:39 -0700 (Sun, 01 Apr 2018)
Log Message
JSC crash in JIT code with for-of loop and Array/Set iterators
https://bugs.webkit.org/show_bug.cgi?id=183174
Reviewed by Saam Barati.
JSTests:
* microbenchmarks/hoist-get-by-offset-tower-with-inferred-types.js: Added. This test shows that fixing the bug didn't break hoisting of GetByOffset with inferred types. I confirmed that if I did break it, this test slows down by >7x.
(foo):
* stress/hoist-get-by-offset-with-control-dependent-inferred-type.js: Added. This test shows that the bug is fixed.
(f):
Source/_javascript_Core:
* dfg/DFGSafeToExecute.h:
(JSC::DFG::safeToExecute): Fix the bug by making GetByOffset and friends verify that they are getting the type proof they want at the desired hoisting site.
Modified Paths
Added Paths
Diff
Modified: trunk/JSTests/ChangeLog (230142 => 230143)
--- trunk/JSTests/ChangeLog 2018-04-01 16:47:59 UTC (rev 230142)
+++ trunk/JSTests/ChangeLog 2018-04-01 17:08:39 UTC (rev 230143)
@@ -1,3 +1,15 @@
+2018-03-31 Filip Pizlo <fpi...@apple.com>
+
+ JSC crash in JIT code with for-of loop and Array/Set iterators
+ https://bugs.webkit.org/show_bug.cgi?id=183174
+
+ Reviewed by Saam Barati.
+
+ * microbenchmarks/hoist-get-by-offset-tower-with-inferred-types.js: Added. This test shows that fixing the bug didn't break hoisting of GetByOffset with inferred types. I confirmed that if I did break it, this test slows down by >7x.
+ (foo):
+ * stress/hoist-get-by-offset-with-control-dependent-inferred-type.js: Added. This test shows that the bug is fixed.
+ (f):
+
2018-03-30 JF Bastien <jfbast...@apple.com>
WebAssembly: support DataView compilation
Added: trunk/JSTests/microbenchmarks/hoist-get-by-offset-tower-with-inferred-types.js (0 => 230143)
--- trunk/JSTests/microbenchmarks/hoist-get-by-offset-tower-with-inferred-types.js (rev 0)
+++ trunk/JSTests/microbenchmarks/hoist-get-by-offset-tower-with-inferred-types.js 2018-04-01 17:08:39 UTC (rev 230143)
@@ -0,0 +1,19 @@
+function foo(o)
+{
+ var result = 0;
+ for (var i = 0; i < 100; ++i)
+ result += o.f.g.h.i.j.k.l.m.n.o.p.q.r.s.t.u.v.w.x.y.z;
+ return result;
+}
+
+noInline(foo);
+
+for (var i = 0; i < 1000; ++i) {
+ var o = {f:{g:{h:{i:{j:{k:{l:{m:{n:{o:{p:{q:{r:{s:{t:{u:{v:{w:{x:{y:{z:42}}}}}}}}}}}}}}}}}}}}};
+ for (var j = 0; j < 100; ++j) {
+ var result = foo(o);
+ if (result != 4200)
+ throw "Bad result in loop: " + result;
+ }
+}
+
Added: trunk/JSTests/stress/hoist-get-by-offset-with-control-dependent-inferred-type.js (0 => 230143)
--- trunk/JSTests/stress/hoist-get-by-offset-with-control-dependent-inferred-type.js (rev 0)
+++ trunk/JSTests/stress/hoist-get-by-offset-with-control-dependent-inferred-type.js 2018-04-01 17:08:39 UTC (rev 230143)
@@ -0,0 +1,14 @@
+function f() {
+ var a = Array(100).fill(0);
+ var ta = new Set(a.map((v,k)=>k));
+ var xs = [a, ta];
+ var q = 0;
+ var t = Date.now();
+ for (var i = 0; i < 100000; ++i) {
+ for (var x of xs[i&1]) q+=x;
+ }
+ return [Date.now()-t,q];
+}
+noInline(f);
+f();
+
Modified: trunk/Source/_javascript_Core/ChangeLog (230142 => 230143)
--- trunk/Source/_javascript_Core/ChangeLog 2018-04-01 16:47:59 UTC (rev 230142)
+++ trunk/Source/_javascript_Core/ChangeLog 2018-04-01 17:08:39 UTC (rev 230143)
@@ -1,3 +1,13 @@
+2018-03-31 Filip Pizlo <fpi...@apple.com>
+
+ JSC crash in JIT code with for-of loop and Array/Set iterators
+ https://bugs.webkit.org/show_bug.cgi?id=183174
+
+ Reviewed by Saam Barati.
+
+ * dfg/DFGSafeToExecute.h:
+ (JSC::DFG::safeToExecute): Fix the bug by making GetByOffset and friends verify that they are getting the type proof they want at the desired hoisting site.
+
2018-03-30 Filip Pizlo <fpi...@apple.com>
Strings and Vectors shouldn't do index masking
Modified: trunk/Source/_javascript_Core/dfg/DFGSafeToExecute.h (230142 => 230143)
--- trunk/Source/_javascript_Core/dfg/DFGSafeToExecute.h 2018-04-01 16:47:59 UTC (rev 230142)
+++ trunk/Source/_javascript_Core/dfg/DFGSafeToExecute.h 2018-04-01 17:08:39 UTC (rev 230143)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2013-2017 Apple Inc. All rights reserved.
+ * Copyright (C) 2013-2018 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -504,9 +504,33 @@
case GetByOffset:
case GetGetterSetterByOffset:
case PutByOffset: {
- PropertyOffset offset = node->storageAccessData().offset;
+ StorageAccessData& data = ""
+ PropertyOffset offset = data.offset;
+ UniquedStringImpl* uid = graph.identifiers()[data.identifierNumber];
- if (state.structureClobberState() == StructuresAreWatched) {
+ InferredType::Descriptor inferredType;
+ switch (node->op()) {
+ case GetByOffset:
+ case GetGetterSetterByOffset:
+ inferredType = data.inferredType;
+ break;
+ case PutByOffset:
+ // PutByOffset knows about inferred types because it's the enforcer of that type rather
+ // than the consumer of that type. Therefore, PutByOffset expects TOP for the purpose of
+ // safe-to-execute in the sense that it will be happy with anything as general as TOP
+ // (so any type).
+ inferredType = InferredType::Top;
+ break;
+ default:
+ DFG_CRASH(graph, node, "Bad opcode");
+ break;
+ }
+
+ // Graph::isSafeToLoad() is all about proofs derived from PropertyConditions. Those don't
+ // know anything about inferred types. But if we have a proof derived from watching a
+ // structure that has a type proof, then the next case below will deal with it.
+ if (state.structureClobberState() == StructuresAreWatched
+ && inferredType.kind() == InferredType::Top) {
if (JSObject* knownBase = node->child1()->dynamicCastConstant<JSObject*>(graph.m_vm)) {
if (graph.isSafeToLoad(knownBase, offset))
return true;
@@ -517,8 +541,15 @@
if (value.isInfinite())
return false;
for (unsigned i = value.size(); i--;) {
- if (!value[i]->isValidOffset(offset))
+ Structure* thisStructure = value[i].get();
+ if (!thisStructure->isValidOffset(offset))
return false;
+ if (inferredType.kind() != InferredType::Top) {
+ InferredType::Descriptor thisInferredType =
+ graph.inferredTypeForProperty(thisStructure, uid);
+ if (!inferredType.subsumes(thisInferredType))
+ return false;
+ }
}
return true;
}
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
