Only copy captured variables into activation
Created attachment 68489 [details] Patch
Attachment 68489 [details] did not build on win: Build output: http://queues.webkit.org/results/4041105
Comment on attachment 68489 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=68489&action=review Review comments by Geoff Garen. > JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:371 > + if (nonCapturedVarPass) > + codeBlock->m_numCapturedVars = codeBlock->m_numVars; > + for (size_t i = 0; i < functionStack.size(); ++i) { > + FunctionBodyNode* function = functionStack[i]; > + const Identifier& ident = function->ident(); > + if (functionBody->captures(ident) != nonCapturedVarPass) { > + m_functions.add(ident.impl()); > + emitNewFunction(addVar(ident, false), function); > + } > + } > + for (size_t i = 0; i < varStack.size(); ++i) { > + const Identifier& ident = *varStack[i].first; > + if (functionBody->captures(ident) != nonCapturedVarPass) > + addVar(ident, varStack[i].second & DeclarationStacks::IsConstant); > + } > + if (!nonCapturedVarPass) > + codeBlock->m_numCapturedVars = codeBlock->m_numVars; > } The meaning of 'nonCapturedVarPass' in this loop is just too subtle. I think it would be better to unroll this loop instead. > JavaScriptCore/runtime/JSActivation.cpp:96 > +inline bool JSActivation::symbolTableGet(const Identifier& propertyName, PropertySlot& slot) > +{ > + SymbolTableEntry entry = symbolTable().inlineGet(propertyName.impl()); > + // Cant't access variables that weren't captured > + if (!entry.isNull() && entry.getIndex() < static_cast<int>(d()->functionExecutable->capturedVariableCount())) { > + slot.setRegisterSlot(®isterAt(entry.getIndex())); > + return true; > + } > + return false; > +} > + > +inline bool JSActivation::symbolTablePut(const Identifier& propertyName, JSValue value) > +{ > + ASSERT(!Heap::heap(value) || Heap::heap(value) == Heap::heap(this)); > + > + SymbolTableEntry entry = symbolTable().inlineGet(propertyName.impl()); > + if (entry.isNull()) > + return false; > + if (entry.isReadOnly()) > + return true; > + // Can't access variables that weren't captured > + if (entry.getIndex() >= static_cast<int>(d()->functionExecutable->capturedVariableCount())) > + return false; > + registerAt(entry.getIndex()) = value; > + return true; > +} As discussed, it's not possible to get or put a non-captured in an activation. You can change the checks to ASSERTs. > JavaScriptCore/runtime/JSActivation.cpp:122 > + return false; Ditto.
Committed r68171
http://trac.webkit.org/changeset/68171 might have broken GTK Linux 64-bit Debug The following changes are on the blame list: http://trac.webkit.org/changeset/68171 http://trac.webkit.org/changeset/68172
This change may have broken these layout tests. They all fail the newly added assert: ASSERTION FAILED: entry.getIndex() < static_cast<int>(d()->functionExecutable->capturedVariableCount()) (/work/WebKit/JavaScriptCore/runtime/JSActivation.cpp:75 bool JSC::JSActivation::symbolTableGet(const JSC::Identifier&, JSC::PropertySlot&)) fast/xmlhttprequest/xmlhttprequest-no-file-access.html ietestcenter/Javascript/12.14-3.html ietestcenter/Javascript/12.14-4.html ietestcenter/Javascript/12.14-5.html ietestcenter/Javascript/12.14-6.html ietestcenter/Javascript/12.14-7.html ietestcenter/Javascript/12.14-8.html
(In reply to comment #6) > This change may have broken these layout tests. They all fail the newly added assert: > > ASSERTION FAILED: entry.getIndex() < static_cast<int>(d()->functionExecutable->capturedVariableCount()) > (/work/WebKit/JavaScriptCore/runtime/JSActivation.cpp:75 bool JSC::JSActivation::symbolTableGet(const JSC::Identifier&, JSC::PropertySlot&)) > > fast/xmlhttprequest/xmlhttprequest-no-file-access.html > ietestcenter/Javascript/12.14-3.html > ietestcenter/Javascript/12.14-4.html > ietestcenter/Javascript/12.14-5.html > ietestcenter/Javascript/12.14-6.html > ietestcenter/Javascript/12.14-7.html > ietestcenter/Javascript/12.14-8.html what revision?
(In reply to comment #7) > (In reply to comment #6) > > This change may have broken these layout tests. They all fail the newly added assert: > > > > ASSERTION FAILED: entry.getIndex() < static_cast<int>(d()->functionExecutable->capturedVariableCount()) > > (/work/WebKit/JavaScriptCore/runtime/JSActivation.cpp:75 bool JSC::JSActivation::symbolTableGet(const JSC::Identifier&, JSC::PropertySlot&)) > > > > fast/xmlhttprequest/xmlhttprequest-no-file-access.html > > ietestcenter/Javascript/12.14-3.html > > ietestcenter/Javascript/12.14-4.html > > ietestcenter/Javascript/12.14-5.html > > ietestcenter/Javascript/12.14-6.html > > ietestcenter/Javascript/12.14-7.html > > ietestcenter/Javascript/12.14-8.html > > what revision? I was on r68285. Snow Leopard.
(In reply to comment #8) > (In reply to comment #7) > > (In reply to comment #6) > > > This change may have broken these layout tests. They all fail the newly added assert: > > > > > > ASSERTION FAILED: entry.getIndex() < static_cast<int>(d()->functionExecutable->capturedVariableCount()) > > > (/work/WebKit/JavaScriptCore/runtime/JSActivation.cpp:75 bool JSC::JSActivation::symbolTableGet(const JSC::Identifier&, JSC::PropertySlot&)) > > > > > > fast/xmlhttprequest/xmlhttprequest-no-file-access.html > > > ietestcenter/Javascript/12.14-3.html > > > ietestcenter/Javascript/12.14-4.html > > > ietestcenter/Javascript/12.14-5.html > > > ietestcenter/Javascript/12.14-6.html > > > ietestcenter/Javascript/12.14-7.html > > > ietestcenter/Javascript/12.14-8.html > > > > what revision? > > I was on r68285. Snow Leopard. Check with a revision >= 68288
(In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #7) > > > (In reply to comment #6) > > > > This change may have broken these layout tests. They all fail the newly added assert: > > > > > > > > ASSERTION FAILED: entry.getIndex() < static_cast<int>(d()->functionExecutable->capturedVariableCount()) > > > > (/work/WebKit/JavaScriptCore/runtime/JSActivation.cpp:75 bool JSC::JSActivation::symbolTableGet(const JSC::Identifier&, JSC::PropertySlot&)) > > > > > > > > fast/xmlhttprequest/xmlhttprequest-no-file-access.html > > > > ietestcenter/Javascript/12.14-3.html > > > > ietestcenter/Javascript/12.14-4.html > > > > ietestcenter/Javascript/12.14-5.html > > > > ietestcenter/Javascript/12.14-6.html > > > > ietestcenter/Javascript/12.14-7.html > > > > ietestcenter/Javascript/12.14-8.html > > > > > > what revision? > > > > I was on r68285. Snow Leopard. > > Check with a revision >= 68288 All happy at r68401.