Bug 157972 – super should not depend on __proto__

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Alexey Shvayka]

Consider the following code:

```js
class C extends Object {}
delete Object.prototype.__proto__
let c = new C
```

WebKit Nightly throws `TypeError: undefined is not a constructor`.
However, `super` should call internal `[[GetPrototypeOf]]`, not the `__proto__` getter.
This code works fine both in Chrome Canary and Firefox.

[Alexey Shvayka]

Created attachment 400002 [details]
Patch

[Alexey Shvayka]

(In reply to Alexey Shvayka from comment #1)
> Created attachment 400002 [details]
> Patch

With warm-up runs, --outer 128:

                                                 r262029                    patch                                       

super-get-by-id-with-this-monomorphic        22.6679+-0.4237     ^     20.5501+-0.3550        ^ definitely 1.1031x faster
super-get-by-id-with-this-polymorphic        17.9731+-0.2928           17.5325+-0.2612          might be 1.0251x faster
super-get-by-val-with-this-monomorphic       24.4027+-1.1266     ^     22.5436+-0.7088        ^ definitely 1.0825x faster
super-get-by-val-with-this-polymorphic       21.3870+-1.2635           20.6429+-0.6245          might be 1.0360x faster
super-getter                                 16.4953+-0.2982     ?     16.6465+-0.3252        ?

[Alexey Shvayka]

Created attachment 400048 [details]
Patch

Fix 32-bit build and handle JSFunctionType by JIT fast path.

[Alexey Shvayka]

Created attachment 400051 [details]
Patch

Fix 32-bit build.

[Alexey Shvayka]

Created attachment 400063 [details]
Patch

Drop emitLoadStructure() and use OBJECT_OFFSETOF for 32-bit targets.

[Alexey Shvayka]

Created attachment 400464 [details]
Patch

Drop baseline JIT fast path for 32-bit targets.

[Alexey Shvayka]

Comment on attachment 400464 [details]
Patch

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

> Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:1307
> +    addSlowCase(jump());

If we compile this op properly (see prev. patch), stress/arrowfunction-lexical-bind-supercall-2.js segfaults on ARM & MIPS EWS.
I couldn't reproduce segfaults locally on both 32-bit Intel and ARMv7 (via qemu), yet I think it is fine to slow-case it unconditionally.

[Saam Barati]

Comment on attachment 400464 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:12
> +        This patch defines OpGetPrototypeOf for LLInt and baseline JIT, ensuring

makes sense

> Source/JavaScriptCore/jit/JITOpcodes.cpp:1534
> +    addSlowCase(branchIfNotType(regT0, FinalObjectType, JSFunctionType));

This feels like a precarious implementation without having some kind of assert this is true.

Also, couldn't we just inline the fast path case here by looking at the class info function ptr?

Like the JSObject implementation:
```
    MethodTable::GetPrototypeFunctionPtr defaultGetPrototype = JSObject::getPrototype;
    if (LIKELY(getPrototypeMethod == defaultGetPrototype))
        return getPrototypeDirect(vm);
```

This can be done in a followup. We should probably also inline it in the DFG/FTL

Maybe we could add an out of line type info bit to make it even faster?

>> Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:1307
>> +    addSlowCase(jump());
> 
> If we compile this op properly (see prev. patch), stress/arrowfunction-lexical-bind-supercall-2.js segfaults on ARM & MIPS EWS.
> I couldn't reproduce segfaults locally on both 32-bit Intel and ARMv7 (via qemu), yet I think it is fine to slow-case it unconditionally.

you can just make this a SLOW_CASE_OP (or whatever the macro is) in the switch statement for 32-bit and then you don't need to implement this.

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:1766
> +slowPathOp(get_prototype_of)

let's implement a fast path, at least for 64-bit

[Alexey Shvayka]

Created attachment 400749 [details]
Patch

Add LLInt fast path (64-bit only), add bytecode intrinsic & test, and introduce OverridesGetPrototype out of line flag.

[Alexey Shvayka]

(In reply to Saam Barati from comment #8)

Thank you for detailed comments, Saam.

> Also, couldn't we just inline the fast path case here by looking at the
> class info function ptr?

Comparing GetPrototypeFunctionPtr led to 70% regression (w/o DFG) for microbenchmarks/super-getter.js
and ~10% for other `super` microbenchmarks. Code I was testing:

```
addSlowCase(branchIfNotCell(regT0));
emitLoadStructure(vm(), regT0, regT2, regT1);

static constexpr MethodTable::GetPrototypeFunctionPtr defaultGetPrototype = JSObject::getPrototype;
static ptrdiff_t getPrototypeOffset = Structure::classInfoOffset() + ClassInfo::offsetOfMethodTable() + MethodTable::offsetOfGetPrototype();
addSlowCase(branchPtr(NotEqual, Address(regT2, getPrototypeOffset), TrustedImmPtr(defaultGetPrototype)));
```

So I went with out of line flag, which is performance-neutral and a nice refactor.

> We should probably also inline it in the DFG/FTL

Looks like DFG already inlines GetPrototypeOf, please see the changed bit in DFGAbstractInterpreterInlines.h.

[Alexey Shvayka]

Created attachment 400794 [details]
Patch

Fix tests and 32-bit build, use JSValue::getPrototype() in objectConstructorGetPrototypeOf().

[Alexey Shvayka]

Comment on attachment 400794 [details]
Patch

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

With this change, Object.getPrototypeOf() uses synthesizePrototype() for primitives instead of allocating wrapper object. --outer 128: 

                                             trunk                     patch                                       

object-get-prototype-of-primitive      111.1243+-1.0766     ^     79.5175+-0.7592        ^ definitely 1.3975x faster

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1492
> +    const OverridesGetPrototype = 0x400 # (1 << 18) >> 8

We can improve this by tweaking LLInt parser to support bitwise shifts in a follow-up.

[Saam Barati]

Comment on attachment 400794 [details]
Patch

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

r=me with a few comments

> Source/JavaScriptCore/jit/JITOpcodes.cpp:1528
> +void JIT::emit_op_get_prototype_of(const Instruction* currentInstruction)

Can we also add fast paths for DFG/FTL that look like this? Maybe in a follow-up?

> Source/JavaScriptCore/jit/JITOpcodes.cpp:1537
> +    addSlowCase(branchTest32(NonZero, Address(regT2, Structure::outOfLineTypeFlagsOffset()), TrustedImm32(OverridesGetPrototype >> 8)));

nit: can we define 8 somewhere in JSTypeInfo like "numberOfInlineBits"?

>> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1492
>> +    const OverridesGetPrototype = 0x400 # (1 << 18) >> 8
> 
> We can improve this by tweaking LLInt parser to support bitwise shifts in a follow-up.

constexpr expressions in LLInt doesn't work here?

If not, I'd prefer to define this in the JSTypeInfo header and just use the value here, and we can static assert it's proper there? Could also be used in baseline JIT code for this.

> Source/JavaScriptCore/runtime/JSTypeInfo.h:60
> +static constexpr unsigned OverridesGetPrototype = 1 << 18;

Can you add validation for this into the new Structure::validateFlags that was added after you uploaded this change.

[Alexey Shvayka]

Created attachment 401781 [details]
Patch

Define numberOfInlineBits and OverridesGetPrototypeOutOfLine, add overridesGetPrototype() validation.

[Alexey Shvayka]

(In reply to Saam Barati from comment #13)
> Can we also add fast paths for DFG/FTL that look like this? Maybe in a follow-up?

Looks like we already have those: please see DFGSpeculativeJIT.cpp/compileGetPrototypeOf() and FTLLowerDFGToB3.cpp/compileGetPrototypeOf().
LLInt and baseline JIT fast paths are inspired by DFG one.

> constexpr expressions in LLInt doesn't work here?

constexpr does work, but no the bit shifts.

[Yusuke Suzuki]

Comment on attachment 401781 [details]
Patch

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

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1511
> +    btqnz t0, notCellMask, .opGetPrototypeOfSlow
> +    bbb JSCell::m_type[t0], ObjectType, .opGetPrototypeOfSlow
> +
> +    loadStructureWithScratch(t0, t2, t1, t3)
> +    btinz Structure::m_outOfLineTypeFlags[t2], OverridesGetPrototypeOutOfLine, .opGetPrototypeOfSlow
> +
> +    loadq Structure::m_prototype[t2], t2
> +    btqz t2, .opGetPrototypeOfPolyProto
> +    return(t2)
> +

Ah, no. I think what Saam says is adding these fast path for void SpeculativeJIT::compileGetPrototypeOf(Node* node)'s ObjectUse and generic cases.


  13945     case ObjectUse: {
  13946         SpeculateCellOperand value(this, node->child1());
  13947         JSValueRegsTemporary result(this);
  13948
  13949         GPRReg valueGPR = value.gpr();
  13950         JSValueRegs resultRegs = result.regs();
  13951
  13952         speculateObject(node->child1(), valueGPR);
  13953
  13954         flushRegisters();
  13955         callOperation(operationGetPrototypeOfObject, resultRegs, TrustedImmPtr::weakPointer(m_graph, m_graph.globalObjectFor(node->origin.semantic)), valueGPR);
  13956         m_jit.exceptionCheck();
  13957         jsValueResult(resultRegs, node);
  13958         return;
  13959     }
  13960     default: {
  13961         JSValueOperand value(this, node->child1());
  13962         JSValueRegsTemporary result(this);
  13963
  13964         JSValueRegs valueRegs = value.jsValueRegs();
  13965         JSValueRegs resultRegs = result.regs();
  13966
  13967         flushRegisters();
  13968         callOperation(operationGetPrototypeOf, resultRegs, TrustedImmPtr::weakPointer(m_graph, m_graph.globalObjectFor(node->origin.semantic)), valueRegs);
  13969         m_jit.exceptionCheck();
  13970         jsValueResult(resultRegs, node);
  13971         return;
  13972     }

We do not have this. And please add it to FTL too.

[Saam Barati]

(In reply to Yusuke Suzuki from comment #16)
> Comment on attachment 401781 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401781&action=review
> 
> > Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1511
> > +    btqnz t0, notCellMask, .opGetPrototypeOfSlow
> > +    bbb JSCell::m_type[t0], ObjectType, .opGetPrototypeOfSlow
> > +
> > +    loadStructureWithScratch(t0, t2, t1, t3)
> > +    btinz Structure::m_outOfLineTypeFlags[t2], OverridesGetPrototypeOutOfLine, .opGetPrototypeOfSlow
> > +
> > +    loadq Structure::m_prototype[t2], t2
> > +    btqz t2, .opGetPrototypeOfPolyProto
> > +    return(t2)
> > +
> 
> Ah, no. I think what Saam says is adding these fast path for void
> SpeculativeJIT::compileGetPrototypeOf(Node* node)'s ObjectUse and generic
> cases.
> 
> 
>   13945     case ObjectUse: {
>   13946         SpeculateCellOperand value(this, node->child1());
>   13947         JSValueRegsTemporary result(this);
>   13948
>   13949         GPRReg valueGPR = value.gpr();
>   13950         JSValueRegs resultRegs = result.regs();
>   13951
>   13952         speculateObject(node->child1(), valueGPR);
>   13953
>   13954         flushRegisters();
>   13955         callOperation(operationGetPrototypeOfObject, resultRegs,
> TrustedImmPtr::weakPointer(m_graph,
> m_graph.globalObjectFor(node->origin.semantic)), valueGPR);
>   13956         m_jit.exceptionCheck();
>   13957         jsValueResult(resultRegs, node);
>   13958         return;
>   13959     }
>   13960     default: {
>   13961         JSValueOperand value(this, node->child1());
>   13962         JSValueRegsTemporary result(this);
>   13963
>   13964         JSValueRegs valueRegs = value.jsValueRegs();
>   13965         JSValueRegs resultRegs = result.regs();
>   13966
>   13967         flushRegisters();
>   13968         callOperation(operationGetPrototypeOf, resultRegs,
> TrustedImmPtr::weakPointer(m_graph,
> m_graph.globalObjectFor(node->origin.semantic)), valueRegs);
>   13969         m_jit.exceptionCheck();
>   13970         jsValueResult(resultRegs, node);
>   13971         return;
>   13972     }
> 
> We do not have this. And please add it to FTL too.

Yeah this is what I meant

[Saam Barati]

I’m ok if it’s a follow up as well.

[Yusuke Suzuki]

Comment on attachment 401781 [details]
Patch

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

>>> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1511
>>> +
>> 
>> Ah, no. I think what Saam says is adding these fast path for void SpeculativeJIT::compileGetPrototypeOf(Node* node)'s ObjectUse and generic cases.
>> 
>> 
>>   13945     case ObjectUse: {
>>   13946         SpeculateCellOperand value(this, node->child1());
>>   13947         JSValueRegsTemporary result(this);
>>   13948
>>   13949         GPRReg valueGPR = value.gpr();
>>   13950         JSValueRegs resultRegs = result.regs();
>>   13951
>>   13952         speculateObject(node->child1(), valueGPR);
>>   13953
>>   13954         flushRegisters();
>>   13955         callOperation(operationGetPrototypeOfObject, resultRegs, TrustedImmPtr::weakPointer(m_graph, m_graph.globalObjectFor(node->origin.semantic)), valueGPR);
>>   13956         m_jit.exceptionCheck();
>>   13957         jsValueResult(resultRegs, node);
>>   13958         return;
>>   13959     }
>>   13960     default: {
>>   13961         JSValueOperand value(this, node->child1());
>>   13962         JSValueRegsTemporary result(this);
>>   13963
>>   13964         JSValueRegs valueRegs = value.jsValueRegs();
>>   13965         JSValueRegs resultRegs = result.regs();
>>   13966
>>   13967         flushRegisters();
>>   13968         callOperation(operationGetPrototypeOf, resultRegs, TrustedImmPtr::weakPointer(m_graph, m_graph.globalObjectFor(node->origin.semantic)), valueRegs);
>>   13969         m_jit.exceptionCheck();
>>   13970         jsValueResult(resultRegs, node);
>>   13971         return;
>>   13972     }
>> 
>> We do not have this. And please add it to FTL too.
> 
> Yeah this is what I meant

So can you add a FIXME with bugzilla URL?

[Alexey Shvayka]

Committed r263035: <https://trac.webkit.org/changeset/263035>

[Radar WebKit Bug Importer]

<rdar://problem/64362505>

[Alexey Shvayka]

Comment on attachment 401781 [details]
Patch

(In reply to Yusuke Suzuki from comment #19)
> So can you add a FIXME with bugzilla URL?

Asked Yusuke offline: I will add DFG/FTL fast paths in https://bugs.webkit.org/show_bug.cgi?id=213191 since it requires additional tests and microbenchmarks.
Landed with a FIXME.

[Ryan Haddad]

(In reply to Alexey Shvayka from comment #20)
> Committed r263035: <https://trac.webkit.org/changeset/263035>
This change appears to have broken the CLoop build:
/Volumes/Data/slave/catalina-cloop-debug/build/Source/JavaScriptCore/offlineasm/cloop.rb:649:in `lowerC_LOOP': undefined method `uint32MemRef' for #<Immediate:0x00007f8fb1bc5fa0> (due to JavaScriptCore/llint/LowLevelInterpreter64.asm:1517) (LoweringError)

https://build.webkit.org/builders/Apple-Catalina-LLINT-CLoop-BuildAndTest/builds/4871/steps/compile-webkit/logs/stdio

[Yusuke Suzuki]

Comment on attachment 401781 [details]
Patch

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

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1517
> +    loadi knownPolyProtoOffset, t1

This is wrong. It should be mov.

[Yusuke Suzuki]

Committed r263043: <https://trac.webkit.org/changeset/263043>

[Saam Barati]

(In reply to Yusuke Suzuki from comment #25)
> Committed r263043: <https://trac.webkit.org/changeset/263043>

This code in general is a little silly that it's using a macro that branches on a known offset.

We should just load directly from the object. Maybe we can split a new macro called "loadInlineOffset" or something

macro loadPropertyAtVariableOffset(propertyOffsetAsInt, objectAndStorage, value)
    bilt propertyOffsetAsInt, firstOutOfLineOffset, .isInline
    loadp JSObject::m_butterfly[objectAndStorage], objectAndStorage
    negi propertyOffsetAsInt
    sxi2q propertyOffsetAsInt, propertyOffsetAsInt
    jmp .ready
.isInline:
    addp sizeof JSObject - (firstOutOfLineOffset - 2) * 8, objectAndStorage
.ready:
    loadq (firstOutOfLineOffset - 2) * 8[objectAndStorage, propertyOffsetAsInt, 8], value
end

[Michael Catanzaro]

This change regressed JSC stress tests on s390x. I'm not sure why. See bug #213307.