Closed
Bug 1322069
Opened 8 years ago
Closed 7 years ago
Create a class to handle JumpList, TryFinallyControl, and depth in try-catch-finally in BytecodeEmitter.
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: arai, Assigned: arai)
References
Details
Attachments
(1 file, 3 obsolete files)
22.33 KB,
patch
|
shu
:
review+
|
Details | Diff | Splinter Review |
Just like bug 1308383, it would be nice to have TryEmitter that handles details about JumpList, TryFinallyControl, JumpTarget, depth, note, etc for try-catch-finally. Bug 1147371 will require additional try-catch or try-finally in several places, so having helper class will make it cleaner. I already have draft patch, will post after some more test.
Assignee | ||
Comment 1•8 years ago
|
||
here's WIP patch.
Assignee | ||
Comment 2•8 years ago
|
||
example patch that uses TryEmitter, for bug 1147371.
Assignee | ||
Comment 3•8 years ago
|
||
Assignee | ||
Comment 4•7 years ago
|
||
Just like IfThenElseEmitter. Usage example: > TryEmitter tryCatch(this /* = bce */, TryEmitter::TryCatchFinally); > > if (!tryCatch.emitTry()) > return false; > > // emit try-block here > > if (!tryCatch.emitCatch()) > return false; > > // emit catch-block here > > if (!tryCatch.emitFinally()) > return false; > > // emit finally-block here > > if (!tryCatch.emitEnd()) > return false; catch block can be multiple, by calling emitCatch for each > TryEmitter tryCatch(this /* = bce */, TryEmitter::TryCatchFinally); > > if (!tryCatch.emitTry()) > return false; > > // emit try-block here > > if (!tryCatch.emitCatch()) > return false; > > // emit conditional catch-block here > > if (!tryCatch.emitCatch()) > return false; > > // emit conditional catch-block here > > if (!tryCatch.emitCatch()) > return false; > > // emit maybe-conditional catch-block here > > if (!tryCatch.emitFinally()) > return false; > > // emit finally-block here > > if (!tryCatch.emitEnd()) > return false; TryEmitter constructor receives several kind of flags: Kind the existence of each block (to emit proper bytecode for each case, before reaching that block) * TryCatch * TryCatchFinally * TryFinally RetValKind whether to handle return value (RETVAL) or not if TryEmitter is used for internal-use, we don't have to maintain return value ControlKind provided for yield* case, to keep the bytecode same see code comment for the detail
Attachment #8816808 -
Attachment is obsolete: true
Attachment #8816809 -
Attachment is obsolete: true
Attachment #8816852 -
Attachment is obsolete: true
Attachment #8831522 -
Flags: review?(shu)
Comment 5•7 years ago
|
||
Comment on attachment 8831522 [details] [diff] [review] Add TryEmitter. Review of attachment 8831522 [details] [diff] [review]: ----------------------------------------------------------------- Really nice refactoring. r=me with the enum collapsed. ::: js/src/frontend/BytecodeEmitter.cpp @@ +1576,5 @@ > + TryCatch, > + TryCatchFinally, > + TryFinally > + }; > + enum RetValKind { Nit: the Kind suffix doesn't feel quite right for these yes/no enums. How about |enum ShouldUseRetVal| and |enum ShouldUseControl|? @@ +1583,5 @@ > + }; > + enum ControlKind { > + UseControl, > + DontUseControl, > + }; I can't imagine when we would have |UseRetVal && DontUseControl| or |DontUseRetVal && UseControl|. We should collapse the two. I wonder if "IsSyntactic" is a simpler name to distinguish the internal desugaring use in yield*. @@ +1596,5 @@ > + // When a finally block is active, non-local jumps (including > + // jumps-over-catches) result in a GOSUB being written into the bytecode > + // stream and fixed-up later. > + // > + // If ControlKind is DontUseControl, all those handling is skipped. Nit: those -> that @@ +1603,5 @@ > + // * has no catch guard > + // * has JSOP_GOTO at the end of catch-block > + // * has no non-local-jump > + // * doesn't use finally block for normal completion of try-block and > + // catch-block Very helpful comment. @@ +1734,5 @@ > + private: > + bool emitCatchEnd(bool hasNext) { > + MOZ_ASSERT(state_ == Catch); > + > + if (!controlInfo_.isSome()) You can use Maybe<T> like a pointer. It'd be more idiomatic to do |if (!controlInfo_)| @@ +1752,5 @@ > + > + // If this catch block had a guard clause, patch the guard jump to > + // come here. > + if (controlInfo_.ref().guardJump.offset != -1) { > + if (!bce_->emitJumpTargetAndPatch(controlInfo_.ref().guardJump)) |controlInfo_->guardJump| should work. There are several other uses of .ref() below. Please change those too.
Attachment #8831522 -
Flags: review?(shu) → review+
Assignee | ||
Comment 6•7 years ago
|
||
Thank you for reviewing :) (In reply to Shu-yu Guo [:shu] from comment #5) > @@ +1583,5 @@ > > + }; > > + enum ControlKind { > > + UseControl, > > + DontUseControl, > > + }; > > I can't imagine when we would have |UseRetVal && DontUseControl| or > |DontUseRetVal && UseControl|. We should collapse the two. > > I wonder if "IsSyntactic" is a simpler name to distinguish the internal > desugaring use in yield*. sorry, forgot to note, in bug 1331092 I need |DontUseRetVal && UseControl| case. https://tc39.github.io/proposal-async-iteration/#sec-runtime-semantics-forin-div-ofbodyevaluation-lhs-stmt-iterator-lhskind-labelset > 3.2.9 Runtime Semantics: ForIn/OfBodyEvaluation > ... > 6. Repeat > ... > l. If status is an abrupt completion, then > i. Set the running execution context's LexicalEnvironment to oldEnv. > ii. If iteratorKind is async, return ? AsyncIteratorClose(iterator, status). > iii. Return ? IteratorClose(iterator, status). > ... to handle abrupt completion in for-await-of body, I'm about to use non-syntactic try-catch. there try-block can contain non-local jump, so I need UseControl. here's comment from for-await-of patch. > + // for (x of y) { > + // // Operations for iterator (IteratorNext etc) are outside of > + // // try-block. > + // try { > + // ... > + // if (...) { > + // // 1. process StatementKind::Finally > + // // 2. close iterator before break/non-local continue/return, > + // // outside of try-catch > + // AsyncIteratorClose(iterator, { break }); > + // break; > + // } > + // ... > + // } catch (e) { > + // // This catch block isn't executed when AsyncIteratorClose for > + // // non-local jump throws. > + // AsyncIteratorClose(iterator, { throw, e }); > + // throw e; > + // } > + // } that case can use DontUseRetVal, since the catch-block always throws, so we don't have to clear return value immediately at the top of the catch-block > iii. Return ? IteratorClose(iterator, status). > + if (retValKind_ == UseRetVal) { > + // Clear the frame's return value that might have been set by the > + // try block: > + // > + // eval("try { 1; throw 2 } catch(e) {}"); // undefined, not 1 > + if (!bce_->emit1(JSOP_UNDEFINED)) > + return false; > + if (!bce_->emit1(JSOP_SETRVAL)) > + return false; > + } I'll go with |enum ShouldUseRetVal| and |enum ShouldUseControl|, okay?
Assignee | ||
Comment 7•7 years ago
|
||
regarding "r=me with the enum collapsed", I think I should ask explicitly if it's okay to go without the enum collapsed.
Flags: needinfo?(shu)
Comment 8•7 years ago
|
||
(In reply to Tooru Fujisawa [:arai] from comment #7) > regarding "r=me with the enum collapsed", I think I should ask explicitly if > it's okay to go without the enum collapsed. Yep, r=me with your explanation for why you need the two cases above.
Flags: needinfo?(shu)
Assignee | ||
Comment 9•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/73193fbc03ec5d35265c3a915a91848f82084a69 Bug 1322069 - Add TryEmitter. r=shu
Comment 10•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/73193fbc03ec
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 11•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/0362a10123ca
You need to log in
before you can comment on or make changes to this bug.
Description
•