Fix limit calculation in String.prototype.split

Fix limit calculation in String.prototype.split

We are supposed to calculate the limit by using 2^53-1 if it
is undefined, and ToLength otherwise. Instead, we were using
32-bit unsigned integer values, which don't make sense and
were breaking a test.

R=littledan@chromium.org
LOG=N
BUG=v8:4245

[arv (Not doing code reviews), 2015-07-10 16:24:52]

You mentioned that we are breaking a test? Which one? Do you need to update
test262-es6.status as well?

Also, can you add a test for this unless this is already covered by existing
tests (test262-es6 test is sufficient)

[Eric Caruso, 2015-07-10 16:57:29]

On 2015/07/10 16:24:52, arv (Not doing code reviews) wrote:
> You mentioned that we are breaking a test? Which one? Do you need to update
> test262-es6.status as well?
> 
> Also, can you add a test for this unless this is already covered by existing
> tests (test262-es6 test is sufficient)

We're failing built-ins/String/prototype/split/S15.5.4.14_A2_T37 as shown in
the bug.

I'll update test262-es6.status.

[Dan Ehrenberg, 2015-07-13 23:19:13]

This is great. Just one little fix needed.

https://codereview.chromium.org/1226143009/diff/20001/src/string.js
File src/string.js (right):

https://codereview.chromium.org/1226143009/diff/20001/src/string.js#newcode612
src/string.js:612: limit = IS_UNDEFINED(limit) ? GlobalNumber.MAX_SAFE_INTEGER
Defined this way, a user could modify Number.MAX_SAFE_INTEGER and change the
behavior of String.prototype.split, which violates the spec. Instead, you can
copy that value into another variable, so that it's evaluated at bootstrap time
before a user messes everything up.

[adamk, 2015-07-14 00:16:48]

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

[adamk, 2015-07-14 00:16:49]

https://codereview.chromium.org/1226143009/diff/20001/src/string.js
File src/string.js (right):

https://codereview.chromium.org/1226143009/diff/20001/src/string.js#newcode612
src/string.js:612: limit = IS_UNDEFINED(limit) ? GlobalNumber.MAX_SAFE_INTEGER
On 2015/07/13 23:19:13, Dan Ehrenberg wrote:
> Defined this way, a user could modify Number.MAX_SAFE_INTEGER and change the
> behavior of String.prototype.split, which violates the spec. Instead, you can
> copy that value into another variable, so that it's evaluated at bootstrap
time
> before a user messes everything up.

MAX_SAFE_INTEGER is non-configurable, non-writable, and GlobalNumber is
guaranteed to the be the Number constructor at bootup time, so I think this is
actually safe.

[Dan Ehrenberg, 2015-07-15 00:02:32]

lgtm

Oh, right.

[adamk, 2015-07-15 00:04:32]

lgtm

[Eric Caruso, 2015-07-15 17:28:46]

The CQ bit was checked by ejcaruso@chromium.org

[commit-bot: I haz the power, 2015-07-15 17:29:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1226143009/20001

[commit-bot: I haz the power, 2015-07-15 17:38:18]

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

[commit-bot: I haz the power, 2015-07-15 17:38:18]

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

(exceeded global retry quota)

[Eric Caruso, 2015-07-15 17:51:56]

On 2015/07/15 17:38:18, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)
> 
> (exceeded global retry quota)

The failure is on the following assertion:

assertArrayEquals(["as","fas","fas","f"], "asdfasdfasdf".split("d", -1));

[10:42] v8 $ cat test/mjsunit/{mjsunit,string-split}.js > ../string-split.js
[10:43] v8 $ out/Debug/d8 ../string-split.js 
../string-split.js:178: Failure (array length): expected <4> found <0>
    throw new MjsUnitAssertionError(message);
    ^
Error
    at new MjsUnitAssertionError (../string-split.js:31:16)
    at fail (../string-split.js:178:11)
    at assertEquals (../string-split.js:248:7)
    at assertArrayEquals (../string-split.js:264:5)
    at ../string-split.js:440:1

This seems like an incorrect failure now. ToLength is supposed to clamp negative
numbers to zero, and a zero limit should return an empty array. I suppose the
ES6 spec changed the behavior in this instance?

[adamk, 2015-07-15 22:56:36]

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

[adamk, 2015-07-15 22:56:36]

Yes, per spec, this test is now wrong. I wondered at first why this was changed,
but it seems to be related to allowing Strings to have lengths up to 2^53-1 in
ES6. This particular bit of spec text dates to rev20 of ES6:

https://bugs.ecmascript.org/show_bug.cgi?id=1858
http://wiki.ecmascript.org/lib/exe/fetch.php?id=harmony%3Aspecification_draft...

Firefox doesn't currently implement it this way, though. And for positive
limits, I doubt any engine will be able to produce varying results, since it
would require allocating a string with length > 2^32.

It seems to me this change has a slightly higher chance of breaking existing
code than most other ToUint32 -> ToLength changes in the spec, since the
passed-in argument isn't the "length" property of any object. I'm not sure the
risk is worth the reward (passing one test262 test). But it's also such a small
thing that wrapping it with a flag seems hardly worth the effort. I'm curious
for rossberg's thoughts on the matter of minor spec incompatibilities like this
one.

[adamk, 2015-07-15 22:59:01]

The broader question I have for rossberg is something like: why did TC39 make
ToLength have different behavior for negative values than ToUint32?

[rossberg, 2015-07-16 10:45:40]

On 2015/07/15 22:59:01, adamk wrote:
> The broader question I have for rossberg is something like: why did TC39 make
> ToLength have different behavior for negative values than ToUint32?

The wrap-around semantics of ToUint is completely crazy, and would be even
crazier when applied to random ranges like 0..2^53. Using -1 as a (non-)limit
used to work more by accident than by design. But admittedly, the crop semantics
of ToLength isn't great either. It would be best to throw, but that might have
broken too much code. Just guessing, I'm afraid I don't know the exact
motivation.