| Summary: | [JSC] Validate JSPropertyNameEnumerator via watchpoints | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||
| Component: | New Bugs | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Yusuke Suzuki
2021-09-02 19:32:20 PDT
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> |