[webkit-reviews] review granted: [Bug 195131] Fix incorrect handling of try-finally completion values. : [Attachment 363763] proposed patch.

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 195131: Fix incorrect handling of try-finally completion values.
https://bugs.webkit.org/show_bug.cgi?id=195131

Attachment 363763: proposed patch.

https://bugs.webkit.org/attachment.cgi?id=363763&action=review




--- Comment #6 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 363763
  --> https://bugs.webkit.org/attachment.cgi?id=363763
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=363763&action=review

> Source/JavaScriptCore/ChangeLog:39
> +	   One might be deceived into thinking that the above example should
complete with
> +	   the exception thrown at line 7.  However, according to Section
13.15.8 of the
> +	   ECMAScript spec, the 'continue' in the finally at line 9 counts as
an abrupt
> +	   completion.	As a result, it overrides the throw from line 7.  After
the continue,
> +	   execution resumes at the top of the top of the loop at line 5,
followed by a
> +	   normal completion at line 11.

Crazy example.

> Source/JavaScriptCore/ChangeLog:60
> +	   4. Allocate the FinallyContext on the stack instead of as a member
of the
> +	      heap allocated ControlFlowScope.	This simplifies its life-cycle
management
> +	      and reduces the amount of needed copying.

I'm not sure it's worth reverting behavior on this. I feel like it'd be cleaner
just to make it an inline field again and use a move constructor to avoid
copying the object.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:201
> +FinallyContext::~FinallyContext()
> +{
> +    m_completion.typeRegister = nullptr;
> +    m_completion.valueRegister = nullptr;
> +}

Why? The default destructor already does this.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:273
> +	       // finally need to rethrow. catch does not.

make this: Finally needs to rethrow. Catch does not.
Alternatively, just remove the comment.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4814
> +	       if (hasBreaksOrContinuesNotCoveredByJumps) {

name nit: Let's call this something like hasBreaksOrContinuesThatEscapeFinally
(or similar). As we discussed in person, renaming may be more apt for a
follow-up

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4816
> +		   emitJumpIf<OpBeloweq>(context.completionTypeRegister(),
CompletionType::Throw, isThrowOrNormalLabel.get());

Can you also add static_assert(return > throw && return > normal)?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:160
> +	   struct {
> +	       RefPtr<RegisterID> typeRegister;
> +	       RefPtr<RegisterID> valueRegister;
> +	   } m_completion;

Why in a struct?