[JSC] Guard JSPropertyNameEnumerator validation via watchpoints
Created attachment 437235 [details] Patch
Created attachment 437244 [details] Patch
Comment on attachment 437244 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=437244&action=review Maybe file a bug for Baseline/LLInt too? > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:13946 > SpeculateCellOperand base(this, node->child1()); Maybe file a bug to constant fold this if we have the watchpoint and a known constant structure? > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:13963 > + static_assert(ArrayWithUndecided <= ArrayWithUndecided); I assume this should be `static_assert(NonArrayWithUndecided <= ArrayWithUndecided)`? > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13240 > + static_assert(ArrayWithUndecided <= ArrayWithUndecided); Ditto. > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13244 > + LBasicBlock lastNext = m_out.appendTo(checkExistingCase, notNullCase); Nit: Seems like notNull would be the wrong continuation if anyone tried to fall-through right? Then again I never understood what lastNext was intended to represent lol > Source/JavaScriptCore/runtime/JSPropertyNameEnumerator.h:126 > + uint32_t m_validatedViaWatchpoint { 0 }; Nit: I'm not sure if m_modeSet increased the size of the struct but we could use a bit in m_modeSet if it did. That said I don't think there's a noticeable number of enumerator objects anyway.
Comment on attachment 437244 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=437244&action=review Thanks! >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:13946 >> SpeculateCellOperand base(this, node->child1()); > > Maybe file a bug to constant fold this if we have the watchpoint and a known constant structure? Sounds good. Will file. >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:13963 >> + static_assert(ArrayWithUndecided <= ArrayWithUndecided); > > I assume this should be `static_assert(NonArrayWithUndecided <= ArrayWithUndecided)`? There is no NonArrayWithUndecided since UndecidedShape is only valid for Array :) >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13240 >> + static_assert(ArrayWithUndecided <= ArrayWithUndecided); > > Ditto. Ditto. >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13244 >> + LBasicBlock lastNext = m_out.appendTo(checkExistingCase, notNullCase); > > Nit: Seems like notNull would be the wrong continuation if anyone tried to fall-through right? Then again I never understood what lastNext was intended to represent lol I think this is correct, right? After the branch, we are putting notNullCase >> Source/JavaScriptCore/runtime/JSPropertyNameEnumerator.h:126 >> + uint32_t m_validatedViaWatchpoint { 0 }; > > Nit: I'm not sure if m_modeSet increased the size of the struct but we could use a bit in m_modeSet if it did. That said I don't think there's a noticeable number of enumerator objects anyway. That sounds good. Changing.
Committed r282014 (241319@main): <https://commits.webkit.org/241319@main>
<rdar://problem/82735046>