Reland "Fixes for size_t LookupIterator"

jakobkummerow · Commit Bot · commit 2b0b80d286f1 · 2019-11-25T22:58:40.000Z
This is a reland of e1ad9b8 Original change's description: > Fixes for size_t LookupIterator > > Fixing some fallout from c968607 > 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: verwaest@chromium.org,mstarzinger@chromium.org Bug: chromium:1026729, chromium:1026856, chromium:1026909, chromium:1026974 Change-Id: I66695f05c4910c46f3c75209e14135075721f2cf Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1932839 Reviewed-by: Jakob Kummerow <jkummerow@chromium.org> Commit-Queue: Jakob Kummerow <jkummerow@chromium.org> Cr-Commit-Position: refs/heads/master@{#65162}
diff --git a/src/codegen/code-stub-assembler.cc b/src/codegen/code-stub-assembler.cc
@@ -9766,6 +9766,13 @@ 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);
   }
diff --git a/src/codegen/code-stub-assembler.h b/src/codegen/code-stub-assembler.h
@@ -941,6 +941,13 @@ 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
diff --git a/src/compiler/effect-control-linearizer.cc b/src/compiler/effect-control-linearizer.cc
@@ -214,6 +214,8 @@ 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,
@@ -2429,6 +2431,31 @@ 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 =
@@ -2500,17 +2527,15 @@ Node* EffectControlLinearizer::LowerCheckedTaggedToArrayIndex(
   __ Goto(&done, ChangeSmiToIntPtr(value));
 
   // In the non-Smi case, check the heap numberness, load the number and convert
-  // to int32.
+  // to integer.
   __ Bind(&if_not_smi);
   auto if_not_heap_number = __ MakeDeferredLabel();
   Node* value_map = __ LoadField(AccessBuilder::ForMap(), value);
   Node* is_heap_number = __ TaggedEqual(value_map, __ HeapNumberMapConstant());
   __ GotoIfNot(is_heap_number, &if_not_heap_number);
 
   Node* number = __ LoadField(AccessBuilder::ForHeapNumberValue(), value);
-  number =
-      BuildCheckedFloat64ToInt32(CheckForMinusZeroMode::kDontCheckForMinusZero,
-                                 params.feedback(), number, frame_state);
+  number = BuildCheckedFloat64ToIndex(params.feedback(), number, frame_state);
   __ Goto(&done, number);
 
   __ Bind(&if_not_heap_number);
diff --git a/src/ic/ic.cc b/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 >= std::numeric_limits<intptr_t>::min())) return kBailout;
-    if (num > std::numeric_limits<intptr_t>::max()) return kBailout;
+    if (!(num >= -kMaxSafeInteger)) return kBailout;
+    if (num > kMaxSafeInteger) return kBailout;
     *index_out = static_cast<intptr_t>(num);
     if (*index_out != num) return kBailout;
     return kIntPtr;
diff --git a/src/json/json-parser.cc b/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 !TryAddIndexChar(&index, c);
+            return !TryAddArrayIndexChar(&index, c);
           });
 
           if (CurrentCharacter() == '"') {
@@ -374,7 +374,7 @@ JsonString JsonParser<Char>::ScanJsonPropertyKey(JsonContinuation* cont) {
           }
 
           if (CurrentCharacter() == '\\' && NextCharacter() == 'u') {
-            if (TryAddIndexChar(&index, ScanUnicodeCharacter())) continue;
+            if (TryAddArrayIndexChar(&index, ScanUnicodeCharacter())) continue;
           }
 
           break;
diff --git a/src/objects/lookup-inl.h b/src/objects/lookup-inl.h
@@ -71,13 +71,18 @@ 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()) {
+  if (index_ > JSArray::kMaxArrayIndex && !holder->IsJSTypedArray()) {
     if (key_as_string.is_null()) {
       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>();
 }
diff --git a/src/objects/name.h b/src/objects/name.h
@@ -95,14 +95,13 @@ 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 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);
+  // 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)});
 
   // 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
diff --git a/src/objects/objects-inl.h b/src/objects/objects-inl.h
@@ -823,9 +823,11 @@ bool Object::ToIntegerIndex(size_t* index) const {
   if (IsHeapNumber()) {
     double num = HeapNumber::cast(*this).value();
     if (!(num >= 0)) return false;  // Negation to catch NaNs.
-    // 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;
+    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;
     size_t result = static_cast<size_t>(num);
     if (num != result) return false;  // Conversion lost fractional precision.
     *index = result;
diff --git a/src/objects/string.cc b/src/objects/string.cc
@@ -1397,7 +1397,8 @@ bool String::SlowAsIntegerIndex(size_t* index) {
   }
   if (length == 0 || length > kMaxIntegerIndexSize) return false;
   StringCharacterStream stream(*this);
-  return StringToIndex(&stream, index);
+  return StringToIndex<StringCharacterStream, size_t, kToIntegerIndex>(&stream,
+                                                                       index);
 }
 
 void String::PrintOn(FILE* file) {
diff --git a/src/runtime/runtime-object.cc b/src/runtime/runtime-object.cc
@@ -270,21 +270,23 @@ 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;
-  uint32_t index;
-  bool key_is_array_index = property->ToArrayIndex(&index);
-
-  if (!key_is_array_index) {
+  size_t index;
+  bool key_is_index = property->ToIntegerIndex(&index);
+  if (!key_is_index) {
     ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, key,
                                        Object::ToName(isolate, property));
-    key_is_array_index = key->AsArrayIndex(&index);
+    key_is_index = key->AsIntegerIndex(&index);
   }
 
   Handle<Object> object = args.at(0);
 
   if (object->IsJSModuleNamespace()) {
     if (key.is_null()) {
-      DCHECK(key_is_array_index);
+      DCHECK(key_is_index);
       // Namespace objects can't have indexed properties.
       return ReadOnlyRoots(isolate).false_value();
     }
@@ -304,8 +306,8 @@ RUNTIME_FUNCTION(Runtime_ObjectHasOwnProperty) {
     {
       LookupIterator::Configuration c = LookupIterator::OWN_SKIP_INTERCEPTOR;
       LookupIterator it =
-          key_is_array_index ? LookupIterator(isolate, js_obj, index, js_obj, c)
-                             : LookupIterator(js_obj, key, js_obj, c);
+          key_is_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());
@@ -314,14 +316,15 @@ RUNTIME_FUNCTION(Runtime_ObjectHasOwnProperty) {
 
     Map map = js_obj->map();
     if (!map.IsJSGlobalProxyMap() &&
-        (key_is_array_index ? !map.has_indexed_interceptor()
-                            : !map.has_named_interceptor())) {
+        (key_is_index && index <= JSArray::kMaxArrayIndex
+             ? !map.has_indexed_interceptor()
+             : !map.has_named_interceptor())) {
       return ReadOnlyRoots(isolate).false_value();
     }
 
     // Slow case.
     LookupIterator::Configuration c = LookupIterator::OWN;
-    LookupIterator it = key_is_array_index
+    LookupIterator it = key_is_index
                             ? LookupIterator(isolate, js_obj, index, js_obj, c)
                             : LookupIterator(js_obj, key, js_obj, c);
 
@@ -332,19 +335,18 @@ RUNTIME_FUNCTION(Runtime_ObjectHasOwnProperty) {
 
   } else if (object->IsJSProxy()) {
     if (key.is_null()) {
-      DCHECK(key_is_array_index);
-      key = isolate->factory()->Uint32ToString(index);
+      DCHECK(key_is_index);
+      key = isolate->factory()->SizeToString(index);
     }
-
     Maybe<bool> result =
         JSReceiver::HasOwnProperty(Handle<JSProxy>::cast(object), key);
     if (result.IsNothing()) return ReadOnlyRoots(isolate).exception();
     return isolate->heap()->ToBoolean(result.FromJust());
 
   } else if (object->IsString()) {
     return isolate->heap()->ToBoolean(
-        key_is_array_index
-            ? index < static_cast<uint32_t>(String::cast(*object).length())
+        key_is_index
+            ? index < static_cast<size_t>(String::cast(*object).length())
             : key->Equals(ReadOnlyRoots(isolate).length_string()));
   } else if (object->IsNullOrUndefined(isolate)) {
     THROW_NEW_ERROR_RETURN_FAILURE(
diff --git a/src/strings/string-hasher-inl.h b/src/strings/string-hasher-inl.h
@@ -60,16 +60,15 @@ 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.
-        index = chars[0] - '0';
+        uint32_t index = chars[0] - '0';
         int i = 1;
         do {
           if (i == length) {
             return MakeArrayIndexHash(index, length);
           }
-        } while (TryAddIndexChar(&index, chars[i++]));
+        } while (TryAddArrayIndexChar(&index, chars[i++]));
       }
       // The following block wouldn't do anything on 32-bit platforms,
       // because kMaxArrayIndexSize == kMaxIntegerIndexSize there, and
@@ -85,10 +84,11 @@ 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 = index;
+        uint64_t index_big = 0;
         const uchar* end = &chars[length];
         while (chars != end) {
-          if (is_integer_index == 0 && !TryAddIndexChar(&index_big, *chars)) {
+          if (is_integer_index == 0 &&
+              !TryAddIntegerIndexChar(&index_big, *chars)) {
             is_integer_index = String::kIsNotIntegerIndexMask;
           }
           running_hash = AddCharacterCore(running_hash, *chars++);
diff --git a/src/utils/utils-inl.h b/src/utils/utils-inl.h
@@ -33,7 +33,7 @@ class TimedScope {
 };
 
 template <typename Char>
-bool TryAddIndexChar(uint32_t* index, Char c) {
+bool TryAddArrayIndexChar(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,19 +46,14 @@ bool TryAddIndexChar(uint32_t* index, Char c) {
 }
 
 template <typename Char>
-bool TryAddIndexChar(uint64_t* index, Char c) {
+bool TryAddIntegerIndexChar(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 true;
+  return (*index <= kMaxSafeIntegerUint64);
 }
 
-template <typename Stream, typename index_t>
+template <typename Stream, typename index_t, enum ToIndexMode mode>
 bool StringToIndex(Stream* stream, index_t* index) {
   uint16_t ch = stream->GetNext();
 
@@ -76,14 +71,16 @@ 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(index_t) == 8) {
-      if (!TryAddIndexChar(reinterpret_cast<uint64_t*>(&result),
-                           stream->GetNext())) {
+    if (sizeof(result) == 8) {
+      DCHECK_EQ(kToIntegerIndex, mode);
+      if (!TryAddIntegerIndexChar(reinterpret_cast<uint64_t*>(&result),
+                                  stream->GetNext())) {
         return false;
       }
     } else {
-      if (!TryAddIndexChar(reinterpret_cast<uint32_t*>(&result),
-                           stream->GetNext()))
+      // Either mode is fine here.
+      if (!TryAddArrayIndexChar(reinterpret_cast<uint32_t*>(&result),
+                                stream->GetNext()))
         return false;
     }
   }
diff --git a/src/utils/utils.h b/src/utils/utils.h
@@ -632,8 +632,11 @@ 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>
+template <typename Stream, typename index_t,
+          enum ToIndexMode mode = kToArrayIndex>
 bool StringToIndex(Stream* stream, index_t* index);
 
 // Returns the current stack top. Works correctly with ASAN and SafeStack.
diff --git a/test/cctest/test-strings.cc b/test/cctest/test-strings.cc
@@ -1922,12 +1922,14 @@ TEST(HashArrayIndexStrings) {
 #if V8_TARGET_ARCH_32_BIT
     {"4294967295", false, 0, false, 0},  // Valid length but not index.
     {"4294967296", false, 0, false, 0},
-    {"18446744073709551615", false, 0, false, 0},
+    {"9007199254740991", false, 0, false, 0},
 #else
     {"4294967295", false, 0, true, 4294967295u},
     {"4294967296", false, 0, true, 4294967296ull},
-    {"18446744073709551615", false, 0, true, 18446744073709551615ull},
+    {"9007199254740991", false, 0, true, 9007199254740991ull},
 #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++) {
diff --git a/test/mjsunit/regress/large-indices.js b/test/mjsunit/regress/large-indices.js

Original file line number	Diff line number	Diff line change
`@@ -362,7 +362,7 @@ JsonString JsonParser<Char>::ScanJsonPropertyKey(JsonContinuation* cont) {`
`362`	`362`	`uint32_t index = first - '0';`
`363`	`363`	`while (true) {`
`364`	`364`	`cursor_ = std::find_if(cursor_ + 1, end_, [&index](Char c) {`
`365`		`- return !TryAddIndexChar(&index, c);`
	`365`	`+ return !TryAddArrayIndexChar(&index, c);`
`366`	`366`	`});`
`367`	`367`
`368`	`368`	`if (CurrentCharacter() == '"') {`
`@@ -374,7 +374,7 @@ JsonString JsonParser<Char>::ScanJsonPropertyKey(JsonContinuation* cont) {`
`374`	`374`	`}`
`375`	`375`
`376`	`376`	`if (CurrentCharacter() == '\\' && NextCharacter() == 'u') {`
`377`		`- if (TryAddIndexChar(&index, ScanUnicodeCharacter())) continue;`
	`377`	`+ if (TryAddArrayIndexChar(&index, ScanUnicodeCharacter())) continue;`
`378`	`378`	`}`
`379`	`379`
`380`	`380`	`break;`
Original file line number	Diff line number	Diff line change
`@@ -1397,7 +1397,8 @@ bool String::SlowAsIntegerIndex(size_t* index) {`
`1397`	`1397`	`}`
`1398`	`1398`	`if (length == 0 \|\| length > kMaxIntegerIndexSize) return false;`
`1399`	`1399`	`StringCharacterStream stream(*this);`
`1400`		`- return StringToIndex(&stream, index);`
	`1400`	`+ return StringToIndex<StringCharacterStream, size_t, kToIntegerIndex>(&stream,`
	`1401`	`+ index);`
`1401`	`1402`	`}`
`1402`	`1403`
`1403`	`1404`	`void String::PrintOn(FILE* file) {`