ES6: Array.prototype.slice and friends should use ToLength instead of ToUint32

ES6: Array.prototype.slice and friends should use ToLength instead of ToUint32

- Use 'ToLength' instead of 'ToUint32' where necessary.

BUG=v8:3087
LOG=

[Diego Pino, 2014-09-09 10:31:02]

dpino@igalia.com changed reviewers:
+ aperez@igalia.com, gemont@igalia.com, vjazquez@igalia.com, wingo@igalia.com

[aperez, 2014-09-09 10:59:36]

https://codereview.chromium.org/553623004/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/553623004/diff/1/src/runtime.cc#newcode6130
src/runtime.cc:6130: RUNTIME_FUNCTION(Runtime_ToLength) {
Is there any particular reason not to implement this in JS? The CL for
“Array.from” has one version that looks okay:
https://codereview.chromium.org/363833006/diff/290001/src/runtime.js

That being said, I think it is better to have separate, smaller CL like this to
introduce a ToLength() function. Once this is merged, then the CL for
“Array.from” can be made simpler, too.

[Diego Pino, 2014-09-09 12:01:19]

dpino@igalia.com changed reviewers:
+ caitpotter88@gmail.com

[Diego Pino, 2014-09-09 12:14:18]

On 2014/09/09 10:59:36, aperez wrote:
> https://codereview.chromium.org/553623004/diff/1/src/runtime.cc
> File src/runtime.cc (right):
> 
> https://codereview.chromium.org/553623004/diff/1/src/runtime.cc#newcode6130
> src/runtime.cc:6130: RUNTIME_FUNCTION(Runtime_ToLength) {
> Is there any particular reason not to implement this in JS? The CL for
> “Array.from” has one version that looks okay:
> https://codereview.chromium.org/363833006/diff/290001/src/runtime.js
> 
> That being said, I think it is better to have separate, smaller CL like this
to
> introduce a ToLength() function. Once this is merged, then the CL for
> “Array.from” can be made simpler, too.

No particular reason for implementing it as a runtime function. Since ToLength()
is already coded, and reviewed, as part of the Array.from() CL, I think the
JavaScript implementation is the way to go. So, caitp could you create a
separate CL for implementing ToLength()? WDYT?

[caitp (gmail), 2014-09-09 12:33:32]

On 2014/09/09 12:14:18, Diego Pino wrote:
> On 2014/09/09 10:59:36, aperez wrote:
> > https://codereview.chromium.org/553623004/diff/1/src/runtime.cc
> > File src/runtime.cc (right):
> > 
> > https://codereview.chromium.org/553623004/diff/1/src/runtime.cc#newcode6130
> > src/runtime.cc:6130: RUNTIME_FUNCTION(Runtime_ToLength) {
> > Is there any particular reason not to implement this in JS? The CL for
> > “Array.from” has one version that looks okay:
> > https://codereview.chromium.org/363833006/diff/290001/src/runtime.js
> > 
> > That being said, I think it is better to have separate, smaller CL like this
> to
> > introduce a ToLength() function. Once this is merged, then the CL for
> > “Array.from” can be made simpler, too.
> 
> No particular reason for implementing it as a runtime function. Since
ToLength()
> is already coded, and reviewed, as part of the Array.from() CL, I think the
> JavaScript implementation is the way to go. So, caitp could you create a
> separate CL for implementing ToLength()? WDYT?

sure, I'll put something up in an hour or 2

[Diego Pino, 2014-09-09 16:02:58]

On 2014/09/09 12:33:32, caitp wrote:
> On 2014/09/09 12:14:18, Diego Pino wrote:
> > On 2014/09/09 10:59:36, aperez wrote:
> > > https://codereview.chromium.org/553623004/diff/1/src/runtime.cc
> > > File src/runtime.cc (right):
> > > 
> > >
https://codereview.chromium.org/553623004/diff/1/src/runtime.cc#newcode6130
> > > src/runtime.cc:6130: RUNTIME_FUNCTION(Runtime_ToLength) {
> > > Is there any particular reason not to implement this in JS? The CL for
> > > “Array.from” has one version that looks okay:
> > > https://codereview.chromium.org/363833006/diff/290001/src/runtime.js
> > > 
> > > That being said, I think it is better to have separate, smaller CL like
this
> > to
> > > introduce a ToLength() function. Once this is merged, then the CL for
> > > “Array.from” can be made simpler, too.
> > 
> > No particular reason for implementing it as a runtime function. Since
> ToLength()
> > is already coded, and reviewed, as part of the Array.from() CL, I think the
> > JavaScript implementation is the way to go. So, caitp could you create a
> > separate CL for implementing ToLength()? WDYT?
> 
> sure, I'll put something up in an hour or 2

Thanks. For future reference, https://codereview.chromium.org/552273002/

[arv (Not doing code reviews), 2014-09-09 19:20:52]

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

[arv (Not doing code reviews), 2014-09-09 19:20:52]

FYI

https://codereview.chromium.org/553623004/diff/1/src/array.js
File src/array.js (right):

https://codereview.chromium.org/553623004/diff/1/src/array.js#newcode360
src/array.js:360: var len = %ToLength(array.length);
Remember that calling a runtime function has high overhead and can not be
optimized.

[rossberg, 2014-09-10 07:26:24]

On 2014/09/09 19:20:52, arv wrote:
> FYI
> 
> https://codereview.chromium.org/553623004/diff/1/src/array.js
> File src/array.js (right):
> 
> https://codereview.chromium.org/553623004/diff/1/src/array.js#newcode360
> src/array.js:360: var len = %ToLength(array.length);
> Remember that calling a runtime function has high overhead and can not be
> optimized.

Right, it should be implemented in runtime.js.

[Diego Pino, 2014-09-18 14:13:14]

dpino@igalia.com changed reviewers:
- aperez@igalia.com, caitpotter88@gmail.com, gemont@igalia.com,
vjazquez@igalia.com

[Diego Pino, 2014-09-18 17:38:23]

Patchset #2 (id:20001) has been deleted

[wingo, 2014-09-19 10:28:57]

Looking good, needs more tests though for the new cases that the change exposes.
(i.e. negative lengths, lengths >= 2^32)

[wingo, 2014-09-23 18:28:53]

Some notes on the tests.

https://codereview.chromium.org/553623004/diff/60001/test/mjsunit/array-lengt...
File test/mjsunit/array-length.js (right):

https://codereview.chromium.org/553623004/diff/60001/test/mjsunit/array-lengt...
test/mjsunit/array-length.js:133: Array.prototype.toString.apply(o);
probably best to assert not that the length didn't change, but rather that the
result is sane.  note that not all MAX_VALUE / MAX_SAFE_INTEGER expressions will
be testable due to resource limitations.

https://codereview.chromium.org/553623004/diff/60001/test/mjsunit/array-lengt...
test/mjsunit/array-length.js:161: Array.prototype.toLocaleString.apply(o);
What does this evaluate to?  Please tell me it's not a [ followed by 2^53 commas
:)

https://codereview.chromium.org/553623004/diff/60001/test/mjsunit/array-lengt...
test/mjsunit/array-length.js:165: assertTrue(threw);
use assertThrows

[Diego Pino, 2014-09-24 11:25:08]

https://codereview.chromium.org/553623004/diff/60001/test/mjsunit/array-lengt...
File test/mjsunit/array-length.js (right):

https://codereview.chromium.org/553623004/diff/60001/test/mjsunit/array-lengt...
test/mjsunit/array-length.js:133: Array.prototype.toString.apply(o);
On 2014/09/23 18:28:53, wingo wrote:
> probably best to assert not that the length didn't change, but rather that the
> result is sane.  note that not all MAX_VALUE / MAX_SAFE_INTEGER expressions
will
> be testable due to resource limitations.

Acknowledged.

https://codereview.chromium.org/553623004/diff/60001/test/mjsunit/array-lengt...
test/mjsunit/array-length.js:161: Array.prototype.toLocaleString.apply(o);
On 2014/09/23 18:28:53, wingo wrote:
> What does this evaluate to?  Please tell me it's not a [ followed by 2^53
commas
> :)

LOL, got to test all cases.

https://codereview.chromium.org/553623004/diff/60001/test/mjsunit/array-lengt...
test/mjsunit/array-length.js:165: assertTrue(threw);
On 2014/09/23 18:28:53, wingo wrote:
> use assertThrows

I tried that, but it didn't work. This is what I get when trying to use
assertThrows:

RangeError: Invalid array length
    at Object.toLocaleString (native)
    at /home/dpino/workspace/v8/test/mjsunit/array-length.js:169:45
Command: /home/dpino/workspace/v8/out/x64.release/d8 --test
--random-seed=1621081974 --stress-opt --always-opt --nohard-abort
--nodead-code-elimination --nofold-constants
/home/dpino/workspace/v8/test/mjsunit/mjsunit.js
/home/dpino/workspace/v8/test/mjsunit/array-length.js
=== mjsunit/array-length ===                                      
/home/dpino/workspace/v8/test/mjsunit/array-length.js:169: RangeError: Invalid
array length
assertThrows(Array.prototype.toLocaleString.apply(o));

If I run the same statement in the console:

d8> Array.prototype.toLocaleString.apply(o);
(d8):1: RangeError: Invalid array length
Array.prototype.toLocaleString.apply(o);
                               ^
RangeError: Invalid array length
    at Object.toLocaleString (native)
    at (d8):1:32

So, I coded it this way, taking as an example
test/mjsunit/stack-traces.js:testCallerCensorship().

[Diego Pino, 2014-09-25 08:38:03]

Patchset #4 (id:80001) has been deleted

[wingo, 2014-10-07 11:24:08]

Some more nits

https://codereview.chromium.org/553623004/diff/60001/test/mjsunit/array-lengt...
File test/mjsunit/array-length.js (right):

https://codereview.chromium.org/553623004/diff/60001/test/mjsunit/array-lengt...
test/mjsunit/array-length.js:161: Array.prototype.toLocaleString.apply(o);
On 2014/09/24 11:25:08, Diego Pino wrote:
> On 2014/09/23 18:28:53, wingo wrote:
> > What does this evaluate to?  Please tell me it's not a [ followed by 2^53
> commas
> > :)
> 
> LOL, got to test all cases.

It's a serious question!  We can't generate such a value during the tests.  What
does it evaluate to?

https://codereview.chromium.org/553623004/diff/60001/test/mjsunit/array-lengt...
test/mjsunit/array-length.js:165: assertTrue(threw);
On 2014/09/24 11:25:08, Diego Pino wrote:
> On 2014/09/23 18:28:53, wingo wrote:
> > use assertThrows
> 
> I tried that, but it didn't work. This is what I get when trying to use
> assertThrows:

What code did you try?  In this case it can look like:

assertThrows(function() { Array.prototype.toLocaleString.apply(o) }, RangeError)

https://codereview.chromium.org/553623004/diff/100001/test/mjsunit/array-leng...
File test/mjsunit/array-length.js (right):

https://codereview.chromium.org/553623004/diff/100001/test/mjsunit/array-leng...
test/mjsunit/array-length.js:127: assertEquals(o.length, Number.MIN_VALUE);
the order is assertEquals(expected, found); so we need to reverse this one and
others

[Diego Pino, 2014-10-12 20:02:04]

https://codereview.chromium.org/553623004/diff/60001/test/mjsunit/array-lengt...
File test/mjsunit/array-length.js (right):

https://codereview.chromium.org/553623004/diff/60001/test/mjsunit/array-lengt...
test/mjsunit/array-length.js:161: Array.prototype.toLocaleString.apply(o);
On 2014/10/07 11:24:08, wingo wrote:
> On 2014/09/24 11:25:08, Diego Pino wrote:
> > On 2014/09/23 18:28:53, wingo wrote:
> > > What does this evaluate to?  Please tell me it's not a [ followed by 2^53
> > commas
> > > :)
> > 
> > LOL, got to test all cases.
> 
> It's a serious question!  We can't generate such a value during the tests. 
What
> does it evaluate to?

Sorry about that. I didn't mean to make fun of it.

When that expression is evaluated I got an exception. The same happens for
Integer.MAX_SAFE_INTEGER. However, if I try 0xffffffff there's a crash and I got
a core dump:

d8> var o = { length: 0xffffffff };
undefined
d8>  Array.prototype.toLocaleString.apply(o)

#
# Fatal error in invalid table size
# Allocation failed - process out of memory
#

Illegal instruction (core dumped)

I'm not sure about what to do. Should I avoid testing any big positive number to
avoid the possibility that the tests may crash?

https://codereview.chromium.org/553623004/diff/60001/test/mjsunit/array-lengt...
test/mjsunit/array-length.js:165: assertTrue(threw);
On 2014/10/07 11:24:08, wingo wrote:
> On 2014/09/24 11:25:08, Diego Pino wrote:
> > On 2014/09/23 18:28:53, wingo wrote:
> > > use assertThrows
> > 
> > I tried that, but it didn't work. This is what I get when trying to use
> > assertThrows:
> 
> What code did you try?  In this case it can look like:
> 
> assertThrows(function() { Array.prototype.toLocaleString.apply(o) },
RangeError)

OK, this works. Before I tried
assertThrows(Array.prototype.toLocaleString.apply(o), RangeError)

https://codereview.chromium.org/553623004/diff/100001/test/mjsunit/array-leng...
File test/mjsunit/array-length.js (right):

https://codereview.chromium.org/553623004/diff/100001/test/mjsunit/array-leng...
test/mjsunit/array-length.js:127: assertEquals(o.length, Number.MIN_VALUE);
On 2014/10/07 11:24:08, wingo wrote:
> the order is assertEquals(expected, found); so we need to reverse this one and
> others

Acknowledged.

[wingo, 2014-10-13 07:36:22]

https://codereview.chromium.org/553623004/diff/60001/test/mjsunit/array-lengt...
File test/mjsunit/array-length.js (right):

https://codereview.chromium.org/553623004/diff/60001/test/mjsunit/array-lengt...
test/mjsunit/array-length.js:161: Array.prototype.toLocaleString.apply(o);
On 2014/10/12 20:02:03, Diego Pino wrote:
> When that expression is evaluated I got an exception. The same happens for
> Integer.MAX_SAFE_INTEGER.

What exception?  Should we test for that specific exception?

> However, if I try 0xffffffff there's a crash and I got a core dump:
> 
> d8> var o = { length: 0xffffffff };
> undefined
> d8>  Array.prototype.toLocaleString.apply(o)
> 
> #
> # Fatal error in invalid table size
> # Allocation failed - process out of memory
> #

Indeed, this is how V8 deals with out-of-memory situations.

> I'm not sure about what to do. Should I avoid testing any big positive number
to
> avoid the possibility that the tests may crash?

Basically, yes. We can't allocate gigabytes of memory in the normal test suite. 
Unhappily though this makes this patch harder to test -- I suspect
integer-overflow vulnerabilities might be present but can't disprove them :(

[Diego Pino, 2014-10-13 11:58:35]

On 2014/10/13 07:36:22, wingo wrote:

> Basically, yes. We can't allocate gigabytes of memory in the normal test
suite. 
> Unhappily though this makes this patch harder to test -- I suspect
> integer-overflow vulnerabilities might be present but can't disprove them :(

Fixed. I removed the tests that involve large numbers (MAX_VALUE or
MAX_SAFE_INTEGER). Patched updated, please take a look :)

[wingo, 2014-10-13 12:27:58]

Some more comments :)  Sorry for the back and forth but there are a lot of edge
cases now!

https://codereview.chromium.org/553623004/diff/100001/test/mjsunit/array-leng...
File test/mjsunit/array-length.js (right):

https://codereview.chromium.org/553623004/diff/100001/test/mjsunit/array-leng...
test/mjsunit/array-length.js:172: assertEquals(o.length, Number.MAX_SAFE_INTEGER
+ 1);
This test is valid -- it will create a sparse array.  Keep it.

https://codereview.chromium.org/553623004/diff/100001/test/mjsunit/array-leng...
test/mjsunit/array-length.js:186: assertEquals(o.length, Number.MAX_SAFE_INTEGER
- 1);
This test is also valid, I think.

https://codereview.chromium.org/553623004/diff/140001/test/mjsunit/array-leng...
File test/mjsunit/array-length.js (right):

https://codereview.chromium.org/553623004/diff/140001/test/mjsunit/array-leng...
test/mjsunit/array-length.js:125: assertEquals(Number.MIN_VALUE, o.length);
TBH I don't find this test to be useful -- toString() isn't going to mutate the
array-like that it receives, right?  Here the right thing to do is to test that
the result is equal to "" or whatever the spec says it should be..

https://codereview.chromium.org/553623004/diff/140001/test/mjsunit/array-leng...
test/mjsunit/array-length.js:131: assertEquals(Number.MIN_VALUE, o.length);
Likewise, just check that the result is what you expect.

https://codereview.chromium.org/553623004/diff/140001/test/mjsunit/array-leng...
test/mjsunit/array-length.js:137: assertEquals(Number.MIN_VALUE, o.length);
Likewise

https://codereview.chromium.org/553623004/diff/140001/test/mjsunit/array-leng...
test/mjsunit/array-length.js:144: assertEquals(1, o.length);
Here OTOH it does make sense to check the length, cool.  Perhaps check that o[0]
is 1.

https://codereview.chromium.org/553623004/diff/140001/test/mjsunit/array-leng...
test/mjsunit/array-length.js:200: assertEquals(1, o.length);
need to test negative indexes with >2^32 length; see the spec

https://codereview.chromium.org/553623004/diff/140001/test/mjsunit/array-leng...
test/mjsunit/array-length.js:201: 
need to test also:
*slice* with large indexes (a small slice at the end)
slice with large indexes and big arrays (take a small slice at the end)
lastIndexOf and indexOf with a large array and a large fromIndex (close to the
length of the array)
fill with large array, negative start offset, and/or negative end offset

[wingo, 2014-10-22 07:33:13]

dpino is this one ready for another round of review?  remember to comment when
you update patches :)

[Diego Pino, 2014-10-22 08:32:28]

On 2014/10/22 07:33:13, wingo wrote:
> dpino is this one ready for another round of review?  remember to comment when
> you update patches :)

Yep, sorry abut that. I updated the CL, so it needs another round of review.

[wingo, 2014-10-22 09:49:31]

Looking pretty good, just a couple of nits, and if you can test fill with large
array, negative start offset, and/or negative end offset, I think we're good to
go.

https://codereview.chromium.org/553623004/diff/190001/test/mjsunit/array-leng...
File test/mjsunit/array-length.js (right):

https://codereview.chromium.org/553623004/diff/190001/test/mjsunit/array-leng...
test/mjsunit/array-length.js:148: assertEquals(o.length, Number.MAX_SAFE_INTEGER
+ 1);
Perhaps check that pushing another value keeps the same length and overwrites
the old value.

https://codereview.chromium.org/553623004/diff/190001/test/mjsunit/array-leng...
test/mjsunit/array-length.js:216: assertEquals(0, o.length);
These are two of the same test

https://codereview.chromium.org/553623004/diff/190001/test/mjsunit/array-leng...
test/mjsunit/array-length.js:254: // ArrayLasIndexOf
Last

https://codereview.chromium.org/553623004/diff/190001/test/mjsunit/array-leng...
test/mjsunit/array-length.js:264: assertEquals(arrayMaxLength, result);
I guess I am mostly concerned here about
  var o = { length: Number.MAX_SAFE_INTEGER }
  o[Number.MAX_SAFE_INTEGER - 1] = "foo"

Can you try that for indexOf and lastIndexOf?

[Diego Pino, 2014-10-27 18:15:48]

Patchset #7 (id:210001) has been deleted

[Diego Pino, 2014-10-27 18:22:29]

On 2014/10/22 09:49:31, wingo wrote:
> Looking pretty good, just a couple of nits, and if you can test fill with
large
> array, negative start offset, and/or negative end offset, I think we're good
to
> go.
> 

Added test.

> Can you try that for indexOf and lastIndexOf?

Fixed last issues. Needs another round of review.

On the other hand, I searched for expressions that cast array.length as an
UINT32 in "array.js" and there are still some functions that do that. More
precisely: ObservedArrayPush, ObservedArrayUnshift, ObservedArraySplice,
SafeRemoveArrayHoles. It seems reasonable to replace those too, but I couldn't
find these functions in the ES6 spec.

[wingo, 2014-10-28 08:38:03]

I think at this point LGTM, you need higher review powers :)  Adding adamk to Cc
as he has been working in this area recently.

[adamk, 2014-10-28 09:07:01]

adamk@chromium.org changed reviewers:
+ rossberg@chromium.org

[adamk, 2014-10-28 09:07:01]

Sorry to pass the buck, but I don't have OWNERS authority myself. +rossberg, you
reviewed and landed ToLength().

[rossberg, 2014-10-29 09:27:01]

This looks good, my only concern is that it's a breaking change, and yet, not
behind a flag. I wonder if there is an easy (and efficient) way to flag it. If
not, we should probably wait with landing this until after the M40 branch end of
next week.

https://codereview.chromium.org/553623004/diff/230001/test/mjsunit/array-leng...
File test/mjsunit/array-length.js (right):

https://codereview.chromium.org/553623004/diff/230001/test/mjsunit/array-leng...
test/mjsunit/array-length.js:245: var result = Array.prototype.indexOf.apply(o,
["foo", Number.MAX_SAFE_INTEGER - 2]);
nit: line length

https://codereview.chromium.org/553623004/diff/230001/test/mjsunit/array-leng...
test/mjsunit/array-length.js:261: var result =
Array.prototype.lastIndexOf.apply(o, ["foo", Number.MAX_SAFE_INTEGER]);
nit: line length

[Diego Pino, 2014-10-29 10:08:21]

On 2014/10/29 09:27:01, rossberg wrote:
> This looks good, my only concern is that it's a breaking change, and yet, not
> behind a flag. I wonder if there is an easy (and efficient) way to flag it. If
> not, we should probably wait with landing this until after the M40 branch end
of
> next week.
> 

Fixed nits. I'm OK with waiting if that's more convenient.

[Diego Pino, 2014-12-07 15:21:59]

This patch was pending to land after M40 branching. Perhaps it's necessary to
review it again. Thanks!

[rossberg, 2014-12-08 10:32:55]

On 2014/12/07 15:21:59, Diego Pino wrote:
> This patch was pending to land after M40 branching. Perhaps it's necessary to
> review it again. Thanks!

After a recent emergencies we had with a harmless new feature causing web
breakage, I'm even more concerned shipping this without a flag. Here is a
suggestion:

- Instead of calling ToLength directly, call a new helper ToLengthFlagged.
- By default, this just invokes TO_UINT32.
- In harmony-arrays.js, it gets patched to ToLength.

This way, the new behaviour is only turned on with the --harmony-arrays flag.
Also, make sure to have some tests for both the old and the new behaviour.

Does that make sense?

[arv (Not doing code reviews), 2014-12-08 13:52:19]

On 2014/12/08 at 10:32:55, rossberg wrote:
> On 2014/12/07 15:21:59, Diego Pino wrote:
> > This patch was pending to land after M40 branching. Perhaps it's necessary
to
> > review it again. Thanks!
> 
> After a recent emergencies we had with a harmless new feature causing web
breakage, I'm even more concerned shipping this without a flag. Here is a
suggestion:
> 
> - Instead of calling ToLength directly, call a new helper ToLengthFlagged.
> - By default, this just invokes TO_UINT32.
> - In harmony-arrays.js, it gets patched to ToLength.
> 
> This way, the new behaviour is only turned on with the --harmony-arrays flag.
Also, make sure to have some tests for both the old and the new behaviour.
> 
> Does that make sense?

ToLength is an order of magnitude less scary than Array.prototype.values and
String.prototype.contains. I don't think it warrants this kind of caution.

[rossberg, 2014-12-08 13:54:51]

On 2014/12/08 13:52:19, arv wrote:
> On 2014/12/08 at 10:32:55, rossberg wrote:
> > On 2014/12/07 15:21:59, Diego Pino wrote:
> > > This patch was pending to land after M40 branching. Perhaps it's necessary
> to
> > > review it again. Thanks!
> > 
> > After a recent emergencies we had with a harmless new feature causing web
> breakage, I'm even more concerned shipping this without a flag. Here is a
> suggestion:
> > 
> > - Instead of calling ToLength directly, call a new helper ToLengthFlagged.
> > - By default, this just invokes TO_UINT32.
> > - In harmony-arrays.js, it gets patched to ToLength.
> > 
> > This way, the new behaviour is only turned on with the --harmony-arrays
flag.
> Also, make sure to have some tests for both the old and the new behaviour.
> > 
> > Does that make sense?
> 
> ToLength is an order of magnitude less scary than Array.prototype.values and
> String.prototype.contains. I don't think it warrants this kind of caution.

It can't harm either. And the above is very easy to implement.

[Diego Pino, 2014-12-10 12:28:43]

On 2014/12/08 10:32:55, rossberg wrote:

> Does that make sense?

Redefining ToLengthFlagged() in src/harmony-array.js didn't work. I got a crash
when doing it. So instead I followed the same approach as with harmony_regexps.
I created a global flag 'harmony_arrays' that is turned on if --harmony_arrays
is passed as argument.

I moved the new tests added in ./mjsunit/array-length.js to
test/mjsunit/harmony/array-length.js. I also needed to rebase the CL. Please
take a look.

[rossberg, 2014-12-10 13:36:25]

On 2014/12/10 12:28:43, Diego Pino wrote:
> On 2014/12/08 10:32:55, rossberg wrote:
> 
> > Does that make sense?
> 
> Redefining ToLengthFlagged() in src/harmony-array.js didn't work. I got a
crash
> when doing it. So instead I followed the same approach as with
harmony_regexps.
> I created a global flag 'harmony_arrays' that is turned on if --harmony_arrays
> is passed as argument.
> 
> I moved the new tests added in ./mjsunit/array-length.js to
> test/mjsunit/harmony/array-length.js. I also needed to rebase the CL. Please
> take a look.

I'm curious to know what crash you got. Your version looks correct, too, but
it's a bit more expensive.

[Diego Pino, 2014-12-10 14:20:19]

On 2014/12/10 13:36:25, rossberg wrote:

> I'm curious to know what crash you got. Your version looks correct, too, but
> it's a bit more expensive.

I added ToLengthFlagged() to src/array.js and redefined it in
src/harmomy-array.js as:

function ToLengthFlagged(length) {
    return ToLength(length);
}

I put that code at the very beginning of the file, right before the definition
of 'ArrayFind'.

Then if I try to run the new tests or simply start d8 --harmony_array, I got the
following crash:

$ d8 --harmony_arrays
Exception thrown during bootstrapping
Extension or internal compilation error.
Failing script:
Exception thrown during bootstrapping
Extension or internal compilation error.
    1: Failing script:
    1: 

#
# Fatal error in .././src/api.h, line 386
# CHECK(allow_empty_handle || that != __null) failed
#

==== C stack trace ===============================

 1: ??
 2: ??
 3: ??
 4: ??
 5: ??
 6: ??
 7: __libc_start_main
 8: ??
Illegal instruction (core dumped)

This is a backtrace of the crash:

(gdb) bt
#0  0x00000000008bca29 in v8::base::OS::Abort() ()
#1  0x00000000008baf6e in V8_Fatal ()
#2  0x0000000000418078 in v8::Utils::OpenHandle(v8::Context const*, bool) ()
#3  0x0000000000418ec5 in v8::Context::Enter() ()
#4  0x000000000040b44f in v8::Shell::CreateEvaluationContext(v8::Isolate*) ()
#5  0x000000000040c783 in v8::Shell::RunMain(v8::Isolate*, int, char**) ()
#6  0x0000000000410dc4 in v8::Shell::Main(int, char**) ()
#7  0x00007fa8ef450ec5 in __libc_start_main (main=0x407db0 <main>, argc=2,
argv=0x7fff6f017678, init=<optimized out>, fini=<optimized out>, 
    rtld_fini=<optimized out>, stack_end=0x7fff6f017668) at libc-start.c:287

My environment is Ubuntu 14.04, x86_64, gcc (Ubuntu 4.8.2-19ubuntu1) 4.8.2.

I created a gist of the patch https://gist.github.com/dpino/30587a3cbf7acd71fc98

[caitp (gmail), 2014-12-10 14:22:14]

On 2014/12/10 14:20:19, Diego Pino wrote:
> On 2014/12/10 13:36:25, rossberg wrote:
> 
> > I'm curious to know what crash you got. Your version looks correct, too, but
> > it's a bit more expensive.
> 
> I added ToLengthFlagged() to src/array.js and redefined it in
> src/harmomy-array.js as:
> 
> function ToLengthFlagged(length) {
>     return ToLength(length);
> }
> 
> I put that code at the very beginning of the file, right before the definition
> of 'ArrayFind'.
> 
> Then if I try to run the new tests or simply start d8 --harmony_array, I got
the
> following crash:
> 
> $ d8 --harmony_arrays
> Exception thrown during bootstrapping
> Extension or internal compilation error.
> Failing script:
> Exception thrown during bootstrapping
> Extension or internal compilation error.
>     1: Failing script:
>     1: 
> 
> #
> # Fatal error in .././src/api.h, line 386
> # CHECK(allow_empty_handle || that != __null) failed
> #
> 
> ==== C stack trace ===============================
> 
>  1: ??
>  2: ??
>  3: ??
>  4: ??
>  5: ??
>  6: ??
>  7: __libc_start_main
>  8: ??
> Illegal instruction (core dumped)
> 
> This is a backtrace of the crash:
> 
> (gdb) bt
> #0  0x00000000008bca29 in v8::base::OS::Abort() ()
> #1  0x00000000008baf6e in V8_Fatal ()
> #2  0x0000000000418078 in v8::Utils::OpenHandle(v8::Context const*, bool) ()
> #3  0x0000000000418ec5 in v8::Context::Enter() ()
> #4  0x000000000040b44f in v8::Shell::CreateEvaluationContext(v8::Isolate*) ()
> #5  0x000000000040c783 in v8::Shell::RunMain(v8::Isolate*, int, char**) ()
> #6  0x0000000000410dc4 in v8::Shell::Main(int, char**) ()
> #7  0x00007fa8ef450ec5 in __libc_start_main (main=0x407db0 <main>, argc=2,
> argv=0x7fff6f017678, init=<optimized out>, fini=<optimized out>, 
>     rtld_fini=<optimized out>, stack_end=0x7fff6f017668) at libc-start.c:287
> 
> My environment is Ubuntu 14.04, x86_64, gcc (Ubuntu 4.8.2-19ubuntu1) 4.8.2.
> 
> I created a gist of the patch
https://gist.github.com/dpino/30587a3cbf7acd71fc98

Try `var ToLengthFlagged = ToUint32;` in array.js, and `ToLengthFlagged =
ToLength;` in harmony-array.js

That should avoid the problem

[rossberg, 2014-12-10 14:52:54]

On 2014/12/10 14:22:14, caitp wrote:
> On 2014/12/10 14:20:19, Diego Pino wrote:
> > On 2014/12/10 13:36:25, rossberg wrote:
> > 
> > > I'm curious to know what crash you got. Your version looks correct, too,
but
> > > it's a bit more expensive.
> > 
> > I added ToLengthFlagged() to src/array.js and redefined it in
> > src/harmomy-array.js as:
> > 
> > function ToLengthFlagged(length) {
> >     return ToLength(length);
> > }
> > 
> > I put that code at the very beginning of the file, right before the
definition
> > of 'ArrayFind'.
> > 
> > Then if I try to run the new tests or simply start d8 --harmony_array, I got
> the
> > following crash:
> > 
> > $ d8 --harmony_arrays
> > Exception thrown during bootstrapping
> > Extension or internal compilation error.
> > Failing script:
> > Exception thrown during bootstrapping
> > Extension or internal compilation error.
> >     1: Failing script:
> >     1: 
> > 
> > #
> > # Fatal error in .././src/api.h, line 386
> > # CHECK(allow_empty_handle || that != __null) failed
> > #
> > 
> > ==== C stack trace ===============================
> > 
> >  1: ??
> >  2: ??
> >  3: ??
> >  4: ??
> >  5: ??
> >  6: ??
> >  7: __libc_start_main
> >  8: ??
> > Illegal instruction (core dumped)
> > 
> > This is a backtrace of the crash:
> > 
> > (gdb) bt
> > #0  0x00000000008bca29 in v8::base::OS::Abort() ()
> > #1  0x00000000008baf6e in V8_Fatal ()
> > #2  0x0000000000418078 in v8::Utils::OpenHandle(v8::Context const*, bool) ()
> > #3  0x0000000000418ec5 in v8::Context::Enter() ()
> > #4  0x000000000040b44f in v8::Shell::CreateEvaluationContext(v8::Isolate*)
()
> > #5  0x000000000040c783 in v8::Shell::RunMain(v8::Isolate*, int, char**) ()
> > #6  0x0000000000410dc4 in v8::Shell::Main(int, char**) ()
> > #7  0x00007fa8ef450ec5 in __libc_start_main (main=0x407db0 <main>, argc=2,
> > argv=0x7fff6f017678, init=<optimized out>, fini=<optimized out>, 
> >     rtld_fini=<optimized out>, stack_end=0x7fff6f017668) at libc-start.c:287
> > 
> > My environment is Ubuntu 14.04, x86_64, gcc (Ubuntu 4.8.2-19ubuntu1) 4.8.2.
> > 
> > I created a gist of the patch
> https://gist.github.com/dpino/30587a3cbf7acd71fc98

Ah, that is the effect you get when some of the library JavaScript failed at
start-up. It looks more scary than it is (but admittedly isn't very helpful
either).

To be honest, though, I'm not quite sure what the error is here.

> Try `var ToLengthFlagged = ToUint32;` in array.js, and `ToLengthFlagged =
> ToLength;` in harmony-array.js
> 
> That should avoid the problem

Right, that's what I had in mind. Could you try that?

[Diego Pino, 2014-12-10 15:08:57]

On 2014/12/10 14:52:54, rossberg wrote:
> On 2014/12/10 14:22:14, caitp wrote:
 
> > Try `var ToLengthFlagged = ToUint32;` in array.js, and `ToLengthFlagged =
> > ToLength;` in harmony-array.js
> > 
> > That should avoid the problem
> 
> Right, that's what I had in mind. Could you try that?

I tried to reduce the scope of the change and created a small patch on top of
ToT. This new version didn't work either. Here's the gist
https://gist.github.com/dpino/61b06a6b03a71070906a.

And here's the crash I got. This time I built with Clang.

+ exec make console=readline disassembler=on objectprint=on verifyheap=on
extrachecks=on gdbjit=on deprecationwarnings=on werror=no -j4 x64.release
make[1]: Entering directory `/home/dpino/workspace/v8/out'
  ACTION tools_gyp_v8_gyp_js2c_target_js2c
/home/dpino/workspace/v8/out/x64.release/obj/gen/libraries.cc
  ACTION tools_gyp_v8_gyp_js2c_target_js2c_experimental
/home/dpino/workspace/v8/out/x64.release/obj/gen/experimental-libraries.cc
  CXX(target)
/home/dpino/workspace/v8/out/x64.release/obj.target/v8_nosnapshot/gen/libraries.o
  CXX(target)
/home/dpino/workspace/v8/out/x64.release/obj.target/v8_nosnapshot/gen/experimental-libraries.o
  AR(target)
/home/dpino/workspace/v8/out/x64.release/obj.target/tools/gyp/libv8_nosnapshot.a
  LINK(target) /home/dpino/workspace/v8/out/x64.release/mksnapshot
  ACTION tools_gyp_v8_gyp_v8_snapshot_target_run_mksnapshot
/home/dpino/workspace/v8/out/x64.release/obj.target/v8_snapshot/geni/snapshot.cc
Exception thrown during bootstrapping
Extension or internal compilation error.
Failing script:
    1: 

#
# Fatal error in ../src/mksnapshot.cc, line 143
# CHECK(blob.data) failed
#

==== C stack trace ===============================

 1: ??
 2: ??
 3: __libc_start_main
 4: ??
Illegal instruction (core dumped)
make[1]: ***
[/home/dpino/workspace/v8/out/x64.release/obj.target/v8_snapshot/geni/snapshot.cc]
Error 132
make[1]: Leaving directory `/home/dpino/workspace/v8/out'
make: *** [x64.release] Error 2

[rossberg, 2014-12-11 14:31:47]

I'm sorry about that. There seem to be some assumptions about builtins being
immutable somewhere in V8, although I couldn't figure out where exactly. Here is
an easy alternative that works for me:

diff --git a/src/runtime.js b/src/runtime.js
index 4bc377c..f4beadb 100644
--- a/src/runtime.js
+++ b/src/runtime.js
@@ -567,7 +567,10 @@ function ToInteger(x) {
 
 
 // ES6, draft 08-24-14, section 7.1.15
+// TODO(rossberg): Remove use of flag once --harmony-arrays is gone.
+ToLength.harmony = false;
 function ToLength(arg) {
+  if (!ToLength.harmony) return TO_UINT32(x);
   arg = ToInteger(arg);
   if (arg < 0) return 0;
   return arg < $Number.MAX_SAFE_INTEGER ? arg : $Number.MAX_SAFE_INTEGER;
diff --git a/src/harmony-array.js b/src/harmony-array.js
index 06fada7..7121856 100644
--- a/src/harmony-array.js
+++ b/src/harmony-array.js
@@ -145,6 +145,9 @@ function ArrayOf() {
 function HarmonyArrayExtendArrayPrototype() {
   %CheckIsBootstrapping();
 
+  // Upgrade index conversion semantics to ES6.
+  ToLength.harmony = true;
+
   // Set up non-enumerable functions on the Array object.
   InstallFunctions($Array, DONT_ENUM, $Array(
     "of", ArrayOf

[caitp (gmail), 2014-12-11 14:38:23]

On 2014/12/11 14:31:47, rossberg wrote:
> I'm sorry about that. There seem to be some assumptions about builtins being
> immutable somewhere in V8, although I couldn't figure out where exactly. Here
is
> an easy alternative that works for me:
> 
> diff --git a/src/runtime.js b/src/runtime.js
> index 4bc377c..f4beadb 100644
> --- a/src/runtime.js
> +++ b/src/runtime.js
> @@ -567,7 +567,10 @@ function ToInteger(x) {
>  
>  
>  // ES6, draft 08-24-14, section 7.1.15
> +// TODO(rossberg): Remove use of flag once --harmony-arrays is gone.
> +ToLength.harmony = false;
>  function ToLength(arg) {
> +  if (!ToLength.harmony) return TO_UINT32(x);
>    arg = ToInteger(arg);
>    if (arg < 0) return 0;
>    return arg < $Number.MAX_SAFE_INTEGER ? arg : $Number.MAX_SAFE_INTEGER;
> diff --git a/src/harmony-array.js b/src/harmony-array.js
> index 06fada7..7121856 100644
> --- a/src/harmony-array.js
> +++ b/src/harmony-array.js
> @@ -145,6 +145,9 @@ function ArrayOf() {
>  function HarmonyArrayExtendArrayPrototype() {
>    %CheckIsBootstrapping();
>  
> +  // Upgrade index conversion semantics to ES6.
> +  ToLength.harmony = true;
> +
>    // Set up non-enumerable functions on the Array object.
>    InstallFunctions($Array, DONT_ENUM, $Array(
>      "of", ArrayOf

--harmony-tostring has some of the behaviour that I suggested before --- so it's
definitely working there. would have to look and see why it's broken here.

[Diego Pino, 2014-12-12 09:07:42]

Patchset #9 (id:270001) has been deleted

[Diego Pino, 2014-12-12 09:25:38]

On 2014/12/11 14:38:23, caitp wrote:
> On 2014/12/11 14:31:47, rossberg wrote:
> > I'm sorry about that. There seem to be some assumptions about builtins being
> > immutable somewhere in V8, although I couldn't figure out where exactly.
Here
> is
> > an easy alternative that works for me:
> > 
> --harmony-tostring has some of the behaviour that I suggested before --- so
it's
> definitely working there. would have to look and see why it's broken here.

Actually the change seemed good, but unfortunately didn't work :(

I undid the addition of a global flag and applied rossberg suggestion. CL is
rebased and updated. Please take a look.

[rossberg, 2014-12-12 11:47:12]

lgtm

[arv (Not doing code reviews), 2014-12-12 14:16:04]

LGTM

https://codereview.chromium.org/553623004/diff/290001/test/mjsunit/harmony/ar...
File test/mjsunit/harmony/array-fill.js (right):

https://codereview.chromium.org/553623004/diff/290001/test/mjsunit/harmony/ar...
test/mjsunit/harmony/array-fill.js:40: Array.prototype.fill.apply(o, ["foo", 0,
9]);
These could have used call instead of apply. Makes the code slightly cleaner.

[Diego Pino, 2014-12-14 21:09:30]

Patchset #9 (id:290001) has been deleted

[Diego Pino, 2014-12-14 22:24:23]

Patchset #9 (id:310001) has been deleted

[Diego Pino, 2014-12-18 17:11:53]

Patchset #10 (id:350001) has been deleted

[Diego Pino, 2014-12-19 11:03:25]

'v8_linux_rel' is failing. The problem seems to be that there are some tests
executed with the --harmony flag, but their expected result is not correct when
using harmony's ToLength.

For instance, the following test
(test/test262-es6/data/test/suite/ch15/15.4/15.4.4/15.4.4.14/15.4.4.14-3-8.js):

function testcase() {
   var obj = { 0: 0, length: Infinity };

   return Array.prototype.indexOf.call(obj, 0) === -1;
}

Returns -1 in master (as TO_UINT32(Infinity) returns 0 and if length is zero,
indexOf() returns -1). However, ToLength(Infinity) in harmony returns Infinity
and in consequence indexOf(obj, 0) returns 0.

This suite is not part of v8 codebase. I don't know what should I do? Would it
be possible to skip the failing test in Harmony?

[Dmitry Lomov (no reviews), 2014-12-19 11:12:01]

On 2014/12/19 11:03:25, Diego Pino wrote:
> 'v8_linux_rel' is failing. The problem seems to be that there are some tests
> executed with the --harmony flag, but their expected result is not correct
when
> using harmony's ToLength.
> 
> For instance, the following test
>
(test/test262-es6/data/test/suite/ch15/15.4/15.4.4/15.4.4.14/15.4.4.14-3-8.js):
> 
> function testcase() {
>    var obj = { 0: 0, length: Infinity };
> 
>    return Array.prototype.indexOf.call(obj, 0) === -1;
> }
> 
> Returns -1 in master (as TO_UINT32(Infinity) returns 0 and if length is zero,
> indexOf() returns -1). However, ToLength(Infinity) in harmony returns Infinity
> and in consequence indexOf(obj, 0) returns 0.
> 
> This suite is not part of v8 codebase. I don't know what should I do? Would it
> be possible to skip the failing test in Harmony?
See test/test262-es6/test262-es6.status

[Diego Pino, 2015-01-08 18:15:45]

Patchset #12 (id:410001) has been deleted

[Diego Pino, 2015-01-12 11:21:58]

Patchset #12 (id:430001) has been deleted

[Diego Pino, 2015-01-12 14:43:00]

Patchset #12 (id:340006) has been deleted

[Diego Pino, 2015-01-20 17:51:24]

Patchset #12 (id:460001) has been deleted

[Diego Pino, 2015-02-27 03:29:56]

Patchset #13 (id:500001) has been deleted

[Diego Pino, 2015-02-27 04:27:36]

Patchset #13 (id:520001) has been deleted

[aperez, 2015-08-28 23:31:43]

I have agreed with Diego to take over his patch, and I have an updated CL posted
(unfortunately I am not the owner of this one, so I cannot update it...), here
it
goes:

  https://codereview.chromium.org/1309243003