Attachment 362738 Details for Bug 194953 – Patch

[patch] Patch

patch194953 (text/plain), 4.38 KB, created by Robin Morisset on 2019-02-22 11:52:33 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Robin Morisset

Created: 2019-02-22 11:52:33 PST

Size: 4.38 KB

patch

obsolete

>Index: JSTests/ChangeLog
>===================================================================
>--- JSTests/ChangeLog	(revision 241955)
>+++ JSTests/ChangeLog	(working copy)
>@@ -1,3 +1,15 @@
>+2019-02-22  Robin Morisset  <rmorisset@apple.com>
>+
>+        DFGBytecodeParser should not declare that a node won't clobberExit if DFGFixupPhase can later declare it does clobberExit
>+        https://bugs.webkit.org/show_bug.cgi?id=194953
>+        <rdar://problem/47595253>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        I could not make this work without the infinite loop, so I am using a watchdog to be able to use it as a regression test.
>+
>+        * stress/has-indexed-property-with-worsening-array-mode.js: Added.
>+
> 2019-02-19  Joseph Pecoraro  <pecoraro@apple.com>
> 
>         Web Inspector: Improve ES6 Class instances in Heap Snapshot instances view
>Index: JSTests/stress/has-indexed-property-with-worsening-array-mode.js
>===================================================================
>--- JSTests/stress/has-indexed-property-with-worsening-array-mode.js	(nonexistent)
>+++ JSTests/stress/has-indexed-property-with-worsening-array-mode.js	(working copy)
>@@ -0,0 +1,6 @@
>+//@ requireOptions("--watchdog=1000", "--watchdog-exception-ok", "--useMaximalFlushInsertionPhase=1")
>+// This test only seems to reproduce the issue when it runs in an infinite loop. So we use the watchdog to time it out.
>+for (let x in [0]) {
>+  break
>+}
>+while(1);
>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 241955)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,40 @@
>+2019-02-22  Robin Morisset  <rmorisset@apple.com>
>+
>+        DFGBytecodeParser should not declare that a node won't clobberExit if DFGFixupPhase can later declare it does clobberExit
>+        https://bugs.webkit.org/show_bug.cgi?id=194953
>+        <rdar://problem/47595253>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        For each node that
>+        (a) may or may not clobberExit depending on their arrayMode
>+        (b) and get their arrayMode from profiling information in DFGBytecodeParser
>+        (c) and can have their arrayMode refined by DFGFixupPhase,
>+        We must make sure to be conservative in the DFGBytecodeParser and treat it as if it unconditionnally clobbered the exit.
>+        Otherwise we are likely to hit validation failure after fixup.
>+
>+        The list of nodes that fit (a) is:
>+        - StringCharAt
>+        - HasIndexProperty
>+        - GetByVal
>+        - PutByValDirect
>+        - PutByVal
>+        - PutByValAlias
>+        - GetIndexedPropertyStorage
>+
>+        Out of these, the following also fit (b) and (c):
>+        - HasIndexedProperty
>+        - GetByVal
>+        - PutByValDirect
>+        - PutByVal
>+
>+        GetByVal already had "m_exitOK = false; // GetByVal must be treated as if it clobbers exit state, since FixupPhase may make it generic."
>+        So we just have to fix the other three the same way.
>+
>+        * dfg/DFGByteCodeParser.cpp:
>+        (JSC::DFG::ByteCodeParser::parseBlock):
>+        (JSC::DFG::ByteCodeParser::handlePutByVal):
>+
> 2019-02-22  Yusuke Suzuki  <ysuzuki@apple.com>
> 
>         Unreviewed, build fix after r241954
>Index: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp	(revision 241955)
>+++ Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp	(working copy)
>@@ -6834,6 +6834,7 @@
>             addVarArgChild(property);
>             addVarArgChild(nullptr);
>             Node* hasIterableProperty = addToGraph(Node::VarArg, HasIndexedProperty, OpInfo(arrayMode.asWord()), OpInfo(static_cast<uint32_t>(PropertySlot::InternalMethodType::GetOwnProperty)));
>+            m_exitOK = false; // HasIndexedProperty must be treated as if it clobbers exit state, since FixupPhase may make it generic.
>             set(bytecode.m_dst, hasIterableProperty);
>             NEXT_OPCODE(op_has_indexed_property);
>         }
>@@ -7212,6 +7213,7 @@
>         addVarArgChild(0); // Leave room for property storage.
>         addVarArgChild(0); // Leave room for length.
>         addToGraph(Node::VarArg, isDirect ? PutByValDirect : PutByVal, OpInfo(arrayMode.asWord()), OpInfo(0));
>+        m_exitOK = false; // PutByVal and PutByValDirect must be treated as if they clobber exit state, since FixupPhase may make them generic.
>     }
> }
>

Flags:

saam: review+
saam: commit-queue-

Actions: View | Formatted Diff | Diff

Attachments on bug 194953: 362738 | 362758