1026438 - Make irregexp work with Latin1 strings

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Pass HandleLinearString

Pass HandleLinearString instead of chars + length, to work better with Latin1 strings and moving GC.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Sampling

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Store Latin1/TwoByte code separately

Allow storing both Latin1 and TwoByte regex JitCode or byteCode.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Store Latin1/TwoByte code separately

Forgot to qref.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Part 4 - StringMatch case

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Part 5 - Run regexps

Just enough changes to run some simple regexps. There are other places in irregexp that I have to fix, but I think I'll do that as part of fixing jit-tests/jstests.

Comment 7

[Brian Hackett [Laid off!]]

Comment on attachment 8441311 [details] [diff] [review]
Part 3 - Store Latin1/TwoByte code separately

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

Is isCompiled() used anywhere now?

Comment 8

[Brian Hackett [Laid off!]]

Comment on attachment 8441386 [details] [diff] [review]
Part 5 - Run regexps

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

I think irregexp::InterpretCode can GC via the interrupt callback.

Comment 9

[Jan de Mooij [:jandem]]

Parts 1 and 2:

https://hg.mozilla.org/integration/mozilla-inbound/rev/93ab210dd907
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0e518026f4b

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/93ab210dd907
https://hg.mozilla.org/mozilla-central/rev/b0e518026f4b

Comment 11

[Jan de Mooij [:jandem]]

Parts 3 and 4:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea0a96320d86
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd69b2caad20

(In reply to Brian Hackett (:bhackett) from comment #7)
> Is isCompiled() used anywhere now?

Yes, an assert in getParenCount: isCompiled() || canStringMatch.

Comment 12

[Jan de Mooij [:jandem]]

And part 5:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4df3e4664b11

(In reply to Brian Hackett (:bhackett) from comment #8)
> I think irregexp::InterpretCode can GC via the interrupt callback.

Right, good catch! I changed it to use AutoStableStringChars instead for the regex interpreter.

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Part 6 - StringHasRegExpMetaChars

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ea0a96320d86
https://hg.mozilla.org/mozilla-central/rev/bd69b2caad20

Comment 15

[Jan de Mooij [:jandem]]

Attachment: Part 7 - CheckSpecialCharacterClass, CheckNotBackReference

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4df3e4664b11
https://hg.mozilla.org/mozilla-central/rev/14bc5bddd097

Comment 17

[Jan de Mooij [:jandem]]

Part 6:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ca9a7a10ac4

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2ca9a7a10ac4

Comment 19

[Jan de Mooij [:jandem]]

Attachment: Part 7 - GetCaseIndependentLetters

V8 has the code below, but I think it's wrong. Latin1 char 0xff has an upper-case char outside the Latin1 range. This patch is more conservative just to be safe. I'll also see if I can write a testcase that fails with V8.

  if (!ascii_subject || character <= String::kMaxOneByteCharCode) {
    return length;
  }
  // The standard requires that non-ASCII characters cannot have ASCII
  // character codes in their equivalence class.
  return 0;

Comment 20

[Jan de Mooij [:jandem]]

Attachment: Part 8 - GetCaseIndependentLetters

OK, what V8 does is fine (see previous comment), because their FilterASCII code converts 0x178 to 0xff. This patch makes our FilterASCII and GetCaseIndependentLettersbehave work more like theirs, and the test I wrote passes with this.

Comment 21

[Jan de Mooij [:jandem]]

Parts 7 and 8:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d77d20e48ffe
https://hg.mozilla.org/integration/mozilla-inbound/rev/efc771d4648c

Comment 22

[Jan de Mooij [:jandem]]

Attachment: Part 9 - CheckNotBackReferenceIgnoreCase

Fixes the last regex-related jit-test/jstest failures.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d77d20e48ffe
https://hg.mozilla.org/mozilla-central/rev/efc771d4648c

Comment 24

[Brian Hackett [Laid off!]]

Comment on attachment 8444299 [details] [diff] [review]
Part 9 - CheckNotBackReferenceIgnoreCase

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

::: js/src/irregexp/NativeRegExpMacroAssembler.cpp
@@ +734,5 @@
> +        // Save register contents to make the registers available below. After
> +        // this, the temp0, backtrack_stack_pointer, and current_position
> +        // registers are available.
> +        masm.push(current_position);
> +        masm.push(backtrack_stack_pointer);

I think you can use temp2 (which v8's version doesn't have) to avoid one of these pushes.

Comment 25

[Jan de Mooij [:jandem]]

And part 9:

https://hg.mozilla.org/integration/mozilla-inbound/rev/61400645a576

(In reply to Brian Hackett (:bhackett) from comment #24)
> I think you can use temp2 (which v8's version doesn't have) to avoid one of
> these pushes.

Ah great, done.

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/61400645a576