Closed
Bug 890722
Opened 11 years ago
Closed 11 years ago
IonMonkey: implement JSOP_SETRVAL and JSOP_RETRVAL
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla27
People
(Reporter: h4writer, Assigned: h4writer)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 4 obsolete files)
892 bytes,
patch
|
Details | Diff | Splinter Review | |
34.37 KB,
patch
|
Details | Diff | Splinter Review |
This mostly happens with try blocks, so I think because we don't support try blocks we didn't hit this often enough to bother implementing it. Now I got some reports we hit this with for in loops and I also think this is quite easy, so lets fix this.
Assignee | ||
Updated•11 years ago
|
Attachment #774266 -
Flags: review?(jdemooij)
Comment 2•11 years ago
|
||
Comment on attachment 774266 [details] [diff] [review] Patch Review of attachment 774266 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! ::: js/src/ion/IonBuilder.h @@ +649,5 @@ > > // If this is an inline builder, the call info for the builder. > const CallInfo *inlineCallInfo_; > + > + // return value (if not immediatly returning) Nit: typo: immediately, and s/return/Return while you're here.
Attachment #774266 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 3•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/476a4bbbfdb2
Assignee | ||
Comment 4•11 years ago
|
||
JSOP_POPV is almost totally the same. Let us add this too.
Attachment #776050 -
Flags: review?(jdemooij)
Assignee | ||
Comment 5•11 years ago
|
||
Comment on attachment 776050 [details] [diff] [review] implement JSOP_POPV Oops, it is a bit harder than that.
Attachment #776050 -
Attachment is obsolete: true
Attachment #776050 -
Flags: review?(jdemooij)
(In reply to Hannes Verschore [:h4writer] from comment #3) > https://hg.mozilla.org/integration/mozilla-inbound/rev/476a4bbbfdb2 I think this broke several tests, at least on debug builds: https://tbpl.mozilla.org/php/getParsedLog.php?id=25303797&tree=Mozilla-Inbound 16:50:19 INFO - Assertion failure: retval_ && current == retval_->block(), at ../../../js/src/ion/IonBuilder.cpp:3199 looks really close to where your patch touched that file... Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b2006b48cb54
Assignee | ||
Comment 7•11 years ago
|
||
Now hopefully done right ;)
Attachment #774266 -
Attachment is obsolete: true
Attachment #790912 -
Flags: review?(jdemooij)
Assignee | ||
Comment 8•11 years ago
|
||
I forgot to add the testcase I used to test the working. I'll land this too.
Assignee | ||
Comment 9•11 years ago
|
||
Comment on attachment 790912 [details] [diff] [review] implement JSOP_POPV, JSOP_RETRVAL, JSOP_SETRVAL Review of attachment 790912 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/IonFrameIterator.h @@ +415,5 @@ > unsigned argsObjAdj = it.script()->argumentsHasVarBinding() ? 1 : 0; > SnapshotIterator parent_s(it.snapshotIterator()); > > // Skip over all slots untill we get to the last slots (= arguments slots of callee) > + // the +2 is for [this], [returnvalue], [scopechain], and maybe +1 for [argsObj] s/+2/+3/
Comment 10•11 years ago
|
||
Comment on attachment 790912 [details] [diff] [review] implement JSOP_POPV, JSOP_RETRVAL, JSOP_SETRVAL Review of attachment 790912 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/BaselineIC.cpp @@ +836,5 @@ > > + // Set return value. > + if (frame->hasReturnValue()) { > + *((uint32_t *) (stackFrame + StackFrame::offsetOfFlags())) |= StackFrame::HAS_RVAL; > + *((Value *) (stackFrame + StackFrame::offsetOfReturnValue())) = *(frame->returnValue()); Hm this reminds me, we no longer do interpreter -> Ion OSR so we should rewrite Baseline -> Ion OSR to no longer use a fake StackFrame. Will file a bug. ::: js/src/jit/CompileInfo.h @@ +185,3 @@ > } > uint32_t thisSlot() const { > + JS_ASSERT(fun() && nimplicit_ > 0); Nit: use a separate JS_ASSERT(nimplicit_ > 0), so that if it fails it's easier to find out what's going on. ::: js/src/jit/IonBuilder.cpp @@ +3362,5 @@ > > case JSOP_STOP: > + case JSOP_RETRVAL: > + def = current->getSlot(info().returnValueSlot()); > + break; Can you add a MConstant(UndefinedValue()) like before if op == JSOP_STOP && script()->noScriptRval I know it shouldn't matter usually due to phi elimination etc, but since this case is so common it seems best to be explicit about it.
Attachment #790912 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 11•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef139b6034a5
Comment 12•11 years ago
|
||
Backed out for SM rootanalysis failures. https://hg.mozilla.org/integration/mozilla-inbound/rev/1718a2f065c6 https://tbpl.mozilla.org/php/getParsedLog.php?id=26773853&tree=Mozilla-Inbound
Comment 13•11 years ago
|
||
This apparently also broke B2G mochitests. https://tbpl.mozilla.org/php/getParsedLog.php?id=26778600&tree=Mozilla-Inbound
Assignee | ||
Comment 14•11 years ago
|
||
Updated patch to tip
Attachment #790912 -
Attachment is obsolete: true
Assignee | ||
Comment 15•11 years ago
|
||
Comment on attachment 806899 [details] [diff] [review] bug890722 Would it be possible to fuzz this patch? Thanks!
Attachment #806899 -
Flags: feedback?(choller)
Assignee | ||
Updated•11 years ago
|
Attachment #806899 -
Flags: feedback?(gary)
Updated•11 years ago
|
Attachment #806899 -
Flags: feedback?(gary) → feedback+
Comment 16•11 years ago
|
||
(feedback+ in that I didn't find any issues that might help resolve the orange issue, judging from what I heard from decoder)
Comment 17•11 years ago
|
||
Comment #9 still need to be addressed, also #10?
Assignee | ||
Comment 18•11 years ago
|
||
(In reply to Fabio from comment #17) > Comment #9 still need to be addressed, also #10? Comment #10 is addressed in the patch. It indeed looks like I forget to adjust the comment from +3 to +2. (In reply to Gary Kwong [:gkw] [:nth10sd] (PTO Sep 24 - 27) from comment #16) > (feedback+ in that I didn't find any issues that might help resolve the > orange issue, judging from what I heard from decoder) The first time I'm not really happy to have feedback+ from you :P. Thanks for testing, though ;)
Comment 19•11 years ago
|
||
Sorry, this got delayed. Can you give me the revision this patch applies to? I tried it on the backout revision and applying failed.
Assignee | ||
Comment 21•11 years ago
|
||
Sorry about the delay, still on taking PTO to visit Toronto now ;). The (last) patch works on revision 9db2450f2a16.
Flags: needinfo?(hv1989)
Updated•11 years ago
|
Flags: needinfo?(choller)
Comment 22•11 years ago
|
||
I've been fuzzing this for days now, and I haven't found anything specific to this patch. Could it be that the issue is ARM specific? Or did this also happen on x86 b2g?
Flags: needinfo?(choller)
Assignee | ||
Comment 23•11 years ago
|
||
Updated to tip
Attachment #806899 -
Attachment is obsolete: true
Attachment #806899 -
Flags: feedback?(choller)
Assignee | ||
Comment 24•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/820aa1824ce0 I think I found the issue. Try push was finally green :D. Arm was failing on runtime on va_list with a Value as argument in code that is actually a nop in optimized builds. Very strange.
Comment 25•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/820aa1824ce0
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Comment 26•11 years ago
|
||
(In reply to Hannes Verschore [:h4writer] from comment #18) > (In reply to Fabio from comment #17) > > Comment #9 still need to be addressed. > It indeed looks like I forget to > adjust the comment from +3 to +2. This is still missing in the committed code.
Assignee | ||
Comment 27•11 years ago
|
||
@Fabio: Thanks! https://hg.mozilla.org/integration/mozilla-inbound/rev/645e0afbd37b
You need to log in
before you can comment on or make changes to this bug.
Description
•