Bug 160168 – De-duplicate finally blocks.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

The current implementation clone finally blocks into return paths and fall thru paths.  Let's use an alternate algorithm for threading the code through finally blocks without cloning them.  This should save us some bytecode memory.

[Mark Lam]

Created attachment 284504 [details]
work in progress finally tests.

[Mark Lam]

Created attachment 284505 [details]
work in progress patch for archiving: has bugs, and needs clean up.

[Mark Lam]

Created attachment 297262 [details]
work in progress: almost there.  Let's get some EWS testing.

[Filip Pizlo]

Comment on attachment 297262 [details]
work in progress: almost there.  Let's get some EWS testing.

Looks pretty cool!

Am I understanding right that the return simply cascades through the possible exits and branches on each one after the other?

[Mark Lam]

Created attachment 297264 [details]
wip: fixed build issue.

[Mark Lam]

(In reply to comment #4)
> Comment on attachment 297262 [details]
> work in progress: almost there.  Let's get some EWS testing.
> 
> Looks pretty cool!
> 
> Am I understanding right that the return simply cascades through the
> possible exits and branches on each one after the other?

If we encounter a return, it knows about every finally block it needs to execute on the way out.  So, it turns into a series of jumps, and the outermost finally will do the actual op_return ... that is unless something in one of the finally blocks change the control flow (e.g. an exception being thrown).

[Mark Lam]

Created attachment 297268 [details]
wip: one more time with proper tests.

[Build Bot]

Comment on attachment 297268 [details]
wip: one more time with proper tests.

Attachment 297268 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2733114

New failing tests:
sputnik/Conformance/12_Statement/12.14_The_try_Statement/S12.14_A13_T3.html
inspector/sampling-profiler/call-frame-with-dom-functions.html

[Build Bot]

Created attachment 297284 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 297268 [details]
wip: one more time with proper tests.

Attachment 297268 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2733099

New failing tests:
sputnik/Conformance/12_Statement/12.14_The_try_Statement/S12.14_A13_T3.html
inspector/sampling-profiler/call-frame-with-dom-functions.html

[Build Bot]

Created attachment 297285 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 297268 [details]
wip: one more time with proper tests.

Attachment 297268 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2733242

New failing tests:
sputnik/Conformance/12_Statement/12.14_The_try_Statement/S12.14_A13_T3.html
inspector/sampling-profiler/call-frame-with-dom-functions.html

[Build Bot]

Created attachment 297286 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 297268 [details]
wip: one more time with proper tests.

Attachment 297268 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2733362

New failing tests:
sputnik/Conformance/12_Statement/12.14_The_try_Statement/S12.14_A13_T3.html

[Build Bot]

Created attachment 297290 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Mark Lam]

Created attachment 297307 [details]
wip: with bug fixes.

[Mark Lam]

Created attachment 297317 [details]
wip: fixed win build failure.

Tests look good.  Moving on to benchmarking next.

[Mark Lam]

Created attachment 297365 [details]
proposed patch.

Ready for review.

[Keith Miller]

Comment on attachment 297365 [details]
proposed patch.

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

r=me with some comments.

> Source/JavaScriptCore/ChangeLog:9
> +        associated its try or catch block.  The abrupt completion types include Break,

nix the associated

> Source/JavaScriptCore/ChangeLog:131
> +              If the FinallyContext for this block has register FinallyJumps, we'll

registered

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4215
> +                emitJumpIfCompletionTypeIsThrowCompletion(originalFinallyActionRegister.get(), throwLabel.get());

maybe this should be emitJumpIfCompletionTypeIsThrow? I'm not sure what information the extra Completion provides.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4860
> +    emitJumpIfFinallyActionIsNormalCompletion(normalCompletionLabel);

As discussed offline, It might be a good idea to put a FIXME to use numbers for all the cases. rather than using numbers some of the time and objects / empty JSValues other times.

[Build Bot]

Comment on attachment 297365 [details]
proposed patch.

Attachment 297365 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2739935

New failing tests:
http/tests/navigation/keyboard-events-during-provisional-navigation.html

[Build Bot]

Created attachment 297375 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Mark Lam]

(In reply to comment #20)
> New failing tests:
> http/tests/navigation/keyboard-events-during-provisional-navigation.html

I don't think this is due to my patch.

[Mark Lam]

Created attachment 297377 [details]
Patch for landing.

[Mark Lam]

Thanks for the review.  Landed in r209952: <http://trac.webkit.org/r209952>.

[Michael Saboff]

Comment on attachment 297377 [details]
Patch for landing.

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

> Source/JavaScriptCore/ChangeLog:41
> +              2. CompletionType::Break
> +                 - finallyActionRegister is set to the int jumpID for the site of the break statement.
> +              3. CompletionType::Continue
> +                 - finallyActionRegister is set to the int jumpID for the site of the continue statement.
> +              4. CompletionType::Return
> +                 - finallyActionRegister is set to CompletionType::Return as an int JSValue.
> +                 - finallyReturnValueRegister is set to the value to be returned. 

Isn't there and issue with collision between the integer jumpID values and CompletionType::Return?

> Source/JavaScriptCore/ChangeLog:43
> +                 - finallyActionRegister is set to the exception object that was caught by the finally block.

Can't you throw an integer?

> Source/JavaScriptCore/ChangeLog:138
> +              block.  In that case, jump to the outer finally block's normal entry.

Should this read *next out*?

> Source/JavaScriptCore/ChangeLog:142
> +              CompletionType::Return, then jump to the outer finally block's normal entry.

Should this read *next out*?

[Mark Lam]

Comment on attachment 297377 [details]
Patch for landing.

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

>> Source/JavaScriptCore/ChangeLog:41
>> +                 - finallyReturnValueRegister is set to the value to be returned. 
> 
> Isn't there and issue with collision between the integer jumpID values and CompletionType::Return?

No, no collision because jumpID is computed as CompletionType::NumberOfTypes + bytecodeOffset.  Hence, the jumpIDs are always greater than the CompletionType values.

>> Source/JavaScriptCore/ChangeLog:43
>> +                 - finallyActionRegister is set to the exception object that was caught by the finally block.
> 
> Can't you throw an integer?

In the VM, we always wrap the value we throw in an Exception object.  The Exception object captures the stack for our debugger to use, even if we thrown a primitive value.  op_catch receives both the Exception object and the thrown value.  Catch blocks work with the thrown value (which is observable from JS code) and the Exception object is unused.  Finally blocks work with the Exception object (which is not observable to JS code) and the thrown value is unused.  This is how finally blocks can rethrow the exception while retaining the stack from the original throw.

The "exception object" above refers to the Exception object which wraps the thrown value.  So, even if the JS code throws an integer, it will come wrapped in an Exception object.  Hence, there's also no collision between thrown integers and other integer values in the finallyActionRegister.

>> Source/JavaScriptCore/ChangeLog:138
>> +              block.  In that case, jump to the outer finally block's normal entry.
> 
> Should this read *next out*?

Yeah, "next outer" would have been a better term.  But I already landed the patch.

[Mark Lam]

Rolling out r209974 and r209952.  They break some websites in mysterious ways.

[Mark Lam]

Part 1: r209974 is rolled out in r210007: <http://trac.webkit.org/r210007>.
Part 2: r209952 is rolled out in r210010: <http://trac.webkit.org/r210010>.

[Mark Lam]

*** Bug 165979 has been marked as a duplicate of this bug. ***

[Mark Lam]

Created attachment 297621 [details]
proposed patch with fixes.

The bug in the previous patch are due to Detail 1 and 2 in the ChangeLog.  This is now fixed.

[Saam Barati]

Comment on attachment 297621 [details]
proposed patch with fixes.

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

> Source/JavaScriptCore/ChangeLog:75
> +           emit op_break as usual.  Otherwise:

I think you mean op_jump

> Source/JavaScriptCore/ChangeLog:96
> +           at all on the m_controlFlowScopeStack.  If we don't emit op_ret as usual.

don't emit => don't, emit

(I read this incorrectly at first)

[Saam Barati]

Comment on attachment 297621 [details]
proposed patch with fixes.

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

Some fly-by comments. I need to go, but will finish reviewing later tonight or tomorrow morning

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3504
> +        if (mightBeDerived && (srcIsThis || from == ReturnFrom::Finally))
>              emitTDZCheck(src);

Why is this change needed?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3847
> +    if (lexicalScopeIndex == CurrentLexicalScopeIndex)

Nit: This feels like the wrong place to have this optimization. It seems cleaner to me to just make sure emitMove() doesn't emit an op_mov when it's moving a register into itself.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3870
> +int BytecodeGenerator::labelScopeDepthToLexicalScopeIndex(int targetLabelScopeDepth)

This feels like the wrong name for this function. Idk why this or the other function have "label" in the name.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3891
>  int BytecodeGenerator::labelScopeDepth() const
> -{ 
> -    return localScopeDepth() + m_finallyDepth;
> +{
> +    int depth = localScopeDepth() + m_finallyDepth;
> +    ASSERT(depth == static_cast<int>(m_controlFlowScopeStack.size()));
> +    return depth;

Nit: This function is quite confusingly named if it's just returning the controlFlowScopeStack size. The name makes no sense to me. Why can't we just use the control flow scope stack explicitly everywhere this is called?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:768
> +            int index = size - 1;
> +            return index;

It's pretty tricky that this happens to return OutermostLexicalScopeIndex because of the math it does.
It'd be much clearer if you had this branch:

```
if (!m_lexicalScopeStack.size()) return OutermostLexicalScopeIndex;
```

[Mark Lam]

Comment on attachment 297621 [details]
proposed patch with fixes.

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

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3504
>>              emitTDZCheck(src);
> 
> Why is this change needed?

Because we'll now be calling this emitReturn() from a finally block, and the register to return has been copied to the completionValueRegister().  Hence, we won't know if the value being returned came from m_thisRegister or not initially.  So, I took the strategy of just being conservative: if we're doing an emitReturn() from a finally block inside a constructor (which should be rare, I'm guessing), I'll just emit the TDZ check conservatively.  The alternative would be to do more tricky tracking of whether the return value came from m_thisRegister originally.  I suppose we could do that in emitReturnViaFinallyIfNeeded() and set a "srcIsKnownToBeThis" flag in the FinallyContext, to pass that in here instead.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3847
>> +    if (lexicalScopeIndex == CurrentLexicalScopeIndex)
> 
> Nit: This feels like the wrong place to have this optimization. It seems cleaner to me to just make sure emitMove() doesn't emit an op_mov when it's moving a register into itself.

This is actually needed.  The code below actually does not produce the right scope value to move into the register scope when lexicalScopeIndex == CurrentLexicalScopeIndex.  Without this check here, some tests will start to fail.  That's how I discover this need in the first place.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3870
>> +int BytecodeGenerator::labelScopeDepthToLexicalScopeIndex(int targetLabelScopeDepth)
> 
> This feels like the wrong name for this function. Idk why this or the other function have "label" in the name.

The "labelScopeDepth" in the name came from the labelScopeDepth() method, and that was a pre-existing name that I did't change.  I decided to stick with the pre-existing naming so as to minimize the changes for this patch.  We can look into renaming some of these pre-existing methods and data structures later.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3891
>> +    return depth;
> 
> Nit: This function is quite confusingly named if it's just returning the controlFlowScopeStack size. The name makes no sense to me. Why can't we just use the control flow scope stack explicitly everywhere this is called?

The name came from the pre-existing code, and I agree that it's not a good name.  I merely added an assert here based on what I inferred to be how the code works.  I think the assert documents a detail that was more tricky to infer before.  We can look into renaming label scopes later if needed.  I also don't think that controlFlowScope is a good name.  I might have been the guilty person who named them in the past.  We can do another pass to simplify them after this.  In this patch, for the scope management, I merely implemented it based on how popScopes() used to work.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:768
>> +            return index;
> 
> It's pretty tricky that this happens to return OutermostLexicalScopeIndex because of the math it does.
> It'd be much clearer if you had this branch:
> 
> ```
> if (!m_lexicalScopeStack.size()) return OutermostLexicalScopeIndex;
> ```

I chose the values deliberately so that the math would work out.  I can add an assert after the computation of index that:
    ASSERT(m_lexicalScopeStack.size() || index == OutermostLexicalScopeIndex);

Is that sufficient?  Then again, I can just go with the if statement as you've suggested.  I don't think it will hurt much.

[Saam Barati]

(In reply to comment #33)
> Comment on attachment 297621 [details]
> proposed patch with fixes.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=297621&action=review
> 
> >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3504
> >>              emitTDZCheck(src);
> > 
> > Why is this change needed?
> 
> Because we'll now be calling this emitReturn() from a finally block, and the
> register to return has been copied to the completionValueRegister().  Hence,
> we won't know if the value being returned came from m_thisRegister or not
> initially.  So, I took the strategy of just being conservative: if we're
> doing an emitReturn() from a finally block inside a constructor (which
> should be rare, I'm guessing), I'll just emit the TDZ check conservatively. 
> The alternative would be to do more tricky tracking of whether the return
> value came from m_thisRegister originally.  I suppose we could do that in
> emitReturnViaFinallyIfNeeded() and set a "srcIsKnownToBeThis" flag in the
> FinallyContext, to pass that in here instead.
Why not just emit the TDZ check in emitReturnViaFinallyIfNeeded()? I feel like we usually try to emit TDZ earlier rather than later in the program. I wonder if this actually has semantic differences in the program. I haven't thought too deeply about it but it's worth considering.

> 
> >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3847
> >> +    if (lexicalScopeIndex == CurrentLexicalScopeIndex)
> > 
> > Nit: This feels like the wrong place to have this optimization. It seems cleaner to me to just make sure emitMove() doesn't emit an op_mov when it's moving a register into itself.
> 
> This is actually needed.  The code below actually does not produce the right
> scope value to move into the register scope when lexicalScopeIndex ==
> CurrentLexicalScopeIndex.  Without this check here, some tests will start to
> fail.  That's how I discover this need in the first place.
Do you know why? Looking at your code, it doesn't seem like it should be any semantic difference and it's quite surprising that it is.

> 
> >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3870
> >> +int BytecodeGenerator::labelScopeDepthToLexicalScopeIndex(int targetLabelScopeDepth)
> > 
> > This feels like the wrong name for this function. Idk why this or the other function have "label" in the name.
> 
> The "labelScopeDepth" in the name came from the labelScopeDepth() method,
> and that was a pre-existing name that I did't change.  I decided to stick
> with the pre-existing naming so as to minimize the changes for this patch. 
> We can look into renaming some of these pre-existing methods and data
> structures later.
Ok I think it would be good to come back and do a renaming. FWIW, I think that would make this patch easier to read/follow, not harder.
> 
> >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3891
> >> +    return depth;
> > 
> > Nit: This function is quite confusingly named if it's just returning the controlFlowScopeStack size. The name makes no sense to me. Why can't we just use the control flow scope stack explicitly everywhere this is called?
> 
> The name came from the pre-existing code, and I agree that it's not a good
> name.  I merely added an assert here based on what I inferred to be how the
> code works.  I think the assert documents a detail that was more tricky to
> infer before.  We can look into renaming label scopes later if needed.  I
> also don't think that controlFlowScope is a good name.  I might have been
> the guilty person who named them in the past.  We can do another pass to
> simplify them after this.  In this patch, for the scope management, I merely
> implemented it based on how popScopes() used to work.
> 
> >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:768
> >> +            return index;
> > 
> > It's pretty tricky that this happens to return OutermostLexicalScopeIndex because of the math it does.
> > It'd be much clearer if you had this branch:
> > 
> > ```
> > if (!m_lexicalScopeStack.size()) return OutermostLexicalScopeIndex;
> > ```
> 
> I chose the values deliberately so that the math would work out.  I can add
> an assert after the computation of index that:
>     ASSERT(m_lexicalScopeStack.size() || index ==
> OutermostLexicalScopeIndex);
> 
> Is that sufficient?  Then again, I can just go with the if statement as
> you've suggested.  I don't think it will hurt much.
I'm not a fan of subtle/tricky optimizations in places that will get no benefit from them.

[Saam Barati]

Comment on attachment 297621 [details]
proposed patch with fixes.

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

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4814
> +    while (topScope > bottomScope) {

Please us indices here. It's super weird that this is pointer math and relies too heavily IMO on how Vector<> works.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3004
> +        generator.restoreScopeRegister(lexicalScopeIndex);

I think restoring the scope needs to be unconditional, not whether or not a finally is there.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3035
> +        generator.restoreScopeRegister(lexicalScopeIndex);

Ditto

[Saam Barati]

Comment on attachment 297621 [details]
proposed patch with fixes.

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

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3004
>> +        generator.restoreScopeRegister(lexicalScopeIndex);
> 
> I think restoring the scope needs to be unconditional, not whether or not a finally is there.

Oh ignore me, I was reading the condition backwards.

[Saam Barati]

Comment on attachment 297621 [details]
proposed patch with fixes.

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

r=me 
This code is really nice, and way better than before. I just have a few style suggestions/nits.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:-3982
> -void BytecodeGenerator::emitComplexPopScopes(RegisterID* scope, ControlFlowScope* topScope, ControlFlowScope* bottomScope)
> -{
> -    while (topScope > bottomScope) {
> -        // First we count the number of dynamic scopes we need to remove to get
> -        // to a finally block.
> -        int numberOfNormalScopes = 0;
> -        while (topScope > bottomScope) {
> -            if (topScope->isFinallyBlock)
> -                break;
> -            ++numberOfNormalScopes;
> -            --topScope;
> -        }
> -
> -        if (numberOfNormalScopes) {
> -            // We need to remove a number of dynamic scopes to get to the next
> -            // finally block
> -            RefPtr<RegisterID> parentScope = newTemporary();
> -            while (numberOfNormalScopes--) {
> -                parentScope = emitGetParentScope(parentScope.get(), scope);
> -                emitMove(scope, parentScope.get());
> -            }
>  
> -            // If topScope == bottomScope then there isn't a finally block left to emit.
> -            if (topScope == bottomScope)
> -                return;
> -        }
> -        
> -        Vector<ControlFlowScope> savedControlFlowScopeStack;
> -        Vector<SwitchInfo> savedSwitchContextStack;
> -        Vector<RefPtr<ForInContext>> savedForInContextStack;
> -        Vector<TryContext> poppedTryContexts;
> -        Vector<LexicalScopeStackEntry> savedLexicalScopeStack;
> -        LabelScopeStore savedLabelScopes;
> -        while (topScope > bottomScope && topScope->isFinallyBlock) {
> -            RefPtr<Label> beforeFinally = emitLabel(newLabel().get());
> -            
> -            // Save the current state of the world while instating the state of the world
> -            // for the finally block.
> -            FinallyContext finallyContext = topScope->finallyContext;
> -            bool flipScopes = finallyContext.controlFlowScopeStackSize != m_controlFlowScopeStack.size();
> -            bool flipSwitches = finallyContext.switchContextStackSize != m_switchContextStack.size();
> -            bool flipForIns = finallyContext.forInContextStackSize != m_forInContextStack.size();
> -            bool flipTries = finallyContext.tryContextStackSize != m_tryContextStack.size();
> -            bool flipLabelScopes = finallyContext.labelScopesSize != m_labelScopes.size();
> -            bool flipLexicalScopeStack = finallyContext.lexicalScopeStackSize != m_lexicalScopeStack.size();
> -            int topScopeIndex = -1;
> -            int bottomScopeIndex = -1;
> -            if (flipScopes) {
> -                topScopeIndex = topScope - m_controlFlowScopeStack.begin();
> -                bottomScopeIndex = bottomScope - m_controlFlowScopeStack.begin();
> -                savedControlFlowScopeStack = m_controlFlowScopeStack;
> -                m_controlFlowScopeStack.shrink(finallyContext.controlFlowScopeStackSize);
> -            }
> -            if (flipSwitches) {
> -                savedSwitchContextStack = m_switchContextStack;
> -                m_switchContextStack.shrink(finallyContext.switchContextStackSize);
> -            }
> -            if (flipForIns) {
> -                savedForInContextStack = m_forInContextStack;
> -                m_forInContextStack.shrink(finallyContext.forInContextStackSize);
> -            }
> -            if (flipTries) {
> -                while (m_tryContextStack.size() != finallyContext.tryContextStackSize) {
> -                    ASSERT(m_tryContextStack.size() > finallyContext.tryContextStackSize);
> -                    TryContext context = m_tryContextStack.takeLast();
> -                    TryRange range;
> -                    range.start = context.start;
> -                    range.end = beforeFinally;
> -                    range.tryData = context.tryData;
> -                    m_tryRanges.append(range);
> -                    poppedTryContexts.append(context);
> -                }
> -            }
> -            if (flipLabelScopes) {
> -                savedLabelScopes = m_labelScopes;
> -                while (m_labelScopes.size() > finallyContext.labelScopesSize)
> -                    m_labelScopes.removeLast();
> -            }
> -            if (flipLexicalScopeStack) {
> -                savedLexicalScopeStack = m_lexicalScopeStack;
> -                m_lexicalScopeStack.shrink(finallyContext.lexicalScopeStackSize);
> -            }
> -            int savedFinallyDepth = m_finallyDepth;
> -            m_finallyDepth = finallyContext.finallyDepth;
> -            int savedDynamicScopeDepth = m_localScopeDepth;
> -            m_localScopeDepth = finallyContext.dynamicScopeDepth;
> -            
> -            if (finallyContext.finallyBlock) {
> -                // Emit the finally block.
> -                emitNode(finallyContext.finallyBlock);
> -            } else {
> -                // Emit the IteratorClose block.
> -                ASSERT(finallyContext.iterator);
> -                emitIteratorClose(finallyContext.iterator, finallyContext.enumerationNode);
> -            }
> -
> -            RefPtr<Label> afterFinally = emitLabel(newLabel().get());
> -            
> -            // Restore the state of the world.
> -            if (flipScopes) {
> -                m_controlFlowScopeStack = savedControlFlowScopeStack;
> -                topScope = &m_controlFlowScopeStack[topScopeIndex]; // assert it's within bounds
> -                bottomScope = m_controlFlowScopeStack.begin() + bottomScopeIndex; // don't assert, since it the index might be -1.
> -            }
> -            if (flipSwitches)
> -                m_switchContextStack = savedSwitchContextStack;
> -            if (flipForIns)
> -                m_forInContextStack = savedForInContextStack;
> -            if (flipTries) {
> -                ASSERT(m_tryContextStack.size() == finallyContext.tryContextStackSize);
> -                for (unsigned i = poppedTryContexts.size(); i--;) {
> -                    TryContext context = poppedTryContexts[i];
> -                    context.start = afterFinally;
> -                    m_tryContextStack.append(context);
> -                }
> -                poppedTryContexts.clear();
> -            }
> -            if (flipLabelScopes)
> -                m_labelScopes = savedLabelScopes;
> -            if (flipLexicalScopeStack)
> -                m_lexicalScopeStack = savedLexicalScopeStack;
> -            m_finallyDepth = savedFinallyDepth;
> -            m_localScopeDepth = savedDynamicScopeDepth;
> -            
> -            --topScope;
> -        }
> -    }
> -}

Glorious.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4845
> +    while (topScope >= bottomScope) {

Please use indices and not pointers to do this loop.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4882
> +            for (size_t i = 0; i < numberOfJumps; i++) {
> +                RefPtr<Label> nextLabel = newLabel();
> +                auto& jump = context.jumps(i);
> +                emitJumpIf(op_nstricteq, completionTypeRegister, jump.jumpID, nextLabel.get());
> +
> +                restoreScopeRegister(jump.targetLexicalScopeIndex);
> +                emitSetCompletionType(CompletionType::Normal);
> +                emitJump(jump.targetLabel.get());
> +
> +                emitLabel(nextLabel.get());
> +            }

This is identical to the loop below. Maybe hoist the loop out of the if/else and then have the other code inside the if/else. I think it'd make this code easier to read w.r.t seeing the differences between !outerContext and !!outerContext.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4944
> +    RegisterID* valueConstant = addConstantValue(JSValue(static_cast<int>(type)));

Nit: This could be jsNumber(...) instead of JSValue(...)

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:126
> +            , m_finallyDepth(finallyDepth)

You should ASSERT(m_finallyDepth >= 0) or move to unsigned here.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:128
> +            ASSERT(!m_jumps || m_jumps->isEmpty());

This should just be ASSERT(!m_jumps). It's impossible for m_jumps to be anything but nullptr here.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:139
> +            ASSERT(m_numberOfBreaksOrContinues < INT_MAX);
> +            m_numberOfBreaksOrContinues++;

This should be UINT_MAX, no? And if you're worried about overflow why not just use checked arithmetic. This code is never hot anyways.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:158
> +        int m_finallyDepth { 0 };

Why not unsigned?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:161
> +        std::unique_ptr<Vector<FinallyJump>> m_jumps;

Why not just Vector<FinallyJump> instead of the unique_ptr?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:180
> +        int lexicalScopeIndex;

Your int encoding is still slightly confusing to me.
- It seems like your CurrentLexicalScope should just fall out naturally from any algorithm you write.
- For the OutermostLexicalScopeIndex constant, it appears you need this because we don't create a stack entry for the top most scope. I feel like it'd be cleaner to have m_controlFlowScope have an opaque entry for the m_topMostScope. Maybe this can be done later.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:762
> +        int currentLexicalScopeIndex() const

I don't think this needs to be public.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3368
> +        generator.emitCatch(generator.completionValueRegister(), unused);

Hmm, why do you use completionValueRegister() here? Just so you don't burn a register? If so, why not use unused twice?

> Source/WTF/wtf/SegmentedVector.h:133
> +        T& last() { return at(size() - 1); }
> +        const T& last() const { return at(size() - 1); }

Please ASSERT(size()) or maybe use checked arithmetic since this code isn't hot at the moment

[Mark Lam]

Comment on attachment 297621 [details]
proposed patch with fixes.

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

>> Source/JavaScriptCore/ChangeLog:75
>> +           emit op_break as usual.  Otherwise:
> 
> I think you mean op_jump

Fixed.

>> Source/JavaScriptCore/ChangeLog:96
>> +           at all on the m_controlFlowScopeStack.  If we don't emit op_ret as usual.
> 
> don't emit => don't, emit
> 
> (I read this incorrectly at first)

Fixed.

>>>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3504
>>>>              emitTDZCheck(src);
>>> 
>>> Why is this change needed?
>> 
>> Because we'll now be calling this emitReturn() from a finally block, and the register to return has been copied to the completionValueRegister().  Hence, we won't know if the value being returned came from m_thisRegister or not initially.  So, I took the strategy of just being conservative: if we're doing an emitReturn() from a finally block inside a constructor (which should be rare, I'm guessing), I'll just emit the TDZ check conservatively.  The alternative would be to do more tricky tracking of whether the return value came from m_thisRegister originally.  I suppose we could do that in emitReturnViaFinallyIfNeeded() and set a "srcIsKnownToBeThis" flag in the FinallyContext, to pass that in here instead.
> 
> Why not just emit the TDZ check in emitReturnViaFinallyIfNeeded()? I feel like we usually try to emit TDZ earlier rather than later in the program. I wonder if this actually has semantic differences in the program. I haven't thought too deeply about it but it's worth considering.

The old code would run through all the finally code (via emitPopScopes()) before emitReturn i.e. the TDZ is done at the end.  Hence, I have not changed the order in which the TDZ check is done.  So, no issue here.

>>>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3847
>>>> +    if (lexicalScopeIndex == CurrentLexicalScopeIndex)
>>> 
>>> Nit: This feels like the wrong place to have this optimization. It seems cleaner to me to just make sure emitMove() doesn't emit an op_mov when it's moving a register into itself.
>> 
>> This is actually needed.  The code below actually does not produce the right scope value to move into the register scope when lexicalScopeIndex == CurrentLexicalScopeIndex.  Without this check here, some tests will start to fail.  That's how I discover this need in the first place.
> 
> Do you know why? Looking at your code, it doesn't seem like it should be any semantic difference and it's quite surprising that it is.

See labelScopeDepthToLexicalScopeIndex() below.  
1. The targetLabelScopeDepth passed to labelScopeDepthToLexicalScopeIndex() may equal labelScopeDepth() (indicating the current scope).  Note: this is an expectation from pre-existing scope management code. 
2. labelScopeDepth() equals m_controlFlowScopeStack.size().
3. to get the targetScope lexicalScopeIndex, we fetch the targetScope from m_controlFlowScopeStack[targetLabelScopeDepth].

If targetLabelScopeDepth equals labelScopeDepth(), then m_controlFlowScopeStack[targetLabelScopeDepth] overflows and crash.

We can fix all of this when I replace labelScopeDepth() with something more meaningful and intuitive later.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4814
>> +    while (topScope > bottomScope) {
> 
> Please us indices here. It's super weird that this is pointer math and relies too heavily IMO on how Vector<> works.

I believe that it is an invariant in our code that Vector's backing store is allocated as a contiguous array.  The old emitComplexPopScopes() pretty much relies on this for correctness.  I'll leave it as is for now.  We can refactor this in a subsequent patch.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4882
>> +            }
> 
> This is identical to the loop below. Maybe hoist the loop out of the if/else and then have the other code inside the if/else. I think it'd make this code easier to read w.r.t seeing the differences between !outerContext and !!outerContext.

Yeah.  This loop didn't use to be at the top of the 2 blocks, but it migrated there in the end.  I will hoist the loop out.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4944
>> +    RegisterID* valueConstant = addConstantValue(JSValue(static_cast<int>(type)));
> 
> Nit: This could be jsNumber(...) instead of JSValue(...)

Fixed.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:126
>> +            , m_finallyDepth(finallyDepth)
> 
> You should ASSERT(m_finallyDepth >= 0) or move to unsigned here.

Added assert.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:128
>> +            ASSERT(!m_jumps || m_jumps->isEmpty());
> 
> This should just be ASSERT(!m_jumps). It's impossible for m_jumps to be anything but nullptr here.

Changed to ASSERT(m_jump.isEmpty()) since m_jump is now a Vector instead of a unique_ptr.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:139
>> +            m_numberOfBreaksOrContinues++;
> 
> This should be UINT_MAX, no? And if you're worried about overflow why not just use checked arithmetic. This code is never hot anyways.

I changed m_numberOfBreaksOrContinues to be a Checked<uint32_t, CrashOnOverflow>.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:158
>> +        int m_finallyDepth { 0 };
> 
> Why not unsigned?

Legacy reasons: because BytecodeGenerator::m_finallyDepth is int type (and supporting code expects it).  We can change this to an unsigned later in another patch.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:161
>> +        std::unique_ptr<Vector<FinallyJump>> m_jumps;
> 
> Why not just Vector<FinallyJump> instead of the unique_ptr?

Because we need to WTFMove the FinallyContext when we pop it in TryNode::emitBytecode().  This avoids a copy.  But then again, the Vector is not that much bigger than a unique_ptr.  I'll change it to just a Vector.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:762
>> +        int currentLexicalScopeIndex() const
> 
> I don't think this needs to be public.

I made it private.

>>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:768
>>> +            return index;
>> 
>> It's pretty tricky that this happens to return OutermostLexicalScopeIndex because of the math it does.
>> It'd be much clearer if you had this branch:
>> 
>> ```
>> if (!m_lexicalScopeStack.size()) return OutermostLexicalScopeIndex;
>> ```
> 
> I chose the values deliberately so that the math would work out.  I can add an assert after the computation of index that:
>     ASSERT(m_lexicalScopeStack.size() || index == OutermostLexicalScopeIndex);
> 
> Is that sufficient?  Then again, I can just go with the if statement as you've suggested.  I don't think it will hurt much.

I added the "if (!m_lexicalScopeStack.size()) return OutermostLexicalScopeIndex;" check.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3368
>> +        generator.emitCatch(generator.completionValueRegister(), unused);
> 
> Hmm, why do you use completionValueRegister() here? Just so you don't burn a register? If so, why not use unused twice?

Because if we entered this finally block with Throw completion type, we want to rethrow this value in emitFinallyCompletion() below.  emitFinallyCompletion() is expecting the Exception object for re-thowing to be in this register.

>> Source/WTF/wtf/SegmentedVector.h:133
>> +        const T& last() const { return at(size() - 1); }
> 
> Please ASSERT(size()) or maybe use checked arithmetic since this code isn't hot at the moment

I plan to make SegmentedVector support CheckedArithmetic later in https://bugs.webkit.org/show_bug.cgi?id=165980.  I'll add an assert for now.

[Mark Lam]

Created attachment 297676 [details]
Patch for landing.

[Mark Lam]

Thanks for the review.  Landed in r210116: <http://trac.webkit.org/r210116>.