[webkit-reviews] review granted: [Bug 46330] Only copy captured variables into activation : [Attachment 68489] Patch

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver at apple.com> has granted Oliver Hunt <oliver at apple.com>'s
request for review:
Bug 46330: Only copy captured variables into activation
https://bugs.webkit.org/show_bug.cgi?id=46330

Attachment 68489: Patch
https://bugs.webkit.org/attachment.cgi?id=68489&action=review

------- Additional Comments from Oliver Hunt <oliver at apple.com>
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(&registerAt(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.