1207922 - Inlined RegExp.prototype.exec doesn't set lastIndex to 0 if match fails.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Tooru Fujisawa [:arai]]

Code:
  let r = /abc/; function f () { r.lastIndex = 3;  r.exec("093u4jalfdkjaodfuahsd"); return r.lastIndex; }; for (let i = 0; i < 10000; i++) f()

Expected Result:
  0

Actual Result with --ion-eager:
  3

Actual Result with --no-ion:
  0

In PrepareAndExecuteRegExp (or generateRegExpExecStub), lastIndex is not set to 0 when result is RegExpRunStatus_Success_NotFound.

As part of bug 887016, I'm going to Self-host RegExp.prototype.exec and do lastIndex handling in self-hosted code, to inline global/sticky case.  I think NotFound case could also be handled there.

Comment 1

[Tooru Fujisawa [:arai]]

Here's WIP patch that self-hosts RegExp.prototype.exec and RegExp.prototype.test: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8602309ab88

It almost passes the tests, except the TIMEOUT with v8-v5/check-regexp.js on cgc build.
it passes very quickly on normal opt build.
will investigate it, but not sure if it's critical.

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Self-host RegExp.prototype.{exec,test}.

looks like the TIMEOUT issue could be ignored for now.


overview of this patch series:
  * RegExp.prototype.exec
    * removed lastIndex handling from RegExpExec, and rename it to RegExpMatcher
    * added RegExpMatcher self-hosting builtin, and call it from RegExp_Exec,
      that is the implementation of RegExp.prototype.exec

  * RegExp.prototype.test
    * removed lastIndex handling from RegExpTest, and rename it to RegExpTester
    * RegExpTester returns integer (>= -1), instead of boolean,
      that is same as String.prototype.search's return value.
      the integer value is internally called |endIndex| (as opposed to |startIndex|)

[Part 1]

==== RegExp.cpp ====

regexp_methods
  * self-hosted RegExp.prototype.exec and RegExp.prototype.test

ExecuteRegExpImpl
  * added sticky parameter
  * added endIndex out parameter

ExecuteRegExp
  * added lastIndex and sticky parameters
  * added endIndex out parameter
  * removed code to get/check/set lastIndex property
    lastIndex is now handled in self-hosted code

regexp_exec_impl -> RegExpMatcher -- new self-hosting builtin
  * receives 4 parameters
    * RegExp object
    * input string
    * lastIndex
    * sticky
  * returns MatchObject on success
  * returns null on failure

regexp_exec_raw -> RegExpMatcherRaw -- function used by JIT
  * added lastIndex and sticky parameters

regexp_test_impl -> RegExpTester -- new self-hosting builtin
  * receives 4 parameters
    * RegExp object
    * input string
    * lastIndex
    * sticky
  * returns index on success
  * returns -1 on failure

regexp_test_raw -> RegExpTesterRaw -- function used by JIT
  * added lastIndex and sticky parameters

intrinsic_IsRegExpObject -- new self-hosting builtin
  * checks if 1st argument is RegExpObject

==== RegExp.js ====

RegExp_Exec -- implementation of RegExp.prototype.exec
  * this is not RegExpExec (ES6 21.2.5.2.1)
  * RegExpBuiltinExec is inlined
  * calls RegExpMatcher

RegExpTest -- implementation of RegExp.prototype.test
  * RegExpExec is inlined.
  * RegExpBuiltinExec is partially inlined
  * calls RegExpBuiltinExecForTest

RegExpBuiltinExecForTest -- ES6 RegExpBuiltinExec optimized for RegExp.prototype.test
  * separated from RegExpTest to call CallRegExpMethodIfWrapped
  * calls RegExpTester instead of RegExpMatcher

==== NativeRegExpMacroAssembler.cpp ====

NativeRegExpMacroAssembler::GenerateCode
  * stores endIndex when match_only==true

==== NativeRegExpMacroAssembler.h ====

InputOutputData struct
  * added endIndex pointer

==== RegExpEngine.cpp ====

irregexp::ExecuteCode
  * added endIndex parameter

==== RegExpInterpreter.cpp ====

irregexp::InterpretCode
  * added endIndex parameter
  * stores endIndex when matches is null

==== CodeGenerator.cpp ====

PrepareAndExecuteRegExp
  * added lastIndex and sticky register parameters
  * added code to handle unicode RegExp with global or sticky, and lastIndex
    points trail surrogate that has corresponding lead surrogate
    (corresponds to bug 1135377 part 10)
  * calculate corePointer based on {mode, latin1, sticky} combination
  * returns endIndex for MatchOnly, via temp3 register

JitCompartment::generateRegExpMatcherStub
  * Use dedicated register constants
  * temp3 and temp4 are now possibly InvalidReg on x86
    (because 2 more registers for lastIndex and sticky are needed now)
    * if they are InvalidReg, use registers for sticky and lastIndex
      * saves sticky and lastIndex to temporary space.
        now it uses object.initializedLength and object.length, as
        they're overwritten just after restoring sticky and lastIndex
      * it's hard to push them to stack, because of stringIndexAddress etc

RegExpTesterResultNotFound and RegExpTesterResultFailed
  * to return status from RegExpTesterStub, defined 2 constants
    * RegExpTesterResultNotFound = -1
      same meaning as -1 returned by String.prototype.search
    * RegExpTesterResultFailed = -2
      failed and need to run OOL code

JitCompartment::generateRegExpTesterStub
  * Use dedicated register constants
  * return one of following
    * endIndex
    * RegExpTesterResultNotFound
    * RegExpTesterResultFailed

CodeGenerator::visitRegExpTester
  * if returned value is RegExpTesterResultFailed, jump to OOL
  * return endIndex or -1

==== MCallOptimize.cpp ====

IonBuilder::inlineIsRegExpObject
  * inlined on following condition
    * argument may not be Object
      -> returns false
    * argument is RegExpObject (is this code correct?)
      -> returns true

==== Assembler-*.h ====

* Added dedicated Register constants for RegExpMatcher and RegExpTester
  because available registers are different for each arch

==== jsstr.cpp ====

* pass re.sticky() to re.execute
  will be changed again in bug 887016
* pass sticky to res->updateLazily

==== RegExpObject.cpp ====

RegExpShared::compile and RegExpShared::compileIfNecessary
  * added sticky parameter

RegExpShared::execute
  * added sticky parameter
  * added endIndex out parameter
    * matches and endIndex are mutual exclusive
      * |matches != null| means RegExp.prototype.exec
      * |endIndex != null| means RegExp.prototype.test

==== RegExpObject.h ====

RegExpShared class
  * compilationArray now has 8 elements
    (mode x2, latin1 x2, sticky x2)

==== RegExpStatics.cpp ===

RegExpStatics::updateLazily
  * added sticky parameter
  * use sticky to update

==== cgc-jittest-timeouts.txt ===

v8-v5/check-regexp.js fails with TIMEOUT on cgc build with --ion-eager. it's doing JIT compile most of time.  so, adding it timeouts list for now.  bug 1146867 will fix it.

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Propagate OOM thrown from stub generation.

[Part 2]

==== MCallOptimize.cpp ====

IonBuilder::inlineRegExpMatcher
  * added oom() call to propagate oom error in ensureRegExpMatcherStubExists

IonBuilder::inlineRegExpTester
  * added oom() call to propagate oom error in ensureRegExpTesterStubExists

==== IonBuilder.cpp ====

IonBuilder::inlineScriptedCall
  * propagate inlineBuilder.abortReason_ == AbortReason_Alloc

==== oomInRegExp.js ====

* added test for RegExp.prototype.exec

Comment 4

[Till Schneidereit [:till]]

Comment on attachment 8700843 [details] [diff] [review]
Part 1: Self-host RegExp.prototype.{exec,test}.

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

This mostly looks great - at least the parts I'm competent to review. I didn't look at the JIT changes too closely, as I don't know remotely enough to review them.

I'd like to see a new version of the RegExp.js changes with my somewhat long-ish comments addressed. I don't think that that'll affect the other changes, though, so those can be reviewed in parallel.

@h4writer, please review the jit/ parts - I'm way out of my depth for those.

::: js/src/builtin/RegExp.js
@@ +64,5 @@
> +
> +    // ES6 21.2.5.2 steps 4-5.
> +    var S = ToString(string);
> +
> +    // ES6 21.2.5.2 step 6.

I'm not too happy with how the code is factored for this and .test. Would it be too expensive performance-wise to implement one version of RegExpBuiltinExec that takes an additional "forTest" parameter? Based on that, steps 15-17 would be implemented differently, but the rest could stay the same (with endIndex assigned manually in the .exec case.)

That way, you could get rid of all these multiple step comments for the same line of code, and following what's going on / comparing with what the spec does would be quite a bit easier.

If that does turn out to be too costly (in tests that use both .test and .exec, to require the JITted code to be fully general instead of specialized for one of the cases), then I think I'd still prefer an outright duplication of RegExpBuiltinExec for both .exec and .test, with a comment for each saying that it's optimized for being called by .exec/.test.

@@ +83,5 @@
> +    } else {
> +        if (lastIndex < 0 || lastIndex > S.length) {
> +            // ES6 21.2.5.2.2 steps 15.a.i-ii, 15.c.i.1-2.
> +            R.lastIndex = 0;
> +

Nit: remove this blank line.

@@ +95,5 @@
> +        // ES6 21.2.5.2.2 steps 15.a.i-ii, 15.c.i.1-2.
> +        R.lastIndex = 0;
> +    } else {
> +      // ES6 21.2.5.2.2 step 18.
> +      if (global || sticky) {

Nit: no braces required.

@@ +120,5 @@
> +    // ES6 21.2.5.2.1 steps 3-4.
> +    var exec = R.exec;
> +
> +    // ES6 21.2.5.2.1 step 5.
> +    // Optimize if R.exec is RegExp.prototype.exec.

add " or R.exec isn't callable"

@@ +121,5 @@
> +    var exec = R.exec;
> +
> +    // ES6 21.2.5.2.1 step 5.
> +    // Optimize if R.exec is RegExp.prototype.exec.
> +    if (exec !== RegExp_Exec && IsCallable(exec)) {

I think it'd be better to invert the conditions here:
if (exec === RegExp_Exec || !IsCallable(exec))
    return RegExpBuiltinExecForTest(R, S);

That's easier to follow because it makes it clearer that a shortcut is taken.

Comment 5

[Till Schneidereit [:till]]

Comment on attachment 8700844 [details] [diff] [review]
Part 2: Propagate OOM thrown from stub generation.

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

Ok, these changes are simple enough that even I can review them :)

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Self-host RegExp.prototype.{exec,test}.

Thank you for reviewing :)

(In reply to Till Schneidereit [:till] from comment #4)
> ::: js/src/builtin/RegExp.js
> @@ +64,5 @@
> > +
> > +    // ES6 21.2.5.2 steps 4-5.
> > +    var S = ToString(string);
> > +
> > +    // ES6 21.2.5.2 step 6.
> 
> I'm not too happy with how the code is factored for this and .test. Would it
> be too expensive performance-wise to implement one version of
> RegExpBuiltinExec that takes an additional "forTest" parameter? Based on
> that, steps 15-17 would be implemented differently, but the rest could stay
> the same (with endIndex assigned manually in the .exec case.)
> 
> That way, you could get rid of all these multiple step comments for the same
> line of code, and following what's going on / comparing with what the spec
> does would be quite a bit easier.
> 
> If that does turn out to be too costly (in tests that use both .test and
> .exec, to require the JITted code to be fully general instead of specialized
> for one of the cases), then I think I'd still prefer an outright duplication
> of RegExpBuiltinExec for both .exec and .test, with a comment for each
> saying that it's optimized for being called by .exec/.test.

The performance difference is 1-2% for .exec and 3-4% for .test.  It would be better to simplify them for now :)
Merged RegExpBuiltinExec, and separated RegExpExec.

> @@ +120,5 @@
> > +    // ES6 21.2.5.2.1 steps 3-4.
> > +    var exec = R.exec;
> > +
> > +    // ES6 21.2.5.2.1 step 5.
> > +    // Optimize if R.exec is RegExp.prototype.exec.
> 
> add " or R.exec isn't callable"

|!IsCallable(exec)| case is not an optimization.  |exec === RegExp_Exec| case uses same code for an optimization.
Added some comments for the code.  Does it make sense?

Comment 7

[Till Schneidereit [:till]]

Comment on attachment 8701562 [details] [diff] [review]
Part 1: Self-host RegExp.prototype.{exec,test}.

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

Thanks, this is much easier to follow. Definitely worth a 1-4% performance cost.

r=me for the non-JIT parts.

::: js/src/builtin/RegExp.js
@@ +77,5 @@
> +    var exec = R.exec;
> +
> +    // Step 5.
> +    // If R.exec is RegExp.prototype.exec, same code for the case R.exec is not
> +    // callable can be used as an optimization.

How about
"If exec is the original RegExp.prototype.exec, use the same, faster, path as for the case where exec isn't callable."

@@ +96,5 @@
> +    // Step 5.d.
> +    if (forTest)
> +        return result !== null;
> +
> +    return result;

A ternary would be slightly clearer here:
return forTest ? result !== null : result;

@@ +124,5 @@
> +    } else {
> +        if (lastIndex < 0 || lastIndex > S.length) {
> +            // Steps 15.a.i-ii, 15.c.i.1-2.
> +            R.lastIndex = 0;
> +            if (forTest)

A ternary would be slightly clearer here:
return forTest ? false : null;

Comment 8

[Hannes Verschore [:h4writer]]

Sry for the delay. This is on my todo list to review tomorrow. Good work. Do you expect this to change our performance characteristics or should this be fine?

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Performance comparison of exec/test, with different input each time

Here's performance comparison.
The gradient in Ion execution is almost same as before.
There is little offset (around 1-2 ms) due to perf regression in Interpreter and Baseline execution, and increased Ion compilation time, because of additional JS code.

Comment 10

[Tooru Fujisawa [:arai]]

Attachment: Performance comparison of exec/test, with same input each time

If the input is same, Ion execution for global/sticky case gets faster than before.

Comment 11

[Hannes Verschore [:h4writer]]

Comment on attachment 8701562 [details] [diff] [review]
Part 1: Self-host RegExp.prototype.{exec,test}.

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

Looks good! Gonna check ion bits tomorrow.

::: js/src/builtin/RegExp.cpp
@@ +845,5 @@
>           *   obtained from element index of str.
>           *
>           * In the spec, pattern match is performed with decoded Unicode code
>           * points, but our implementation performs it with UTF-16 encoded
> +         * string.  In step 2, we should decrement lastIndex (index) if it

s/ihdex/index

@@ +893,5 @@
>      if (status == RegExpRunStatus_Success_NotFound) {
>          rval.setNull();
>          return true;
>      }
>  

Add:
/* Steps 19-29 */

::: js/src/builtin/RegExp.js
@@ +55,5 @@
>  }
> +
> +// ES6 21.2.5.2.
> +// NOTE: This is not RegExpExec (21.2.5.2.1).
> +function RegExp_Exec(string) {

Should we call this RegExp_prototype_exec ? To make the distinction a bit bigger?

Comment 12

[Hannes Verschore [:h4writer]]

Comment on attachment 8701562 [details] [diff] [review]
Part 1: Self-host RegExp.prototype.{exec,test}.

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

Nice work!

I'm giving r+, though there are some things that still need to get adjusted.
If not sure or if disagreeing please ask questions on IRC or in bug report.
Else I think we can land this with that requested changes and the given performance profile.

::: js/src/jit/CodeGenerator.cpp
@@ +1145,5 @@
>      {
>          masm.loadStringChars(input, temp2);
>          masm.storePtr(temp2, inputStartAddress);
> +
> +        Label stickyCode, doneSticky;

I think we don't need "doneSticky".
=> Everywhere where we jump to "done" (in this snippit) we can just jump to "doneSticky". And afterwards remove the "done" label and rename the "doneSticky" label to "done"

@@ +1151,2 @@
>          {
> +            masm.loadStringLength(input, temp3);

Can you hoist this line. It is similar for stickyCode and nonStickyCode.
And we don't use the temp3 register. I see no reason this patch moved them here.

@@ +1154,5 @@
> +            masm.branchTest32(Assembler::NonZero, Address(input, JSString::offsetOfFlags()),
> +                              Imm32(JSString::LATIN1_CHARS_BIT), &isLatin1);
> +            {
> +                masm.lshiftPtr(Imm32(1), temp3);
> +                masm.loadPtr(Address(temp1, RegExpShared::offsetOfJitCode(mode, false, false)), codePointer);

Having unnamed boolean is bad. At least can you add what they depict in comments?
RegExpShared::offsetOfJitCode(mode, /* sticky= */ false, /* latin1= */ false)

I would even think that creating more expressive function names would be better:
RegExpShared::offsetOfStickyLatin1JitCode(mode)
RegExpShared::offsetOfNotStickyLatin1JitCode(mode)
RegExpShared::offsetOfStickyTwoByteJitCode(mode)
RegExpShared::offsetOfNotStickyTwoByteJitCode(mode)

This comment applies to all next offsetOfJitCode calls here.

@@ +1169,5 @@
> +            masm.bind(&stickyCode);
> +            masm.loadStringLength(input, temp3);
> +            Label isLatin1, done;
> +            masm.branchTest32(Assembler::NonZero, Address(input, JSString::offsetOfFlags()),
> +                              Imm32(JSString::LATIN1_CHARS_BIT), &isLatin1);

Can we create part 3 or follow up bug to create:
masm.branchLatin1String(Register string, Label label)
masm.branchTwoByteString(Register string, Label label)

and use that here?

@@ +1519,5 @@
> +            masm.jump(&done);
> +
> +            masm.bind(&failureRestore);
> +            masm.load32(Address(object, elementsOffset + ObjectElements::offsetOfInitializedLength()), sticky);
> +            masm.load32(Address(object, elementsOffset + ObjectElements::offsetOfLength()), lastIndex);

I think we should restore the objectElements::length and objectElements::initializedLength to the default values again? Else I think GC of this object can lead to issues...

Comment 13

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Self-host RegExp.prototype.{exec,test}.

Thank you for reviewing :)

(In reply to Hannes Verschore [:h4writer] from comment #12)
> @@ +1151,2 @@
> >          {
> > +            masm.loadStringLength(input, temp3);
> 
> Can you hoist this line. It is similar for stickyCode and nonStickyCode.
> And we don't use the temp3 register. I see no reason this patch moved them
> here.

Oops, it was a remainder from previous approach for register usage.
It should be reverted.


> @@ +1154,5 @@
> > +            masm.branchTest32(Assembler::NonZero, Address(input, JSString::offsetOfFlags()),
> > +                              Imm32(JSString::LATIN1_CHARS_BIT), &isLatin1);
> > +            {
> > +                masm.lshiftPtr(Imm32(1), temp3);
> > +                masm.loadPtr(Address(temp1, RegExpShared::offsetOfJitCode(mode, false, false)), codePointer);
> 
> Having unnamed boolean is bad. At least can you add what they depict in
> comments?
> RegExpShared::offsetOfJitCode(mode, /* sticky= */ false, /* latin1= */ false)
> 
> I would even think that creating more expressive function names would be
> better:
> RegExpShared::offsetOfStickyLatin1JitCode(mode)
> RegExpShared::offsetOfNotStickyLatin1JitCode(mode)
> RegExpShared::offsetOfStickyTwoByteJitCode(mode)
> RegExpShared::offsetOfNotStickyTwoByteJitCode(mode)

Will go with this.

Comment 14

[Tooru Fujisawa [:arai]]

Attachment: Part 1 followup: Restore length of match object on failure case.

> @@ +1519,5 @@
> > +            masm.jump(&done);
> > +
> > +            masm.bind(&failureRestore);
> > +            masm.load32(Address(object, elementsOffset + ObjectElements::offsetOfInitializedLength()), sticky);
> > +            masm.load32(Address(object, elementsOffset + ObjectElements::offsetOfLength()), lastIndex);
> 
> I think we should restore the objectElements::length and
> objectElements::initializedLength to the default values again? Else I think
> GC of this object can lead to issues...

Thank you for pointing this out :D
Added code to restore to templateObject's length, and assertion for it.
Can you review that part?

Comment 15

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Add masm.branchLatin1String and masm.branchTwoByteString.

> @@ +1169,5 @@
> > +            masm.bind(&stickyCode);
> > +            masm.loadStringLength(input, temp3);
> > +            Label isLatin1, done;
> > +            masm.branchTest32(Assembler::NonZero, Address(input, JSString::offsetOfFlags()),
> > +                              Imm32(JSString::LATIN1_CHARS_BIT), &isLatin1);
> 
> Can we create part 3 or follow up bug to create:
> masm.branchLatin1String(Register string, Label label)
> masm.branchTwoByteString(Register string, Label label)
> 
> and use that here?

Added them, and replaced duplicated codes.

Comment 16

[Hannes Verschore [:h4writer]]

Comment on attachment 8704608 [details] [diff] [review]
Part 1 followup: Restore length of match object on failure case.

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

Thanks!

::: js/src/jit/CodeGenerator.cpp
@@ +1459,5 @@
> +        Label initLengthOK, lengthOK;
> +        masm.load32(Address(object, elementsOffset + ObjectElements::offsetOfInitializedLength()),
> +                    temp2);
> +        masm.branch32(Assembler::Equal, temp2, Imm32(templateObject->getDenseInitializedLength()),
> +                      &initLengthOK);

We have: masm.branch32(XXX, Address, Imm32(), XXX);
That way there is no reason to do the load yourself ;).

Comment 17

[Hannes Verschore [:h4writer]]

Comment on attachment 8704609 [details] [diff] [review]
Part 3: Add masm.branchLatin1String and masm.branchTwoByteString.

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

Thanks!

Comment 18

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/28cc01acfd024c6ba4066fc192c6ccff502c0b00
Bug 1207922 - Part 1: Self-host RegExp.prototype.{exec,test}. r=till,h4writer

https://hg.mozilla.org/integration/mozilla-inbound/rev/d39655c43ff0ab6378f2fc82ec56270d3bd88013
Bug 1207922 - Part 2: Propagate OOM thrown from stub generation. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/a0c3bdd0559b936e5b2865783411231181c84d3c
Bug 1207922 - Part 3: Add masm.branchLatin1String and masm.branchTwoByteString. r=h4writer

Comment 19

[Guilherme Lima]

These patches caused a ~30% regression on ss-validate-input on AWFY

Comment 20

[Till Schneidereit [:till]]

(In reply to Guilherme Lima from comment #19)
> These patches caused a ~30% regression on ss-validate-input on AWFY

That's probably the 1-2ms mentioned in comment 9. Would be nice to confirm that that's really what's going on, but if so, I think we should just ignore this.

Comment 21

[Tooru Fujisawa [:arai]]

Thanks.
SunSpider string-validate-input [1] score with replacing 'new Date()' with 'elapsed()' is following:
  before: 7283 [us]
  after:  9480 [us]

There only .test() is used.
When I modify the test to perform same thing 4 times, the score is following:

         | 1st   | 2nd  | 3rd  | 4th
---------+-------+------+-0----+------
  before |  7955 | 4005 | 2926 | 3045
  after  | 11086 | 4135 | 3129 | 3031

So, at least the difference in string-validate-input comes from the same reason in comment #9.

Will investigate other perf later today .


[1] https://dxr.mozilla.org/mozilla-central/source/build/pgo/js-input/sunspider/string-validate-input.html

Comment 22

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

https://hg.mozilla.org/mozilla-central/rev/28cc01acfd02
https://hg.mozilla.org/mozilla-central/rev/d39655c43ff0
https://hg.mozilla.org/mozilla-central/rev/a0c3bdd0559b

Comment 23

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8704607 [details] [diff] [review]
Part 1: Self-host RegExp.prototype.{exec,test}.

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

::: js/src/jit/CodeGenerator.cpp
@@ +1619,5 @@
> +    pushArg(lastIndex);
> +    pushArg(input);
> +    pushArg(regexp);
> +
> +    callVM(RegExpMatcherRawInfo, lir);

Any reason for not using oolCallVM ?

@@ +1655,5 @@
> +    OutOfLineRegExpMatcher* ool = new(alloc()) OutOfLineRegExpMatcher(lir);
> +    addOutOfLineCode(ool, lir->mir());
> +
> +    JitCode* regExpMatcherStub = gen->compartment->jitCompartment()->regExpMatcherStubNoBarrier();
> +    masm.call(regExpMatcherStub);

Can the RegExpMatcherStub allocate anything?  Such as RegExp.$1 strings?

Comment 24

[Nicolas B. Pierron [:nbp]]

Attachment: Clean-up RRegExp{Match,Test}er function in Recover.cpp.

Comment 25

[Tooru Fujisawa [:arai]]

(In reply to Nicolas B. Pierron [:nbp] from comment #23)
> ::: js/src/jit/CodeGenerator.cpp
> @@ +1619,5 @@
> > +    pushArg(lastIndex);
> > +    pushArg(input);
> > +    pushArg(regexp);
> > +
> > +    callVM(RegExpMatcherRawInfo, lir);
> 
> Any reason for not using oolCallVM ?

No particular reason, just because I didn't know it can be used there.
Will look into it, thanks :)


> @@ +1655,5 @@
> > +    OutOfLineRegExpMatcher* ool = new(alloc()) OutOfLineRegExpMatcher(lir);
> > +    addOutOfLineCode(ool, lir->mir());
> > +
> > +    JitCode* regExpMatcherStub = gen->compartment->jitCompartment()->regExpMatcherStubNoBarrier();
> > +    masm.call(regExpMatcherStub);
> 
> Can the RegExpMatcherStub allocate anything?  Such as RegExp.$1 strings?

Yes, it allocates match object and its properties.
can you tell me the what the problem is?

Comment 26

[Tooru Fujisawa [:arai]]

Comment on attachment 8706333 [details] [diff] [review]
Clean-up RRegExp{Match,Test}er function in Recover.cpp.

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

thanks!

::: js/src/jit/Recover.cpp
@@ -1013,5 @@
>      bool sticky = iter.read().toBoolean();
>  
>      RootedValue result(cx);
> -
> -    fprintf(stderr, "Recover\n");

I'm going to remove this in bug 1238417.

Comment 27

[Nicolas B. Pierron [:nbp]]

(In reply to Tooru Fujisawa [:arai] from comment #25)
> (In reply to Nicolas B. Pierron [:nbp] from comment #23)
> > @@ +1655,5 @@
> > > +    OutOfLineRegExpMatcher* ool = new(alloc()) OutOfLineRegExpMatcher(lir);
> > > +    addOutOfLineCode(ool, lir->mir());
> > > +
> > > +    JitCode* regExpMatcherStub = gen->compartment->jitCompartment()->regExpMatcherStubNoBarrier();
> > > +    masm.call(regExpMatcherStub);
> > 
> > Can the RegExpMatcherStub allocate anything?  Such as RegExp.$1 strings?
> 
> Yes, it allocates match object and its properties.
> can you tell me the what the problem is?

Can RegExpMatcher re-enter JS code?

I am worried about one simple rule of thumb which is that no CodeGenerator visitor should have consecutive calls.  The reason for that is that in case of invalidation, this will corrupt the code and might cause a Random Memory Execution security issue.

The comment in CodeGeneratorShared::ensureOsiSpace() will give you a hint of what can go wrong.

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/46db83228504

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/46db83228504