1230490 - make_unicode.py failing on unicode > 6.0.0

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Morgan Phillips [:mrrrgn]]

Running make_unicode.py gives the error: 

Downloading...
Generating...
Best: 2048+8224 bins at shift 5; 12320 bytes
Size of original table: 65536 bytes
Traceback (most recent call last):
  File "make_unicode.py", line 377, in <module>
    open('../tests/ecma_5/String/string-space-trim.js', 'w'))
  File "make_unicode.py", line 282, in generate_unicode_stuff
    dump(index1, 'index1', data_file)
  File "make_unicode.py", line 268, in dump
    assert entry < 256
AssertionError

On all versions of unicode > 6.0.0 but works fine on versions <= 6.0.0

hg shows that the last modification of Unicode.cpp happened right around the time that unicode 7.0.0 was *announced* so I'm guessing this isn't an issue particular to my machine.

Comment 1

[Wei Wu [:wuwei UTC+8]]

(In reply to Morgan Phillips [:mrrrgn] from comment #0)
> Running make_unicode.py gives the error: 
> 
> Downloading...
> Generating...
> Best: 2048+8224 bins at shift 5; 12320 bytes
> Size of original table: 65536 bytes
> Traceback (most recent call last):
>   File "make_unicode.py", line 377, in <module>
>     open('../tests/ecma_5/String/string-space-trim.js', 'w'))
>   File "make_unicode.py", line 282, in generate_unicode_stuff
>     dump(index1, 'index1', data_file)
>   File "make_unicode.py", line 268, in dump
>     assert entry < 256
> AssertionError
see: https://hg.mozilla.org/mozilla-central/rev/6f3285fc0d02 and bug 717501
change the type back to uint16_t might be feasible, if lucky.

Comment 2

[Morgan Phillips [:mrrrgn]]

Attachment: makeunicode.diff

Like Wei Wu suggested, I reverted https://hg.mozilla.org/mozilla-central/rev/6f3285fc0d02

Comment 3

[Tom S [:evilpie]]

Comment on attachment 8724919 [details] [diff] [review]
makeunicode.diff

Review of attachment 8724919 [details] [diff] [review]:
-----------------------------------------------------------------

Please split the changes to make_unicode.cpp and the update into two patches. Just out of curiosity, which codepoint(s) require this?

::: js/src/tests/ecma_5/String/string-space-trim.js
@@ -3,5 @@
>  /*
>   * Any copyright is dedicated to the Public Domain.
>   * http://creativecommons.org/licenses/publicdomain/
>   */
> -var onlySpace = String.fromCharCode(0x9, 0xa, 0xb, 0xc, 0xd, 0x20, 0xa0, 0x1680, 0x180e, 0x2000, 0x2001, 0x2002, 0x2003, 0x2004, 0x2005, 0x2006, 0x2007, 0x2008, 0x2009, 0x200a, 0x2028, 0x2029, 0x202f, 0x205f, 0x3000, 0xfeff);

So 0x180e was removed from the whitespace set? I am not up to date if that is correct per ECMAScript.

::: js/src/vm/Unicode.h
@@ +241,5 @@
>      uint16_t reverse2;
>      uint16_t reverse3;
>  };
>  
> +extern const uint16_t folding_index1[];

Doesn't look like these actually need to be uin16_t. Maybe special case by name and assert < 256 in make_unicode.

Comment 4

[Tom S [:evilpie]]

https://github.com/tc39/test262/pull/506 confirms the U+180E change.

Comment 5

[Mathias Bynens]

(In reply to Tom Schuster [:evilpie] from comment #4)
> https://github.com/tc39/test262/pull/506 confirms the U+180E change.

https://github.com/tc39/ecma262/pull/300#issuecomment-181376767

Comment 6

[Tooru Fujisawa [:arai]]

what's blocking this?
can the patch be landed?

Comment 7

[Morgan Phillips [:mrrrgn]]

(In reply to Tooru Fujisawa [:arai] from comment #6)
> what's blocking this?
> can the patch be landed?

Arg, the patch is rotten. I'll have to redo it and land. Apologies!

Comment 8

[Morgan Phillips [:mrrrgn]]

(In reply to Tooru Fujisawa [:arai] from comment #6)
> what's blocking this?
> can the patch be landed?

To be clear, nothing is blocking it (except bit-rot).

Comment 9

[Morgan Phillips [:mrrrgn]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=325b5a8dea2f

Comment 10

[Morgan Phillips [:mrrrgn]]

There is a problem, with the new make_unicode.py 

`Number("\u0009\u000C\u0020\u00A0\u000B\u000A\u000D\u2028\u2029\u1680\u180E\u2000\u2001\u2002\u2003\u2004\u2005\u2006\u2007\u2008\u2009\u200A\u202F\u205F\u3000")` returns NaN instead of 0

Need to address that before the patch can land.

Comment 11

[Tooru Fujisawa [:arai]]

Thank you :D

(In reply to Morgan Phillips [:mrrrgn] from comment #10)
> There is a problem, with the new make_unicode.py 
> 
> `Number("\u0009\u000C\u0020\u00A0\u000B\u000A\u000D\u2028\u2029\u1680\u180E\u
> 2000\u2001\u2002\u2003\u2004\u2005\u2006\u2007\u2008\u2009\u200A\u202F\u205F\
> u3000")` returns NaN instead of 0
> 
> Need to address that before the patch can land.

\u180E was removed from the testcase
https://github.com/tc39/test262/blob/master/test/built-ins/Number/S9.3.1_A3_T1.js
https://github.com/tc39/test262/commit/3a5a09eba2adb6a8c24cf6e603aefe9bfa8d75b9

and es6draft also returns NaN for previous testcase.
We just need to update the testcase :)

Comment 12

[Morgan Phillips [:mrrrgn]]

Nice,

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5aa29d00ae96

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3cf7753c4d5c
Allow 16 bit character codes 1/2; r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ee1aee9ddc0
Allow 16 bit character codes 2/2; r=evilpie

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3cf7753c4d5c
https://hg.mozilla.org/mozilla-central/rev/5ee1aee9ddc0

Comment 15

[André Bargull [:anba]]

Was backed out in bug 1296846.

Comment 16

[André Bargull [:anba]]

Attachment: bug1230490-part1.patch

Funny thing this code gets removed before it was even merged to inbound.

Applies on top of bug 917436.

Comment 17

[André Bargull [:anba]]

Attachment: bug1230490-part2.patch

I've changed the shift amount for CharInfo in make_unicode.py and Unicode.h from 5 to 6. All other changes are automatic updates from `./make_unicode.py --version=UNIDATA`.

Comment 18

[André Bargull [:anba]]

Attachment: bug1230490-part3.patch

These test262 tests expect that U+180E is a white space character, but that's no longer true for any Unicode version >= 6.3.0.

Comment 19

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1437478afac823e4bb562e1e511c4f2204c4ffe5

Comment 20

[André Bargull [:anba]]

And I have absolutely no idea why it's no longer required to change index tables from uint8_t to uint16_t (comment #0).

Comment 21

[Tooru Fujisawa [:arai]]

Comment on attachment 8808419 [details] [diff] [review]
bug1230490-part1.patch

Review of attachment 8808419 [details] [diff] [review]:
-----------------------------------------------------------------

Nice simplification :)

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f05a94693082
Part 1: Remove support for separate Unicode version for case-folding from make_unicode. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a1fa3068f56
Part 2: Update JS Unicode support to 9.0.0. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a2b9edaff2e
Part 3: Disable test262 tests which expect Unicode 5.1 is used. r=arai

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f05a94693082
https://hg.mozilla.org/mozilla-central/rev/4a1fa3068f56
https://hg.mozilla.org/mozilla-central/rev/1a2b9edaff2e