Bug 138707 – Add scope operand to op_new_func* byte codes

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

[Michael Saboff]

These byte codes currently use implicit exec->scope() to get there scope.  This needs to be changed to use a passed in scope.

[Michael Saboff]

Created attachment 241505 [details]
Patch

[Mark Lam]

Comment on attachment 241505 [details]
Patch

r=me

[Geoffrey Garen]

Comment on attachment 241505 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4519
> +        SpeculateCellOperand scope(this, node->child2());

You don't want SpeculateCellOperand here; that will emit a branch to check that the scope is a cell, even though we are certain that it is a cell.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4549
> +        SpeculateCellOperand scope(this, node->child2());

Ditto.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4162
> +    SpeculateCellOperand scope(this, node->child1());

Ditto.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4174
> +    SpeculateCellOperand scope(this, node->child1());

Ditto.

[Michael Saboff]

(In reply to comment #3)
> Comment on attachment 241505 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=241505&action=review
> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4519
> > +        SpeculateCellOperand scope(this, node->child2());
> 
> You don't want SpeculateCellOperand here; that will emit a branch to check
> that the scope is a cell, even though we are certain that it is a cell.
> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4549
> > +        SpeculateCellOperand scope(this, node->child2());
> 
> Ditto.
> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4162
> > +    SpeculateCellOperand scope(this, node->child1());
> 
> Ditto.
> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4174
> > +    SpeculateCellOperand scope(this, node->child1());
> 
> Ditto.

The way I read fillSpeculateCell(), we won't issue the check for DataFormatCell or DataFormatJSCell:

    case DataFormatCell:
    case DataFormatJSCell: {
        GPRReg gpr = info.gpr();
        m_gprs.lock(gpr);
        if (!ASSERT_DISABLED) {
            MacroAssembler::Jump checkCell = branchIsCell(JSValueRegs(gpr));
            m_jit.abortWithReason(DFGIsNotCell);
            checkCell.link(&m_jit);
        }
        return gpr;
    }

I'm just following the existing pattern for other accesses to the scope chain.

    case SkipScope: {
        SpeculateCellOperand scope(this, node->child1());
        GPRTemporary result(this, Reuse, scope);
        m_jit.loadPtr(JITCompiler::Address(scope.gpr(), JSScope::offsetOfNext()), result.gpr());
        cellResult(result.gpr(), node);
        break;
    }

    case GetClosureRegisters: {
 ...        
        SpeculateCellOperand scope(this, node->child1());
        GPRTemporary result(this);
        GPRReg scopeGPR = scope.gpr();
        GPRReg resultGPR = result.gpr();

The current code that uses the call frame ScopeChain register slot will either have the VirtualRegister as a cell or a constant.  For non-inlined frames the child is from a GetMyScope node, which returns a cellResult.  For inlined frames, child is from a JSConstant of the JSScope* value.  Which won't create a check.

When this work is done, op_get_scope will turn into a GetScope node which also has a cellResult.

[Michael Saboff]

Committed r176109: <http://trac.webkit.org/changeset/176109>