1322069 - Create a class to handle JumpList, TryFinallyControl, and depth in try-catch-finally in BytecodeEmitter.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tooru Fujisawa [:arai]]

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.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: (WIP) Add TryEmitter.

here's WIP patch.

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: (WIP) Bug 1147371 - Close iterator in array destructuring with TryEmitter.

example patch that uses TryEmitter, for bug 1147371.

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Add TryEmitter

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Add TryEmitter.

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

Comment 5

[Shu-yu Guo [:shu]]

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.

Comment 6

[Tooru Fujisawa [:arai]]

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?

Comment 7

[Tooru Fujisawa [:arai]]

regarding "r=me with the enum collapsed", I think I should ask explicitly if it's okay to go without the enum collapsed.

Comment 8

[Shu-yu Guo [:shu]]

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

Comment 9

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/73193fbc03ec5d35265c3a915a91848f82084a69
Bug 1322069 - Add TryEmitter. r=shu

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/73193fbc03ec

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0362a10123ca