Avoid calling %AddElement with a number out of array index range

Avoid calling %AddElement with a number out of array index range

This patch wraps callsites to %AddElement to fall back to adding a
named property in case it is given an argument of 2**32 or greater.
The change is needed because %AddElement is called by Array functions
in various places, and ES2015 changes these Array functions to use
ToLength rather than ToUint32, so several callsites of %AddElement
which used to be reliable array indices may be larger numbers. While
the proper long-term solution may be to call out to
Object.defineProperty, this fix should allow the ToLength semantics
to be shipped while preserving correctness and not requiring a
rewrite.

BUG=v8:4516
LOG=Y
R=adamk
TEST=Interactively ran Array.prototype.slice on an Array-like which
exceeded array bounds, and found that this did not check-fail at
runtime as it did before.
Microbenchmarked this technique against the previous version on a
simple reverse implementation and found at most a 1% slowdown, as
opposed to other techniques, like calling %DefineDataPropertyUnchecked,
which had a 20% slowdown or Object.defineProperty with a 80% slowdown.

Committed: https://crrev.com/700bbdc673c9b023a7e9339f8f1a68dbe6b8a74d
Cr-Commit-Position: refs/heads/master@{#31640}

[Dan Ehrenberg, 2015-10-27 20:29:11]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-27 20:29:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1420663003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1420663003/1

[Dan Ehrenberg, 2015-10-27 20:31:18]

Description was changed from

==========
Avoid calling %AddElement with a number out of array index range

This patch wraps callsites to %AddElement to fall back to adding a
named property in case it is given an argument of 2**32 or greater.
The change is needed because %AddElement is called by Array functions
in various places, and ES2015 changes these Array functions to use
ToLength rather than ToUint32, so several callsites of %AddElement
which used to be reliable array indices may be larger numbers. While
the proper long-term solution may be to call out to Object.defineProperty,
this fix should allow the ToLength semantics to be shipped while
preserving correctness and not requiring a rewrite.

BUG=v8:4516
LOG=Y
R=adamk
TEST=Interactively ran Array.prototype.slice on an Array-like which
exceeded array bounds, and found that this did not check-fail at
runtime as it did before.
==========

to

==========
Avoid calling %AddElement with a number out of array index range

This patch wraps callsites to %AddElement to fall back to adding a
named property in case it is given an argument of 2**32 or greater.
The change is needed because %AddElement is called by Array functions
in various places, and ES2015 changes these Array functions to use
ToLength rather than ToUint32, so several callsites of %AddElement
which used to be reliable array indices may be larger numbers. While
the proper long-term solution may be to call out to
Object.defineOwnProperty, this fix should allow the ToLength semantics
to be shipped while preserving correctness and not requiring a
rewrite.

BUG=v8:4516
LOG=Y
R=adamk
TEST=Interactively ran Array.prototype.slice on an Array-like which
exceeded array bounds, and found that this did not check-fail at
runtime as it did before.
Microbenchmarked this technique against the previous version on a
simple reverse implementation and found at most a 1% slowdown, as
opposed to other techniques, like calling %DefineDataPropertyUnchecked,
which had a 20% slowdown or Object.defineOwnProperty with a 80% slowdown.
==========

[Dan Ehrenberg, 2015-10-27 20:33:11]

Description was changed from

==========
Avoid calling %AddElement with a number out of array index range

This patch wraps callsites to %AddElement to fall back to adding a
named property in case it is given an argument of 2**32 or greater.
The change is needed because %AddElement is called by Array functions
in various places, and ES2015 changes these Array functions to use
ToLength rather than ToUint32, so several callsites of %AddElement
which used to be reliable array indices may be larger numbers. While
the proper long-term solution may be to call out to
Object.defineOwnProperty, this fix should allow the ToLength semantics
to be shipped while preserving correctness and not requiring a
rewrite.

BUG=v8:4516
LOG=Y
R=adamk
TEST=Interactively ran Array.prototype.slice on an Array-like which
exceeded array bounds, and found that this did not check-fail at
runtime as it did before.
Microbenchmarked this technique against the previous version on a
simple reverse implementation and found at most a 1% slowdown, as
opposed to other techniques, like calling %DefineDataPropertyUnchecked,
which had a 20% slowdown or Object.defineOwnProperty with a 80% slowdown.
==========

to

==========
Avoid calling %AddElement with a number out of array index range

This patch wraps callsites to %AddElement to fall back to adding a
named property in case it is given an argument of 2**32 or greater.
The change is needed because %AddElement is called by Array functions
in various places, and ES2015 changes these Array functions to use
ToLength rather than ToUint32, so several callsites of %AddElement
which used to be reliable array indices may be larger numbers. While
the proper long-term solution may be to call out to
Object.defineProperty, this fix should allow the ToLength semantics
to be shipped while preserving correctness and not requiring a
rewrite.

BUG=v8:4516
LOG=Y
R=adamk
TEST=Interactively ran Array.prototype.slice on an Array-like which
exceeded array bounds, and found that this did not check-fail at
runtime as it did before.
Microbenchmarked this technique against the previous version on a
simple reverse implementation and found at most a 1% slowdown, as
opposed to other techniques, like calling %DefineDataPropertyUnchecked,
which had a 20% slowdown or Object.defineProperty with a 80% slowdown.
==========

[commit-bot: I haz the power, 2015-10-27 20:38:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-27 20:38:28]

Dry run: Try jobs failed on following builders:
  v8_mac_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/11201)

[Dan Ehrenberg, 2015-10-27 20:48:51]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-27 20:48:58]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1420663003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1420663003/20001

[commit-bot: I haz the power, 2015-10-27 21:11:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-27 21:11:41]

Dry run: This issue passed the CQ dry run.

[adamk, 2015-10-27 22:03:07]

Tried to think of a way to test this code in a reasonable amount of time,
failed. That's too bad. Wish we could mark a test as "run this once a {day,
week}"

https://codereview.chromium.org/1420663003/diff/20001/src/js/harmony-array.js
File src/js/harmony-array.js (right):

https://codereview.chromium.org/1420663003/diff/20001/src/js/harmony-array.js...
src/js/harmony-array.js:185: function AddArrayElement(constructor, array, i,
value) {
Do we still need this wrapper? Is it guarding something different than the one
you're adding?

https://codereview.chromium.org/1420663003/diff/20001/src/js/runtime.js
File src/js/runtime.js (right):

https://codereview.chromium.org/1420663003/diff/20001/src/js/runtime.js#newco...
src/js/runtime.js:212: function AddLargeElement(obj, index, value) {
Bikeshedding: "LargeElement" sounds to me like the value is large, not the key.
What about "AddIndexedProperty"?

https://codereview.chromium.org/1420663003/diff/20001/src/js/runtime.js#newco...
src/js/runtime.js:213: if (index === index >>> 0) {
We have a macro for the RHS of this, TO_UINT32(index), please call that instead.

https://codereview.chromium.org/1420663003/diff/20001/src/js/runtime.js#newco...
src/js/runtime.js:216: %AddNamedProperty(obj, GlobalString(index), value, 0);
Two things:

- Please use TO_STRING() instead of GlobalString()
- "0" is spelled "NONE" in macros.py

[Dan Ehrenberg, 2015-10-27 22:09:31]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-27 22:09:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1420663003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1420663003/40001

[commit-bot: I haz the power, 2015-10-27 22:27:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-27 22:27:43]

Dry run: This issue passed the CQ dry run.

[Dan Ehrenberg, 2015-10-28 18:44:43]

PTAL

https://codereview.chromium.org/1420663003/diff/20001/src/js/harmony-array.js
File src/js/harmony-array.js (right):

https://codereview.chromium.org/1420663003/diff/20001/src/js/harmony-array.js...
src/js/harmony-array.js:185: function AddArrayElement(constructor, array, i,
value) {
On 2015/10/27 at 22:03:07, adamk wrote:
> Do we still need this wrapper? Is it guarding something different than the one
you're adding?

I had the same impulse at first, to remove it, but I don't think it's legitimate
to assume that a random object has an array backing store. I think in the other
cases, we are able to call %AddElement because we're constructing a new array,
but that's not the case for Array.from, Array.of when called with another object
as the receiver.

https://codereview.chromium.org/1420663003/diff/20001/src/js/runtime.js
File src/js/runtime.js (right):

https://codereview.chromium.org/1420663003/diff/20001/src/js/runtime.js#newco...
src/js/runtime.js:212: function AddLargeElement(obj, index, value) {
On 2015/10/27 at 22:03:07, adamk wrote:
> Bikeshedding: "LargeElement" sounds to me like the value is large, not the
key. What about "AddIndexedProperty"?

Done

https://codereview.chromium.org/1420663003/diff/20001/src/js/runtime.js#newco...
src/js/runtime.js:213: if (index === index >>> 0) {
On 2015/10/27 at 22:03:07, adamk wrote:
> We have a macro for the RHS of this, TO_UINT32(index), please call that
instead.

Done

https://codereview.chromium.org/1420663003/diff/20001/src/js/runtime.js#newco...
src/js/runtime.js:216: %AddNamedProperty(obj, GlobalString(index), value, 0);
On 2015/10/27 at 22:03:07, adamk wrote:
> Two things:
> 
> - Please use TO_STRING() instead of GlobalString()
> - "0" is spelled "NONE" in macros.py

Done

[adamk, 2015-10-28 19:52:50]

https://codereview.chromium.org/1420663003/diff/20001/src/js/harmony-array.js
File src/js/harmony-array.js (right):

https://codereview.chromium.org/1420663003/diff/20001/src/js/harmony-array.js...
src/js/harmony-array.js:185: function AddArrayElement(constructor, array, i,
value) {
On 2015/10/28 18:44:43, Dan Ehrenberg wrote:
> On 2015/10/27 at 22:03:07, adamk wrote:
> > Do we still need this wrapper? Is it guarding something different than the
one
> you're adding?
> 
> I had the same impulse at first, to remove it, but I don't think it's
legitimate
> to assume that a random object has an array backing store. I think in the
other
> cases, we are able to call %AddElement because we're constructing a new array,
> but that's not the case for Array.from, Array.of when called with another
object
> as the receiver.

All objects have en element backing store for indexed properties that are
representable as a uint32. As long as the property is known to not pre-exist on
the instance, %AddElement should be safe.

https://codereview.chromium.org/1420663003/diff/20001/src/js/runtime.js
File src/js/runtime.js (right):

https://codereview.chromium.org/1420663003/diff/20001/src/js/runtime.js#newco...
src/js/runtime.js:212: function AddLargeElement(obj, index, value) {
On 2015/10/28 18:44:43, Dan Ehrenberg wrote:
> On 2015/10/27 at 22:03:07, adamk wrote:
> > Bikeshedding: "LargeElement" sounds to me like the value is large, not the
> key. What about "AddIndexedProperty"?
> 
> Done

Hmm, "IndexedElement" sounds redundant given the V8 meaning of "element". Can
you change this to "IndexedProperty"?

https://codereview.chromium.org/1420663003/diff/40001/src/js/array.js
File src/js/array.js (right):

https://codereview.chromium.org/1420663003/diff/40001/src/js/array.js#newcode14
src/js/array.js:14: var AddLargeElement;
Missed a rename here.

[adamk, 2015-10-28 20:03:07]

lgtm to land once naming is fixed

https://codereview.chromium.org/1420663003/diff/20001/src/js/harmony-array.js
File src/js/harmony-array.js (right):

https://codereview.chromium.org/1420663003/diff/20001/src/js/harmony-array.js...
src/js/harmony-array.js:185: function AddArrayElement(constructor, array, i,
value) {
On 2015/10/28 19:52:50, adamk wrote:
> On 2015/10/28 18:44:43, Dan Ehrenberg wrote:
> > On 2015/10/27 at 22:03:07, adamk wrote:
> > > Do we still need this wrapper? Is it guarding something different than the
> one
> > you're adding?
> > 
> > I had the same impulse at first, to remove it, but I don't think it's
> legitimate
> > to assume that a random object has an array backing store. I think in the
> other
> > cases, we are able to call %AddElement because we're constructing a new
array,
> > but that's not the case for Array.from, Array.of when called with another
> object
> > as the receiver.
> 
> All objects have en element backing store for indexed properties that are
> representable as a uint32. As long as the property is known to not pre-exist
on
> the instance, %AddElement should be safe.

But Array.from and Array.of call arbitrary user code to construct their objects,
so there's no guarantee about the non-existence of properties on the object. So
I'm wrong, and this should stay as-is.

[Dan Ehrenberg, 2015-10-28 20:08:19]

The CQ bit was checked by littledan@chromium.org

[Dan Ehrenberg, 2015-10-28 20:08:19]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/1420663003/#ps60001
(title: "Fix naming")

[commit-bot: I haz the power, 2015-10-28 20:08:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1420663003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1420663003/60001

[commit-bot: I haz the power, 2015-10-28 20:36:51]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-10-28 20:37:24]

Patchset 4 (id:??) landed as
https://crrev.com/700bbdc673c9b023a7e9339f8f1a68dbe6b8a74d
Cr-Commit-Position: refs/heads/master@{#31640}

[Benedikt Meurer, 2015-10-29 04:46:10]

bmeurer@chromium.org changed reviewers:
+ bmeurer@chromium.org

[Benedikt Meurer, 2015-10-29 04:46:10]

How about changing that to a runtime entry %AddIndexedProperty, because you end
up in the runtime anyway, and this way we can easily use it even when we
"inline"/desugar ConcatIterableToArray at use sites (hopefully) soon.

[Dan Ehrenberg, 2015-10-29 05:17:44]

On 2015/10/29 at 04:46:10, bmeurer wrote:
> How about changing that to a runtime entry %AddIndexedProperty, because you
end up in the runtime anyway, and this way we can easily use it even when we
"inline"/desugar ConcatIterableToArray at use sites (hopefully) soon.

That's roughly what my first patch attempt did. I'm OK with either
implementation strategy.

[Benedikt Meurer, 2015-10-29 05:20:04]

On 2015/10/29 05:17:44, Dan Ehrenberg wrote:
> On 2015/10/29 at 04:46:10, bmeurer wrote:
> > How about changing that to a runtime entry %AddIndexedProperty, because you
> end up in the runtime anyway, and this way we can easily use it even when we
> "inline"/desugar ConcatIterableToArray at use sites (hopefully) soon.
> 
> That's roughly what my first patch attempt did. I'm OK with either
> implementation strategy.

I think adding %AddIndexedProperty is better, because

(a) we can use it in the parser directly (without having to use the context
lookup indirection trick), and
(b) fewer JSFunction overhead for the context (although that's just a few
bytes in this case).

[Dan Ehrenberg, 2015-10-29 05:27:33]

On 2015/10/29 at 05:20:04, bmeurer wrote:
> On 2015/10/29 05:17:44, Dan Ehrenberg wrote:
> > On 2015/10/29 at 04:46:10, bmeurer wrote:
> > > How about changing that to a runtime entry %AddIndexedProperty, because
you
> > end up in the runtime anyway, and this way we can easily use it even when we
> > "inline"/desugar ConcatIterableToArray at use sites (hopefully) soon.
> > 
> > That's roughly what my first patch attempt did. I'm OK with either
> > implementation strategy.
> 
> I think adding %AddIndexedProperty is better, because
> 
> (a) we can use it in the parser directly (without having to use the context
> lookup indirection trick), and
> (b) fewer JSFunction overhead for the context (although that's just a few
> bytes in this case).

Toon expressed another opinion here: https://codereview.chromium.org/1425743002
I don't have an opinion one way or another. Why don't you two discuss it? I'd
appreciate guidance on how this should be done going forward so I don't go back
and forth writing multiple versions of a patch.

[Benedikt Meurer, 2015-10-29 05:30:15]

On 2015/10/29 05:27:33, Dan Ehrenberg wrote:
> On 2015/10/29 at 05:20:04, bmeurer wrote:
> > On 2015/10/29 05:17:44, Dan Ehrenberg wrote:
> > > On 2015/10/29 at 04:46:10, bmeurer wrote:
> > > > How about changing that to a runtime entry %AddIndexedProperty, because
> you
> > > end up in the runtime anyway, and this way we can easily use it even when
we
> > > "inline"/desugar ConcatIterableToArray at use sites (hopefully) soon.
> > > 
> > > That's roughly what my first patch attempt did. I'm OK with either
> > > implementation strategy.
> > 
> > I think adding %AddIndexedProperty is better, because
> > 
> > (a) we can use it in the parser directly (without having to use the context
> > lookup indirection trick), and
> > (b) fewer JSFunction overhead for the context (although that's just a few
> > bytes in this case).
> 
> Toon expressed another opinion here:
https://codereview.chromium.org/1425743002
> I don't have an opinion one way or another. Why don't you two discuss it? I'd
> appreciate guidance on how this should be done going forward so I don't go
back
> and forth writing multiple versions of a patch.

I talked with Toon about this briefly on Tuesday, and his concern was that you
changed %AddElement, which is about adding elements. %AddIndexedProperty would
be a new runtime entry that exists together with %AddElement.
But I'll talk to Toon to make sure we agree.

[Dan Ehrenberg, 2015-10-29 06:17:35]

On 2015/10/29 at 05:30:15, bmeurer wrote:
> On 2015/10/29 05:27:33, Dan Ehrenberg wrote:
> > On 2015/10/29 at 05:20:04, bmeurer wrote:
> > > On 2015/10/29 05:17:44, Dan Ehrenberg wrote:
> > > > On 2015/10/29 at 04:46:10, bmeurer wrote:
> > > > > How about changing that to a runtime entry %AddIndexedProperty,
because
> > you
> > > > end up in the runtime anyway, and this way we can easily use it even
when we
> > > > "inline"/desugar ConcatIterableToArray at use sites (hopefully) soon.
> > > > 
> > > > That's roughly what my first patch attempt did. I'm OK with either
> > > > implementation strategy.
> > > 
> > > I think adding %AddIndexedProperty is better, because
> > > 
> > > (a) we can use it in the parser directly (without having to use the
context
> > > lookup indirection trick), and
> > > (b) fewer JSFunction overhead for the context (although that's just a few
> > > bytes in this case).
> > 
> > Toon expressed another opinion here:
https://codereview.chromium.org/1425743002
> > I don't have an opinion one way or another. Why don't you two discuss it?
I'd
> > appreciate guidance on how this should be done going forward so I don't go
back
> > and forth writing multiple versions of a patch.
> 
> I talked with Toon about this briefly on Tuesday, and his concern was that you
> changed %AddElement, which is about adding elements. %AddIndexedProperty would
> be a new runtime entry that exists together with %AddElement.
> But I'll talk to Toon to make sure we agree.

If we added such a call, we would probably replace all callers of %AddElement
with %AddIndexedProperty.

[Benedikt Meurer, 2015-10-29 06:18:45]

On 2015/10/29 06:17:35, Dan Ehrenberg wrote:
> On 2015/10/29 at 05:30:15, bmeurer wrote:
> > On 2015/10/29 05:27:33, Dan Ehrenberg wrote:
> > > On 2015/10/29 at 05:20:04, bmeurer wrote:
> > > > On 2015/10/29 05:17:44, Dan Ehrenberg wrote:
> > > > > On 2015/10/29 at 04:46:10, bmeurer wrote:
> > > > > > How about changing that to a runtime entry %AddIndexedProperty,
> because
> > > you
> > > > > end up in the runtime anyway, and this way we can easily use it even
> when we
> > > > > "inline"/desugar ConcatIterableToArray at use sites (hopefully) soon.
> > > > > 
> > > > > That's roughly what my first patch attempt did. I'm OK with either
> > > > > implementation strategy.
> > > > 
> > > > I think adding %AddIndexedProperty is better, because
> > > > 
> > > > (a) we can use it in the parser directly (without having to use the
> context
> > > > lookup indirection trick), and
> > > > (b) fewer JSFunction overhead for the context (although that's just a
few
> > > > bytes in this case).
> > > 
> > > Toon expressed another opinion here:
> https://codereview.chromium.org/1425743002
> > > I don't have an opinion one way or another. Why don't you two discuss it?
> I'd
> > > appreciate guidance on how this should be done going forward so I don't go
> back
> > > and forth writing multiple versions of a patch.
> > 
> > I talked with Toon about this briefly on Tuesday, and his concern was that
you
> > changed %AddElement, which is about adding elements. %AddIndexedProperty
would
> > be a new runtime entry that exists together with %AddElement.
> > But I'll talk to Toon to make sure we agree.
> 
> If we added such a call, we would probably replace all callers of %AddElement
> with %AddIndexedProperty.

Hm, ok, then %AddElement is probably the wrong abstraction.

[Toon Verwaest, 2015-10-29 10:41:00]

verwaest@chromium.org changed reviewers:
+ verwaest@chromium.org

[Toon Verwaest, 2015-10-29 10:41:01]

My opinion is indeed that we should separate AddElement (adding only elements)
from AddNamedProperty (adding only named properties) so the runtime has a clear
idea of what you are doing. Sure, right now it could be that all such calls
would have to go through this helper; but shouldn't we know exactly which ranges
of properties end up where? Shouldn't we rather have 2 loops, one which adds
elements until the end of the valid element range, and one which adds named
properties for the rest?

https://codereview.chromium.org/1420663003/diff/60001/src/js/runtime.js
File src/js/runtime.js (right):

https://codereview.chromium.org/1420663003/diff/60001/src/js/runtime.js#newco...
src/js/runtime.js:213: if (index === TO_UINT32(index)) {
kMaxUint32 isn't a valid element either. Max length of arrays is kMaxUint32, so
max element is kMaxUint32-1.

[Toon Verwaest, 2015-10-29 10:41:44]

(And if we have those 2 already, adding yet another runtime function just do
mimick what this function does, doesn't seem very helpful...)

[Toon Verwaest, 2015-10-29 10:56:18]

For that matter, how can slice create an array with elements that are out of
bounds (need named properties)?

[Toon Verwaest, 2015-10-29 11:06:56]

(Ok I see, Array-like... ignore my last comment :))

[Jakob Kummerow, 2015-10-29 12:07:21]

DBC: before anyone starts adding new runtime functions, let me point out that
Runtime_SetProperty (which is the slow fallback path for KeyedStoreICs) already
handles both named and indexed properties. Maybe s/%AddElement/%SetProperty/
would solve all known problems here?

[adamk, 2015-10-29 14:29:01]

On 2015/10/29 12:07:21, Jakob wrote:
> DBC: before anyone starts adding new runtime functions, let me point out that
> Runtime_SetProperty (which is the slow fallback path for KeyedStoreICs)
already
> handles both named and indexed properties. Maybe s/%AddElement/%SetProperty/
> would solve all known problems here?

SetProperty is equivalent to [[Set]] (looks up the proto chain), while what's
needed here is an equivalent to [[DefineOwnProperty]].

[Dan Ehrenberg, 2015-10-29 18:15:41]

On 2015/10/29 at 10:41:01, verwaest wrote:
> My opinion is indeed that we should separate AddElement (adding only elements)
from AddNamedProperty (adding only named properties) so the runtime has a clear
idea of what you are doing. Sure, right now it could be that all such calls
would have to go through this helper; but shouldn't we know exactly which ranges
of properties end up where? Shouldn't we rather have 2 loops, one which adds
elements until the end of the valid element range, and one which adds named
properties for the rest?

Toon, doing the conditional each time turned out to be really cheap in my
microbenchmark. I don't think it'd be a worthwhile optimization to change this
into two loops.
> 
> https://codereview.chromium.org/1420663003/diff/60001/src/js/runtime.js
> File src/js/runtime.js (right):
> 
>
https://codereview.chromium.org/1420663003/diff/60001/src/js/runtime.js#newco...
> src/js/runtime.js:213: if (index === TO_UINT32(index)) {
> kMaxUint32 isn't a valid element either. Max length of arrays is kMaxUint32,
so max element is kMaxUint32-1.

Oh, oops. I wonder why this seemed to fix the 'illegal access' issue. I'll write
up a fix.