Attachment 341140 Details for Bug 185923 – Patch

[patch] Patch

bug-185923-20180523155842.patch (text/plain), 3.46 KB, created by Keith Miller on 2018-05-23 15:58:43 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Keith Miller

Created: 2018-05-23 15:58:43 PDT

Size: 3.46 KB

patch

obsolete

>Subversion Revision: 232113
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 5d6abeb4cf87e6cbebbd787ef91ac95ec5ccb92a..af2084514588ed0429a692253576b855e5f2f704 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,26 @@
>+2018-05-23  Keith Miller  <keith_miller@apple.com>
>+
>+        InPlaceAbstractState should filter variables at the tail from a GetLocal by their flush format
>+        https://bugs.webkit.org/show_bug.cgi?id=185923
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Previously, we could confuse AI by overly broadening a type. This happens when a block in a
>+        loop has a local mutated following a GetLocal but never SetLocaled to the stack. For example,
>+
>+        Block 1:
>+        @1: GetLocal(loc42, FlushedInt32);
>+        @2: PutStructure(Check: Cell: @1);
>+        @3: Jump(Block 1);
>+
>+        Would cause us to claim that loc42 could be either an int32 or a some cell. However,
>+        the type of an local cannot change without writing to it.
>+
>+        This fixes a crash in destructuring-rest-element.js
>+
>+        * dfg/DFGInPlaceAbstractState.cpp:
>+        (JSC::DFG::InPlaceAbstractState::endBasicBlock):
>+
> 2018-05-23  Rick Waldron  <waldron.rick@gmail.com>
> 
>         Conversion misspelled "Convertion" in error message string
>diff --git a/Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.cpp b/Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.cpp
>index 8d4a656552549875210577a51fbff28136085b03..859373e52bd7a1c40e39cf7d8ce7747fcc79b5f7 100644
>--- a/Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.cpp
>+++ b/Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.cpp
>@@ -236,21 +236,39 @@ bool InPlaceAbstractState::endBasicBlock()
>             case Phi:
>             case SetArgument:
>             case PhantomLocal:
>-            case Flush:
>+            case Flush: {
>                 // The block transfers the value from head to tail.
>                 destination = variableAt(index);
>                 break;
>+            }
>                 
>-            case GetLocal:
>+            case GetLocal: {
>                 // The block refines the value with additional speculations.
>                 destination = forNode(node);
>+
>+                // We need to make sure that we don't broaden the type beyond what the flush
>+                // format says it will be. The value may claim to have changed abstract state
>+                // but it's type from abstract state. For example:
>+                //
>+                // Block #1:
>+                // 0: GetLocal(loc42, FlushFormatInt32)
>+                // 1: PutStructure(Check: Cell: @0, ArrayStructure)
>+                // ...
>+                // 2: Branch(T: #1, F: #2)
>+                //
>+                // In this case the AbstractState of @0 will say it's an SpecArray but the only
>+                // reason that would have happened is because we would have exited the cell check.
>+
>+                FlushFormat flushFormat = node->variableAccessData()->flushFormat();
>+                destination.filter(typeFilterFor(flushFormat));
>                 break;
>-                
>-            case SetLocal:
>+            }
>+            case SetLocal: {
>                 // The block sets the variable, and potentially refines it, both
>                 // before and after setting it.
>                 destination = forNode(node->child1());
>                 break;
>+            }
>                 
>             default:
>                 RELEASE_ASSERT_NOT_REACHED();

Flags:

saam: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 185923: 341136 | 341140