Closed
Bug 1202134
Opened 8 years ago
Closed 7 years ago
Nested try-finally overwrite .genrval
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla44
People
(Reporter: anba, Assigned: arai)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 1 obsolete file)
97.55 KB,
text/plain
|
Details | |
37.70 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
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•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
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.)
Reporter | ||
Comment 4•8 years ago
|
||
(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.
Reporter | ||
Comment 5•8 years ago
|
||
(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: }:-)
Reporter | ||
Comment 6•8 years ago
|
||
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•8 years ago
|
||
(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•8 years ago
|
||
(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..)
Assignee | ||
Comment 9•7 years ago
|
||
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 // }
Assignee | ||
Comment 10•7 years ago
|
||
looks like storing return value onto the stack is easier. WIP patch is working for non-generator.
Assignee | ||
Comment 11•7 years ago
|
||
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).
Assignee | ||
Comment 12•7 years ago
|
||
maybe dupe of bug 819125?
Assignee | ||
Comment 13•7 years ago
|
||
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
Assignee: nobody → arai.unmht
Attachment #8675407 -
Flags: review?(jdemooij)
Reporter | ||
Comment 14•7 years ago
|
||
(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•7 years ago
|
||
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 :)
Attachment #8675407 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 16•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/85bf82ac6e9a21b4b5aa956f98867ec5d19b6e20 Bug 1202134 - Save return value onto the stack before executing finally block. r=jandem
Assignee | ||
Comment 17•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3888eea6aaf2329e5f5f44fa2b56346627ebdc7e Backed out changeset 85bf82ac6e9a (bug 1202134) for OSX xpcshell test failure
Assignee | ||
Comment 18•7 years ago
|
||
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 :)
Assignee | ||
Comment 19•7 years ago
|
||
Pushed to try with the fix for BaselineCompiler::emit_JSOP_GETRVAL, suggested by jandem :) https://treeherder.mozilla.org/#/jobs?repo=try&revision=547be131f118
Assignee | ||
Comment 20•7 years ago
|
||
Oops, I pushed value to different stack :P https://treeherder.mozilla.org/#/jobs?repo=try&revision=ceba25cf3ff6
Assignee | ||
Comment 21•7 years ago
|
||
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
Attachment #8675407 -
Attachment is obsolete: true
Attachment #8678345 -
Flags: review?(jdemooij)
Updated•7 years ago
|
Attachment #8678345 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 22•7 years ago
|
||
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•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/00dac1d05d60
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in
before you can comment on or make changes to this bug.
Description
•