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.
Created attachment 257071 [details] patch
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.
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.
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.
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.
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).
landed in: http://trac.webkit.org/changeset/187033