Changeset 238411 in webkit

Timestamp:

Nov 20, 2018 9:15:31 PM (5 years ago)

Author:

sbarati@apple.com

Message:

Merging an IC variant may lead to the IC status containing overlapping structure sets
​https://bugs.webkit.org/show_bug.cgi?id=191869
<rdar://problem/45403453>

Reviewed by Mark Lam.

JSTests:

• stress/merging-ic-variants-should-bail-if-structures-overlap.js: Added.

Source/JavaScriptCore:

When merging two IC variant lists, we may end up in a world where we have
overlapping structure sets. We defend against this when we append a new
variant, but we should also defend against it once we merge in a new variant.

Consider this case with MultiPutByOffset, where we merge two PutByIdStatuses
together, P1 and P2.

Let's consider these structures:
s1 = {}
s2 = {p: 0}
s3 = {p: 0, p2: 1}

P1 contains these variants:
Transition: [s1 => s2]
Replace: [s2, s3]

P2 contains:
Replace: [s2]

Because of the ordering of the variants, we may end up combining
P2's replace into P1's transition, forming this new list:
Transition: [(s1, s2) => s2]
Replace: [s2, s3]

Obviously the ideal thing here is to have some ordering when we merge
in variants to choose the most ideal option. It'd be ideal for P2's
Replace to be merged into P1's replace.

If we notice that this is super important, we can implement some kind
of ordering. None of our tests (until this patch) stress this. This patch
just makes it so we defend against this crazy scenario by falling back
to the slow path gracefully. This prevents us from emitting invalid
IR in FTL->B3 lowering by creating a switch with two case labels being
identical values.

• bytecode/ICStatusUtils.h:

(JSC::appendICStatusVariant):

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/stress/merging-ic-variants-should-bail-if-structures-overlap.js (added)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/bytecode/ICStatusUtils.h (modified) (1 diff)

trunk/JSTests/ChangeLog

@@ +1 @@
+2018-11-20  Saam barati  <sbarati@apple.com>
+
+        Merging an IC variant may lead to the IC status containing overlapping structure sets
+        https://bugs.webkit.org/show_bug.cgi?id=191869
+        <rdar://problem/45403453>
+
+        Reviewed by Mark Lam.
+
+        * stress/merging-ic-variants-should-bail-if-structures-overlap.js: Added.
+
 2018-11-19  Mark Lam  <mark.lam@apple.com>
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2018-11-20  Saam barati  <sbarati@apple.com>
+
+        Merging an IC variant may lead to the IC status containing overlapping structure sets
+        https://bugs.webkit.org/show_bug.cgi?id=191869
+        <rdar://problem/45403453>
+
+        Reviewed by Mark Lam.
+
+        When merging two IC variant lists, we may end up in a world where we have
+        overlapping structure sets. We defend against this when we append a new
+        variant, but we should also defend against it once we merge in a new variant.
+        
+        Consider this case with MultiPutByOffset, where we merge two PutByIdStatuses
+        together, P1 and P2.
+        
+        Let's consider these structures:
+        s1 = {}
+        s2 = {p: 0}
+        s3 = {p: 0, p2: 1}
+        
+        P1 contains these variants:
+        Transition: [s1 => s2]
+        Replace: [s2, s3]
+        
+        P2 contains:
+        Replace: [s2]
+        
+        Because of the ordering of the variants, we may end up combining
+        P2's replace into P1's transition, forming this new list:
+        Transition: [(s1, s2) => s2]
+        Replace: [s2, s3]
+        
+        Obviously the ideal thing here is to have some ordering when we merge
+        in variants to choose the most ideal option. It'd be ideal for P2's
+        Replace to be merged into P1's replace.
+        
+        If we notice that this is super important, we can implement some kind
+        of ordering. None of our tests (until this patch) stress this. This patch
+        just makes it so we defend against this crazy scenario by falling back
+        to the slow path gracefully. This prevents us from emitting invalid
+        IR in FTL->B3 lowering by creating a switch with two case labels being
+        identical values.
+
+        * bytecode/ICStatusUtils.h:
+        (JSC::appendICStatusVariant):
+
 2018-11-20  Fujii Hironori  <Hironori.Fujii@sony.com>
 

trunk/Source/JavaScriptCore/bytecode/ICStatusUtils.h

@@ -35 +35 @@
     // Attempt to merge this variant with an already existing variant.
     for (unsigned i = 0; i < variants.size(); ++i) {
-        if (variants[i].attemptToMerge(variant))
+        VariantType& mergedVariant = variants[i];
+        if (mergedVariant.attemptToMerge(variant)) {
+            for (unsigned j = 0; j < variants.size(); ++j) {
+                if (i == j)
+                    continue;
+                if (variants[j].structureSet().overlaps(mergedVariant.structureSet()))
+                    return false;
+            }
             return true;
+        }
     }