AuroraMiddleware/v8
1
0
Fork 0
Code Issues 2 Pull Requests Releases Wiki Activity

Reland "Fixes for size_t LookupIterator"

Browse Source 
This is a reland of e1ad9b899b

Original change's description:
> Fixes for size_t LookupIterator
>
> Fixing some fallout from c968607e12
> 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}
This commit is contained in:
Jakob Kummerow 2019-11-25 23:06:23 +01:00 committed by Commit Bot
parent 825f65d3bd
commit 2b0b80d286
15 changed files with 169 additions and 59 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
src/codegen/code-stub-assembler.cc
View File

@ -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);
}

7
src/codegen/code-stub-assembler.h
View File

@ -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

33
src/compiler/effect-control-linearizer.cc
View File

@ -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,7 +2527,7 @@ 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);
@ -2508,9 +2535,7 @@ Node* EffectControlLinearizer::LowerCheckedTaggedToArrayIndex(
__ 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);

4
src/ic/ic.cc
View File

@ -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;

4
src/json/json-parser.cc
View File

@ -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;

7
src/objects/lookup-inl.h
View File

@ -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>();
}

15
src/objects/name.h
View File

@ -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

8
src/objects/objects-inl.h
View File

@ -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;

3
src/objects/string.cc
View File

@ -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) {

34
src/runtime/runtime-object.cc
View File

@ -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,10 +335,9 @@ 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();
@ -343,8 +345,8 @@ RUNTIME_FUNCTION(Runtime_ObjectHasOwnProperty) {
} 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(

10
src/strings/string-hasher-inl.h
View File

@ -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++);

25
src/utils/utils-inl.h
View File

@ -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;
}
}

5
src/utils/utils.h
View File

@ -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.

6
test/cctest/test-strings.cc
View File

@ -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++) {

60
test/mjsunit/regress/large-indices.js Normal file
View File

@ -0,0 +1,60 @@
// 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);
})();