Closed Bug 1024609 Opened 8 years ago Closed 8 years ago

IonMonkey: Implement ArgumentsLength Recover Instruction

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: nbp, Assigned: caiolima, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good next bug][lang=c++])

Attachments

(3 files, 10 obsolete files)

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

Note that opposed to other Recover instruction, this bug is not a "good first bug", and implies that we recover some dynamic property which is not handled by the snapshot yet.

  function f() {
    return arguments.length;
  }

  assertEq(f(1,2), 2);
  assertEq(f(1,2,3,4,5), 5);

To add this feature, I recommend to have a look at how the arguments length[1] is implemented in the CodeGenerator.cpp, and see how we can translate it into something useable in the SnapshotIterator.  Notice that the number of actual arguments (aka arguments.length of the outer frame) is already available in the IonJSFrameLayout, as well as the JitFrameIterator.

[1] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/CodeGenerator.cpp#6441
Mentor: nicolas.b.pierron
Whiteboard: [good next bug][mentor=nbp][lang=c++] → [good next bug][lang=c++]
AS I can see Analysing the http://dxr.mozilla.org/mozilla-central/source/js/src/jit/IonBuilder.cpp#8640, It is generated when the property "length" is called from "arguments" objects. So, I was looking at SnapshotIterator[1] and It has a IonJSFrameLayout that can give us the addr of "argc". Based on that, I'm thinking in retrieve it and store on "inter.storeInstructionResult"

Do you think that it's the right way?

[1] - http://dxr.mozilla.org/mozilla-central/source/js/src/jit/JitFrameIterator.h?from=SnapshotIterator&case=true#267
(In reply to Caio Lima(:caiolima) from comment #1)
> So, I was looking at SnapshotIterator[1] and It has a
> IonJSFrameLayout that can give us the addr of "argc". Based on that, I'm
> thinking in retrieve it and store on "inter.storeInstructionResult"

The SnapshotIterator is here to abstract from where the information is coming from, so I would be disappointed if we transmit this information in terms of of pointer, could we add a function such as readOuterNumActualArgs() on the Snapshot iterator, and use it in the recover function?
(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> (In reply to Caio Lima(:caiolima) from comment #1)
> > So, I was looking at SnapshotIterator[1] and It has a
> > IonJSFrameLayout that can give us the addr of "argc". Based on that, I'm
> > thinking in retrieve it and store on "inter.storeInstructionResult"
> 
> The SnapshotIterator is here to abstract from where the information is
> coming from, so I would be disappointed if we transmit this information in
> terms of of pointer, could we add a function such as
> readOuterNumActualArgs() on the Snapshot iterator, and use it in the recover
> function?

The other solution, might be to have it as part of the value allocations, such as we do not even need a recover instruction and it can save the memory needed by the recover instruction, see how the value allocations are encoded.
(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> (In reply to Caio Lima(:caiolima) from comment #1)
> > So, I was looking at SnapshotIterator[1] and It has a
> > IonJSFrameLayout that can give us the addr of "argc". Based on that, I'm
> > thinking in retrieve it and store on "inter.storeInstructionResult"
> 
> The SnapshotIterator is here to abstract from where the information is
> coming from, so I would be disappointed if we transmit this information in
> terms of of pointer, could we add a function such as
> readOuterNumActualArgs() on the Snapshot iterator, and use it in the recover
> function?

I was thinking exactly by this way. 

(In reply to Nicolas B. Pierron [:nbp] from comment #3)

> The other solution, might be to have it as part of the value allocations,
> such as we do not even need a recover instruction and it can save the memory
> needed by the recover instruction, see how the value allocations are encoded.

Sory, but i didn't understand what we can do here.
Attached image func09-pass16-DCE-mir.gv.png (obsolete) —
Attached file uce_argument_length.js (obsolete) —
Attached patch 1024609.patch (obsolete) — Splinter Review
Nicolas, I've implemented the RArgumentsLength like the way before, but I've identified a problem on this instruction Bailout. As you can see on [1], the boundscheck instruction uses the value of argumentslength, so how the boundscheck is notRecoveredOnBailout, the argumentslength neither can be bailout.

So, What do you think? how could we proceed? The iongraph is about the script [2]

[1] - https://bugzilla.mozilla.org/attachment.cgi?id=8443182
[2] - https://bugzilla.mozilla.org/attachment.cgi?id=8443183
(In reply to Caio Lima(:caiolima) from comment #6)
> Created attachment 8443183 [details]
> uce_argument_length.js

The fact that the argument "i" is extracted from the arguments vector add a MArgumentsLength and the bound check to bailout if there is not enough arguments.  I suggest you define the function as 

  function rargumentslength(i) {

;)
Attached file uce_argument_length.js (obsolete) —
New test case
Attachment #8443183 - Attachment is obsolete: true
Attached image func07-pass17-DCE-mir.gv.png (obsolete) —
New IonGraph for the new test case.
Attachment #8443182 - Attachment is obsolete: true
Depends on: 1028262
Nicilas, assign this bug for me, please.
Attached image func01-pass16-DCE-mir.gv.png (obsolete) —
IonGraph without the patch applied
Attachment #8443473 - Attachment is obsolete: true
I'm working on this bug agin, but now, when I've applied this patch the js engine is returning this following error:

Hit MOZ_CRASH(indexing into zero-length array) at ../../dist/include/mozilla/Array.h:49
[New Thread 0x170b of process 16391]
[New Thread 0x1803 of process 16391]
[New Thread 0x1903 of process 16391]
[New Thread 0x1a03 of process 16391]
[New Thread 0x1b03 of process 16391]

Catchpoint 1 (signal SIGSEGV), 0x0000000100518ad2 in mozilla::Array<js::jit::MUse, 0ul>::operator[](unsigned long) const ()


Without the patch, the iondraph is this:
https://bugzilla.mozilla.org/attachment.cgi?id=8445950

I don't know the reason for this error. I was guessing that it could be generated by the http://dxr.mozilla.org/mozilla-central/source/js/src/jit/MIR.cpp#377 but It's not true, because I've debugged it. The source of the problem can be http://dxr.mozilla.org/mozilla-central/source/js/src/jit/IonAnalysis.cpp#2471 but I've not debuged it yet. Any tips?
Assignee: nobody → ticaiolima
(In reply to Caio Lima(:caiolima) from comment #14)
> Any tips?

What is the backtrace, when is this called?
Did the patch apply completely & correctly?
Can you attach the diff?
The backtrace is https://pastebin.mozilla.org/5476520.

I've applied the complete patch.
Attached patch rargumentslength.patch (obsolete) — Splinter Review
I've rebased the patch.
Attachment #8443184 - Attachment is obsolete: true
Attached patch jump_operands_loop.diff (obsolete) — Splinter Review
Nicolas, I was trying to identify the error on comment 16, But I don't know how to proceed. I've tried to skip the operands loops when the num of operands is null (see on the this diff patch), but I've got no success, becase the SanapShot is generated with inconsistent values. My guess is that this bug is because of the negligence of NullAry Intructions cases, in another words, how to encode Snapshots to instructions without operands.

The Backtrace is
https://pastebin.mozilla.org/5478913

Could you help me?
(In reply to Caio Lima(:caiolima) from comment #18)
> My guess is that
> this bug is because of the negligence of NullAry Intructions cases, in
> another words, how to encode Snapshots to instructions without operands.

The iterator should correctly iterate valid instruction, so adding such work-around is wrong.
Have a look at the operator++.
(In reply to Nicolas B. Pierron [:nbp] from comment #19)
> The iterator should correctly iterate valid instruction, so adding such
> work-around is wrong.
> Have a look at the operator++.

I've debugged it one more time and the operator++ doesn't execute when the code reach teh error. Let me explain the program behavior when I was debugging, It's interesting if you reproduce it.

1. When the OperandIter begin is setted with recoverInfo.begin(), the MArgumentsLength is retrieved;
2. When the it->isRecoveredOnBailout() on http://dxr.mozilla.org/mozilla-central/source/js/src/jit/LIR.cpp#189 is called, the operator-> from OperandIter is called.
3. The operator-> function calls http://dxr.mozilla.org/mozilla-central/source/js/src/jit/LIR.h#950, at this point, op_ is 0;
4. getOperand() , method called is http://dxr.mozilla.org/mozilla-central/source/js/src/jit/MIR.h#817 MAryInstruction::getOperand, because MArgumentsLength is an specialization of MNullaryInstruction that is a subclass of MAryInstruction<0>.
5. As you can see on http://dxr.mozilla.org/mozilla-central/source/js/src/jit/MIR.h#802, the operands_ is created as a zero-length array, and It's the problem's source.

This is the analysis that I've found debugging.
(In reply to Caio Lima(:caiolima) from comment #20)
> (In reply to Nicolas B. Pierron [:nbp] from comment #19)
> > The iterator should correctly iterate valid instruction, so adding such
> > work-around is wrong.
> > Have a look at the operator++.
> 
> I've debugged it one more time and the operator++ doesn't execute when the
> code reach teh error. Let me explain the program behavior when I was
> debugging, It's interesting if you reproduce it.
> 
> 1. When the OperandIter begin is setted with recoverInfo.begin(), the
> MArgumentsLength is retrieved;

So you probably need to add a settle() function to ensure that we settle on a valid operand.

And use it also inside the operator++() function, such as it would work too if ArgumentsLength is not the first one but the second one.
(In reply to Nicolas B. Pierron [:nbp] from comment #21)
> (In reply to Caio Lima(:caiolima) from comment #20)
> > (In reply to Nicolas B. Pierron [:nbp] from comment #19)
> > > The iterator should correctly iterate valid instruction, so adding such
> > > work-around is wrong.
> > > Have a look at the operator++.
> > 
> > I've debugged it one more time and the operator++ doesn't execute when the
> > code reach teh error. Let me explain the program behavior when I was
> > debugging, It's interesting if you reproduce it.
> > 
> > 1. When the OperandIter begin is setted with recoverInfo.begin(), the
> > MArgumentsLength is retrieved;
> 
> So you probably need to add a settle() function to ensure that we settle on
> a valid operand.
> 
> And use it also inside the operator++() function, such as it would work too
> if ArgumentsLength is not the first one but the second one.

Sorry, but i have no ideia what can be the settle().
Depends on: 1031682
Attached patch rargumentslength.patch (obsolete) — Splinter Review
Implemented RArgumentsLength and the test case.
Attachment #8443471 - Attachment is obsolete: true
Attachment #8445950 - Attachment is obsolete: true
Attachment #8446946 - Attachment is obsolete: true
Attachment #8446955 - Attachment is obsolete: true
Attachment #8450720 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8450720 [details] [diff] [review]
rargumentslength.patch

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

This patch looks good, I just want to make sure that we do test all context in which we can produce such recover instruction.

::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
@@ +322,5 @@
>  }
>  
> +var uceFault_arguments_length = eval(uneval(uceFault).replace('uceFault', 'uceFault_arguments_length'));
> +function rarguments_length(i) {
> +    var x = arguments.length;

copy this function 4 times for:

1. calling it with 1 actual argument.
2. calling it with 3 actual arguments (while expecting 1 formal argument).
3. calling it with 1 actual argument, but returning arguments.length from an inlined function called with fun.apply.
4. calling it with 3 actual argument, but returning arguments.length from an inlined function called with fun.apply.
Attachment #8450720 - Flags: review?(nicolas.b.pierron) → feedback+
(In reply to Nicolas B. Pierron [:nbp] from comment #24)
> Comment on attachment 8450720 [details] [diff] [review]
> rargumentslength.patch
> 
> Review of attachment 8450720 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch looks good, I just want to make sure that we do test all context
> in which we can produce such recover instruction.
> 
> ::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
> @@ +322,5 @@
> >  }
> >  
> > +var uceFault_arguments_length = eval(uneval(uceFault).replace('uceFault', 'uceFault_arguments_length'));
> > +function rarguments_length(i) {
> > +    var x = arguments.length;
> 
> copy this function 4 times for:
> 
> 1. calling it with 1 actual argument.
> 2. calling it with 3 actual arguments (while expecting 1 formal argument).
> 3. calling it with 1 actual argument, but returning arguments.length from an
> inlined function called with fun.apply.
When do you say inlined function, are you refering to this kind of test case [1]? 

I've tested this case, and ist's not rising the recover instruction.

[1] - https://pastebin.mozilla.org/5517809
(In reply to Caio Lima(:caiolima) from comment #25)
> > 3. calling it with 1 actual argument, but returning arguments.length from an
> > inlined function called with fun.apply.
> When do you say inlined function, are you refering to this kind of test case
> [1]? 
> 
> I've tested this case, and ist's not rising the recover instruction.
> 
> [1] - https://pastebin.mozilla.org/5517809

I meant:

function ret_argumentsLength() { return arguments.length; }

var uceFault_inline_arguments_length_3 = eval(uneval(uceFault).replace('uceFault', 'uceFault_inline_arguments_length_3'));
function rinline_arguments_length_3(i) {
    var x = ret_argumentsLength.apply(this, arguments);
    if (uceFault_inline_arguments_length_3(i) || uceFault_inline_arguments_length_3(i))
        assertEq(x, 3);
    return i;
}

with rinline_arguments_length_3 being called with 3 arguments.
This test case either did not rise the Recover Intruction, as you can see here [1].

I'm attatching the or rinline_arguments_length_3 ioncode either [2].

Maybe the box instruction is not allowing the recvover, but I actually don't understand why it's a test case that can rise the RArgumentsLength, since It's another function and the UCE fault will not perform nothing on it. I guess that it sshould be a test case if the function ret_argumentsLength is inlined on rinline_arguments_length_3 before the UCE fault, but that's  not the case.

[1] - https://bugzilla.mozilla.org/attachment.cgi?id=8451042
[2] - https://bugzilla.mozilla.org/attachment.cgi?id=8451041
Attached patch rargumentslength.patch (obsolete) — Splinter Review
Created new test cases for cases that rise the RArgumentsLength. It's important to notice that uceFault_inline_arguments_length_1 and uceFault_inline_arguments_length_3 are not rising the instruction because it should inline the ret_argumentsLength because of ret_argumentsLength.apply(this, arguments); but it's not happening. But I guess that it's not a problem to land this patch, is it?
Attachment #8450720 - Attachment is obsolete: true
Attachment #8451108 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8451108 [details] [diff] [review]
rargumentslength.patch

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

Do the modification, test it on try, and ask for landing it ;)

::: js/src/jit/Recover.cpp
@@ +763,5 @@
> +RArgumentsLength::RArgumentsLength(CompactBufferReader &reader)
> +{ }
> +
> +bool
> +RArgumentsLength::recover(JSContext *cx, SnapshotIterator &iter) const

Move these functions between StringLength and Floor.
Attachment #8451108 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8451137 - Flags: review+
Keywords: checkin-needed
Attachment #8451108 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/14ab3f4d2526
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.