making heap verification more aggressive

src/objects-debug.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/objects.h"
 
 #include "src/bootstrapper.h"
 #include "src/disasm.h"
 #include "src/disassembler.h"
 #include "src/field-type.h"
@@ skipping 82 matching lines @@
 #undef VERIFY_TYPED_ARRAY
 
     case CODE_TYPE:
       Code::cast(this)->CodeVerify();
       break;
     case ODDBALL_TYPE:
       Oddball::cast(this)->OddballVerify();
       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;
     case JS_VALUE_TYPE:
       JSValue::cast(this)->JSValueVerify();
       break;
     case JS_DATE_TYPE:
       JSDate::cast(this)->JSDateVerify();
       break;
     case JS_BOUND_FUNCTION_TYPE:
@@ skipping 119 matching lines @@
 }
 
 
 void FreeSpace::FreeSpaceVerify() {
   CHECK(IsFreeSpace());
 }
 
 
 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());
   } else {
     CHECK(base_pointer() == nullptr);
   }
 }
 
 
 bool JSObject::ElementsAreSafeToExamine() {
   // If a GC was caused while constructing this object, the elements
   // pointer may point to a one pointer filler map.
   return reinterpret_cast<Map*>(elements()) !=
       GetHeap()->one_pointer_filler_map();
 }
 
+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);
+  }
+  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;
+    }
+    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 JSObject::JSObjectVerify() {
-  VerifyHeapPointer(properties());
-  VerifyHeapPointer(elements());
+void VerifySlowProperties(JSReceiver* obj) {
+  CHECK(obj->properties()->IsDictionary());
+}
 
-  if (HasSloppyArgumentsElements()) {
-    CHECK(this->elements()->IsFixedArray());
-    CHECK_GE(this->elements()->length(), 2);
+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);
   }
+}
 
-  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);
+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 (!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()));
+      }
     }
-    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)));
-        }
+    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());
       }
     }
   }
-
-  // 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());
-  }
 }
 
+}  // 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()));
   CHECK(GetBackPointer()->IsUndefined(heap->isolate()) ||
         !Map::cast(GetBackPointer())->is_stable());
   VerifyHeapPointer(prototype());
   VerifyHeapPointer(instance_descriptors());
   SLOW_DCHECK(instance_descriptors()->IsSortedNoDuplicates());
   SLOW_DCHECK(TransitionArray::IsSortedNoDuplicates(this));
   SLOW_DCHECK(TransitionArray::IsConsistentWithBackPointers(this));
@@ skipping 71 matching lines @@
 
 void JSGeneratorObject::JSGeneratorObjectVerify() {
   // In an expression like "new g()", there can be a point where a generator
   // object is allocated but its fields are all undefined, as it hasn't yet been
   // initialized by the generator.  Hence these weak checks.
   VerifyObjectField(kFunctionOffset);
   VerifyObjectField(kContextOffset);
   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.

+  }
 }
 
 
 void JSValue::JSValueVerify() {
   Object* v = value();
   if (v->IsHeapObject()) {
     VerifyHeapPointer(v);
   }
+  JSObjectVerify();
 }
 
 
 void JSDate::JSDateVerify() {

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

JSDate and all other JSObject builtins should also be JSObjectVerify()-ed.

   if (value()->IsHeapObject()) {
     VerifyHeapPointer(value());
   }
   Isolate* isolate = GetIsolate();
   CHECK(value()->IsUndefined(isolate) || value()->IsSmi() ||
         value()->IsHeapNumber());
   CHECK(year()->IsUndefined(isolate) || year()->IsSmi() || year()->IsNaN());
   CHECK(month()->IsUndefined(isolate) || month()->IsSmi() || month()->IsNaN());
   CHECK(day()->IsUndefined(isolate) || day()->IsSmi() || day()->IsNaN());
   CHECK(weekday()->IsUndefined(isolate) || weekday()->IsSmi() ||
@@ skipping 87 matching lines @@
   VerifyObjectField(kBoundTargetFunctionOffset);
   VerifyObjectField(kBoundArgumentsOffset);
   CHECK(bound_target_function()->IsCallable());
   CHECK(IsCallable());
   CHECK_EQ(IsConstructor(), bound_target_function()->IsConstructor());
 }
 
 
 void JSFunction::JSFunctionVerify() {
   CHECK(IsJSFunction());
+  JSObjectVerify();
   VerifyObjectField(kPrototypeOrInitialMapOffset);
   VerifyObjectField(kNextFunctionLinkOffset);
   CHECK(code()->IsCode());
   CHECK(next_function_link() == NULL ||
         next_function_link()->IsUndefined(GetIsolate()) ||
         next_function_link()->IsJSFunction());
   CHECK(map()->is_callable());
 }
 
 
@@ skipping 26 matching lines @@
 }
 
 
 void JSGlobalObject::JSGlobalObjectVerify() {
   CHECK(IsJSGlobalObject());
   // Do not check the dummy global object for the builtins.
   if (GlobalDictionary::cast(properties())->NumberOfElements() == 0 &&
       elements()->length() == 0) {
     return;
   }
+  CHECK(!HasFastProperties());
   JSObjectVerify();
 }
 
 
 void Oddball::OddballVerify() {
   CHECK(IsOddball());
   Heap* heap = GetHeap();
   VerifyHeapPointer(to_string());
   Object* number = to_number();
   if (number->IsHeapObject()) {
@@ skipping 83 matching lines @@
   for (RelocIterator it(this, mode_mask); !it.done(); it.next()) {
     Object* obj = it.rinfo()->target_object();
     if (IsWeakObject(obj)) {
       if (obj->IsMap()) {
         Map* map = Map::cast(obj);
         CHECK(map->dependent_code()->Contains(DependentCode::kWeakCodeGroup,
                                               cell));
       } 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));
             if (!obj_cell->cleared() && obj_cell->value() == obj &&
                 WeakCell::cast(list->Get(i + 1)) == cell) {
               found = true;
               break;
             }
           }
           CHECK(found);
         } 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);
         }
       }
     }
   }
 }
 
 
 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();
   VerifyHeapPointer(table());
   CHECK(table()->IsOrderedHashTable() || table()->IsUndefined(GetIsolate()));
   // TODO(arv): Verify OrderedHashTable too.
 }
 
 
 void JSMap::JSMapVerify() {
@@ skipping 546 matching lines @@
   if (isolate->bootstrapper()->IsActive()) return;
 
   static const int mask = RelocInfo::kCodeTargetMask;
   RelocIterator old_it(old_code, mask);
   RelocIterator new_it(new_code, mask);
   Code* stack_check = isolate->builtins()->builtin(Builtins::kStackCheck);
 
   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();
   }
 
   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();
   }
 
   // Either both are done because there is no stack check.
   // Or we are past the prologue for both.
   CHECK_EQ(new_it.done(), old_it.done());
 
   // After the prologue, each call in the old code has a corresponding call
   // in the new code.
@@ skipping 15 matching lines @@
 
   // Both are done at the same time.
   CHECK_EQ(new_it.done(), old_it.done());
 }
 
 
 #endif  // DEBUG
 
 }  // namespace internal
 }  // namespace v8