make_unicode.py failing on unicode > 6.0.0

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: mrrrgn, Assigned: anba)

Tracking

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
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.
(Reporter)

Updated

3 years ago
Blocks: 917436
(Reporter)

Updated

3 years ago
Assignee: nobody → winter2718
(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.
(Reporter)

Comment 2

3 years ago
Posted patch makeunicode.diff (obsolete) — Splinter Review
Like Wei Wu suggested, I reverted https://hg.mozilla.org/mozilla-central/rev/6f3285fc0d02
Attachment #8724919 - Flags: review?(evilpies)
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.
Attachment #8724919 - Flags: review?(evilpies) → review+
See Also: → 1280046

Comment 5

3 years ago
(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
what's blocking this?
can the patch be landed?
(Reporter)

Comment 7

3 years ago
(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!
(Reporter)

Comment 8

3 years ago
(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).
(Reporter)

Comment 10

3 years ago
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.
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 13

3 years ago
Pushed by mphillips@mozilla.com:
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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3cf7753c4d5c
https://hg.mozilla.org/mozilla-central/rev/5ee1aee9ddc0
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(Assignee)

Comment 15

3 years ago
Was backed out in bug 1296846.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 16

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

Applies on top of bug 917436.
Assignee: winter2718 → andrebargull
Attachment #8724919 - Attachment is obsolete: true
Status: REOPENED → ASSIGNED
Attachment #8808419 - Flags: review?(arai.unmht)
(Assignee)

Comment 17

3 years ago
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`.
Attachment #8808422 - Flags: review?(arai.unmht)
(Assignee)

Comment 18

3 years ago
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.
Attachment #8808423 - Flags: review?(arai.unmht)
(Assignee)

Comment 20

3 years ago
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 on attachment 8808419 [details] [diff] [review]
bug1230490-part1.patch

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

Nice simplification :)
Attachment #8808419 - Flags: review?(arai.unmht) → review+
Attachment #8808422 - Flags: review?(arai.unmht) → review+
Attachment #8808423 - Flags: review?(arai.unmht) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
Target Milestone: mozilla51 → ---

Comment 22

3 years ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed

Comment 23

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f05a94693082
https://hg.mozilla.org/mozilla-central/rev/4a1fa3068f56
https://hg.mozilla.org/mozilla-central/rev/1a2b9edaff2e
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52

Updated

2 years ago
Depends on: 1347869
You need to log in before you can comment on or make changes to this bug.