Bug 229846 – [JSC] Validate JSPropertyNameEnumerator via watchpoints

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Guard JSPropertyNameEnumerator validation via watchpoints

[Yusuke Suzuki]

Created attachment 437235 [details]
Patch

[Yusuke Suzuki]

Created attachment 437244 [details]
Patch

[Keith Miller]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r282014 (241319@main): <https://commits.webkit.org/241319@main>

[Radar WebKit Bug Importer]

<rdar://problem/82735046>