Attachment 361420 Details for Bug 194399 – proposed patch.

[patch] proposed patch.

bug-194399.patch (text/plain), 6.92 KB, created by Mark Lam on 2019-02-07 11:09:15 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Mark Lam

Created: 2019-02-07 11:09:15 PST

Size: 6.92 KB

patch

obsolete

>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 241039)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,44 @@
>+2019-02-05  Mark Lam  <mark.lam@apple.com>
>+
>+        Fix more doesGC() for CheckTraps, GetMapBucket, and Switch nodes.
>+        https://bugs.webkit.org/show_bug.cgi?id=194399
>+        <rdar://problem/47889777>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Fix doesGC() for the following nodes:
>+
>+            CheckTraps:
>+                We normally will not emit this node because Options::usePollingTraps() is
>+                false by default.  However, as it is implemented now, CheckTraps can GC
>+                because it can allocate a TerminatedExecutionException.  If we make the
>+                TerminatedExecutionException a singleton allocated at initialization time,
>+                doesGC() can return false for CheckTraps.
>+                https://bugs.webkit.org/show_bug.cgi?id=194323
>+
>+            GetMapBucket:
>+                Can call operationJSMapFindBucket() or operationJSSetFindBucket(),
>+                which calls HashMapImpl::findBucket(), which calls jsMapHash(), which
>+                can resolve a rope.
>+
>+            Switch:
>+                If switchData kind is SwitchChar, can call operationResolveRope() .
>+                If switchData kind is SwitchString and the child use kind is not StringIdentUse,
>+                    can call operationSwitchString() which resolves ropes.
>+
>+            DirectTailCall:
>+            ForceOSRExit:
>+            Return:
>+            TailCallForwardVarargs:
>+            TailCallVarargs:
>+            Throw:
>+                These are terminal nodes.  It shouldn't really matter what doesGC() returns
>+                for them, but following our conservative practice, unless we have a good
>+                reason for doesGC() to return false, we should just return true.
>+
>+        * dfg/DFGDoesGC.cpp:
>+        (JSC::DFG::doesGC):
>+
> 2019-02-06  Yusuke Suzuki  <ysuzuki@apple.com>
> 
>         [JSC] Unify indirectEvalExecutableSpace and directEvalExecutableSpace
>Index: Source/JavaScriptCore/dfg/DFGDoesGC.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGDoesGC.cpp	(revision 241039)
>+++ Source/JavaScriptCore/dfg/DFGDoesGC.cpp	(working copy)
>@@ -41,8 +41,19 @@ bool doesGC(Graph& graph, Node* node)
>         return true;
>     
>     // Now consider nodes that don't clobber the world but that still may GC. This includes all
>-    // nodes. By convention we put world-clobbering nodes in the block of "false" cases but we can
>-    // put them anywhere.
>+    // nodes. By default, we should assume every node can GC and return true. This includes the
>+    // world-clobbering nodes. We should only return false if we have proven that the node cannot
>+    // GC. Typical examples of how a node can GC is if the code emitted for the node does any of the
>+    // following:
>+    //     1. Allocates any objects.
>+    //     2. Resolves a rope string, which allocates a string.
>+    //     3. Produces a string (which allocates the string) except when we can prove that
>+    //        the string will always be one of the pre-allcoated SmallStrings.
>+    //     4. Triggers a structure transition (which can allocate a new structure)
>+    //        unless it is a known transition between previously allocated structures
>+    //        such as between Array types.
>+    //     5. Calls to a JS function, which can execute arbitrary code including allocating objects.
>+
>     switch (node->op()) {
>     case JSConstant:
>     case DoubleConstant:
>@@ -130,7 +141,6 @@ bool doesGC(Graph& graph, Node* node)
>     case CompareEq:
>     case CompareStrictEq:
>     case CompareEqPtr:
>-    case TailCallForwardVarargs:
>     case ProfileType:
>     case ProfileControlFlow:
>     case OverridesHasInstance:
>@@ -149,20 +159,12 @@ bool doesGC(Graph& graph, Node* node)
>     case LogicalNot:
>     case Jump:
>     case Branch:
>-    case Switch:
>     case EntrySwitch:
>-    case Return:
>-    case DirectTailCall:
>-    case TailCallVarargs:
>-    case Throw:
>     case CountExecution:
>     case SuperSamplerBegin:
>     case SuperSamplerEnd:
>-    case ForceOSRExit:
>     case CPUIntrinsic:
>-    case CheckTraps:
>     case NormalizeMapKey:
>-    case GetMapBucket:
>     case GetMapBucketHead:
>     case GetMapBucketNext:
>     case LoadKeyFromMapBucket:
>@@ -257,6 +259,7 @@ bool doesGC(Graph& graph, Node* node)
>     case DataViewSet:
>         return false;
> 
>+#if !ASSERT_DISABLED
>     case ArrayPush:
>     case ArrayPop:
>     case PushWithScope:
>@@ -278,7 +281,9 @@ bool doesGC(Graph& graph, Node* node)
>     case DeleteByVal:
>     case DirectCall:
>     case DirectConstruct:
>+    case DirectTailCall:
>     case DirectTailCallInlinedCaller:
>+    case ForceOSRExit:
>     case GetById:
>     case GetByIdDirect:
>     case GetByIdDirectFlush:
>@@ -287,6 +292,7 @@ bool doesGC(Graph& graph, Node* node)
>     case GetByValWithThis:
>     case GetDirectPname:
>     case GetDynamicVar:
>+    case GetMapBucket:
>     case HasGenericProperty:
>     case HasOwnProperty:
>     case HasStructureProperty:
>@@ -318,10 +324,14 @@ bool doesGC(Graph& graph, Node* node)
>     case RegExpTest:
>     case ResolveScope:
>     case ResolveScopeForHoistingFuncDeclInEval:
>+    case Return:
>     case TailCall:
>+    case TailCallForwardVarargs:
>     case TailCallForwardVarargsInlinedCaller:
>     case TailCallInlinedCaller:
>+    case TailCallVarargs:
>     case TailCallVarargsInlinedCaller:
>+    case Throw:
>     case ToNumber:
>     case ToObject:
>     case ToPrimitive:
>@@ -378,6 +388,11 @@ bool doesGC(Graph& graph, Node* node)
>     case ValueMul:
>     case ValueDiv:
>     case ValueNegate:
>+#else
>+    // See comment at the top for why be default for all nodes should be to
>+    // return true.
>+    default:
>+#endif
>         return true;
> 
>     case CallStringConstructor:
>@@ -391,6 +406,11 @@ bool doesGC(Graph& graph, Node* node)
>         }
>         return true;
> 
>+    case CheckTraps:
>+        // FIXME: https://bugs.webkit.org/show_bug.cgi?id=194323
>+        ASSERT(Options::usePollingTraps());
>+        return true;
>+
>     case GetIndexedPropertyStorage:
>         if (node->arrayMode().type() == Array::String)
>             return true;
>@@ -423,6 +443,22 @@ bool doesGC(Graph& graph, Node* node)
>             return false;
>         return true;
> 
>+    case Switch:
>+        switch (node->switchData()->kind) {
>+        case SwitchCell:
>+            ASSERT(graph.m_plan.isFTL());
>+            FALLTHROUGH;
>+        case SwitchImm:
>+            return false;
>+        case SwitchChar:
>+            return true;
>+        case SwitchString:
>+            if (node->child1().useKind() == StringIdentUse)
>+                return false;
>+            ASSERT(node->child1().useKind() == StringUse || node->child1().useKind() == UntypedUse);
>+            return true;
>+        }
>+
>     case LastNodeType:
>         RELEASE_ASSERT_NOT_REACHED();
>         return true;

Flags:

ysuzuki: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 194399: 361420