Improve elements validation and object printing

src/elements.cc

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/elements.h"
 
 #include "src/arguments.h"
 #include "src/conversions.h"
 #include "src/factory.h"
 #include "src/isolate-inl.h"
@@ skipping 1554 matching lines @@
       if (IsFastPackedElementsKind(KindTraits::Kind) ||
           HasEntryImpl(*elements, i)) {
         accumulator->AddKey(Subclass::GetImpl(*elements, i), convert);
       }
     }
   }
 
   static void ValidateContents(Handle<JSObject> holder, int length) {
 #if DEBUG
     Isolate* isolate = holder->GetIsolate();
+    Heap* heap = isolate->heap();
     HandleScope scope(isolate);
     Handle<FixedArrayBase> elements(holder->elements(), isolate);
     Map* map = elements->map();
-    DCHECK((IsFastSmiOrObjectElementsKind(KindTraits::Kind) &&
-            (map == isolate->heap()->fixed_array_map() ||
-             map == isolate->heap()->fixed_cow_array_map())) ||
-           (IsFastDoubleElementsKind(KindTraits::Kind) ==
-            ((map == isolate->heap()->fixed_array_map() && length == 0) ||
-             map == isolate->heap()->fixed_double_array_map())));
+    if (IsFastSmiOrObjectElementsKind(KindTraits::Kind)) {
+      DCHECK(map != heap->fixed_double_array_map());
+    } else if (IsFastDoubleElementsKind(KindTraits::Kind)) {
+      DCHECK(map != heap->fixed_cow_array_map());

[Toon Verwaest, 2016/04/07 13:01:39]

DCHECK_NE Would already be included in the suggested check below.

+      if (map == heap->fixed_array_map()) DCHECK_EQ(0, length);

[Toon Verwaest, 2016/04/07 13:01:39]

DCHECK(map == fixed_double_array_map() ||
       (map == fixed_array_map() && 0 == length)); ?

[Camillo Bruni, 2016/04/07 13:12:38]

I prefer the separate checks, makes it easier for debugging and adding more
checks.

+    } else {
+      UNREACHABLE();
+    }
     if (length == 0) return;  // nothing to do!
     DisallowHeapAllocation no_gc;
     Handle<BackingStore> backing_store = Handle<BackingStore>::cast(elements);
     if (IsFastSmiElementsKind(KindTraits::Kind)) {
       for (int i = 0; i < length; i++) {
         DCHECK(BackingStore::get(*backing_store, i, isolate)->IsSmi() ||
                (IsFastHoleyElementsKind(KindTraits::Kind) &&
                 backing_store->is_the_hole(i)));
       }
+    } else if (KindTraits::Kind == FAST_ELEMENTS ||
+               KindTraits::Kind == FAST_DOUBLE_ELEMENTS) {
+      for (int i = 0; i < length; i++) {

[Toon Verwaest, 2016/04/07 13:01:39]

This probably should be SLOW_ASSERTS

[Camillo Bruni, 2016/04/07 13:12:38]

yup

+        DCHECK(!backing_store->is_the_hole(i));
+      }
+    } else {
+      DCHECK(IsFastHoleyElementsKind(KindTraits::Kind));
     }
 #endif
   }
 
   static Handle<Object> PopImpl(Handle<JSArray> receiver) {
     return Subclass::RemoveElement(receiver, AT_END);
   }
 
   static Handle<Object> ShiftImpl(Handle<JSArray> receiver) {
     return Subclass::RemoveElement(receiver, AT_START);
@@ skipping 195 matching lines @@
 
     if (IsHoleyElementsKind(kind) && result->IsTheHole()) {
       return isolate->factory()->undefined_value();
     }
     return result;
   }
 
   static uint32_t AddArguments(Handle<JSArray> receiver,
                                Handle<FixedArrayBase> backing_store,
                                Arguments* args, uint32_t add_size,
-                               Where remove_position) {
+                               Where add_position) {
     uint32_t length = Smi::cast(receiver->length())->value();
     DCHECK(0 < add_size);
     uint32_t elms_len = backing_store->length();
     // Check we do not overflow the new_length.
     DCHECK(add_size <= static_cast<uint32_t>(Smi::kMaxValue - length));
     uint32_t new_length = length + add_size;
 
     if (new_length > elms_len) {
       // New backing storage is needed.
       uint32_t capacity = JSObject::NewElementsCapacity(new_length);
       // If we add arguments to the start we have to shift the existing objects.
-      int copy_dst_index = remove_position == AT_START ? add_size : 0;
+      int copy_dst_index = add_position == AT_START ? add_size : 0;
       // Copy over all objects to a new backing_store.
       backing_store = Subclass::ConvertElementsWithCapacity(
           receiver, backing_store, KindTraits::Kind, capacity, 0,
           copy_dst_index, ElementsAccessor::kCopyToEndAndInitializeToHole);
       receiver->set_elements(*backing_store);
-    } else if (remove_position == AT_START) {
+    } else if (add_position == AT_START) {
       // If the backing store has enough capacity and we add elements to the
       // start we have to shift the existing objects.
       Isolate* isolate = receiver->GetIsolate();
       Subclass::MoveElements(isolate, receiver, backing_store, add_size, 0,
                              length, 0, 0);
     }
 
-    int insertion_index = remove_position == AT_START ? 0 : length;
+    int insertion_index = add_position == AT_START ? 0 : length;
     // Copy the arguments to the start.
     Subclass::CopyArguments(args, backing_store, add_size, 1, insertion_index);
     // Set the length.
     receiver->set_length(Smi::FromInt(new_length));
     return new_length;
   }
 
   static void CopyArguments(Arguments* args, Handle<FixedArrayBase> dst_store,
                             uint32_t copy_size, uint32_t src_index,
                             uint32_t dst_index) {
     // Add the provided values.
     DisallowHeapAllocation no_gc;
     FixedArrayBase* raw_backing_store = *dst_store;
     WriteBarrierMode mode = raw_backing_store->GetWriteBarrierMode(no_gc);
     for (uint32_t i = 0; i < copy_size; i++) {
-      Object* argument = (*args)[i + src_index];
-      Subclass::SetImpl(raw_backing_store, i + dst_index, argument, mode);
+      Object* argument = (*args)[src_index + i];
+      DCHECK(!argument->IsTheHole());
+      Subclass::SetImpl(raw_backing_store, dst_index + i, argument, mode);
     }
   }
 };
 
 template <typename Subclass, typename KindTraits>
 class FastSmiOrObjectElementsAccessor
     : public FastElementsAccessor<Subclass, KindTraits> {
  public:
   explicit FastSmiOrObjectElementsAccessor(const char* name)
       : FastElementsAccessor<Subclass, KindTraits>(name) {}
@@ skipping 1047 matching lines @@
   Handle<FixedArrayBase> elms;
   if (IsFastDoubleElementsKind(elements_kind)) {
     elms = Handle<FixedArrayBase>::cast(
         factory->NewFixedDoubleArray(number_of_elements));
   } else {
     elms = Handle<FixedArrayBase>::cast(
         factory->NewFixedArrayWithHoles(number_of_elements));
   }
 
   // Fill in the content
-  switch (array->GetElementsKind()) {
+  switch (elements_kind) {
     case FAST_HOLEY_SMI_ELEMENTS:
     case FAST_SMI_ELEMENTS: {
       Handle<FixedArray> smi_elms = Handle<FixedArray>::cast(elms);
       for (int entry = 0; entry < number_of_elements; entry++) {
         smi_elms->set(entry, (*args)[entry], SKIP_WRITE_BARRIER);
       }
       break;
     }
     case FAST_HOLEY_ELEMENTS:
     case FAST_ELEMENTS: {
@@ skipping 109 matching lines @@
     insertion_index += len;
   }
 
   DCHECK_EQ(insertion_index, result_len);
   return result_array;
 }
 
 ElementsAccessor** ElementsAccessor::elements_accessors_ = NULL;
 }  // namespace internal
 }  // namespace v8