[webkit-reviews] review granted: [Bug 229846] [JSC] Validate JSPropertyNameEnumerator via watchpoints : [Attachment 437244] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Sep 3 09:10:32 PDT 2021

Keith Miller <keith_miller at apple.com> has granted Yusuke Suzuki
<ysuzuki at apple.com>'s request for review:
Bug 229846: [JSC] Validate JSPropertyNameEnumerator via watchpoints

Attachment 437244: Patch


--- Comment #3 from Keith Miller <keith_miller at apple.com> ---
Comment on attachment 437244
  --> https://bugs.webkit.org/attachment.cgi?id=437244

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 <=

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13240
> +	       static_assert(ArrayWithUndecided <= ArrayWithUndecided);


> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13244
> +	       LBasicBlock lastNext = m_out.appendTo(checkExistingCase,

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.

More information about the webkit-reviews mailing list