making heap verification more aggressive

src/objects-debug.cc

Index: src/objects-debug.cc
diff --git a/src/objects-debug.cc b/src/objects-debug.cc
index 13342ca36a405b187a862c54dca11a972d085f9e..615c8b65e05546dec063a8c56e4c2d9066684a21 100644
--- a/src/objects-debug.cc
+++ b/src/objects-debug.cc
@@ -100,13 +100,15 @@ void HeapObject::HeapObjectVerify() {
       break;
     case JS_OBJECT_TYPE:
     case JS_ERROR_TYPE:
-    case JS_ARGUMENTS_TYPE:
     case JS_API_OBJECT_TYPE:
     case JS_SPECIAL_API_OBJECT_TYPE:
     case JS_CONTEXT_EXTENSION_OBJECT_TYPE:
     case JS_PROMISE_TYPE:
       JSObject::cast(this)->JSObjectVerify();
       break;
+    case JS_ARGUMENTS_TYPE:
+      JSArgumentsObject::cast(this)->JSArgumentsObjectVerify();
+      break;
     case JS_GENERATOR_OBJECT_TYPE:
       JSGeneratorObject::cast(this)->JSGeneratorObjectVerify();
       break;
@@ -246,9 +248,9 @@ void FreeSpace::FreeSpaceVerify() {
 
 template <class Traits>
 void FixedTypedArray<Traits>::FixedTypedArrayVerify() {
-  CHECK(IsHeapObject() &&
-        HeapObject::cast(this)->map()->instance_type() ==
-            Traits::kInstanceType);
+  CHECK(IsHeapObject());
+  CHECK(HeapObject::cast(this)->map()->instance_type() ==
+        Traits::kInstanceType);
   if (base_pointer() == this) {
     CHECK(external_pointer() ==
           ExternalReference::fixed_typed_array_base_data_offset().address());
@@ -265,76 +267,145 @@ bool JSObject::ElementsAreSafeToExamine() {
       GetHeap()->one_pointer_filler_map();
 }
 
-
-void JSObject::JSObjectVerify() {
-  VerifyHeapPointer(properties());
-  VerifyHeapPointer(elements());
-
-  if (HasSloppyArgumentsElements()) {
-    CHECK(this->elements()->IsFixedArray());
-    CHECK_GE(this->elements()->length(), 2);
+namespace {

[Igor Sheludko, 2016/07/08 08:38:22]

Add a blank line after this line.

[Camillo Bruni, 2016/07/11 11:46:38]

done

+void VerifyFastProperties(JSReceiver* receiver) {
+  // TODO(cbruni): JSProxy support for slow properties
+  if (!receiver->IsJSObject()) return;
+  Isolate* isolate = receiver->GetIsolate();
+  Map* map = receiver->map();
+  JSObject* obj = JSObject::cast(receiver);
+  int actual_unused_property_fields = map->GetInObjectProperties() +
+                                      obj->properties()->length() -
+                                      map->NextFreePropertyIndex();
+  if (map->unused_property_fields() != actual_unused_property_fields) {
+    // This could actually happen in the middle of StoreTransitionStub
+    // when the new extended backing store is already set into the object and
+    // the allocation of the MutableHeapNumber triggers GC (in this case map
+    // is not updated yet).
+    CHECK_EQ(map->unused_property_fields(),
+             actual_unused_property_fields - JSObject::kFieldsAdded);
   }
-
-  if (HasFastProperties()) {
-    int actual_unused_property_fields = map()->GetInObjectProperties() +
-                                        properties()->length() -
-                                        map()->NextFreePropertyIndex();
-    if (map()->unused_property_fields() != actual_unused_property_fields) {
-      // This could actually happen in the middle of StoreTransitionStub
-      // when the new extended backing store is already set into the object and
-      // the allocation of the MutableHeapNumber triggers GC (in this case map
-      // is not updated yet).
-      CHECK_EQ(map()->unused_property_fields(),
-               actual_unused_property_fields - JSObject::kFieldsAdded);
+  DescriptorArray* descriptors = map->instance_descriptors();
+  for (int i = 0; i < map->NumberOfOwnDescriptors(); i++) {
+    if (descriptors->GetDetails(i).type() != DATA) continue;
+    Representation r = descriptors->GetDetails(i).representation();
+    FieldIndex index = FieldIndex::ForDescriptor(map, i);
+    if (obj->IsUnboxedDoubleField(index)) {
+      DCHECK(r.IsDouble());
+      continue;
     }
-    DescriptorArray* descriptors = map()->instance_descriptors();
-    Isolate* isolate = GetIsolate();
-    for (int i = 0; i < map()->NumberOfOwnDescriptors(); i++) {
-      if (descriptors->GetDetails(i).type() == DATA) {
-        Representation r = descriptors->GetDetails(i).representation();
-        FieldIndex index = FieldIndex::ForDescriptor(map(), i);
-        if (IsUnboxedDoubleField(index)) {
-          DCHECK(r.IsDouble());
-          continue;
-        }
-        Object* value = RawFastPropertyAt(index);
-        if (r.IsDouble()) DCHECK(value->IsMutableHeapNumber());
-        if (value->IsUninitialized(isolate)) continue;
-        if (r.IsSmi()) DCHECK(value->IsSmi());
-        if (r.IsHeapObject()) DCHECK(value->IsHeapObject());
-        FieldType* field_type = descriptors->GetFieldType(i);
-        bool type_is_none = field_type->IsNone();
-        bool type_is_any = field_type->IsAny();
-        if (r.IsNone()) {
-          CHECK(type_is_none);
-        } else if (!type_is_any && !(type_is_none && r.IsHeapObject())) {
-          // If allocation folding is off then GC could happen during inner
-          // object literal creation and we will end up having and undefined
-          // value that does not match the field type.
-          CHECK(!field_type->NowStable() || field_type->NowContains(value) ||
-                (!FLAG_use_allocation_folding && value->IsUndefined(isolate)));
-        }
-      }
+    Object* value = obj->RawFastPropertyAt(index);
+    if (r.IsDouble()) DCHECK(value->IsMutableHeapNumber());
+    if (value->IsUninitialized(isolate)) continue;
+    if (r.IsSmi()) DCHECK(value->IsSmi());
+    if (r.IsHeapObject()) DCHECK(value->IsHeapObject());
+    FieldType* field_type = descriptors->GetFieldType(i);
+    bool type_is_none = field_type->IsNone();
+    bool type_is_any = field_type->IsAny();
+    if (r.IsNone()) {
+      CHECK(type_is_none);
+    } else if (!type_is_any && !(type_is_none && r.IsHeapObject())) {
+      // If allocation folding is off then GC could happen during inner
+      // object literal creation and we will end up having and undefined
+      // value that does not match the field type.
+      CHECK(!field_type->NowStable() || field_type->NowContains(value) ||
+            (!FLAG_use_allocation_folding && value->IsUndefined(isolate)));
     }
   }
+}
 
+void VerifySlowProperties(JSReceiver* obj) {
+  CHECK(obj->properties()->IsDictionary());
+}
+
+void VerifyProperties(JSReceiver* obj) {
+  obj->VerifyHeapPointer(obj->properties());
+  if (obj->HasFastProperties()) {
+    VerifyFastProperties(obj);
+  } else {

[Igor Sheludko, 2016/07/08 08:38:22]

Instead of VerifySlowProperties:

} else if (obj->IsJSGlobalObject()) {
  VerifyDictionary(JSObject::cast(obj)->global_dictionary());
} else {
  VerifyDictionary(obj->property_dictionary());
}

[Camillo Bruni, 2016/07/11 11:46:39]

Kept the separate method to do further checks but getting the dictionary the
suggested way.

+    VerifySlowProperties(obj);
+  }
+}
+
+template <typename T>
+void VerifyDictionary(T* dict) {
+  CHECK(dict->IsFixedArray());
+  CHECK(dict->IsDictionary());
+  int capacity = dict->Capacity();
+  int nof = dict->NumberOfElements();
+  int nod = dict->NumberOfDeletedElements();
+  CHECK_LE(capacity, dict->length());
+  CHECK_LE(nof, capacity);
+  CHECK_LE(0, nof + nod);
+  CHECK_LE(nof + nod, capacity);
+}
+
+void VerifyElements(JSObject* obj) {
+  obj->VerifyHeapPointer(obj->elements());
   // If a GC was caused while constructing this object, the elements
   // pointer may point to a one pointer filler map.
-  if (ElementsAreSafeToExamine()) {
-    CHECK_EQ((map()->has_fast_smi_or_object_elements() ||
-              (elements() == GetHeap()->empty_fixed_array()) ||
-              HasFastStringWrapperElements()),
-             (elements()->map() == GetHeap()->fixed_array_map() ||
-              elements()->map() == GetHeap()->fixed_cow_array_map()));
-    CHECK(map()->has_fast_object_elements() == HasFastObjectElements());
+  if (!obj->ElementsAreSafeToExamine()) return;
+  Map* map = obj->map();
+  Heap* heap = obj->GetHeap();
+  FixedArrayBase* elements = obj->elements();
+  CHECK_EQ((map->has_fast_smi_or_object_elements() ||
+            (elements == heap->empty_fixed_array()) ||
+            obj->HasFastStringWrapperElements()),
+           (elements->map() == heap->fixed_array_map() ||
+            elements->map() == heap->fixed_cow_array_map()));
+  CHECK(map->has_fast_object_elements() == obj->HasFastObjectElements());

[Igor Sheludko, 2016/07/08 08:38:22]

CHECK_EQ?

[Camillo Bruni, 2016/07/11 11:46:39]

copy pasta

+
+  // If a GC was caused while constructing this object, the elements

[Igor Sheludko, 2016/07/08 08:38:22]

I think this comment belongs only to ElementAreSafeToExamine().

[Camillo Bruni, 2016/07/11 11:46:39]

right.

+  // pointer may point to a one pointer filler map.
+  ElementsKind kind = obj->GetElementsKind();
+  if (IsFastSmiOrObjectElementsKind(kind)) {
+    CHECK(elements->map() == heap->fixed_array_map() ||
+          elements->map() == heap->fixed_cow_array_map());
+  } else if (IsFastDoubleElementsKind(kind)) {
+    CHECK(elements->IsFixedDoubleArray() ||
+          elements == heap->empty_fixed_array());
+  } else if (kind == DICTIONARY_ELEMENTS) {
+    CHECK(elements->IsSeededNumberDictionary());
+    VerifyDictionary(SeededNumberDictionary::cast(elements));

[Igor Sheludko, 2016/07/08 08:38:22]

I think this check also applies to SLOW_SLOPPY_ARGUMENTS_ELEMENTS and
SLOW_STRING_WRAPPER_ELEMENTS.

[Camillo Bruni, 2016/07/11 11:46:39]

eventually I will have to put a switch statement here over all elements kind.
Will do in a follow-up CL...

+    return;
+  } else {
+    CHECK(kind > DICTIONARY_ELEMENTS);
+  }
+  CHECK(!IsSloppyArgumentsElements(kind) ||
+        (elements->IsFixedArray() && elements->length() >= 2));
+
+  if (IsFastPackedElementsKind(kind)) {
+    uint32_t length = elements->length();
+    if (obj->IsJSArray()) {
+      Object* number = JSArray::cast(obj)->length();
+      if (number->IsSmi()) {
+        length = Min(length, static_cast<uint32_t>(Smi::cast(number)->value()));
+      }
+    }
+    if (kind == FAST_DOUBLE_ELEMENTS) {
+      for (uint32_t i = 0; i < length; i++) {
+        CHECK(!FixedDoubleArray::cast(elements)->is_the_hole(i));
+      }
+    } else {
+      for (uint32_t i = 0; i < length; i++) {
+        CHECK_NE(FixedArray::cast(elements)->get(i), heap->the_hole_value());
+      }
+    }
   }
 }
 
+}  // namespace
+
+void JSObject::JSObjectVerify() {
+  VerifyProperties(this);
+  VerifyElements(this);
+}
 
 void Map::MapVerify() {
   Heap* heap = GetHeap();
   CHECK(!heap->InNewSpace(this));
-  CHECK(FIRST_TYPE <= instance_type() && instance_type() <= LAST_TYPE);
+  CHECK(FIRST_TYPE <= instance_type());

[Igor Sheludko, 2016/07/08 08:38:22]

CHECK_LE?

[Camillo Bruni, 2016/07/11 11:46:39]

done.

+  CHECK(instance_type() <= LAST_TYPE);
   CHECK(instance_size() == kVariableSizeSentinel ||
          (kPointerSize <= instance_size() &&
           instance_size() < heap->Capacity()));
@@ -426,6 +497,37 @@ void JSGeneratorObject::JSGeneratorObjectVerify() {
   VerifyObjectField(kReceiverOffset);
   VerifyObjectField(kOperandStackOffset);
   VerifyObjectField(kContinuationOffset);
+  JSObjectVerify();
+}
+
+void JSArgumentsObject::JSArgumentsObjectVerify() {
+  VerifyObjectField(kLengthOffset);
+  JSObjectVerify();
+  Isolate* isolate = GetIsolate();
+
+  // TODO(cbruni): replace with NULL check once all hydrogren stubs manage to
+  // set up the contexts properly
+  if (isolate->context()->IsSmi()) return;
+  if (isolate->bootstrapper()->IsActive()) return;
+  if (map() == *isolate->sloppy_arguments_map()) {
+    // TODO(cbruni): normal array-like size verification
+  } else if (map() == *isolate->fast_aliased_arguments_map()) {
+    CHECK(HasFastArgumentsElements());
+    CHECK(this->elements()->IsFixedArray());
+    CHECK_GE(this->elements()->length(), 2);
+  } else if (map() == *isolate->slow_aliased_arguments_map()) {
+    CHECK(HasSlowArgumentsElements());
+    CHECK(this->elements()->IsFixedArray());
+    CHECK_GE(this->elements()->length(), 2);
+  } else if (map() == *isolate->strict_arguments_map()) {
+    Object* length = *Object::GetProperty(handle(this, isolate),

[Igor Sheludko, 2016/07/08 08:38:22]

This is scary. I guess heap verification should not cause any side effects at
all. Can you ensure somehow that you get the length without side effects?

In any case, if this call cause any allocations then when you return back,
"this" could not be a valid pointer anymore therefore all accesses to "this"
below should be done via handle.

[Camillo Bruni, 2016/07/11 11:46:39]

right, also I just noticed that this can be easily overwritten from js side...

+                                          isolate->factory()->length_string())
+                          .ToHandleChecked();
+    CHECK_EQ(Smi::cast(length)->value(), elements()->length());
+    CHECK(HasFastElements());
+  } else {
+    PrintF(".");

[Igor Sheludko, 2016/07/08 08:38:22]

Surprise! :)

[Camillo Bruni, 2016/07/11 11:46:39]

... as good as unreachable.

+  }
 }
 
 
@@ -434,6 +536,7 @@ void JSValue::JSValueVerify() {
   if (v->IsHeapObject()) {
     VerifyHeapPointer(v);
   }
+  JSObjectVerify();
 }
 
 
@@ -545,6 +648,7 @@ void JSBoundFunction::JSBoundFunctionVerify() {
 
 void JSFunction::JSFunctionVerify() {
   CHECK(IsJSFunction());
+  JSObjectVerify();
   VerifyObjectField(kPrototypeOrInitialMapOffset);
   VerifyObjectField(kNextFunctionLinkOffset);
   CHECK(code()->IsCode());
@@ -591,6 +695,7 @@ void JSGlobalObject::JSGlobalObjectVerify() {
       elements()->length() == 0) {
     return;
   }
+  CHECK(!HasFastProperties());
   JSObjectVerify();
 }
 
@@ -694,7 +799,7 @@ void Code::VerifyEmbeddedObjectsDependency() {
       } else if (obj->IsJSObject()) {
         if (isolate->heap()->InNewSpace(obj)) {
           ArrayList* list =
-              GetIsolate()->heap()->weak_new_space_object_to_code_list();
+              isolate->heap()->weak_new_space_object_to_code_list();
           bool found = false;
           for (int i = 0; i < list->Length(); i += 2) {
             WeakCell* obj_cell = WeakCell::cast(list->Get(i));
@@ -708,7 +813,7 @@ void Code::VerifyEmbeddedObjectsDependency() {
         } else {
           Handle<HeapObject> key_obj(HeapObject::cast(obj), isolate);
           DependentCode* dep =
-              GetIsolate()->heap()->LookupWeakObjectToCodeDependency(key_obj);
+              isolate->heap()->LookupWeakObjectToCodeDependency(key_obj);
           dep->Contains(DependentCode::kWeakCodeGroup, cell);
         }
       }
@@ -718,18 +823,23 @@ void Code::VerifyEmbeddedObjectsDependency() {
 
 
 void JSArray::JSArrayVerify() {
+  CHECK(IsJSArray());
   JSObjectVerify();
   Isolate* isolate = GetIsolate();
-  CHECK(length()->IsNumber() || length()->IsUndefined(isolate));
+  // Allow undefined for not fully initialized JSArrays
+  if (length()->IsUndefined(isolate)) {
+    CHECK_EQ(FixedArray::cast(elements()),
+             isolate->heap()->empty_fixed_array());
+    return;
+  }
+  CHECK(length()->IsNumber());
   // If a GC was caused while constructing this array, the elements
   // pointer may point to a one pointer filler map.
   if (ElementsAreSafeToExamine()) {
-    CHECK(elements()->IsUndefined(isolate) || elements()->IsFixedArray() ||
-          elements()->IsFixedDoubleArray());
+    CHECK(elements()->IsFixedArray() || elements()->IsFixedDoubleArray());
   }
 }
 
-
 void JSSet::JSSetVerify() {
   CHECK(IsJSSet());
   JSObjectVerify();
@@ -1296,7 +1406,8 @@ void Code::VerifyRecompiledCode(Code* old_code, Code* new_code) {
   while (!old_it.done()) {
     RelocInfo* rinfo = old_it.rinfo();
     Code* target = Code::GetCodeFromTargetAddress(rinfo->target_address());
-    CHECK(!target->is_handler() && !target->is_inline_cache_stub());
+    CHECK(!target->is_handler());
+    CHECK(!target->is_inline_cache_stub());
     if (target == stack_check) break;
     old_it.next();
   }
@@ -1304,7 +1415,8 @@ void Code::VerifyRecompiledCode(Code* old_code, Code* new_code) {
   while (!new_it.done()) {
     RelocInfo* rinfo = new_it.rinfo();
     Code* target = Code::GetCodeFromTargetAddress(rinfo->target_address());
-    CHECK(!target->is_handler() && !target->is_inline_cache_stub());
+    CHECK(!target->is_handler());
+    CHECK(!target->is_inline_cache_stub());
     if (target == stack_check) break;
     new_it.next();
   }