Changeset 230143 in webkit

Timestamp:

Apr 1, 2018 10:08:39 AM (6 years ago)

Author:

fpizlo@apple.com

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/JavaScriptCore:

• 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.

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/microbenchmarks/hoist-get-by-offset-tower-with-inferred-types.js (added)
• JSTests/stress/hoist-get-by-offset-with-control-dependent-inferred-type.js (added)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/dfg/DFGSafeToExecute.h (modified) (3 diffs)

trunk/JSTests/ChangeLog

@@ +1 @@
+2018-03-31  Filip Pizlo  <fpizlo@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  <jfbastien@apple.com>
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2018-03-31  Filip Pizlo  <fpizlo@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  <fpizlo@apple.com>
 

trunk/Source/JavaScriptCore/dfg/DFGSafeToExecute.h

@@ -1 +1 @@
 /*
- * 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
@@ -505 +505 @@
     case GetGetterSetterByOffset:
     case PutByOffset: {
-        PropertyOffset offset = node->storageAccessData().offset;
-
-        if (state.structureClobberState() == StructuresAreWatched) {
+        StorageAccessData& data = node->storageAccessData();
+        PropertyOffset offset = data.offset;
+        UniquedStringImpl* uid = graph.identifiers()[data.identifierNumber];
+
+        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))
@@ -518 +542 @@
             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;