Closed
Bug 1024609
Opened 10 years ago
Closed 10 years ago
IonMonkey: Implement ArgumentsLength Recover Instruction
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
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
Updated•10 years ago
|
Mentor: nicolas.b.pierron
Whiteboard: [good next bug][mentor=nbp][lang=c++] → [good next bug][lang=c++]
Assignee | ||
Comment 1•10 years ago
|
||
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
Reporter | ||
Comment 2•10 years ago
|
||
(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?
Reporter | ||
Comment 3•10 years ago
|
||
(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.
Assignee | ||
Comment 4•10 years ago
|
||
(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.
Assignee | ||
Comment 5•10 years ago
|
||
Assignee | ||
Comment 6•10 years ago
|
||
Assignee | ||
Comment 7•10 years ago
|
||
Assignee | ||
Comment 8•10 years ago
|
||
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
Reporter | ||
Comment 9•10 years ago
|
||
(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) { ;)
Assignee | ||
Comment 11•10 years ago
|
||
New IonGraph for the new test case.
Attachment #8443182 -
Attachment is obsolete: true
Assignee | ||
Comment 12•10 years ago
|
||
Nicilas, assign this bug for me, please.
Assignee | ||
Comment 13•10 years ago
|
||
IonGraph without the patch applied
Attachment #8443473 -
Attachment is obsolete: true
Assignee | ||
Comment 14•10 years ago
|
||
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?
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → ticaiolima
Reporter | ||
Comment 15•10 years ago
|
||
(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?
Assignee | ||
Comment 16•10 years ago
|
||
The backtrace is https://pastebin.mozilla.org/5476520. I've applied the complete patch.
Assignee | ||
Comment 17•10 years ago
|
||
I've rebased the patch.
Attachment #8443184 -
Attachment is obsolete: true
Assignee | ||
Comment 18•10 years ago
|
||
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?
Reporter | ||
Comment 19•10 years ago
|
||
(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++.
Assignee | ||
Comment 20•10 years ago
|
||
(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.
Reporter | ||
Comment 21•10 years ago
|
||
(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.
Assignee | ||
Comment 22•10 years ago
|
||
(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().
Assignee | ||
Comment 23•10 years ago
|
||
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)
Reporter | ||
Comment 24•10 years ago
|
||
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+
Assignee | ||
Comment 25•10 years ago
|
||
(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
Reporter | ||
Comment 26•10 years ago
|
||
(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.
Assignee | ||
Comment 27•10 years ago
|
||
Assignee | ||
Comment 28•10 years ago
|
||
Assignee | ||
Comment 29•10 years ago
|
||
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
Assignee | ||
Comment 30•10 years ago
|
||
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)
Reporter | ||
Comment 31•10 years ago
|
||
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+
Assignee | ||
Comment 32•10 years ago
|
||
Attachment #8451137 -
Flags: review+
Assignee | ||
Comment 33•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=e4cd2e3b5784
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•10 years ago
|
Attachment #8451108 -
Attachment is obsolete: true
Comment 34•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/14ab3f4d2526
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/14ab3f4d2526
Status: NEW → RESOLVED
Closed: 10 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.
Description
•