1038593 - IonMonkey: Implement RegExpExec Recover Instruction

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

Implement RRegExpExec in js/src/jit/Recover.cpp.
See Bug 1003801 comment 0 for explanation.

Comment 1

[Lawrence Abadier]

I would like to work on this bug, but need some steps to help me get started. I have recover.cpp open but at a loss on how to approach this.

Comment 2

[Nicolas B. Pierron [:nbp]]

(In reply to Lawrence Abadier from comment #1)
> I would like to work on this bug, but need some steps to help me get

Feel free to start working on it, I will assign you to this bug as soon as you submit a patch.

> started. I have recover.cpp open but at a loss on how to approach this.

Have you looked at how the CodeGenerator.cpp is doing for the implementation of MRegExpExec?
Have you looked at other bugs which are listed as the dependencies of Bug 1003801, such as Bug 1028675?

Comment 3

[Lawrence Abadier]

Hi,

I checked out CG.cpp and this was the only form of RegExpExec found:
>bool
>CodeGenerator::visitRegExpExec(LRegExpExec *lir)

How does visitRegExpExec function?

I looked at similarities Bug 1028675 and Bug 1003801 but still at a loss on how to implement the MRegExpExec found in MIR.H. This includes the previous patches, but without understanding what each thing does, it is difficult to go off of that.

Is there any resources I can look over to help me with this? Forgive me if these questions seem simple, this is my first bug.

Comment 4

[Nicolas B. Pierron [:nbp]]

(In reply to Lawrence Abadier from comment #3)
> I checked out CG.cpp and this was the only form of RegExpExec found:
> >bool
> >CodeGenerator::visitRegExpExec(LRegExpExec *lir)
> 
> How does visitRegExpExec function?

This function generates code which will be executing a RegExpExec.

Recover instructions are used to prevent the generation of code in the code generator, but if baseline needs IonMonkey to have executed an instruction, we should do it.  So this is related in the sense that we want to be doing the same thing as the code which is produced by this visitRegExpExec function.

> Is there any resources I can look over to help me with this? Forgive me if
> these questions seem simple, this is my first bug.

The visitRegExpExec function generate some code which is making a call, with a callVM.  Can we re-use the same function for the RRegExpExec::recover function?

Comment 5

[Lawrence Abadier]

I have studied the code in more depth and have come up with the following:

>>CodeGenerator::visitRegExpExec
>>LRegExpExec *lir- registers an array with type lallocation(which returns instructions at index)

Both regexp and string return an instruction from the same array, but not sure the difference aside from index[1] returning a string instruction.

ToRegister:

>>ToRegister: Registers instruction codes to JS_ASSERT(desc.object()); //I'm not sure what does JS_Assert, keeps track of instruction codes?

pushArg:

>>pushArg: Pushes the instruction in MacroAssembler - not sure what this does, could not find masm.Push(t);'s purpose

callVM:

>>RegExpExecRawInfo = linked list = JSContext *cx, HandleObject regexp, HandleString input, MutableHandleValue output

callVM(linked list of vm functions, sub class of LInstruction)  and executes them



>>CodeGeneratorShared::callVM(const VMFunction &fun, LInstruction *ins, const Register *dynStack) 
uses an LInstruction while MRegExpExec needs an MInstruction.(They both retrieve instructions in the same manner).

My first thought is to use the baseline callVM : 
>>BaselineCompilerShared::callVM(const VMFunction &fun, CallVMPhase phase)

But it does not use an MInstruction, so most likely I have to create a new callVM(similar to codegenerator's) that uses MInstructions. Please let me know if I'm on the right path with this.

Thank you

Comment 6

[Nicolas B. Pierron [:nbp]]

(In reply to Lawrence Abadier from comment #5)
> I have studied the code in more depth and have come up with the following:
> 
> CodeGenerator::visitRegExpExec
> >>LRegExpExec *lir- registers an array with type lallocation(which returns instructions at index)
> 

CodeGenerator::visitRegExpExec is used to generate the assembly code for checking a regexp against a string and returning an object which correspond to the result of the matches.

> ToRegister:

Convert the register allocator LAllocations into a register which can be used for generating code.

> pushArg:

This is used to push arguments on the stack, such as the callee can reuse these arguments by reading an offset from the stack pointer.

> callVM:
> 
> >>RegExpExecRawInfo = linked list = JSContext *cx, HandleObject regexp, HandleString input, MutableHandleValue output
> 
> callVM(linked list of vm functions, sub class of LInstruction)  and executes
> them

callVM generate a call to the function which is wrapped by the VMFunction (RegExpExecRawInfo), the fact that this is a linked list is irrelevant.  When this code would be executed, the function wrapped in the VMFunction would be called from the CodeGeneratorShared::CallVM function. (through generateVMWrapper)

What is important to you at the moment, is the wrapped function, because this is the one that you want to call.  callVM is just the logic to transition properly from the generated code (masm) to the C++ (VM) function.

> But it does not use an MInstruction, so most likely I have to create a new
> callVM(similar to codegenerator's) that uses MInstructions. Please let me
> know if I'm on the right path with this.

As you are already writing code in C++, you should have no need for a callVM function, but you might be interested by the wrapped function of RegExpExecRawInfo.

Comment 7

[Lawrence Abadier]

Hi Nicolas,

I worked on the bug a little more and would like some feedback.

Created class RRegExpExec
>>class RRegExpExec MOZ_FINAL : public RInstruction
>>{
>>public:
>>	RINSTRUCTION_HEADER_(RegExpExec)
>>	
>>	virtual uint32_t numOperands() const {
>>	return 2; /// string & obj
>>	}
>>	bool recover(JSContext *cx, SnapshotIterator &iter) const;
>>}

>>	bool MRegExpExec::writeRecoverData(CompactBufferWriter &writer) const
>>	{
>>		MOZ_ASSERT(canRecoverOnBailout()); // Have to add them to MRegExpExec
>>		writer.writeUnsigned(uint32_t(RInstruction::RegExpExec));
>>		return true;
>>	}
>>

>>	bool RRegExpExec::recover(JSContext *cx, SnapshotIterator &iter) const{
>>		RootedString str(cx, iter.read().toString());
>>		RootedObject regExec(cx, iter.read().toObject());
>>		
>>		RootedValue result(cx);
>>		
>>		MOZ_ASSERT(reExec.isString());
SplitRegExpMatcher was the only usable method at http://dxr.mozilla.org/mozilla-central/source/js/src/jsstr.cpp#3823 not sure if its the correct one.
>>		if(/*!SplitRegExpMatcher*/)
>>		
>>		iter.storeInstructionResult(result);
>>		return true;
>>	}

I still have to implement the necessary methods for MRegExpExec, but this is what I have so far. I couldn't make use of getVMWrapper thus far. Please let me know if this is the correct route, and anything that needs to be fixed.

Thank you,

Comment 8

[Nicolas B. Pierron [:nbp]]

(In reply to Lawrence Abadier from comment #7)
> SplitRegExpMatcher was the only usable method at
> http://dxr.mozilla.org/mozilla-central/source/js/src/jsstr.cpp#3823 not sure
> if its the correct one.

What do you think of regexp_exec_raw[1] ?

> I still have to implement the necessary methods for MRegExpExec, but this is
> what I have so far. I couldn't make use of getVMWrapper thus far. Please let
> me know if this is the correct route, and anything that needs to be fixed.

As I mentioned before, there is no need for a VMWrapper, as the code is written in C++, so we are already in the VM.

[1] http://dxr.mozilla.org/mozilla-central/source/js/src/builtin/RegExp.cpp?from=regexp_exec_raw#651

Comment 9

[Lawrence Abadier]

Hi Nicolas,

I worked on the recover function a little more, please let me know if something needs a fix.(Not sure if i need to start patching yet)

>>	#include "builtin/RegExp.h"
	 
>>	bool RRegExpExec::recover(JSContext *cx, SnapshotIterator &iter) const{
>>		HandleString str(cx, iter.read().toString());
>>		RootedObject regexp(cx, iter.read().toObject());
>>		MutableHandleValue rval(cx, iter.read());
>>		/*RegExpStaticsUpdate staticsUpdate(cx , iter.read());*/ //not sure if this is correct.
>>		
>>		RootedValue result(cx);
>>		
>>		MOZ_ASSERT(reExec.isString());
>>		if(!regexp_exec_raw(cx, regexp, str, staticsUpdate, rval))
>>			return false;
>>		iter.storeInstructionResult(result);
>>		return true;
>>	}

Also in terms of dce-with-rinstructions.js, would I still need to implement regexec test instructions? I found 2 implementations of regexp: regexp-exec.js and regexp-clone.js.

thank you,

Comment 10

[Nicolas B. Pierron [:nbp]]

(In reply to Lawrence Abadier from comment #9)
> I worked on the recover function a little more, please let me know if
> something needs a fix.(Not sure if i need to start patching yet)

You should attach patches instead of quoting parts of your modifications in the bug, it is hard to follow what you have done.

> Also in terms of dce-with-rinstructions.js, would I still need to implement
> regexec test instructions? I found 2 implementations of regexp:
> regexp-exec.js and regexp-clone.js.

The RegExpTest instructions are handled by Aetf in Bug 1038592, so you should not worry about it, and focus only on the "exec" one.

Comment 11

[Lawrence Abadier]

Attachment: Bug1038593 Rev.1

Hi,

I was sure about : RegExpStaticsUpdate staticsUpdate(cx , iter.read()); 

I could not find the rooted equiv for it.

Thank you

Comment 12

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8469704 [details] [diff] [review]
Bug1038593 Rev.1

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

Do you have any test case to debug your code?

Also, you should pay attention to your editor configuration, such as it stops adding trailing white spaces all over the patch.

::: js/src/jit/Recover.cpp
@@ +902,5 @@
> +{
> +	RootedString str(cx, iter.read().toString());
> +	RootedObject regexp(cx, iter.read().toObject());
> +	RootedValue rval(cx, iter.read());
> +	RegExpStaticsUpdate staticsUpdate(cx , iter.read()); 

MRegExpExec has 2 operands,
RRegExpExec claims it has 3 operands, (?)
And here you are reading 4. (?!)

I think there is a problem.

@@ +906,5 @@
> +	RegExpStaticsUpdate staticsUpdate(cx , iter.read()); 
> +		
> +	RootedValue result(cx);
> +		
> +	if(!regexp_exec_raw(cx, regexp, str, staticsUpdate, rval))

js::regexp_exec_raw(JSContext *cx, HandleObject regexp, HandleString input, MutableHandleValue output)

Why would you need a RegExpStaticsUpdate?

::: js/src/jit/Recover.h
@@ +467,5 @@
> +public:
> +	RINSTRUCTION_HEADER_(RegExpExec)
> +
> +	virtual uint32_t numOperands() const {
> +	return 3; 

nit: Why do we have 3 operands here?

style-nit: fix the indentation issues
  https://wiki.mozilla.org/JavaScript:SpiderMonkey:C_Coding_Style

Comment 13

[Lawrence Abadier]

Attachment: bug1038593_regexpexec_rev2.diff

Apparently was looking at the wrong function. I have went ahead and updated the fixes and included my test cases. The issue with the trailing white-spaces only shows up after patching (in this case the indenting for the test case). I use vs2012 and notepade++ to make the edits, so not sure how to remove them.

Comment 14

[Lawrence Abadier]

I wish I could edit these posts,

>> assertEq(x[0], "str");

should be:

>> assertEq(res[0], "str");

Thank you,

Comment 15

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8470385 [details] [diff] [review]
bug1038593_regexpexec_rev2.diff

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

::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
@@ +566,5 @@
>  }
>  
> +var uceFault_regexp_exec = eval(uneval(uceFault).replace('uceFault', 'uceFault_regexp_exec'))
> +function rregexp_exec(i) {
> +    var re = new RegExp("\d+(\w+)\d+" , "i");

Can you test with no-flags, "y", "m", "g", in addition to "i"?

@@ +571,5 @@
> +	var res = re.exec("01234str56789");
> +    if (uceFault_regexp_exec(i) || uceFault_regexp_exec(i)) {
> +        assertEq(x[0], "str");
> +    }
> +    return i;

Assert for the value of re.lastIndex.

@@ +741,5 @@
>      rsqrt_object(i);
>      ratan2_number(i);
>      ratan2_object(i);
>      rstr_split(i);
> +	rregexp_exec(i);

style-nit: do not use tabs.

::: js/src/jit/MIR.h
@@ +5661,5 @@
>          return this;
>      }
> +    
> +    bool writeRecoverData(CompactBufferWriter &writer) const;
> +    

style-nit: remove trailing white spaces.

@@ +5663,5 @@
> +    
> +    bool writeRecoverData(CompactBufferWriter &writer) const;
> +    
> +    bool canRecoverOnBailout() const {
> +        return true;

I do not think this is always true.  We can only recover if the instruction is either not effectful, and some of the flags will modify the fields of the RegExpObject based on the last re.exec.

Adding the test cases should highlight which one are not safe to handle.

::: js/src/jit/Recover.cpp
@@ +900,5 @@
> +	RootedObject regexp(cx, iter.read().toObject());
> +	RootedString input(cx, iter.read().toString());	
> +	
> +	RootedValue result(cx);
> +	

style-nit: Do not use tabs, and fix trailing white spaces.

::: js/src/jit/Recover.h
@@ +469,5 @@
> +
> +	  virtual uint32_t numOperands() const {
> +	  return 2; 
> +	  }
> +		 

style-nit: do not use tabs, fix trailing white spaces, and fix indentation issue.

Comment 16

[Lawrence Abadier]

Attachment: bug1038593_regexpexec_rev3.diff

Hi Nicolas,

-I have removed all the tabs,leading whitespaces, and fixed indentation.
-Included test-cases for no-flags, "y", "m", "g", in addition to "i".
-Also included the literal forms of those test-cases (did not know if it was needed)
-Additionally changed the: canRecoverOnBailout()

Please let me know if there is anything else needed from me.

Thank you,

Comment 17

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8471154 [details] [diff] [review]
bug1038593_regexpexec_rev3.diff

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

The patch looks good, but I doubt the test suite Pass the dce-with-rinstruction.js test.
Can you update the test cases and verify that each is compiled as expected, and that they all bailout on 99, to take the branch removed by UCE?

::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
@@ +569,5 @@
> +function rregexp_exec(i) {
> +    var re = new RegExp("str\\d+" + i + "\\d+rts");
> +    var res = re.exec("str01234567899876543210rts");
> +    if (uceFault_regexp_exec(i) || uceFault_regexp_exec(i)) {
> +        assertEq(re.lastIndex, "str");

I think this will always fail.  Are you sure the test case are correct?
What is this code checking, where do we use the result of re.exec?

The goal of uceFault function is to make sure that we do remove this if branch and remove the need for the execution, and thus that we do recover the value correctly.  If we are not checking the result of re.exec in this branch, we are not checking if the result is correctly recovered.

the assertion with re.lastIndex should always fail, as it is supposed to return either 0 or the matching location (in which case we want to garanteee that the matching part is not starting at the first character).  Also we want to always assert re.lastIndex, and not only in this if-branch, as the lastIndex has to be updated, even if we remove re.exec call.

::: js/src/jit/Recover.cpp
@@ +902,5 @@
> +
> +    RootedValue result(cx);
> +
> +	if(!regexp_exec_raw(cx, regexp, input, &result)
> +        return false;

style-nit: avoid tabs before the "if".

Comment 18

[Emanuel Hoogeveen [:ehoogeveen]]

Lawrence, I saw you were asking about running jit-tests on Windows last night. I don't know when you'll be on IRC so I'll just post this here. Compiling the JS shell by itself on Windows is finicky at best - essentially none of the SM devs do it (they all use Linux or OSX), so it might be best to avoid trying. What I suggest doing is the following:

1) Make sure you're using the latest MozillaBuild [1] (1.10.0 right now). That way you shouldn't have to worry about any prerequisites, or the shell not working as expected. I only mention this since you were talking about using python3 last night, which isn't something you should have to worry about.

2) Use something like the following commandline for building and testing:

# From the base of the tree
./mach build
js/src/jit-test/jit_test.py path/to/objdir/dist/bin/js.exe
# Make changes to files under js/src
./mach build js/src
./mach build js/src/shell
js/src/jit-test/jit_test.py path/to/objdir/dist/bin/js.exe

That way you get a threadsafe shell and incremental builds, without having to figure out how to build it separately. I suggest doing a full |./mach build| after every |hg pull| so the tree doesn't end up in an inconsistent state (although this might not matter for the js shell, unless NSPR changes). You can change the options it configures with by editing your .mozconfig file (after making changes I think you can do a |./mach build-backend| to make them take effect).

I also suggest running |./mach mercurial-setup| to setup your .hgrc/mercurial.ini correctly. Colorizing terminal output doesn't work on Windows, and bzpost and firefoxtree seem to depend on things that aren't part of the current MozillaBuild, so keep that in mind. In addition, if you're using patch queues for your development you probably don't want histedit (I think when I naively enabled that recently, it broke things for mq). I don't personally use mqext either but I'll leave that up to you; everything else should work, or at least not break stuff. mercurial-setup might produce a mercurial.ini file with Mac-style carriage return line endings, which seems to confuse the diff it shows at the end; I suggest correcting this to Unix-style line feeds if this happens.

[1] https://wiki.mozilla.org/MozillaBuild

Comment 19

[Lawrence Abadier]

Attachment: bug1038593_regexpexec_rev4.diff

Hi Nicolas,

I just wanted to get the patch in for some feedback. I have spent 2 days fixing issues with building the shell, and had to rebuild from scratch multiple times. Currently I have been able to run both jit-test and jstest as command line args like so:

>>jit_test.py --ion G:\mozilla-central\obj-i686-pc-mingw32\dist\bin\js.exe
Runs all 100+ tests and was unable to have it just test my 1 test case.

>>jstests.py --tbpl G:\mozilla-central\obj-i686-pc-mingw32\dist\bin\js.exe -f PATH_TO_TEST_CASE
I can run it on my test case (which includes 9 functions), but it returns 27 true/false regardless of what I test (tried using just 1 function as well).

I have attempted to run ionflags, as well as tests directly in the JS-Shell but nothing has been working for me.

I must be doing something wrong at this point and would like some direction on how to go about this.

Thank you,

Comment 20

[Nicolas B. Pierron [:nbp]]

(In reply to Lawrence Abadier from comment #19)
> I just wanted to get the patch in for some feedback. I have spent 2 days
> fixing issues with building the shell, and had to rebuild from scratch
> multiple times. Currently I have been able to run both jit-test and jstest
> as command line args like so:
> 
> >>jit_test.py --ion G:\mozilla-central\obj-i686-pc-mingw32\dist\bin\js.exe
> Runs all 100+ tests and was unable to have it just test my 1 test case.

Try:

  jit_test.py G:\mozilla-central\obj-i686-pc-mingw32\dist\bin\js.exe ion/dce-with-rinstructions.js

> I have attempted to run ionflags, as well as tests directly in the JS-Shell
> but nothing has been working for me.

Also, as this test case does not depend on any testing library, you can also run it as:

  G:\mozilla-central\obj-i686-pc-mingw32\dist\bin\js.exe --ion-eager --ion-offthread-compile=off G:\mozilla-central\js\src\jit-test\tests\ion\dce-with-rinstructions.js

Then, You can try to do:

  set IONFLAGS=help
  G:\mozilla-central\obj-i686-pc-mingw32\dist\bin\js.exe --ion-eager --ion-offthread-compile=off G:\mozilla-central\js\src\jit-test\tests\ion\dce-with-rinstructions.js

Then if you want to use iongraph [1], you might have to install graphviz [2].  The makefile of iongraph is mostly useless and it can easily be replaced by whatever you want.

[1] https://developer.mozilla.org/en-US/docs/SpiderMonkey/Hacking_Tips
[2] http://www.graphviz.org/Download_windows.php

Comment 21

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8472880 [details] [diff] [review]
bug1038593_regexpexec_rev4.diff

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

::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
@@ +567,5 @@
>  
> +var uceFault_regexp_exec = eval(uneval(uceFault).replace('uceFault', 'uceFault_regexp_exec'))
> +function rregexp_exec(i) {
> +    var re = new RegExp("str\\d+" + i + "\\d+rts");
> +    var res = re.exec("str01234567899876543210rts");

Prepend something to the strings to ensure that the re.lastIndex is either 0 or non-zero depending on the flag.

@@ +569,5 @@
> +function rregexp_exec(i) {
> +    var re = new RegExp("str\\d+" + i + "\\d+rts");
> +    var res = re.exec("str01234567899876543210rts");
> +    if (uceFault_regexp_exec(i) || uceFault_regexp_exec(i)) {
> +        assertEq(res, "str");

You will have to update the value of the assertion based on the result, because re.exec [1] returns an Array.  So you might want to check the content of the Array.

[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/exec

@@ +572,5 @@
> +    if (uceFault_regexp_exec(i) || uceFault_regexp_exec(i)) {
> +        assertEq(res, "str");
> +    }
> +
> +    assertEq(re.lastIndex == 0, "str");

I doubt that |true| is equal to |"str"|.

Comment 22

[Lawrence Abadier]

Attachment: bug1038593_regexpexec_rev5.diff

I finally got the tests and shell to run and was able to check all the arrays and outputs of each test-case. Jit_test.py path-to-shell, worked best for me as the most recent test cases are ran first.

Thank you,

Comment 23

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8473262 [details] [diff] [review]
bug1038593_regexpexec_rev5.diff

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

This is looking like going in the right direction, but have you verified that the compiler does not complain before testing?

::: js/src/jit/Recover.cpp
@@ +888,5 @@
>  
> +bool MRegExpExec::writeRecoverData(CompactBufferWriter &writer) const
> +{
> +    MOZ_ASSERT(canRecoverOnBailout());
> +    writer.writeUnsigned(uint32_t(RInstruction::RegExpExec));

Isn't the constant supposed to be Recover_RegExpExec?

@@ +896,5 @@
> +RRegExpExec::RRegExpExec(CompactBufferReader &reader)
> +{}
> +
> +bool RRegExpExec::recover(JSContext *cx, SnapshotIterator &iter) const{
> +    RootedObject regexp(cx, iter.read().toObject());

Don't we expect a reference to a JSObject and not a pointer?

@@ +901,5 @@
> +    RootedString input(cx, iter.read().toString());
> +
> +    RootedValue result(cx);
> +
> +    if(!regexp_exec_raw(cx, regexp, input, &result)

fix typo.

::: js/src/jit/Recover.h
@@ +471,5 @@
> +        return 2;
> +    }
> +
> +    bool recover(JSContext *cx, SnapshotIterator &iter) const;
> +}

missing semi-colon.

Comment 24

[Lawrence Abadier]

Attachment: bug1038593_regexpexec_rev6.diff

This patch provides me with no errors on compilation, please let me know how it looks.

Thank you,

Comment 25

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8474768 [details] [diff] [review]
bug1038593_regexpexec_rev6.diff

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

This patch looks good!
Congratulation :)

I pushed this patch to our Try server, which would be checking if it works against some tests cases.
Do you know what you might want to work on next?

Note that you should configure your editor to use Unix-style new lines (no '\r') [1]

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_Guide/Coding_Style

Comment 26

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:nbp] {N/A 22-29/08} from comment #25)
> I pushed this patch to our Try server, which would be checking if it works
> against some tests cases.

remote: You can view the progress of your build at the following URL:
remote:   https://tbpl.mozilla.org/?tree=Try&rev=1bfa8bf9fcb2
remote: Alternatively, view them on Treeherder (experimental):
remote:   https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1bfa8bf9fcb2

Comment 27

[Nicolas B. Pierron [:nbp]]

I fixed the issue locally, you should be able to get the same result by running the check-style makefile rule, which is using <mozilla-central>/config/check_spidermonkey_style.py

remote: You can view the progress of your build at the following URL:
remote:   https://tbpl.mozilla.org/?tree=Try&rev=4ce9fe0c8452
remote: Alternatively, view them on Treeherder (experimental):
remote:   https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4ce9fe0c8452

Comment 28

[Nicolas B. Pierron [:nbp]]

I pushed your patch to mozilla-inbound [1,2], if all goes well a sheriff will merge these changes to mozilla-central. Mozilla-central is the branch out of which we build Nightly versions of Firefox ;)

Congratulation!

[1] https://hg.mozilla.org/integration/mozilla-inbound/rev/8b1bfc63995e
[2] https://treeherder.mozilla.org/ui/#/jobs?repo=mozilla-inbound&revision=8b1bfc63995e

Comment 29

[Lawrence Abadier]

(In reply to Nicolas B. Pierron [:nbp] {N/A 22-29/08} from comment #25)
> Comment on attachment 8474768 [details] [diff] [review]
> bug1038593_regexpexec_rev6.diff
> 
> Review of attachment 8474768 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch looks good!
> Congratulation :)
> 
> I pushed this patch to our Try server, which would be checking if it works
> against some tests cases.
> Do you know what you might want to work on next?
> 
> Note that you should configure your editor to use Unix-style new lines (no
> '\r') [1]
> 
> [1]
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_Guide/Coding_Style

Tyvm, I will find something a little more difficult, that is also in the JS Engine. (Not a first bug)

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8b1bfc63995e