[regexp] implement case-insensitive unicode regexps.

[regexp] implement case-insensitive unicode regexps.

BUG=v8:2952
LOG=N

Committed: https://crrev.com/a2baaaac93efbdd3f2e19e66907a39a7197b8305
Cr-Commit-Position: refs/heads/master@{#33538}

[Yang, 2016-01-19 10:35:16]

yangguo@chromium.org changed reviewers:
+ littledan@chromium.org

[Yang, 2016-01-19 10:35:45]

On 2016/01/19 10:35:17, Yang wrote:

This depends on https://codereview.chromium.org/1578253005/

[Yang, 2016-01-19 11:00:45]

Description was changed from

==========
[regexp] implement case-insensitive unicode regexps.

BUG=v8:2952
LOG=N
==========

to

==========
[regexp] implement case-insensitive unicode regexps.

BUG=v8:2952
LOG=N
==========

[Yang, 2016-01-19 11:00:45]

yangguo@chromium.org changed reviewers:
+ erik.corry@gmail.com

[Yang, 2016-01-21 13:30:06]

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

[commit-bot: I haz the power, 2016-01-21 13:30:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1599303002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1599303002/60001

[commit-bot: I haz the power, 2016-01-21 13:33:34]

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

[commit-bot: I haz the power, 2016-01-21 13:33:35]

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

[Yang, 2016-01-22 07:50:08]

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

[commit-bot: I haz the power, 2016-01-22 07:50:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1599303002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1599303002/120001

[commit-bot: I haz the power, 2016-01-22 08:45:42]

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

[commit-bot: I haz the power, 2016-01-22 08:45:42]

Dry run: This issue passed the CQ dry run.

[Yang, 2016-01-25 07:39:30]

On 2016/01/22 08:45:42, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

Erik, do you want to review this next? :)

[erikcorry, 2016-01-25 10:26:36]

erikcorry@google.com changed reviewers:
+ erikcorry@google.com

[erikcorry, 2016-01-25 10:26:37]

lgtm

https://codereview.chromium.org/1599303002/diff/120001/src/regexp/regexp-macr...
File src/regexp/regexp-macro-assembler-tracer.cc (right):

https://codereview.chromium.org/1599303002/diff/120001/src/regexp/regexp-macr...
src/regexp/regexp-macro-assembler-tracer.cc:364: PrintF("
CheckNotBackReferenceIgnoreCase(register=%d, %s, label[%08x]);\n",
This printf should print whether it is in unicode mode.

https://codereview.chromium.org/1599303002/diff/120001/src/regexp/regexp-macr...
File src/regexp/regexp-macro-assembler.cc (right):

https://codereview.chromium.org/1599303002/diff/120001/src/regexp/regexp-macr...
src/regexp/regexp-macro-assembler.cc:49: if
(unibrow::Utf16::IsLeadSurrogate(c1)) {
This looks incomplete wrt unpaired surrogates at the ends of the capture.

https://codereview.chromium.org/1599303002/diff/120001/src/regexp/regexp-macr...
src/regexp/regexp-macro-assembler.cc:68: #endif  // V8_I18N_SUPPORT
Should this be a #else ?

https://codereview.chromium.org/1599303002/diff/120001/src/regexp/regexp-pars...
File src/regexp/regexp-parser.cc (right):

https://codereview.chromium.org/1599303002/diff/120001/src/regexp/regexp-pars...
src/regexp/regexp-parser.cc:1164: if
(NeedsDesugaringForUnicode(cc->ranges(zone()))) {
It's a bit unfortunate, that for all the standard ranges, like \s and \d, this
will cause an allocation, just to determine that we don't need desugaring.

https://codereview.chromium.org/1599303002/diff/120001/src/regexp/regexp-pars...
src/regexp/regexp-parser.cc:1236: static const uc32 kNonBmpStart = 0x10000;
Don't these constants already exist?

https://codereview.chromium.org/1599303002/diff/120001/src/regexp/regexp-pars...
src/regexp/regexp-parser.cc:1250: #ifdef V8_I18N_SUPPORT
Perhaps a comment explaining what happens in the non-ICU case - hopefully
something sane.

https://codereview.chromium.org/1599303002/diff/120001/src/regexp/regexp-pars...
src/regexp/regexp-parser.cc:1252: if (c < 128) return false;
There's a named constant for this.

[Yang, 2016-01-25 11:46:37]

Also added test for no-i18n builds.

https://codereview.chromium.org/1599303002/diff/120001/src/regexp/regexp-macr...
File src/regexp/regexp-macro-assembler-tracer.cc (right):

https://codereview.chromium.org/1599303002/diff/120001/src/regexp/regexp-macr...
src/regexp/regexp-macro-assembler-tracer.cc:364: PrintF("
CheckNotBackReferenceIgnoreCase(register=%d, %s, label[%08x]);\n",
On 2016/01/25 10:26:37, erikcorry wrote:
> This printf should print whether it is in unicode mode.

Done.

https://codereview.chromium.org/1599303002/diff/120001/src/regexp/regexp-macr...
File src/regexp/regexp-macro-assembler.cc (right):

https://codereview.chromium.org/1599303002/diff/120001/src/regexp/regexp-macr...
src/regexp/regexp-macro-assembler.cc:49: if
(unibrow::Utf16::IsLeadSurrogate(c1)) {
On 2016/01/25 10:26:37, erikcorry wrote:
> This looks incomplete wrt unpaired surrogates at the ends of the capture.

I think this is correct. Unpaired lead surrogate at the end of the capture would
mean that we exceed |length| when we try to fetch the trail surrogate below. So
the condition on line 51 fails, so we fall through to u_foldCase.
If we have a trail surrogate, there is nothing to combine to anyways.

https://codereview.chromium.org/1599303002/diff/120001/src/regexp/regexp-macr...
src/regexp/regexp-macro-assembler.cc:68: #endif  // V8_I18N_SUPPORT
On 2016/01/25 10:26:37, erikcorry wrote:
> Should this be a #else ?

No. If we have ICU, we decide on whether to take the unicode path based on
isolate == nullptr. If isolate is not null, we still have to do the non-unicode
thing, which is below.
If we do not have ICU, the only thing we can do is the non-unicode thing.

https://codereview.chromium.org/1599303002/diff/120001/src/regexp/regexp-pars...
File src/regexp/regexp-parser.cc (right):

https://codereview.chromium.org/1599303002/diff/120001/src/regexp/regexp-pars...
src/regexp/regexp-parser.cc:1164: if
(NeedsDesugaringForUnicode(cc->ranges(zone()))) {
On 2016/01/25 10:26:37, erikcorry wrote:
> It's a bit unfortunate, that for all the standard ranges, like \s and \d, this
> will cause an allocation, just to determine that we don't need desugaring.

Fixed.

https://codereview.chromium.org/1599303002/diff/120001/src/regexp/regexp-pars...
src/regexp/regexp-parser.cc:1236: static const uc32 kNonBmpStart = 0x10000;
On 2016/01/25 10:26:37, erikcorry wrote:
> Don't these constants already exist?

Done.

https://codereview.chromium.org/1599303002/diff/120001/src/regexp/regexp-pars...
src/regexp/regexp-parser.cc:1250: #ifdef V8_I18N_SUPPORT
On 2016/01/25 10:26:37, erikcorry wrote:
> Perhaps a comment explaining what happens in the non-ICU case - hopefully
> something sane.

Done.

https://codereview.chromium.org/1599303002/diff/120001/src/regexp/regexp-pars...
src/regexp/regexp-parser.cc:1252: if (c < 128) return false;
On 2016/01/25 10:26:37, erikcorry wrote:
> There's a named constant for this.

Done.

[erikcorry, 2016-01-25 11:57:47]

Still LGTM

https://codereview.chromium.org/1599303002/diff/120001/src/regexp/regexp-macr...
File src/regexp/regexp-macro-assembler.cc (right):

https://codereview.chromium.org/1599303002/diff/120001/src/regexp/regexp-macr...
src/regexp/regexp-macro-assembler.cc:49: if
(unibrow::Utf16::IsLeadSurrogate(c1)) {
On 2016/01/25 11:46:37, Yang wrote:
> On 2016/01/25 10:26:37, erikcorry wrote:
> > This looks incomplete wrt unpaired surrogates at the ends of the capture.
> 
> I think this is correct. Unpaired lead surrogate at the end of the capture
would
> mean that we exceed |length| when we try to fetch the trail surrogate below.
So
> the condition on line 51 fails, so we fall through to u_foldCase.
> If we have a trail surrogate, there is nothing to combine to anyways.

I guess we are here making use of the fact that there are no case-equivalency
classes that cross the basic plane boundary?

It would be good to have some test cases that check that we don't allow
/([^]*)[^]*\1/ to match thing like:

".L.LT."
".T.LT."
".LT.L."
".LT.T."

Including variations without the leading or trailing dots.

[Yang, 2016-01-25 12:09:02]

https://codereview.chromium.org/1599303002/diff/120001/src/regexp/regexp-macr...
File src/regexp/regexp-macro-assembler.cc (right):

https://codereview.chromium.org/1599303002/diff/120001/src/regexp/regexp-macr...
src/regexp/regexp-macro-assembler.cc:49: if
(unibrow::Utf16::IsLeadSurrogate(c1)) {
On 2016/01/25 11:57:47, erikcorry wrote:
> On 2016/01/25 11:46:37, Yang wrote:
> > On 2016/01/25 10:26:37, erikcorry wrote:
> > > This looks incomplete wrt unpaired surrogates at the ends of the capture.
> > 
> > I think this is correct. Unpaired lead surrogate at the end of the capture
> would
> > mean that we exceed |length| when we try to fetch the trail surrogate below.
> So
> > the condition on line 51 fails, so we fall through to u_foldCase.
> > If we have a trail surrogate, there is nothing to combine to anyways.
> 
> I guess we are here making use of the fact that there are no case-equivalency
> classes that cross the basic plane boundary?
> 
> It would be good to have some test cases that check that we don't allow
> /([^]*)[^]*\1/ to match thing like:
> 
> ".L.LT."
> ".T.LT."
> ".LT.L."
> ".LT.T."
> 
> Including variations without the leading or trailing dots.

I added a comment about the fact that we rely on Non-BMP to only be case
equivalent to other non-BMP.

But these back reference tests would fail since we do not check whether the back
reference starts/ends in a surrogate pair here.

https://codereview.chromium.org/1601653006/ is supposed to solve this issue, and
I'll add some more tests there.

[Yang, 2016-01-25 12:09:49]

The CQ bit was checked by yangguo@chromium.org

[commit-bot: I haz the power, 2016-01-25 12:09:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1599303002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1599303002/140001

[commit-bot: I haz the power, 2016-01-25 12:13:19]

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

[commit-bot: I haz the power, 2016-01-25 12:13:19]

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

[Yang, 2016-01-25 13:29:14]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2016-01-25 13:29:14]

The patchset sent to the CQ was uploaded after l-g-t-m from erikcorry@google.com
Link to the patchset: https://codereview.chromium.org/1599303002/#ps160001
(title: "rebase")

[commit-bot: I haz the power, 2016-01-25 13:29:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1599303002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1599303002/160001

[commit-bot: I haz the power, 2016-01-25 13:30:05]

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

[commit-bot: I haz the power, 2016-01-25 13:30:06]

Try jobs failed on following builders:
  v8_linux64_asan_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/...)
  v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1...)
  v8_linux_arm_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel/builds/12788)
  v8_linux_dbg_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/443)
  v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_mac_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/14492)

[Yang, 2016-01-25 14:15:48]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2016-01-25 14:15:49]

The patchset sent to the CQ was uploaded after l-g-t-m from erikcorry@google.com
Link to the patchset: https://codereview.chromium.org/1599303002/#ps180001
(title: "rebase")

[commit-bot: I haz the power, 2016-01-25 14:15:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1599303002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1599303002/180001

[commit-bot: I haz the power, 2016-01-25 14:18:37]

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

[commit-bot: I haz the power, 2016-01-25 14:18:38]

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

[Yang, 2016-01-25 14:28:40]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2016-01-25 14:28:40]

The patchset sent to the CQ was uploaded after l-g-t-m from erikcorry@google.com
Link to the patchset: https://codereview.chromium.org/1599303002/#ps200001
(title: "remove special handling of ASCII characters")

[commit-bot: I haz the power, 2016-01-25 14:28:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1599303002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1599303002/200001

[commit-bot: I haz the power, 2016-01-25 14:48:55]

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

[commit-bot: I haz the power, 2016-01-25 14:48:56]

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

[Yang, 2016-01-27 07:57:02]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2016-01-27 07:57:03]

The patchset sent to the CQ was uploaded after l-g-t-m from erikcorry@google.com
Link to the patchset: https://codereview.chromium.org/1599303002/#ps220001
(title: "fixes")

[commit-bot: I haz the power, 2016-01-27 07:57:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1599303002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1599303002/220001

[commit-bot: I haz the power, 2016-01-27 08:24:54]

Description was changed from

==========
[regexp] implement case-insensitive unicode regexps.

BUG=v8:2952
LOG=N
==========

to

==========
[regexp] implement case-insensitive unicode regexps.

BUG=v8:2952
LOG=N
==========

[commit-bot: I haz the power, 2016-01-27 08:24:55]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-01-27 08:25:46]

Description was changed from

==========
[regexp] implement case-insensitive unicode regexps.

BUG=v8:2952
LOG=N
==========

to

==========
[regexp] implement case-insensitive unicode regexps.

BUG=v8:2952
LOG=N

Committed: https://crrev.com/a2baaaac93efbdd3f2e19e66907a39a7197b8305
Cr-Commit-Position: refs/heads/master@{#33538}
==========

[commit-bot: I haz the power, 2016-01-27 08:25:47]

Patchset 12 (id:??) landed as
https://crrev.com/a2baaaac93efbdd3f2e19e66907a39a7197b8305
Cr-Commit-Position: refs/heads/master@{#33538}

[brucedawson, 2016-01-29 18:49:14]

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

[brucedawson, 2016-01-29 18:49:15]

https://codereview.chromium.org/1599303002/diff/220001/src/regexp/jsregexp.cc
File src/regexp/jsregexp.cc (right):

https://codereview.chromium.org/1599303002/diff/220001/src/regexp/jsregexp.cc...
src/regexp/jsregexp.cc:5891: // If this is a singleton we just expand the one
character.
The indenting of this line - and the rest of the function - is now incorrect
because the entire function was wrapped in a for loop. This makes the code very
misleading to read.

I noticed this because a subsequent change added another 'range' variable which
shadows the existing one but the indenting makes this shadowing almost
invisible.

[Yang, 2016-02-01 07:21:35]

On 2016/01/29 18:49:15, brucedawson wrote:
> https://codereview.chromium.org/1599303002/diff/220001/src/regexp/jsregexp.cc
> File src/regexp/jsregexp.cc (right):
> 
>
https://codereview.chromium.org/1599303002/diff/220001/src/regexp/jsregexp.cc...
> src/regexp/jsregexp.cc:5891: // If this is a singleton we just expand the one
> character.
> The indenting of this line - and the rest of the function - is now incorrect
> because the entire function was wrapped in a for loop. This makes the code
very
> misleading to read.
> 
> I noticed this because a subsequent change added another 'range' variable
which
> shadows the existing one but the indenting makes this shadowing almost
> invisible.

Thanks for pointing that out!