IonMonkey: implement JSOP_SETRVAL and JSOP_RETRVAL

RESOLVED FIXED in mozilla27

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: h4writer, Assigned: h4writer)

Tracking

(Blocks 1 bug)

unspecified
mozilla27
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

6 years ago
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)

Comment 1

6 years ago
Posted patch Patch (obsolete) — Splinter Review
This was fairly easy. Passes jit-tests.
Assignee: general → hv1989
(Assignee)

Updated

6 years ago
Attachment #774266 - Flags: review?(jdemooij)
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 4

6 years ago
Posted patch implement JSOP_POPV (obsolete) — Splinter Review
JSOP_POPV is almost totally the same. Let us add this too.
Attachment #776050 - Flags: review?(jdemooij)
(Assignee)

Comment 5

6 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)

Updated

6 years ago
Blocks: 897962
(Assignee)

Comment 7

6 years ago
Now hopefully done right ;)
Attachment #774266 - Attachment is obsolete: true
Attachment #790912 - Flags: review?(jdemooij)
(Assignee)

Comment 8

6 years ago
Posted patch TestcaseSplinter Review
I forgot to add the testcase I used to test the working. I'll land this too.
(Assignee)

Comment 9

6 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 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 14

6 years ago
Posted patch bug890722 (obsolete) — Splinter Review
Updated patch to tip
Attachment #790912 - Attachment is obsolete: true
(Assignee)

Comment 15

6 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

6 years ago
Attachment #806899 - Flags: feedback?(gary)
Depends on: 919273
Attachment #806899 - Flags: feedback?(gary) → feedback+
(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

6 years ago
Comment #9 still need to be addressed, also #10?
(Assignee)

Comment 18

6 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 ;)
Sorry, this got delayed. Can you give me the revision this patch applies to? I tried it on the backout revision and applying failed.
Setting needinfo as per comment 19.
Flags: needinfo?(hv1989)
(Assignee)

Comment 21

6 years ago
Sorry about the delay, still on taking PTO to visit Toronto now ;).
The (last) patch works on revision 9db2450f2a16.
Flags: needinfo?(hv1989)
Flags: needinfo?(choller)
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)
Blocks: 922048
(Assignee)

Comment 23

6 years ago
Updated to tip
Attachment #806899 - Attachment is obsolete: true
Attachment #806899 - Flags: feedback?(choller)
Blocks: 922063
(Assignee)

Comment 24

6 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.
https://hg.mozilla.org/mozilla-central/rev/820aa1824ce0
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27

Comment 26

6 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.
You need to log in before you can comment on or make changes to this bug.