Revert "Fixes for size_t LookupIterator"

This reverts commit e1ad9b89.

Reason for revert: Landed test that fails with GC stress

Original change's description:
> Fixes for size_t LookupIterator
> 
> Fixing some fallout from c968607e
> aka r65078
> 
> Bug: chromium:1026729,chromium:1026856,chromium:1026909,chromium:1026974
> Change-Id: I98a4466595fbf1635af403ab58842977882c0453
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1930907
> Commit-Queue: Jakob Kummerow <jkummerow@chromium.org>
> Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
> Reviewed-by: Toon Verwaest <verwaest@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#65158}

TBR=jkummerow@chromium.org,mstarzinger@chromium.org,verwaest@chromium.org

Change-Id: I1f1e95d37b033e53330adae08071cbe34e1d9488
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: chromium:1026729, chromium:1026856, chromium:1026909, chromium:1026974
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1933599Reviewed-by: Shu-yu Guo <syg@chromium.org>
Commit-Queue: Shu-yu Guo <syg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#65159}

src/codegen/code-stub-assembler.cc

@@ -9766,13 +9766,6 @@ TNode<IntPtrT> CodeStubAssembler::TryToIntptr(
     TNode<IntPtrT> int_value = ChangeFloat64ToIntPtr(value);
     GotoIfNot(Float64Equal(value, RoundIntPtrToFloat64(int_value)),
               if_not_intptr);
-    if (Is64()) {
-      // TODO(jkummerow): Investigate whether we can drop support for
-      // negative indices.
-      GotoIfNot(IsInRange(int_value, static_cast<intptr_t>(-kMaxSafeInteger),
-                          static_cast<intptr_t>(kMaxSafeIntegerUint64)),
-                if_not_intptr);
-    }
     var_intptr_key = int_value;
     Goto(&done);
   }

src/codegen/code-stub-assembler.h

@@ -941,13 +941,6 @@ class V8_EXPORT_PRIVATE CodeStubAssembler
                                  Int32Constant(higher_limit - lower_limit));
   }
 
-  TNode<BoolT> IsInRange(TNode<WordT> value, intptr_t lower_limit,
-                         intptr_t higher_limit) {
-    DCHECK_LE(lower_limit, higher_limit);
-    return UintPtrLessThanOrEqual(IntPtrSub(value, IntPtrConstant(lower_limit)),
-                                  IntPtrConstant(higher_limit - lower_limit));
-  }
-
 #if DEBUG
   void Bind(Label* label, AssemblerDebugInfo debug_info);
 #endif  // DEBUG

src/compiler/effect-control-linearizer.cc

@@ -214,8 +214,6 @@ class EffectControlLinearizer {
   Node* BuildCheckedFloat64ToInt64(CheckForMinusZeroMode mode,
                                    const FeedbackSource& feedback, Node* value,
                                    Node* frame_state);
-  Node* BuildCheckedFloat64ToIndex(const FeedbackSource& feedback, Node* value,
-                                   Node* frame_state);
   Node* BuildCheckedHeapNumberOrOddballToFloat64(CheckTaggedInputMode mode,
                                                  const FeedbackSource& feedback,
                                                  Node* value,
@@ -2431,31 +2429,6 @@ Node* EffectControlLinearizer::BuildCheckedFloat64ToInt32(
   return value32;
 }
 
-Node* EffectControlLinearizer::BuildCheckedFloat64ToIndex(
-    const FeedbackSource& feedback, Node* value, Node* frame_state) {
-  if (machine()->Is64()) {
-    Node* value64 = __ ChangeFloat64ToInt64(value);
-    Node* check_same = __ Float64Equal(value, __ ChangeInt64ToFloat64(value64));
-    __ DeoptimizeIfNot(DeoptimizeReason::kLostPrecisionOrNaN, feedback,
-                       check_same, frame_state);
-    Node* check_max =
-        __ IntLessThan(value64, __ Int64Constant(kMaxSafeInteger));
-    __ DeoptimizeIfNot(DeoptimizeReason::kNotAnArrayIndex, feedback, check_max,
-                       frame_state);
-    Node* check_min =
-        __ IntLessThan(__ Int64Constant(-kMaxSafeInteger), value64);
-    __ DeoptimizeIfNot(DeoptimizeReason::kNotAnArrayIndex, feedback, check_min,
-                       frame_state);
-    return value64;
-  } else {
-    Node* value32 = __ RoundFloat64ToInt32(value);
-    Node* check_same = __ Float64Equal(value, __ ChangeInt32ToFloat64(value32));
-    __ DeoptimizeIfNot(DeoptimizeReason::kLostPrecisionOrNaN, feedback,
-                       check_same, frame_state);
-    return value32;
-  }
-}
-
 Node* EffectControlLinearizer::LowerCheckedFloat64ToInt32(Node* node,
                                                           Node* frame_state) {
   const CheckMinusZeroParameters& params =
@@ -2527,7 +2500,7 @@ Node* EffectControlLinearizer::LowerCheckedTaggedToArrayIndex(
   __ Goto(&done, ChangeSmiToIntPtr(value));
 
   // In the non-Smi case, check the heap numberness, load the number and convert
-  // to integer.
+  // to int32.
   __ Bind(&if_not_smi);
   auto if_not_heap_number = __ MakeDeferredLabel();
   Node* value_map = __ LoadField(AccessBuilder::ForMap(), value);
@@ -2535,7 +2508,9 @@ Node* EffectControlLinearizer::LowerCheckedTaggedToArrayIndex(
   __ GotoIfNot(is_heap_number, &if_not_heap_number);
 
   Node* number = __ LoadField(AccessBuilder::ForHeapNumberValue(), value);
-  number = BuildCheckedFloat64ToIndex(params.feedback(), number, frame_state);
+  number =
+      BuildCheckedFloat64ToInt32(CheckForMinusZeroMode::kDontCheckForMinusZero,
+                                 params.feedback(), number, frame_state);
   __ Goto(&done, number);
 
   __ Bind(&if_not_heap_number);

src/ic/ic.cc

@@ -1203,8 +1203,8 @@ KeyType TryConvertKey(Handle<Object> key, Isolate* isolate, intptr_t* index_out,
   }
   if (key->IsHeapNumber()) {
     double num = HeapNumber::cast(*key).value();
-    if (!(num >= -kMaxSafeInteger)) return kBailout;
-    if (num > kMaxSafeInteger) return kBailout;
+    if (!(num >= std::numeric_limits<intptr_t>::min())) return kBailout;
+    if (num > std::numeric_limits<intptr_t>::max()) return kBailout;
     *index_out = static_cast<intptr_t>(num);
     if (*index_out != num) return kBailout;
     return kIntPtr;

src/json/json-parser.cc

@@ -362,7 +362,7 @@ JsonString JsonParser<Char>::ScanJsonPropertyKey(JsonContinuation* cont) {
         uint32_t index = first - '0';
         while (true) {
           cursor_ = std::find_if(cursor_ + 1, end_, [&index](Char c) {
-            return !TryAddArrayIndexChar(&index, c);
+            return !TryAddIndexChar(&index, c);
           });
 
           if (CurrentCharacter() == '"') {
@@ -374,7 +374,7 @@ JsonString JsonParser<Char>::ScanJsonPropertyKey(JsonContinuation* cont) {
           }
 
           if (CurrentCharacter() == '\\' && NextCharacter() == 'u') {
-            if (TryAddArrayIndexChar(&index, ScanUnicodeCharacter())) continue;
+            if (TryAddIndexChar(&index, ScanUnicodeCharacter())) continue;
           }
 
           break;

src/objects/lookup-inl.h

@@ -71,7 +71,6 @@ LookupIterator::LookupIterator(Isolate* isolate, Handle<Object> receiver,
       receiver_(receiver),
       initial_holder_(holder),
       index_(index) {
-  DCHECK_NE(index, kInvalidIndex);
   // If we're not looking at a TypedArray, we will need the key represented
   // as an internalized string.
   if (index_ > JSArray::kMaxArrayIndex && !receiver->IsJSTypedArray()) {
@@ -79,10 +78,6 @@ LookupIterator::LookupIterator(Isolate* isolate, Handle<Object> receiver,
       key_as_string = isolate->factory()->SizeToString(index_);
     }
     name_ = isolate->factory()->InternalizeName(key_as_string);
-  } else if (!key_as_string.is_null() &&
-             key_as_string->IsInternalizedString()) {
-    // Even for TypedArrays: if we have a name, keep it. ICs will need it.
-    name_ = key_as_string;
   }
   Start<true>();
 }

src/objects/name.h

@@ -95,13 +95,14 @@ class Name : public TorqueGeneratedName<Name, PrimitiveHeapObject> {
   // Maximum number of characters to consider when trying to convert a string
   // value into an array index.
   static const int kMaxArrayIndexSize = 10;
-  // Maximum number of characters in a string that can possibly be an
-  // "integer index" in the spec sense, i.e. a canonical representation of a
-  // number in the range up to MAX_SAFE_INTEGER. We parse these into a size_t,
-  // so the size of that type also factors in as a limit: 10 characters per
-  // 32 bits of size_t width.
-  static const int kMaxIntegerIndexSize =
-      std::min(16, int{10 * (sizeof(size_t) / 4)});
+  // Maximum number of characters that might be parsed into a size_t:
+  // 10 characters per 32 bits of size_t width.
+  // We choose this as large as possible (rather than MAX_SAFE_INTEGER range)
+  // because TypedArray accesses will treat all string keys that are
+  // canonical representations of numbers in the range [MAX_SAFE_INTEGER ..
+  // size_t::max] as out-of-bounds accesses, and we can handle those in the
+  // fast path if we tag them as such (see kIsNotIntegerIndexMask).
+  static const int kMaxIntegerIndexSize = 10 * (sizeof(size_t) / 4);
 
   // For strings which are array indexes the hash value has the string length
   // mixed into the hash, mainly to avoid a hash value of zero which would be

src/objects/objects-inl.h

@@ -823,11 +823,9 @@ bool Object::ToIntegerIndex(size_t* index) const {
   if (IsHeapNumber()) {
     double num = HeapNumber::cast(*this).value();
     if (!(num >= 0)) return false;  // Negation to catch NaNs.
-    constexpr double max =
-        std::min(kMaxSafeInteger,
-                 // The maximum size_t is reserved as "invalid" sentinel.
-                 static_cast<double>(std::numeric_limits<size_t>::max() - 1));
-    if (num > max) return false;
+    // We must exclude the max size_t, because the LookupIterator uses that
+    // as the "invalid index" sentinel.
+    if (num >= std::numeric_limits<size_t>::max()) return false;
     size_t result = static_cast<size_t>(num);
     if (num != result) return false;  // Conversion lost fractional precision.
     *index = result;

src/objects/string.cc

@@ -1397,8 +1397,7 @@ bool String::SlowAsIntegerIndex(size_t* index) {
   }
   if (length == 0 || length > kMaxIntegerIndexSize) return false;
   StringCharacterStream stream(*this);
-  return StringToIndex<StringCharacterStream, size_t, kToIntegerIndex>(&stream,
-                                                                       index);
+  return StringToIndex(&stream, index);
 }
 
 void String::PrintOn(FILE* file) {

src/runtime/runtime-object.cc

@@ -270,23 +270,21 @@ RUNTIME_FUNCTION(Runtime_ObjectHasOwnProperty) {
   HandleScope scope(isolate);
   Handle<Object> property = args.at(1);
 
-  // The spec says we must look at the key first, which is why we can't
-  // use LookupIterator::PropertyOrElement here but have to duplicate its
-  // functionality instead.
   Handle<Name> key;
-  size_t index;
-  bool key_is_index = property->ToIntegerIndex(&index);
-  if (!key_is_index) {
+  uint32_t index;
+  bool key_is_array_index = property->ToArrayIndex(&index);
+
+  if (!key_is_array_index) {
     ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, key,
                                        Object::ToName(isolate, property));
-    key_is_index = key->AsIntegerIndex(&index);
+    key_is_array_index = key->AsArrayIndex(&index);
   }
 
   Handle<Object> object = args.at(0);
 
   if (object->IsJSModuleNamespace()) {
     if (key.is_null()) {
-      DCHECK(key_is_index);
+      DCHECK(key_is_array_index);
       // Namespace objects can't have indexed properties.
       return ReadOnlyRoots(isolate).false_value();
     }
@@ -306,8 +304,8 @@ RUNTIME_FUNCTION(Runtime_ObjectHasOwnProperty) {
     {
       LookupIterator::Configuration c = LookupIterator::OWN_SKIP_INTERCEPTOR;
       LookupIterator it =
-          key_is_index ? LookupIterator(isolate, js_obj, index, js_obj, c)
-                       : LookupIterator(js_obj, key, js_obj, c);
+          key_is_array_index ? LookupIterator(isolate, js_obj, index, js_obj, c)
+                             : LookupIterator(js_obj, key, js_obj, c);
       Maybe<bool> maybe = JSReceiver::HasProperty(&it);
       if (maybe.IsNothing()) return ReadOnlyRoots(isolate).exception();
       DCHECK(!isolate->has_pending_exception());
@@ -316,15 +314,14 @@ RUNTIME_FUNCTION(Runtime_ObjectHasOwnProperty) {
 
     Map map = js_obj->map();
     if (!map.IsJSGlobalProxyMap() &&
-        (key_is_index && index <= JSArray::kMaxArrayIndex
-             ? !map.has_indexed_interceptor()
-             : !map.has_named_interceptor())) {
+        (key_is_array_index ? !map.has_indexed_interceptor()
+                            : !map.has_named_interceptor())) {
       return ReadOnlyRoots(isolate).false_value();
     }
 
     // Slow case.
     LookupIterator::Configuration c = LookupIterator::OWN;
-    LookupIterator it = key_is_index
+    LookupIterator it = key_is_array_index
                             ? LookupIterator(isolate, js_obj, index, js_obj, c)
                             : LookupIterator(js_obj, key, js_obj, c);
 
@@ -335,9 +332,10 @@ RUNTIME_FUNCTION(Runtime_ObjectHasOwnProperty) {
 
   } else if (object->IsJSProxy()) {
     if (key.is_null()) {
-      DCHECK(key_is_index);
-      key = isolate->factory()->SizeToString(index);
+      DCHECK(key_is_array_index);
+      key = isolate->factory()->Uint32ToString(index);
     }
+
     Maybe<bool> result =
         JSReceiver::HasOwnProperty(Handle<JSProxy>::cast(object), key);
     if (result.IsNothing()) return ReadOnlyRoots(isolate).exception();
@@ -345,8 +343,8 @@ RUNTIME_FUNCTION(Runtime_ObjectHasOwnProperty) {
 
   } else if (object->IsString()) {
     return isolate->heap()->ToBoolean(
-        key_is_index
-            ? index < static_cast<size_t>(String::cast(*object).length())
+        key_is_array_index
+            ? index < static_cast<uint32_t>(String::cast(*object).length())
             : key->Equals(ReadOnlyRoots(isolate).length_string()));
   } else if (object->IsNullOrUndefined(isolate)) {
     THROW_NEW_ERROR_RETURN_FAILURE(

src/strings/string-hasher-inl.h

@@ -60,15 +60,16 @@ uint32_t StringHasher::HashSequentialString(const char_t* chars_raw, int length,
   DCHECK_IMPLIES(0 < length, chars != nullptr);
   if (length >= 1) {
     if (IsDecimalDigit(chars[0]) && (length == 1 || chars[0] != '0')) {
+      uint32_t index = 0;
       if (length <= String::kMaxArrayIndexSize) {
         // Possible array index; try to compute the array index hash.
-        uint32_t index = chars[0] - '0';
+        index = chars[0] - '0';
         int i = 1;
         do {
           if (i == length) {
             return MakeArrayIndexHash(index, length);
           }
-        } while (TryAddArrayIndexChar(&index, chars[i++]));
+        } while (TryAddIndexChar(&index, chars[i++]));
       }
       // The following block wouldn't do anything on 32-bit platforms,
       // because kMaxArrayIndexSize == kMaxIntegerIndexSize there, and
@@ -84,11 +85,10 @@ uint32_t StringHasher::HashSequentialString(const char_t* chars_raw, int length,
         // if there are non-digit characters.
         uint32_t is_integer_index = 0;
         uint32_t running_hash = static_cast<uint32_t>(seed);
-        uint64_t index_big = 0;
+        uint64_t index_big = index;
         const uchar* end = &chars[length];
         while (chars != end) {
-          if (is_integer_index == 0 &&
-              !TryAddIntegerIndexChar(&index_big, *chars)) {
+          if (is_integer_index == 0 && !TryAddIndexChar(&index_big, *chars)) {
             is_integer_index = String::kIsNotIntegerIndexMask;
           }
           running_hash = AddCharacterCore(running_hash, *chars++);

src/utils/utils-inl.h

@@ -33,7 +33,7 @@ class TimedScope {
 };
 
 template <typename Char>
-bool TryAddArrayIndexChar(uint32_t* index, Char c) {
+bool TryAddIndexChar(uint32_t* index, Char c) {
   if (!IsDecimalDigit(c)) return false;
   int d = c - '0';
   // The maximum index is 4294967294; for the computation below to not
@@ -46,14 +46,19 @@ bool TryAddArrayIndexChar(uint32_t* index, Char c) {
 }
 
 template <typename Char>
-bool TryAddIntegerIndexChar(uint64_t* index, Char c) {
+bool TryAddIndexChar(uint64_t* index, Char c) {
   if (!IsDecimalDigit(c)) return false;
   int d = c - '0';
+  // The maximum uint64_t is 18446744073709551615; for the computation below to
+  // not exceed that, the previous index value must be <= 1844674407370955161
+  // if d <= 5, or <= 1844674407370955160 if d >= 6. The (d+2)>>3 computation
+  // is a branch-free way to express that.
+  if (*index > 1844674407370955161ull - ((d + 2) >> 3)) return false;
   *index = (*index) * 10 + d;
-  return (*index <= kMaxSafeIntegerUint64);
+  return true;
 }
 
-template <typename Stream, typename index_t, enum ToIndexMode mode>
+template <typename Stream, typename index_t>
 bool StringToIndex(Stream* stream, index_t* index) {
   uint16_t ch = stream->GetNext();
 
@@ -71,16 +76,14 @@ bool StringToIndex(Stream* stream, index_t* index) {
   while (stream->HasMore()) {
     // Clang on Mac doesn't think that size_t and uint*_t should be
     // implicitly convertible.
-    if (sizeof(result) == 8) {
-      DCHECK_EQ(kToIntegerIndex, mode);
-      if (!TryAddIntegerIndexChar(reinterpret_cast<uint64_t*>(&result),
-                                  stream->GetNext())) {
+    if (sizeof(index_t) == 8) {
+      if (!TryAddIndexChar(reinterpret_cast<uint64_t*>(&result),
+                           stream->GetNext())) {
         return false;
       }
     } else {
-      // Either mode is fine here.
-      if (!TryAddArrayIndexChar(reinterpret_cast<uint32_t*>(&result),
-                                stream->GetNext()))
+      if (!TryAddIndexChar(reinterpret_cast<uint32_t*>(&result),
+                           stream->GetNext()))
         return false;
     }
   }

src/utils/utils.h

@@ -632,11 +632,8 @@ bool DoubleToBoolean(double d);
 template <typename Char>
 bool TryAddIndexChar(uint32_t* index, Char c);
 
-enum ToIndexMode { kToArrayIndex, kToIntegerIndex };
-
 // {index_t} is meant to be {uint32_t} or {size_t}.
-template <typename Stream, typename index_t,
-          enum ToIndexMode mode = kToArrayIndex>
+template <typename Stream, typename index_t>
 bool StringToIndex(Stream* stream, index_t* index);
 
 // Returns the current stack top. Works correctly with ASAN and SafeStack.

test/cctest/test-strings.cc

@@ -1922,14 +1922,12 @@ TEST(HashArrayIndexStrings) {
 #if V8_TARGET_ARCH_32_BIT
     {"4294967295", false, 0, false, 0},  // Valid length but not index.
     {"4294967296", false, 0, false, 0},
-    {"9007199254740991", false, 0, false, 0},
+    {"18446744073709551615", false, 0, false, 0},
 #else
     {"4294967295", false, 0, true, 4294967295u},
     {"4294967296", false, 0, true, 4294967296ull},
-    {"9007199254740991", false, 0, true, 9007199254740991ull},
+    {"18446744073709551615", false, 0, true, 18446744073709551615ull},
 #endif
-    {"9007199254740992", false, 0, false, 0},
-    {"18446744073709551615", false, 0, false, 0},
     {"18446744073709551616", false, 0, false, 0}
   };
   for (int i = 0, n = arraysize(tests); i < n; i++) {

test/mjsunit/regress/large-indices.js

-// 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.
-
-// Flags: --allow-natives-syntax
-
-// crbug.com/1026974
-(function() {
-  function store(obj, key) {
-    obj[key] = 10;
-  }
-  %PrepareFunctionForOptimization(store);
-  for (let i = 0; i < 3; i++) {
-    let obj = {}
-    store(obj, 1152921506754330624);
-    assertEquals(["1152921506754330600"], Object.keys(obj));
-  }
-})();
-(function() {
-  function store(obj, key) {
-    obj[key] = 10;
-  }
-  %PrepareFunctionForOptimization(store);
-  for (let i = 0; i < 3; i++) {
-    let obj2 = new Int32Array(0);
-    store(obj2, 1152921506754330624);
-    assertEquals([], Object.keys(obj2));
-    store(obj2, "1152921506754330624");
-    assertEquals(["1152921506754330624"], Object.keys(obj2));
-  }
-})();
-
-// crbug.com/1026729
-(function() {
-  let key = 0xFFFFFFFF;
-  let object = {};
-  assertFalse(object.hasOwnProperty(key));
-  let proxy = new Proxy({}, {});
-  assertFalse(proxy.hasOwnProperty(key));
-})();
-
-// crbug.com/1026909
-(function() {
-  function load(obj, key) {
-    return obj[key];
-  }
-  %PrepareFunctionForOptimization(load);
-  const array = new Float64Array();
-  assertEquals(undefined, load(array, 'monomorphic'));
-  assertEquals(undefined, load(array, '4294967296'));
-})();
-
-// crbug.com/1026856
-(function() {
-  let key = 0xFFFFFFFF;
-  let receiver = new Int32Array();
-  var value = {};
-  var target = {};
-  Reflect.set(target, key, value, receiver);
-})();