Update UTF-8 decoder to detect more special cases.

Update UTF-8 decoder to detect more special cases.

The blink version is stricter and for parsing it's important that both
decoders behave the same.

BUG=chromium:489944
R=vogelheim@chromium.org
LOG=n

Committed: https://crrev.com/3d5b2f807bb4f4cd7da56aae27a32bb0b2783fb2
Cr-Commit-Position: refs/heads/master@{#28601}

[jochen (gone - plz use gerrit), 2015-05-21 16:55:52]

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

[commit-bot: I haz the power, 2015-05-21 16:56:10]

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

[vogelheim, 2015-05-21 16:58:38]

I'm afraid this CL has left me largely confused.

https://codereview.chromium.org/1148653007/diff/1/src/unicode.cc
File src/unicode.cc (right):

https://codereview.chromium.org/1148653007/diff/1/src/unicode.cc#newcode205
src/unicode.cc:205: 4, 4, 4, 4, 4, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
The table is difficult to read. It also leaves me rather confused.

- I take it the purpose is to determine the length of an UTF-8 sequence from
it's first byte.
  I'm seeing: 128 x 0, 96 x 2, 16 x 3, 5 x 4, 11 x 0.
  Shouldn't this be: 192 x 0, 32 x 2, 16 x 3, 8 x 4, 8 x 0 ?

- In particular:
  - I don't understand why you count the continuation characters (0b10xxxxxx) as
'2'.
  - I don't understand why you count some of the 4-byte prefixes as
    0/error (0b11110101 + 0b1111011x)

- I'd find an 16x16 layout probably more readable.

https://codereview.chromium.org/1148653007/diff/1/src/unicode.cc#newcode210
src/unicode.cc:210: uchar Utf8::CalculateValue(const byte* str, size_t
max_length, size_t* cursor) {
I believe this deserves some commentary, and if only that it's meant to be
consistent with Blink's UTF decoder.

https://codereview.chromium.org/1148653007/diff/1/src/unicode.cc#newcode210
src/unicode.cc:210: uchar Utf8::CalculateValue(const byte* str, size_t
max_length, size_t* cursor) {
This might also benefit from a unit test that will try some of the edge cases
this is meant to fix.

https://codereview.chromium.org/1148653007/diff/1/src/unicode.cc#newcode217
src/unicode.cc:217: if (length == 2) {
I was trying to figure out *why* these characters are restricted. It seems
ECMAScript specs explicitly allow pretty much all of them. 

"All Unicode code point values from U+0000 to U+10FFFF, including surrogate code
points, may occur in source text where permitted by the ECMAScript grammars."
[From an ES6 draft. ES5 final has similar text.]

https://codereview.chromium.org/1148653007/diff/1/src/unicode.cc#newcode217
src/unicode.cc:217: if (length == 2) {
I find the code below to be somewhat confusing. If I get this correctly, the
code below either does the default UTF-8 processing, or for some 'special' code
point ranges it does additional checks.

Maybe an alternative would be to fold the dispatching of all those 'special'
checks into the length matrix above.

I.e., for the code block starting with 0xE0, make lengths[0xE0] == 0xE0, etc.
Then these blocks below would all look like this:

switch (NonASCIISequenceLength[str[0]) {
  case 0: ... error:
  case 2..4: normal UTF-8 processing
  case 0xE0: extra check, then UTF-8 processing.
  ....
  default: UNREACHABLE();
}

[commit-bot: I haz the power, 2015-05-21 17:10:41]

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

[commit-bot: I haz the power, 2015-05-21 17:10:42]

Dry run: 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/621)

[jochen (gone - plz use gerrit), 2015-05-22 12:38:13]

https://codereview.chromium.org/1148653007/diff/1/src/unicode.cc
File src/unicode.cc (right):

https://codereview.chromium.org/1148653007/diff/1/src/unicode.cc#newcode205
src/unicode.cc:205: 4, 4, 4, 4, 4, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
On 2015/05/21 at 16:58:38, vogelheim wrote:
> The table is difficult to read. It also leaves me rather confused.
> 
> - I take it the purpose is to determine the length of an UTF-8 sequence from
it's first byte.
>   I'm seeing: 128 x 0, 96 x 2, 16 x 3, 5 x 4, 11 x 0.
>   Shouldn't this be: 192 x 0, 32 x 2, 16 x 3, 8 x 4, 8 x 0 ?

I updated the matrix to be 16 x 16, and added comments
> 
> - In particular:
>   - I don't understand why you count the continuation characters (0b10xxxxxx)
as '2'.

fixed

>   - I don't understand why you count some of the 4-byte prefixes as
>     0/error (0b11110101 + 0b1111011x)

the code points those would encode are outside of the unicode range

> 
> - I'd find an 16x16 layout probably more readable.

done

https://codereview.chromium.org/1148653007/diff/1/src/unicode.cc#newcode210
src/unicode.cc:210: uchar Utf8::CalculateValue(const byte* str, size_t
max_length, size_t* cursor) {
On 2015/05/21 at 16:58:38, vogelheim wrote:
> I believe this deserves some commentary, and if only that it's meant to be
consistent with Blink's UTF decoder.

yeah, actually, it's supposed to be consistent with the definition of UTF-8
which the previous version was not :-/

there are tests, and they fail now. I'll update them

https://codereview.chromium.org/1148653007/diff/1/src/unicode.cc#newcode217
src/unicode.cc:217: if (length == 2) {
On 2015/05/21 at 16:58:38, vogelheim wrote:
> I was trying to figure out *why* these characters are restricted. It seems
ECMAScript specs explicitly allow pretty much all of them. 
> 
> "All Unicode code point values from U+0000 to U+10FFFF, including surrogate
code points, may occur in source text where permitted by the ECMAScript
grammars." [From an ES6 draft. ES5 final has similar text.]

right. It's just that UTF-8 cannot encode all of unicode, in particular not
U+D800 through U+DFFF.

https://codereview.chromium.org/1148653007/diff/40001/test/cctest/test-api.cc
File test/cctest/test-api.cc (left):

https://codereview.chromium.org/1148653007/diff/40001/test/cctest/test-api.cc...
test/cctest/test-api.cc:7334: WriteUtf8Helper(context, "b", "alens", 9);
these tests were testing what happens when converting strings with unmatched
surrogate pairs back and forth between utf8 and utf16. now that we no longer
support surrogate pairs in utf8, those tests don't make sense anymore

https://codereview.chromium.org/1148653007/diff/40001/test/cctest/test-api.cc...
test/cctest/test-api.cc:7368: "\355\240\201\355\260\205");  // 2 3-byte
surrogates.
same here.

if you convert the test to utf-16, it's a bit pointless as there are no two
alternative encodings for this code point, it's always an surrogate pair.

[vogelheim, 2015-05-22 15:53:39]

lgtm

https://codereview.chromium.org/1148653007/diff/40001/src/unicode.cc
File src/unicode.cc (right):

https://codereview.chromium.org/1148653007/diff/40001/src/unicode.cc#newcode229
src/unicode.cc:229: uchar Utf8::CalculateValue(const byte* str, size_t
max_length, size_t* cursor) {
Not sure if worth the effort, but this might be more compact if written like
this:

  uchar c = kBadChar;
  size_t length = NonASCIISequenceLength(str[0]);
  the if/switch-es would then only handle the success cases and update c;
  *cursor += (c == kBadChar) ? 1 : length;
  return c;

I guess it's a matter of taste. You pick.

https://codereview.chromium.org/1148653007/diff/40001/src/unicode.cc#newcode282
src/unicode.cc:282: // Code poits outside of the unicode range.
poits -> points

[jochen (gone - plz use gerrit), 2015-05-22 18:12:39]

https://codereview.chromium.org/1148653007/diff/40001/src/unicode.cc
File src/unicode.cc (right):

https://codereview.chromium.org/1148653007/diff/40001/src/unicode.cc#newcode229
src/unicode.cc:229: uchar Utf8::CalculateValue(const byte* str, size_t
max_length, size_t* cursor) {
On 2015/05/22 at 15:53:38, vogelheim wrote:
> Not sure if worth the effort, but this might be more compact if written like
this:
> 
>   uchar c = kBadChar;
>   size_t length = NonASCIISequenceLength(str[0]);
>   the if/switch-es would then only handle the success cases and update c;
>   *cursor += (c == kBadChar) ? 1 : length;
>   return c;
> 
> I guess it's a matter of taste. You pick.

then I pick this version :)

[jochen (gone - plz use gerrit), 2015-05-22 18:12:46]

The CQ bit was checked by jochen@chromium.org

[jochen (gone - plz use gerrit), 2015-05-22 18:12:46]

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

[commit-bot: I haz the power, 2015-05-22 18:13:07]

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

[commit-bot: I haz the power, 2015-05-22 18:47:50]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-05-22 18:47:54]

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