[elements.cc] Introduce InternalIndex class

instead of plain uint32_t as entry. This provides some type safety,
because the compiler will check that we are not mixing up indexes
and entries. It also paves the way to consistently using size_t for
TypedArray indexes.

Bug: v8:4153
Change-Id: Ie0eb63693c871efda9860d3d288896819868b66a
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1852765Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Commit-Queue: Jakob Kummerow <jkummerow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#64236}

BUILD.gn

@@ -2567,6 +2567,7 @@ v8_source_set("v8_base_without_compiler") {
     "src/objects/heap-object.h",
     "src/objects/instance-type-inl.h",
     "src/objects/instance-type.h",
+    "src/objects/internal-index.h",
     "src/objects/intl-objects.cc",
     "src/objects/intl-objects.h",
     "src/objects/js-array-buffer-inl.h",

src/objects/elements.h

@@ -6,6 +6,7 @@
 #define V8_OBJECTS_ELEMENTS_H_
 
 #include "src/objects/elements-kind.h"
+#include "src/objects/internal-index.h"
 #include "src/objects/keys.h"
 #include "src/objects/objects.h"
 
@@ -50,11 +51,9 @@ class ElementsAccessor {
 
   // Note: this is currently not implemented for string wrapper and
   // typed array elements.
-  virtual bool HasEntry(JSObject holder, uint32_t entry) = 0;
+  virtual bool HasEntry(JSObject holder, InternalIndex entry) = 0;
 
-  // TODO(cbruni): HasEntry and Get should not be exposed publicly with the
-  // entry parameter.
-  virtual Handle<Object> Get(Handle<JSObject> holder, uint32_t entry) = 0;
+  virtual Handle<Object> Get(Handle<JSObject> holder, InternalIndex entry) = 0;
 
   virtual bool HasAccessors(JSObject holder) = 0;
   virtual uint32_t NumberOfElements(JSObject holder) = 0;
@@ -105,7 +104,8 @@ class ElementsAccessor {
   static void InitializeOncePerProcess();
   static void TearDown();
 
-  virtual void Set(Handle<JSObject> holder, uint32_t entry, Object value) = 0;
+  virtual void Set(Handle<JSObject> holder, InternalIndex entry,
+                   Object value) = 0;
 
   virtual void Add(Handle<JSObject> object, uint32_t index,
                    Handle<Object> value, PropertyAttributes attributes,
@@ -178,18 +178,18 @@ class ElementsAccessor {
   // indices are equivalent to entries. In the NumberDictionary
   // ElementsAccessor, entries are mapped to an index using the KeyAt method on
   // the NumberDictionary.
-  virtual uint32_t GetEntryForIndex(Isolate* isolate, JSObject holder,
-                                    FixedArrayBase backing_store,
-                                    uint32_t index) = 0;
+  virtual InternalIndex GetEntryForIndex(Isolate* isolate, JSObject holder,
+                                         FixedArrayBase backing_store,
+                                         uint32_t index) = 0;
 
-  virtual PropertyDetails GetDetails(JSObject holder, uint32_t entry) = 0;
+  virtual PropertyDetails GetDetails(JSObject holder, InternalIndex entry) = 0;
   virtual void Reconfigure(Handle<JSObject> object,
-                           Handle<FixedArrayBase> backing_store, uint32_t entry,
-                           Handle<Object> value,
+                           Handle<FixedArrayBase> backing_store,
+                           InternalIndex entry, Handle<Object> value,
                            PropertyAttributes attributes) = 0;
 
   // Deletes an element in an object.
-  virtual void Delete(Handle<JSObject> holder, uint32_t entry) = 0;
+  virtual void Delete(Handle<JSObject> holder, InternalIndex entry) = 0;
 
   // NOTE: this method violates the handlified function signature convention:
   // raw pointer parameter |source_holder| in the function that allocates.

src/objects/internal-index.h

+// Copyright 2019 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.
+
+#ifndef V8_OBJECTS_INTERNAL_INDEX_H_
+#define V8_OBJECTS_INTERNAL_INDEX_H_
+
+#include <stdint.h>
+
+#include <limits>
+
+#include "src/base/logging.h"
+
+namespace v8 {
+namespace internal {
+
+// Simple wrapper around an entry (which is notably different from "index" for
+// dictionary backing stores). Most code should treat this as an opaque
+// wrapper: get it via GetEntryForIndex, pass it on to consumers.
+class InternalIndex {
+ public:
+  explicit InternalIndex(size_t raw) : entry_(raw) {}
+  static InternalIndex NotFound() { return InternalIndex(kNotFound); }
+
+  InternalIndex adjust_down(size_t subtract) {
+    DCHECK_GE(entry_, subtract);
+    return InternalIndex(entry_ - subtract);
+  }
+  InternalIndex adjust_up(size_t add) {
+    DCHECK_LT(entry_, std::numeric_limits<size_t>::max() - add);
+    return InternalIndex(entry_ + add);
+  }
+
+  bool is_found() const { return entry_ != kNotFound; }
+  bool is_not_found() const { return entry_ == kNotFound; }
+
+  size_t raw_value() const { return entry_; }
+  uint32_t as_uint32() const {
+    DCHECK_LE(entry_, std::numeric_limits<uint32_t>::max());
+    return static_cast<uint32_t>(entry_);
+  }
+  int as_int() const {
+    DCHECK(entry_ >= 0 && entry_ <= std::numeric_limits<int>::max());
+    return static_cast<int>(entry_);
+  }
+
+ private:
+  static const size_t kNotFound = std::numeric_limits<size_t>::max();
+
+  size_t entry_;
+};
+
+}  // namespace internal
+}  // namespace v8
+
+#endif  // V8_OBJECTS_INTERNAL_INDEX_H_

src/objects/js-list-format.cc

@@ -252,23 +252,22 @@ namespace {
 // Extract String from JSArray into array of UnicodeString
 Maybe<std::vector<icu::UnicodeString>> ToUnicodeStringArray(
     Isolate* isolate, Handle<JSArray> array) {
-  // In general, ElementsAccessor::Get actually isn't guaranteed to give us the
-  // elements in order. But if it is a holey array, it will cause the exception
-  // with the IsString check.
+  // Thanks to iterable-to-list preprocessing, we never see dictionary-mode
+  // arrays here, so the loop below can construct an entry from the index.
+  DCHECK(array->HasFastElements(isolate));
   auto* accessor = array->GetElementsAccessor();
   uint32_t length = accessor->NumberOfElements(*array);
 
   std::vector<icu::UnicodeString> result;
   for (uint32_t i = 0; i < length; i++) {
-    DCHECK(accessor->HasElement(*array, i));
-    Handle<Object> item = accessor->Get(array, i);
-    DCHECK(!item.is_null());
+    InternalIndex entry(i);
+    DCHECK(accessor->HasEntry(*array, entry));
+    Handle<Object> item = accessor->Get(array, entry);
     DCHECK(item->IsString());
     Handle<String> item_str = Handle<String>::cast(item);
     if (!item_str->IsFlat()) item_str = String::Flatten(isolate, item_str);
     result.push_back(Intl::ToICUUnicodeString(isolate, item_str));
   }
-  DCHECK(!array->HasDictionaryElements());
   return Just(result);
 }
 

src/objects/keys.cc

@@ -518,13 +518,14 @@ V8_WARN_UNUSED_RESULT ExceptionStatus FilterForEnumerableProperties(
 
   uint32_t length = accessor->GetCapacity(*result, result->elements());
   for (uint32_t i = 0; i < length; i++) {
-    if (!accessor->HasEntry(*result, i)) continue;
+    InternalIndex entry(i);
+    if (!accessor->HasEntry(*result, entry)) continue;
 
     // args are invalid after args.Call(), create a new one in every iteration.
     PropertyCallbackArguments args(accumulator->isolate(), interceptor->data(),
                                    *receiver, *object, Just(kDontThrow));
 
-    Handle<Object> element = accessor->Get(result, i);
+    Handle<Object> element = accessor->Get(result, entry);
     Handle<Object> attributes;
     if (type == kIndexed) {
       uint32_t number;

src/objects/lookup.cc

@@ -14,6 +14,7 @@
 #include "src/objects/field-type.h"
 #include "src/objects/hash-table-inl.h"
 #include "src/objects/heap-number-inl.h"
+#include "src/objects/internal-index.h"
 #include "src/objects/struct-inl.h"
 
 namespace v8 {
@@ -534,7 +535,7 @@ void LookupIterator::ReconfigureDataProperty(Handle<Object> value,
     DCHECK(attributes != NONE || !holder_obj->HasFastElements(isolate_));
     Handle<FixedArrayBase> elements(holder_obj->elements(isolate_), isolate());
     holder_obj->GetElementsAccessor(isolate_)->Reconfigure(
-        holder_obj, elements, number_, value, attributes);
+        holder_obj, elements, InternalIndex(number_), value, attributes);
     ReloadPropertyInformation<true>();
   } else if (holder_obj->HasFastProperties(isolate_)) {
     Handle<Map> old_map(holder_obj->map(isolate_), isolate_);
@@ -731,7 +732,7 @@ void LookupIterator::Delete() {
   if (IsElement()) {
     Handle<JSObject> object = Handle<JSObject>::cast(holder);
     ElementsAccessor* accessor = object->GetElementsAccessor(isolate_);
-    accessor->Delete(object, number_);
+    accessor->Delete(object, InternalIndex(number_));
   } else {
     DCHECK(!name()->IsPrivateName(isolate_));
     bool is_prototype_map = holder->map(isolate_).is_prototype_map();
@@ -894,7 +895,7 @@ Handle<Object> LookupIterator::FetchValue() const {
   if (IsElement()) {
     Handle<JSObject> holder = GetHolder<JSObject>();
     ElementsAccessor* accessor = holder->GetElementsAccessor(isolate_);
-    return accessor->Get(holder, number_);
+    return accessor->Get(holder, InternalIndex(number_));
   } else if (holder_->IsJSGlobalObject(isolate_)) {
     Handle<JSGlobalObject> holder = GetHolder<JSGlobalObject>();
     result = holder->global_dictionary(isolate_).ValueAt(isolate_, number_);
@@ -1028,7 +1029,7 @@ void LookupIterator::WriteDataValue(Handle<Object> value,
   if (IsElement()) {
     Handle<JSObject> object = Handle<JSObject>::cast(holder);
     ElementsAccessor* accessor = object->GetElementsAccessor(isolate_);
-    accessor->Set(object, number_, *value);
+    accessor->Set(object, InternalIndex(number_), *value);
   } else if (holder->HasFastProperties(isolate_)) {
     if (property_details_.location() == kField) {
       // Check that in case of VariableMode::kConst field the existing value is
@@ -1164,13 +1165,15 @@ LookupIterator::State LookupIterator::LookupInRegularHolder(
     JSObject js_object = JSObject::cast(holder);
     ElementsAccessor* accessor = js_object.GetElementsAccessor(isolate_);
     FixedArrayBase backing_store = js_object.elements(isolate_);
-    number_ =
+    // TODO(jkummerow): {number_} should have type InternalIndex.
+    InternalIndex entry =
         accessor->GetEntryForIndex(isolate_, js_object, backing_store, index_);
+    number_ = entry.is_found() ? entry.as_uint32() : kMaxUInt32;
     if (number_ == kMaxUInt32) {
       return holder.IsJSTypedArray(isolate_) ? INTEGER_INDEXED_EXOTIC
                                              : NOT_FOUND;
     }
-    property_details_ = accessor->GetDetails(js_object, number_);
+    property_details_ = accessor->GetDetails(js_object, InternalIndex(number_));
     if (map.has_frozen_elements()) {
       property_details_ = property_details_.CopyAddAttributes(FROZEN);
     } else if (map.has_sealed_elements()) {