"let" scoping introduced incoherent story about symbol table cloning
https://bugs.webkit.org/show_bug.cgi?id=147046
Reviewed by Filip Pizlo.
This patch now establishes a clear set of rules for how SymbolTables
are owned by CodeBlock. Every SymbolTable that is used by a bytecode
instruction must live in CodeBlock's constant register pool. When CodeBlock
is being linked, it ensures that every SymbolTable in the constant pool is cloned.
This leaves no room for an un-cloned symbol table to be used by a bytecode instruction.
Some instructions may refer to SymbolTable's indirectly through a JSLexicalEnvironment.
This is fine, all JSLexicalEnvironment's are allocated with references to cloned symbol tables.
Another goal of this patch is to remove the notion that a SymbolTable is 1 to 1
with a CodeBlock. With lexical scoping, this view of the world is no longer
correct. This patch begins to remove this assumption by making CodeBlock's
symbolTable() getter method private. There is still one place where we need
to purge our codebase of this assumption and that is the type profiler. It
has not been updated for lexical scoping. After it is updated in
https://bugs.webkit.org/show_bug.cgi?id=145438
we will be able to remove CodeBlock's symbolTable() getter entirely.
* bytecode/CodeBlock.cpp:
(JSC::CodeBlock::CodeBlock):
(JSC::CodeBlock::nameForRegister):
* bytecode/CodeBlock.h:
(JSC::CodeBlock::addStringSwitchJumpTable):
(JSC::CodeBlock::stringSwitchJumpTable):
(JSC::CodeBlock::evalCodeCache):
(JSC::CodeBlock::symbolTable):
* bytecode/UnlinkedCodeBlock.cpp:
(JSC::UnlinkedFunctionExecutable::visitChildren):
(JSC::UnlinkedFunctionExecutable::link):
(JSC::UnlinkedFunctionExecutable::codeBlockFor):
* bytecode/UnlinkedCodeBlock.h:
(JSC::UnlinkedCodeBlock::addExceptionHandler):
(JSC::UnlinkedCodeBlock::exceptionHandler):
(JSC::UnlinkedCodeBlock::setSymbolTableConstantIndex):
(JSC::UnlinkedCodeBlock::symbolTableConstantIndex):
(JSC::UnlinkedCodeBlock::symbolTable): Deleted.
(JSC::UnlinkedCodeBlock::setSymbolTable): Deleted.
* bytecompiler/BytecodeGenerator.cpp:
(JSC::BytecodeGenerator::generate):
(JSC::BytecodeGenerator::BytecodeGenerator):
(JSC::BytecodeGenerator::pushLexicalScope):
(JSC::BytecodeGenerator::variableForLocalEntry):
(JSC::BytecodeGenerator::createVariable):
(JSC::BytecodeGenerator::resolveType):
(JSC::BytecodeGenerator::emitResolveScope):
* bytecompiler/BytecodeGenerator.h:
(JSC::BytecodeGenerator::thisRegister):
(JSC::BytecodeGenerator::instructions):
(JSC::BytecodeGenerator::symbolTable): Deleted.
* dfg/DFGGraph.h:
(JSC::DFG::Graph::baselineCodeBlockFor):
(JSC::DFG::Graph::isStrictModeFor):
(JSC::DFG::Graph::symbolTableFor): Deleted.
* jit/AssemblyHelpers.h:
(JSC::AssemblyHelpers::baselineCodeBlock):
(JSC::AssemblyHelpers::argumentsStart):
(JSC::AssemblyHelpers::symbolTableFor): Deleted.
* runtime/CommonSlowPaths.cpp:
(JSC::SLOW_PATH_DECL):
* runtime/Executable.cpp:
(JSC::FunctionExecutable::visitChildren):
(JSC::FunctionExecutable::clearUnlinkedCodeForRecompilation):
(JSC::FunctionExecutable::symbolTable): Deleted.
* runtime/Executable.h:
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@187033 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/Source/JavaScriptCore/bytecode/CodeBlock.cpp b/Source/JavaScriptCore/bytecode/CodeBlock.cpp
index d1124a7..041ff08 100644
--- a/Source/JavaScriptCore/bytecode/CodeBlock.cpp
+++ b/Source/JavaScriptCore/bytecode/CodeBlock.cpp
@@ -1750,8 +1750,6 @@
ASSERT(m_heap->isDeferred());
ASSERT(m_scopeRegister.isLocal());
- bool didCloneSymbolTable = false;
-
ASSERT(m_source);
setNumParameters(unlinkedCodeBlock->numParameters());
@@ -1767,24 +1765,25 @@
if (unsigned registerIndex = unlinkedCodeBlock->registerIndexForLinkTimeConstant(type))
m_constantRegisters[registerIndex].set(*m_vm, ownerExecutable, m_globalObject->jsCellForLinkTimeConstant(type));
}
-
- if (SymbolTable* symbolTable = unlinkedCodeBlock->symbolTable()) {
- if (m_vm->typeProfiler()) {
- ConcurrentJITLocker locker(symbolTable->m_lock);
- symbolTable->prepareForTypeProfiling(locker);
+
+ HashSet<int, WTF::IntHash<int>, WTF::UnsignedWithZeroKeyHashTraits<int>> clonedConstantSymbolTables;
+ m_symbolTableConstantIndex = unlinkedCodeBlock->symbolTableConstantIndex();
+ {
+ HashSet<SymbolTable*> clonedSymbolTables;
+ for (unsigned i = 0; i < m_constantRegisters.size(); i++) {
+ if (m_constantRegisters[i].get().isEmpty())
+ continue;
+ if (SymbolTable* symbolTable = jsDynamicCast<SymbolTable*>(m_constantRegisters[i].get())) {
+ RELEASE_ASSERT(clonedSymbolTables.add(symbolTable).isNewEntry);
+ if (m_vm->typeProfiler()) {
+ ConcurrentJITLocker locker(symbolTable->m_lock);
+ symbolTable->prepareForTypeProfiling(locker);
+ }
+ m_constantRegisters[i].set(*m_vm, ownerExecutable, symbolTable->cloneScopePart(*m_vm));
+ clonedConstantSymbolTables.add(i + FirstConstantRegisterIndex);
+ }
}
-
- SymbolTable* newTable;
- if (codeType() == FunctionCode && symbolTable->scopeSize()) {
- newTable = symbolTable->cloneScopePart(*m_vm);
- didCloneSymbolTable = true;
- } else
- newTable = symbolTable;
-
- m_symbolTableConstantIndex = unlinkedCodeBlock->symbolTableConstantIndex();
- replaceConstant(m_symbolTableConstantIndex, newTable);
- } else
- m_symbolTableConstantIndex = 0;
+ }
m_functionDecls.resizeToFit(unlinkedCodeBlock->numberOfFunctionDecls());
for (size_t count = unlinkedCodeBlock->numberOfFunctionDecls(), i = 0; i < count; ++i) {
@@ -1868,8 +1867,6 @@
Vector<Instruction, 0, UnsafeVectorOverflow> instructions(instructionCount);
- HashSet<int, WTF::IntHash<int>, WTF::UnsignedWithZeroKeyHashTraits<int>> clonedConstantSymbolTables;
-
for (unsigned i = 0; !instructionReader.atEnd(); ) {
const UnlinkedInstruction* pc = instructionReader.next();
@@ -1962,6 +1959,12 @@
case op_get_array_length:
CRASH();
+ case op_create_lexical_environment: {
+ int symbolTableIndex = pc[3].u.operand;
+ RELEASE_ASSERT(clonedConstantSymbolTables.contains(symbolTableIndex));
+ break;
+ }
+
case op_resolve_scope: {
const Identifier& ident = identifier(pc[3].u.operand);
ResolveType type = static_cast<ResolveType>(pc[4].u.operand);
@@ -2013,29 +2016,8 @@
if (modeAndType.type() == LocalClosureVar) {
// Only do watching if the property we're putting to is not anonymous.
if (static_cast<unsigned>(pc[2].u.operand) != UINT_MAX) {
- // Different create_lexical_environment instructions may refer to the same symbol table.
- // This is used for ES6's 'for' loops each having a separate activation. We will emit two
- // create_lexical_environment instructions for a given loop to implement this feature,
- // but both instructions should rely on the same underlying symbol table so that the
- // loop's scope isn't mistakenly inferred as a singleton scope.
int symbolTableIndex = pc[5].u.operand;
- auto addResult = clonedConstantSymbolTables.add(symbolTableIndex);
- if (addResult.isNewEntry) {
- SymbolTable* unlinkedTable = jsCast<SymbolTable*>(getConstant(symbolTableIndex));
- SymbolTable* linkedTable;
- if (unlinkedTable->correspondsToLexicalScope()) {
- RELEASE_ASSERT(unlinkedTable->scopeSize());
- linkedTable = unlinkedTable->cloneScopePart(*m_vm);
- } else {
- // There is only one SymbolTable per function that does not correspond
- // to a lexical scope and that is the function's var symbol table.
- // We've already cloned that.
- linkedTable = symbolTable();
- if (linkedTable->scopeSize())
- RELEASE_ASSERT(didCloneSymbolTable);
- }
- replaceConstant(symbolTableIndex, linkedTable);
- }
+ RELEASE_ASSERT(clonedConstantSymbolTables.contains(symbolTableIndex));
SymbolTable* symbolTable = jsCast<SymbolTable*>(getConstant(symbolTableIndex));
const Identifier& ident = identifier(pc[2].u.operand);
ConcurrentJITLocker locker(symbolTable->m_lock);
@@ -2106,6 +2088,10 @@
}
case ProfileTypeBytecodePutToLocalScope:
case ProfileTypeBytecodeGetFromLocalScope: {
+ if (!m_symbolTableConstantIndex) {
+ globalVariableID = TypeProfilerNoGlobalIDExists;
+ break;
+ }
const Identifier& ident = identifier(pc[4].u.operand);
symbolTable = this->symbolTable();
ConcurrentJITLocker locker(symbolTable->m_lock);
@@ -2118,6 +2104,10 @@
}
case ProfileTypeBytecodeHasGlobalID: {
+ if (!m_symbolTableConstantIndex) {
+ globalVariableID = TypeProfilerNoGlobalIDExists;
+ break;
+ }
symbolTable = this->symbolTable();
ConcurrentJITLocker locker(symbolTable->m_lock);
globalVariableID = symbolTable->uniqueIDForOffset(locker, VarOffset(profileRegister), *vm());
@@ -3839,13 +3829,19 @@
String CodeBlock::nameForRegister(VirtualRegister virtualRegister)
{
- ConcurrentJITLocker locker(symbolTable()->m_lock);
- SymbolTable::Map::iterator end = symbolTable()->end(locker);
- for (SymbolTable::Map::iterator ptr = symbolTable()->begin(locker); ptr != end; ++ptr) {
- if (ptr->value.varOffset() == VarOffset(virtualRegister)) {
- // FIXME: This won't work from the compilation thread.
- // https://bugs.webkit.org/show_bug.cgi?id=115300
- return ptr->key.get();
+ for (unsigned i = 0; i < m_constantRegisters.size(); i++) {
+ if (m_constantRegisters[i].get().isEmpty())
+ continue;
+ if (SymbolTable* symbolTable = jsDynamicCast<SymbolTable*>(m_constantRegisters[i].get())) {
+ ConcurrentJITLocker locker(symbolTable->m_lock);
+ auto end = symbolTable->end(locker);
+ for (auto ptr = symbolTable->begin(locker); ptr != end; ++ptr) {
+ if (ptr->value.varOffset() == VarOffset(virtualRegister)) {
+ // FIXME: This won't work from the compilation thread.
+ // https://bugs.webkit.org/show_bug.cgi?id=115300
+ return ptr->key.get();
+ }
+ }
}
}
if (virtualRegister == thisRegister())
