Changeset 230287 in webkit for trunk/Source/JavaScriptCore/dfg/DFGCSEPhase.cpp

Timestamp:

Apr 4, 2018, 5:30:48 PM (7 years ago)

Author:

[email protected]

Message:

REGRESSION(r222563): removed DoubleReal type check causes tons of crashes because CSE has never known how to handle SaneChain
​https://bugs.webkit.org/show_bug.cgi?id=184319

Reviewed by Saam Barati.

JSTests:

• stress/array-push-nan-to-double-array-cse-sane-and-insane-chain.js: Added.

(foo):
(bar):

• stress/array-push-nan-to-double-array.js: Added.

(foo):
(bar):

Source/JavaScriptCore:

In r222581, we replaced type checks about DoubleReal in ArrayPush in the DFG/FTL backends with
assertions. That's correct because FixupPhase was emitting those checks as Check(DoubleRealRep:) before
the ArrayPush.

But this revealed a longstanding CSE bug: CSE will happily match a SaneChain GetByVal with a InBounds
GetByVal. SaneChain can return NaN while InBounds cannot. This means that if we first use AI to
eliminate the Check(DoubleRealRep:) based on the input being a GetByVal(InBounds) but then replace that
with a GetByVal(SaneChain), then we will hit the assertion.

This teaches CSE to not replace GetByVal(InBounds) with GetByVal(SaneChain) and vice versa. That gets
tricky because PutByVal can match either. So, we use the fact that it's legal for a store to def() more
than once: PutByVal now defs() a HeapLocation for InBounds and a HeapLocation for SaneChain.

• dfg/DFGCSEPhase.cpp:
• dfg/DFGClobberize.h:

(JSC::DFG::clobberize):

• dfg/DFGHeapLocation.cpp:

(WTF::printInternal):

• dfg/DFGHeapLocation.h:
• dfg/DFGSpeculativeJIT.cpp:

(JSC::DFG::SpeculativeJIT::compileArrayPush):

File:

• trunk/Source/JavaScriptCore/dfg/DFGCSEPhase.cpp (modified) (2 diffs)

trunk/Source/JavaScriptCore/dfg/DFGCSEPhase.cpp

@@ -1 +1 @@
 /*
- * Copyright (C) 2011-2015 Apple Inc. All rights reserved.
+ * Copyright (C) 2011-2018 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -470 +470 @@
                             if (!mode.isInBounds())
                                 break;
-                            heap = HeapLocation(
-                                indexedPropertyLoc, IndexedInt32Properties, base, index);
+                            heap = HeapLocation(indexedPropertyLoc, IndexedInt32Properties, base, index);
                             break;
                             
-                        case Array::Double:
+                        case Array::Double: {
                             if (!mode.isInBounds())
                                 break;
-                            heap = HeapLocation(
-                                indexedPropertyLoc, IndexedDoubleProperties, base, index);
+                            LocationKind kind = mode.isSaneChain() ? IndexedPropertyDoubleSaneChainLoc : IndexedPropertyDoubleLoc;
+                            heap = HeapLocation(kind, IndexedDoubleProperties, base, index);
                             break;
+                        }
                             
                         case Array::Contiguous:
                             if (!mode.isInBounds())
                                 break;
-                            heap = HeapLocation(
-                                indexedPropertyLoc, IndexedContiguousProperties, base, index);
+                            heap = HeapLocation(indexedPropertyLoc, IndexedContiguousProperties, base, index);
                             break;