Changeset 264643 in webkit

Timestamp:

Jul 20, 2020, 10:16:51 PM (5 years ago)

Author:

mark.lam@apple.com

Message:

TryGetById clobberize rules are wrong.
​https://bugs.webkit.org/show_bug.cgi?id=163834
<rdar://problem/65625807>

Reviewed by Keith Miller.

Theoretically, TryGetById can do the same things GetById does i.e. reify lazy
properties, read the stack, etc. Hence, its clobberize rule should be clobberTop
just like GetById. However, in practice, we don't currently use @tryGetById to
access anything on the stack (and probably never will). But as a conservative
measure, we'll just treat TryGetById like it can. In clobberize terms, this
means we declare TryGetById as doing read(World) (just like GetById) instead of
read(Heap).

• dfg/DFGAbstractInterpreterInlines.h:

(JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):

• dfg/DFGClobberize.h:

(JSC::DFG::clobberize):

• dfg/DFGClobbersExitState.cpp:

(JSC::DFG::clobbersExitState):

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• dfg/DFGAbstractInterpreterInlines.h (modified) (1 diff)
• dfg/DFGClobberize.h (modified) (2 diffs)
• dfg/DFGClobbersExitState.cpp (modified) (1 diff)

trunk/Source/JavaScriptCore/ChangeLog ¶

@@ +1 @@
+2020-07-20  Mark Lam  <mark.lam@apple.com>
+
+        TryGetById clobberize rules are wrong.
+        https://bugs.webkit.org/show_bug.cgi?id=163834
+        <rdar://problem/65625807>
+
+        Reviewed by Keith Miller.
+
+        Theoretically, TryGetById can do the same things GetById does i.e. reify lazy
+        properties, read the stack, etc.  Hence, its clobberize rule should be clobberTop
+        just like GetById.  However, in practice, we don't currently use @tryGetById to
+        access anything on the stack (and probably never will).  But as a conservative
+        measure, we'll just treat TryGetById like it can.  In clobberize terms, this
+        means we declare TryGetById as doing read(World) (just like GetById) instead of
+        read(Heap).
+
+        * dfg/DFGAbstractInterpreterInlines.h:
+        (JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
+        * dfg/DFGClobberize.h:
+        (JSC::DFG::clobberize):
+        * dfg/DFGClobbersExitState.cpp:
+        (JSC::DFG::clobbersExitState):
+
 2020-07-20  Yusuke Suzuki  <ysuzuki@apple.com>
 

trunk/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h ¶

@@ -3335 +3335 @@
         // FIXME: This should constant fold at least as well as the normal GetById case.
         // https://bugs.webkit.org/show_bug.cgi?id=156422
+        clobberWorld();
         makeHeapTopForNode(node);
         break;

trunk/Source/JavaScriptCore/dfg/DFGClobberize.h ¶

@@ -656 +656 @@
     case GetByIdDirectFlush:
     case GetByValWithThis:
+    case TryGetById:
     case PutById:
     case PutByIdWithThis:
@@ -1305 +1306 @@
     }
 
-    case TryGetById: {
-        read(Heap);
-        return;
-    }
-
     case MultiGetByOffset: {
         read(JSCell_structureID);

trunk/Source/JavaScriptCore/dfg/DFGClobbersExitState.cpp ¶

@@ -86 +86 @@
     case FilterInByIdStatus:
     case FilterDeleteByStatus:
+    case TryGetById:
         // These do clobber memory, but nothing that is observable. It may be nice to separate the
         // heaps into those that are observable and those that aren't, but we don't do that right now.