[JSC] JSWrapperObject should use JSInternalFieldObjectImpl
Created attachment 395490 [details] Patch
Comment on attachment 395490 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395490&action=review > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:-9866 > - m_jit.storePtr( > - TrustedImmPtr(StringObject::info()), > - JITCompiler::Address(resultGPR, JSDestructibleObject::classInfoOffset())); This was wrong :P > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:-1533 > - template<typename ClassType> > - void emitAllocateDestructibleObject(GPRReg resultGPR, RegisteredStructure structure, > - GPRReg scratchGPR1, GPRReg scratchGPR2, MacroAssembler::JumpList& slowPath) > - { > - m_jit.emitAllocateDestructibleObject<ClassType>(vm(), resultGPR, structure.get(), scratchGPR1, scratchGPR2, slowPath); > - } We are no longer using this. > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:-6404 > - m_out.storePtr(m_out.constIntPtr(StringObject::info()), fastResultValue, m_heaps.JSDestructibleObject_classInfo); Ditto :P
Created attachment 395491 [details] Patch
Created attachment 395492 [details] Patch
Comment on attachment 395492 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395492&action=review r=me with suggestion. > Source/JavaScriptCore/runtime/JSWrapperObject.cpp:37 > void JSWrapperObject::visitChildren(JSCell* cell, SlotVisitor& visitor) > { > - JSWrapperObject* thisObject = jsCast<JSWrapperObject*>(cell); > + auto* thisObject = jsCast<JSWrapperObject*>(cell); > ASSERT_GC_OBJECT_INHERITS(thisObject, info()); > - JSObject::visitChildren(thisObject, visitor); > - visitor.append(thisObject->m_internalValue); > + Base::visitChildren(thisObject, visitor); > } This no longer adds much value. Can we just get rid of it and use the Base visitChildren()? The only thing done here that is not done by the Base is the ASSERT_GC_OBJECT_INHERITS. How much value is there in keeping that? If we want to keep this assert, can / should we make this function conditional on ASSERT_ENABLED?
Comment on attachment 395492 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395492&action=review > Source/JavaScriptCore/ChangeLog:16 > + We remove this wrong store. I feel like all of the c++ code emitting these stores should come along with a static_assert
Comment on attachment 395492 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395492&action=review >> Source/JavaScriptCore/runtime/JSWrapperObject.cpp:37 >> } > > This no longer adds much value. Can we just get rid of it and use the Base visitChildren()? > > The only thing done here that is not done by the Base is the ASSERT_GC_OBJECT_INHERITS. How much value is there in keeping that? If we want to keep this assert, can / should we make this function conditional on ASSERT_ENABLED? To avoid bothering about template-weak-symbol related linking problems (template weak symbols which is only referenced as a pointer), I think defining this is harmless and easy to make it manageable. For now, I'll just use this.
Comment on attachment 395492 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395492&action=review >> Source/JavaScriptCore/ChangeLog:16 >> + We remove this wrong store. > > I feel like all of the c++ code emitting these stores should come along with a static_assert I think we should eventually put more and more static_assert in FTL / DFG side to ensure that FTL / DFG's class hierarchy assumption is kept.
Committed r259645: <https://trac.webkit.org/changeset/259645>
<rdar://problem/61399515>