RESOLVED FIXED 157972
super should not depend on __proto__
https://bugs.webkit.org/show_bug.cgi?id=157972
Summary super should not depend on __proto__
Alexey Shvayka
Reported 2016-05-21 01:00:12 PDT
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.
Attachments
Patch (23.21 KB, patch)
2020-05-21 17:06 PDT, Alexey Shvayka
no flags
Patch (24.35 KB, patch)
2020-05-22 08:27 PDT, Alexey Shvayka
no flags
Patch (24.47 KB, patch)
2020-05-22 08:44 PDT, Alexey Shvayka
no flags
Patch (24.57 KB, patch)
2020-05-22 12:26 PDT, Alexey Shvayka
no flags
Patch (23.86 KB, patch)
2020-05-28 08:48 PDT, Alexey Shvayka
no flags
Patch (43.17 KB, patch)
2020-06-01 13:01 PDT, Alexey Shvayka
no flags
Patch (44.91 KB, patch)
2020-06-02 01:20 PDT, Alexey Shvayka
no flags
Patch (47.83 KB, patch)
2020-06-12 14:20 PDT, Alexey Shvayka
ysuzuki: commit-queue-
Alexey Shvayka
Comment 1 2020-05-21 17:06:16 PDT
Alexey Shvayka
Comment 2 2020-05-21 17:06:38 PDT
(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
Comment 3 2020-05-22 08:27:05 PDT
Created attachment 400048 [details] Patch Fix 32-bit build and handle JSFunctionType by JIT fast path.
Alexey Shvayka
Comment 4 2020-05-22 08:44:59 PDT
Created attachment 400051 [details] Patch Fix 32-bit build.
Alexey Shvayka
Comment 5 2020-05-22 12:26:36 PDT
Created attachment 400063 [details] Patch Drop emitLoadStructure() and use OBJECT_OFFSETOF for 32-bit targets.
Alexey Shvayka
Comment 6 2020-05-28 08:48:47 PDT
Created attachment 400464 [details] Patch Drop baseline JIT fast path for 32-bit targets.
Alexey Shvayka
Comment 7 2020-05-28 12:25:39 PDT
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 8 2020-05-28 13:57:13 PDT
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
Comment 9 2020-06-01 13:01:32 PDT
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
Comment 10 2020-06-01 13:03:31 PDT
(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
Comment 11 2020-06-02 01:20:25 PDT
Created attachment 400794 [details] Patch Fix tests and 32-bit build, use JSValue::getPrototype() in objectConstructorGetPrototypeOf().
Alexey Shvayka
Comment 12 2020-06-02 13:58:23 PDT
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 13 2020-06-11 12:48:27 PDT
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
Comment 14 2020-06-12 14:20:45 PDT
Created attachment 401781 [details] Patch Define numberOfInlineBits and OverridesGetPrototypeOutOfLine, add overridesGetPrototype() validation.
Alexey Shvayka
Comment 15 2020-06-12 14:27:06 PDT
(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 16 2020-06-12 19:43:01 PDT
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
Comment 17 2020-06-13 10:15:34 PDT
(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
Comment 18 2020-06-13 10:16:34 PDT
I’m ok if it’s a follow up as well.
Yusuke Suzuki
Comment 19 2020-06-14 17:40:07 PDT
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
Comment 20 2020-06-15 07:42:47 PDT
Radar WebKit Bug Importer
Comment 21 2020-06-15 07:43:13 PDT
Alexey Shvayka
Comment 22 2020-06-15 07:46:25 PDT
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
Comment 23 2020-06-15 10:15:07 PDT
(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 24 2020-06-15 10:20:38 PDT
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
Comment 25 2020-06-15 10:24:05 PDT
Saam Barati
Comment 26 2020-06-15 10:44:59 PDT
(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
Comment 27 2020-06-23 13:46:29 PDT
This change regressed JSC stress tests on s390x. I'm not sure why. See bug #213307.
Note You need to log in before you can comment on or make changes to this bug.