1202134 - Nested try-finally overwrite .genrval

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

Test case:
---
function* g() {
  try {
    return 0;
  } finally {
    L: try {
      return 1;
    } finally {
      break L;
    }
  }
}

var it = g();
it.next();
---

Expected: Returns `({value:0, done:true})`
Actual: Returns `({value:1, done:true})`

Comment 1

[Jeff Walden [:Waldo]]

You are a bad person and you should feel bad.

Cool testcase.  ;-)  After much spec-reading, I agree this checks out.  This isn't specific to ES6 generators: it's a fundamental defect of setting aside the return value to a single location, then consulting it if the finally-block doesn't throw.  (Although function* might require somewhat separate code to fix its problem.)  This testcase also fails in SpiderMonkey:

function g() {
  try {
    return 0;
  } finally {
    L: try {
      return 1;
    } finally {
      break L;
    }
  }
}
assertEq(g(), 0);

It looks to me like handling returns in try-blocks by saving the returned value aside, in a single location, simply doesn't work when the finally-block itself might contain a return that ends up aborted by a break statement.  We need more than one location, to handle the runtime possibility that one of those subsequent returns might be used as completion value.  So a return in a try-finally can't write to one canonical location: it has to consume some stack space, in the general case where the finally-block contains a break inside the finally of another try-finally whose try contains a return (to whatever nesting depth).

In short, .genrval delenda est, POP_RETURN_VALUE as currently used delenda est.

Comment 2

[Jeff Walden [:Waldo]]

I'm just gonna throw this out there, but has anyone investigated just how often break statements are used in finally-blocks in try-(catch-)?finally statements whose try- or catch-parts contain return statements?  Is there any chance at all that the spec could clamp down on break statements not in switches or loops to remove this insanity?  Hard to believe many people are even aware of it.

Comment 3

[Lars T Hansen [:lth]]

In general "done: { ... break done; ... }" is a pretty nice "goto done;" idiom and probably fairly well known.  I'm sure I've used it myself in non-test code, though I realize I may not be a typical JS developer.

(Probably 99% of the value of labeled break is to break out of several layers of loops, though.)

Comment 4

[André Bargull [:anba]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> Is there any chance at all that the spec could clamp down on break statements
> not in switches or loops to remove this insanity?

If you prefer loops instead of labelled statements:
---
do try {
  return 1;
} finally {
  break;
} while (0);
---


> I'm just gonna throw this out there, but has anyone investigated just how
> often break statements are used in finally-blocks in try-(catch-)?finally
> statements whose try- or catch-parts contain return statements?

Not only break, but also continue statements:
---
do try {
  return 1;
} finally {
  continue;
} while (0);
---


> You are a bad person and you should feel bad.

Comment 5

[André Bargull [:anba]]

(In reply to André Bargull from comment #4)
> > You are a bad person and you should feel bad.

Ugh, the Unicode smiley was removed. Let's try this one instead:

}:-)

Comment 6

[André Bargull [:anba]]

And I almost forgot the for-in and for-of versions:
---
function f() {
  try {
    return 0;
  } finally {
    var iter = {
      [Symbol.iterator]() { return this; },
      next() { return {value: 0, done:false}; },
      return() { throw null; }
    };
    try {
      for (var a of iter) return 1;
    } catch (e) {
    }

    // Or:
    try {
      for (var a in new Proxy({}, {enumerate() { return iter; })) return 1;
    } catch (e) { }
  }
}
---

Adding implicit try-finally statements to for-in/of was a great idea, right?!

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> Is there
> any chance at all that the spec could clamp down on break statements not in
> switches or loops to remove this insanity?

In addition to the other comments, here's one that doesn't require break/continue:

function g() {
    try {
        return 0;
    } finally {
        try {
            try {
                return 1;
            } finally {
                throw 9;
            }
        } catch(e) {}
    }
}
assertEq(g(), 0);

Does that make me also a bad person? ;)

Comment 8

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #7)
> In addition to the other comments, here's one that doesn't require
> break/continue:

(I now see comment 6 also uses try-catch. This one is a bit simpler though because no for-in/of..)

Comment 9

[Tooru Fujisawa [:arai]]

is it reasonable to make pseudo block that encloses try-finally and store return value to a hidden variable in that scope? (not yet figured out how to do it tho)
just like we're storing return value for generator into function-local .genrval, but using block-local variable instead of function-local, and apply same thing to non-generator.

here's mock up bytecode.

// {
//   let .tryvalue;
     try {                 try
       return 1;           one
                           setaliasedvar ".tryvalue" (hops = ?, slot = ?)
                           pop
                           gosub FINALLY
                           getaliasedvar ".tryvalue" (hops = ?, slot = ?)
                           return
                           // here may be some more codes...
     }
     finally {             FINALLY:
                           finally
     }                     retsub
// }

Comment 10

[Tooru Fujisawa [:arai]]

looks like storing return value onto the stack is easier.
WIP patch is working for non-generator.

Comment 11

[Tooru Fujisawa [:arai]]

here's a bytecode for WIP patch:

try {         try            //
  return 1;   one            // 1
              setrval        //                  -- set return value to frame
              gosub FINALLY  // false NEXT       -- when entering subroutime
                             //                     return value is stored in
                             //                     frame
              NEXT:          //                  -- after leaving subroutime
                             //                     return value is stored in
                             //                     frame
                             // retrval          -- return restored rval
}                            //
finally {     FINALLY:       // false NEXT
              finally        // false NEXT
              getrval        // false NEXT 1     -- get return value from frame
  ...         ...            // false NEXT 1 ... -- while executing finally
                             //                     body, return value is kept
                             //                     on stack
                             // false NEXT 1     -- just before setrval
                             //                     the return value is on the
                             //                     top of the stack
              setrval        // false NEXT       -- set return value to frame
}             retsub         //                  -- when leaving subroutime
                             //                     return value is stored in
                             //                     frame

getrval is the new opcode that pushes frame's return value onto the stack.  It's required to keep stack balanced and make it also work for |throw| case.  (for |return| case, we could push return value before gosub, but it's hard to make it work for |throw| case)

legacy generator in for-in (JSOP_ENDITER) might also need similar fix.
leaving for-of does nothing for now (bug 1147371).

Comment 12

[Tooru Fujisawa [:arai]]

maybe dupe of bug 819125?

Comment 13

[Tooru Fujisawa [:arai]]

Attachment: Save return value onto the stack before executing finally block.

Now .genrval is removed and PNK_RETURN is changed back to PN_UNARY.

As described in comment #11, finally block pushes return value onto the stack and keep it while executing the body, then pops and restores it when returning from subroutine.

JSOP_GETRVAL is supported in Interpreter and Baseline.  Ion does not support it, as try-finally is not yet supported (right?).

In GeneratorObject, now GeneratorThrowOrClose sets frame's return value instantly, as it will be restored by finally block.  SetReturnValueForClosingGenerator does nothing for return value.


Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=43a254f85e36

Comment 14

[André Bargull [:anba]]

(In reply to Tooru Fujisawa [:arai] from comment #12)
> maybe dupe of bug 819125?

I don't think so. bug 819125 should probably turned into a sub-issue for the (not yet created) meta-issue about implementing the new completion value semantics (http://wiki.ecmascript.org/doku.php?id=harmony:completion_reform).

Comment 15

[Jan de Mooij [:jandem]]

Comment on attachment 8675407 [details] [diff] [review]
Save return value onto the stack before executing finally block.

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

Thanks arai, this is so much nicer!

And great tests, I verified they also pass in es6draft.

::: js/src/jit/BaselineCompiler.cpp
@@ +3575,5 @@
>  
>  bool
> +BaselineCompiler::emit_JSOP_GETRVAL()
> +{
> +    masm.loadValue(frame.addressOfReturnValue(), R0);

Nit: add frame.syncStack(0); before this line, to ensure R0 is free.

Right now it's not strictly necessary because the stack should be synced at the start of a finally block, but later this op may be emitted elsewhere.

::: js/src/vm/Opcodes.h
@@ +102,5 @@
>       *   Stack: => undefined
>       */ \
>      macro(JSOP_UNDEFINED, 1,  js_undefined_str, "",       1,  0,  1, JOF_BYTE) \
> +    /*
> +     * Pushes stack frame as 'rval' onto the stack.

s/stack frame as/stack frame's/ ?

@@ +108,5 @@
> +     *   Type: Function
> +     *   Operands:
> +     *   Stack: => rval
> +     */ \
> +    macro(JSOP_GETRVAL,   2,  "getrval",    NULL,         1,  0,  1, JOF_BYTE) \

Don't forget to bump XDR_BYTECODE_VERSION_SUBTRAHEND in vm/Xdr.h :)

Comment 16

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/85bf82ac6e9a21b4b5aa956f98867ec5d19b6e20
Bug 1202134 - Save return value onto the stack before executing finally block. r=jandem

Comment 17

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3888eea6aaf2329e5f5f44fa2b56346627ebdc7e
Backed out changeset 85bf82ac6e9a (bug 1202134) for OSX xpcshell test failure

Comment 18

[Tooru Fujisawa [:arai]]

Attachment: crash.txt

here is the crash dump for the OSX xpcshell test failure
https://treeherder.mozilla.org/logviewer.html#?job_id=16108484&repo=mozilla-inbound

maybe there's some assumption about the stack for finally block inside baseline or GC?
it's not reproducible locally.  I'll try debugging it on try.  any hints are welcome :)

Comment 19

[Tooru Fujisawa [:arai]]

Pushed to try with the fix for BaselineCompiler::emit_JSOP_GETRVAL, suggested by jandem :)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=547be131f118

Comment 20

[Tooru Fujisawa [:arai]]

Oops, I pushed value to different stack :P
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ceba25cf3ff6

Comment 21

[Tooru Fujisawa [:arai]]

Attachment: Save return value onto the stack before executing finally block. r=jandem

fixed BaselineCompiler::emit_JSOP_GETRVAL() to push undefined when HAS_RVAL is not set.
no other changes.

green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ceba25cf3ff6

Comment 22

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/00dac1d05d6097e885f1115ec74a64a9f92d044d
Bug 1202134 - Save return value onto the stack before executing finally block. r=jandem

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/00dac1d05d60