890722 - IonMonkey: implement JSOP_SETRVAL and JSOP_RETRVAL

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

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.

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: Patch

This was fairly easy. Passes jit-tests.

Comment 2

[Jan de Mooij [:jandem]]

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.

Comment 3

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/476a4bbbfdb2

Comment 4

[Hannes Verschore [:h4writer]]

Attachment: implement JSOP_POPV

JSOP_POPV is almost totally the same. Let us add this too.

Comment 5

[Hannes Verschore [:h4writer]]

Comment on attachment 776050 [details] [diff] [review]
implement JSOP_POPV

Oops, it is a bit harder than that.

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(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

Comment 7

[Hannes Verschore [:h4writer]]

Attachment: implement JSOP_POPV, JSOP_RETRVAL, JSOP_SETRVAL

Now hopefully done right ;)

Comment 8

[Hannes Verschore [:h4writer]]

Attachment: Testcase

I forgot to add the testcase I used to test the working. I'll land this too.

Comment 9

[Hannes Verschore [:h4writer]]

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

[Jan de Mooij [:jandem]]

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.

Comment 11

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef139b6034a5

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

[Ryan VanderMeulen [:RyanVM]]

This apparently also broke B2G mochitests.
https://tbpl.mozilla.org/php/getParsedLog.php?id=26778600&tree=Mozilla-Inbound

Comment 14

[Hannes Verschore [:h4writer]]

Attachment: bug890722

Updated patch to tip

Comment 15

[Hannes Verschore [:h4writer]]

Comment on attachment 806899 [details] [diff] [review]
bug890722

Would it be possible to fuzz this patch? Thanks!

Comment 16

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(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

[Fabio]

Comment #9 still need to be addressed, also #10?

Comment 18

[Hannes Verschore [:h4writer]]

(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

[Christian Holler (:decoder)]

Sorry, this got delayed. Can you give me the revision this patch applies to? I tried it on the backout revision and applying failed.

Comment 20

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Setting needinfo as per comment 19.

Comment 21

[Hannes Verschore [:h4writer]]

Sorry about the delay, still on taking PTO to visit Toronto now ;).
The (last) patch works on revision 9db2450f2a16.

Comment 22

[Christian Holler (:decoder)]

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?

Comment 23

[Hannes Verschore [:h4writer]]

Attachment: bug890722-setrval

Updated to tip

Comment 24

[Hannes Verschore [:h4writer]]

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

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/820aa1824ce0

Comment 26

[Fabio]

(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.

Comment 27

[Hannes Verschore [:h4writer]]

@Fabio: Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/645e0afbd37b

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/645e0afbd37b