Bug 147046 – "let" scoping introduced incoherent story about symbol table cloning

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

[Saam Barati]

We need to make sure we clone every symbol table that an op_create_lexical_environment or op_put_to_scope
may reference. Any operation that relies on a symbol table constant index needs to be certain that the symbol
table was cloned. We will ensure this property while linking a code block.

[Saam Barati]

Created attachment 257071 [details]
patch

[Saam Barati]

Comment on attachment 257071 [details]
patch

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

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1605
> +        //ASSERT(!m_symbolTable || !m_symbolTable->contains(variable.ident().impl()) || resolveType() == Dynamic);

I wonder if it's worth re-creating a similar assert.
We would basically just be calling back into ::variable to get a Variable to assert properties about,
which is the function the Variable inside this function call comes from. I'm not sure if it's worth it or not.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:522:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1605:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 2 in 7 files


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

[Filip Pizlo]

Comment on attachment 257071 [details]
patch

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

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1779
> +            if (!symbolTable->correspondsToLexicalScope())

It would be useful to add a comment that not being a lexical scope means that you are the outermost symbol table.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1780
> +                m_symbolTableConstantIndex = i + FirstConstantRegisterIndex;

Would be useful to assert that this only gets set once.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1782
> +            clonedConstantSymbolTables.add(i + FirstConstantRegisterIndex);

Would be good to assert that this never sees the same thing twice, ie that isNewEntry is always true.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:212
> +    Strong<SymbolTable> functionSymbolTable(*m_vm, SymbolTable::create(*m_vm));

Is it necessary to use Strong here?  If a value is on the stack then it will already be strongly marked.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1605
>> +        //ASSERT(!m_symbolTable || !m_symbolTable->contains(variable.ident().impl()) || resolveType() == Dynamic);
> 
> I wonder if it's worth re-creating a similar assert.
> We would basically just be calling back into ::variable to get a Variable to assert properties about,
> which is the function the Variable inside this function call comes from. I'm not sure if it's worth it or not.

I think we should err on the side of just removing the assert if it takes Work to make it work.

[Saam Barati]

Comment on attachment 257071 [details]
patch

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

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1779
>> +            if (!symbolTable->correspondsToLexicalScope())
> 
> It would be useful to add a comment that not being a lexical scope means that you are the outermost symbol table.

Will do.

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1780
>> +                m_symbolTableConstantIndex = i + FirstConstantRegisterIndex;
> 
> Would be useful to assert that this only gets set once.

Will do.

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1782
>> +            clonedConstantSymbolTables.add(i + FirstConstantRegisterIndex);
> 
> Would be good to assert that this never sees the same thing twice, ie that isNewEntry is always true.

I think a better Assert here would be to have a Hashset<SymbolTable*> that
asserts it's always a new entry. These indices always have to be unique because
they're in the loop.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:212
>> +    Strong<SymbolTable> functionSymbolTable(*m_vm, SymbolTable::create(*m_vm));
> 
> Is it necessary to use Strong here?  If a value is on the stack then it will already be strongly marked.

No, it's not. It will be Strong in m_symbolTableStack and it will also be in the constant pool.

[Saam Barati]

Comment on attachment 257071 [details]
patch

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

>>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1780
>>> +                m_symbolTableConstantIndex = i + FirstConstantRegisterIndex;
>> 
>> Would be useful to assert that this only gets set once.
> 
> Will do.

Adding this assert made me realize that this code is stupid. There will be more than
one symbol table with where correspondsToLexicalScope() is false in a world with
catch scopes, function name scopes, etc. I think the correct solution here is to remove
this branch completely and rely on unlinked code block's symbolTableConstantIndex()
and then get rid of symbolTableConstantIndex() asap (I plan to write the necessary
type profiler patch tomorrow).

[Saam Barati]

landed in:
http://trac.webkit.org/changeset/187033