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

[bugzilla-daemon at webkit.org]

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
https://bugs.webkit.org/show_bug.cgi?id=229846

Attachment 437244: Patch

https://bugs.webkit.org/attachment.cgi?id=437244&action=review




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