Reviewers: Toon Verwaest,

Message:
Hi Toon,
Here is the fix we discussed, thx for the look,
--Michael


Description:
Fix for 435073: CHECK failure in CHECK(p->IsSmi()) failed.

The bug was an error when copying arrays in crankshaft. If it's a holey smi
array, the copy must be done as FAST_HOLEY_ELEMENTS to prevent representation
changes from being inserted that deopt on encountering the hole.

Also, prevent inlining array pop() and shift() if the length is read-only.

BUG=435073
LOG=N

Please review this at https://codereview.chromium.org/737383002/

Base URL: https://chromium.googlesource.com/v8/v8.git@master

Affected files (+20, -14 lines):
  M src/hydrogen.cc
  A + test/mjsunit/regress/regress-435073.js


Index: src/hydrogen.cc
diff --git a/src/hydrogen.cc b/src/hydrogen.cc
index 21ef8c019fb410b26214f1751c53e27e3a8579fb..8194d6b8ae49e37a567f175931e88a878be2f678 100644
--- a/src/hydrogen.cc
+++ b/src/hydrogen.cc
@@ -8355,6 +8355,7 @@ bool HOptimizedGraphBuilder::TryInlineBuiltinMethodCall(
       if (receiver_map.is_null()) return false;
       if (receiver_map->instance_type() != JS_ARRAY_TYPE) return false;
       ElementsKind elements_kind = receiver_map->elements_kind();
+      if (JSArray::IsReadOnlyLengthDescriptor(receiver_map)) return false;
       if (!IsFastElementsKind(elements_kind)) return false;
       if (receiver_map->is_observed()) return false;
       if (!receiver_map->is_extensible()) return false;
@@ -8472,6 +8473,7 @@ bool HOptimizedGraphBuilder::TryInlineBuiltinMethodCall(
       if (receiver_map.is_null()) return false;
       if (receiver_map->instance_type() != JS_ARRAY_TYPE) return false;
       ElementsKind kind = receiver_map->elements_kind();
+      if (JSArray::IsReadOnlyLengthDescriptor(receiver_map)) return false;
       if (!IsFastElementsKind(kind)) return false;
       if (receiver_map->is_observed()) return false;
       if (!receiver_map->is_extensible()) return false;
@@ -8544,10 +8546,12 @@ bool HOptimizedGraphBuilder::TryInlineBuiltinMethodCall(
                   graph()->GetConstant0(), new_length, Token::LT);
HValue* key = AddUncasted<HAdd>(new_key, graph()->GetConstant1());
               key->ClearFlag(HValue::kCanOverflow);
+              ElementsKind copy_kind =
+ kind == FAST_HOLEY_SMI_ELEMENTS ? FAST_HOLEY_ELEMENTS : kind;
               HValue* element = AddUncasted<HLoadKeyed>(
-                  elements, key, lengthiszero, kind, ALLOW_RETURN_HOLE);
-              HStoreKeyed* store = Add<HStoreKeyed>(
-                  elements, new_key, element, kind);
+ elements, key, lengthiszero, copy_kind, ALLOW_RETURN_HOLE);
+              HStoreKeyed* store =
+                  Add<HStoreKeyed>(elements, new_key, element, copy_kind);
               store->SetFlag(HValue::kAllowUndefinedAsNaN);
             }
             loop.EndBody();
@@ -11375,11 +11379,13 @@ void HOptimizedGraphBuilder::BuildEmitFixedArray(
       site_context->ExitScope(current_site, value_object);
       Add<HStoreKeyed>(object_elements, key_constant, result, kind);
     } else {
-      HInstruction* value_instruction =
-          Add<HLoadKeyed>(boilerplate_elements, key_constant,
-                          static_cast<HValue*>(NULL), kind,
-                          ALLOW_RETURN_HOLE);
- Add<HStoreKeyed>(object_elements, key_constant, value_instruction, kind);
+      ElementsKind copy_kind =
+          kind == FAST_HOLEY_SMI_ELEMENTS ? FAST_HOLEY_ELEMENTS : kind;
+      HInstruction* value_instruction = Add<HLoadKeyed>(
+          boilerplate_elements, key_constant, static_cast<HValue*>(NULL),
+          copy_kind, ALLOW_RETURN_HOLE);
+      Add<HStoreKeyed>(object_elements, key_constant, value_instruction,
+                       copy_kind);
     }
   }
 }
Index: test/mjsunit/regress/regress-435073.js
diff --git a/test/mjsunit/regress/regress-keyed-store-global.js b/test/mjsunit/regress/regress-435073.js
similarity index 70%
copy from test/mjsunit/regress/regress-keyed-store-global.js
copy to test/mjsunit/regress/regress-435073.js
index 1b127776d6c3635bbf6e57b2256319288b8f411c..dbaa612afae9d33e63c96ea7d639c76896bbd9a2 100644
--- a/test/mjsunit/regress/regress-keyed-store-global.js
+++ b/test/mjsunit/regress/regress-435073.js
@@ -3,10 +3,10 @@
 // found in the LICENSE file.

 // Flags: --allow-natives-syntax --verify-heap
-function f(a) {
-  for (var i = 0; i < 256; i++) a[i] = i;
-}

-f([]);
-f([]);
-f(this);
+function test(x) { [x,,]; }
+
+test(0);
+test(0);
+%OptimizeFunctionOnNextCall(test);
+test(0);


--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
--- You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to [email protected].
For more options, visit https://groups.google.com/d/optout.

Reply via email to