Bug 147666 – Interpreter::unwind shouldn't be responsible for assigning the correct scope.

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

[Saam Barati]

BytecodeGenerator should do this since it will have the necessary info to do so.

[Geoffrey Garen]

👍

[Saam Barati]

Created attachment 258326 [details]
patch

[WebKit Commit Bot]

Attachment 258326 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2635:  Missing space before {  [whitespace/braces] [5]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 258350 [details]
patch

[WebKit Commit Bot]

Attachment 258350 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2635:  Missing space before {  [whitespace/braces] [5]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 258350 [details]
patch

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

r=me, with a fixup below

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2840
> +    m_topMostScope = addVar();
> +    m_topMostScope->ref();
> +    emitMove(m_topMostScope, scopeRegister());

Not new to this patch, but: Does our new lexical scoping have the behavior that we now have var registers and temporary registers interleaved with each other, such that I might have:

[Var][Temp][Var][Temp]

?

That's a little sad because all temporary registers prior to the last vars will "leak" in the sense that the function will never reuse them, thus bloating its stack frame size.

Also, there's no need to ref m_topMostScope here because addVar() already refs.

Also, if you did want to ref(), I would ask you to use RefPtr.

[Saam Barati]

Comment on attachment 258350 [details]
patch

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

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2840
>> +    emitMove(m_topMostScope, scopeRegister());
> 
> Not new to this patch, but: Does our new lexical scoping have the behavior that we now have var registers and temporary registers interleaved with each other, such that I might have:
> 
> [Var][Temp][Var][Temp]
> 
> ?
> 
> That's a little sad because all temporary registers prior to the last vars will "leak" in the sense that the function will never reuse them, thus bloating its stack frame size.
> 
> Also, there's no need to ref m_topMostScope here because addVar() already refs.
> 
> Also, if you did want to ref(), I would ask you to use RefPtr.

We don't do this. I believe we have an assert in addVar() that will prevent this from happening.
It would also be wrong if we did this because it would break op_enter. 

The way we handle register reuse for block scope variables like "let" is that
while we're generating code for the block's statements we'll reserve a non-temp
register for the block scope variable. Once we're done generating code for
that block scope, we will reclaim those block scoped registers. The code that
does this is in pushLexicalScopeInternal and popLexicalScopeInternal and reclaimFreeRegisters. 

This doesn't mean that we won't have some block scoped variables that are live for
the entire function. For example, functions with default parameter values will have
all parameters be in a block scope that never frees its variables. (These registers will
still be allocated after the "var" register locations).

I'll remove the ref and land later.

[Saam Barati]

Created attachment 258502 [details]
patch for landing.

[WebKit Commit Bot]

Attachment 258502 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2635:  Missing space before {  [whitespace/braces] [5]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 258502 [details]
patch for landing.

Clearing flags on attachment: 258502

Committed r188136: <http://trac.webkit.org/changeset/188136>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Committed r188144: <http://trac.webkit.org/changeset/188144>

[Chris Dumez]

(In reply to comment #12)
> Committed r188144: <http://trac.webkit.org/changeset/188144>

Sorry, wrong bug. My changelog got messed up somehow.

[Geoffrey Garen]

> The way we handle register reuse for block scope variables like "let" is that
> while we're generating code for the block's statements we'll reserve a
> non-temp
> register for the block scope variable. Once we're done generating code for
> that block scope, we will reclaim those block scoped registers. The code that
> does this is in pushLexicalScopeInternal and popLexicalScopeInternal and
> reclaimFreeRegisters. 

Oh! I get it now.