Bug 195234 – [JSC] Store bits for JSRopeString in 3 stores

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

Calculate bits before storing.

[Yusuke Suzuki]

Created attachment 363406 [details]
Patch

[Yusuke Suzuki]

Created attachment 363407 [details]
Patch

[Yusuke Suzuki]

Created attachment 363414 [details]
Patch

[Yusuke Suzuki]

Created attachment 363416 [details]
Patch

[Yusuke Suzuki]

Created attachment 363418 [details]
Patch

[Saam Barati]

Comment on attachment 363418 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=363418&action=review

r=me

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6537
> +        // This puts nullptr for the first fiber. It makes visitChildren safe even if this JSRopeString is discarded due to the speculation failure in the following path.
> +        m_out.storePtr(m_out.constIntPtr(JSString::isRopeInPointer), result, m_heaps.JSRopeString_fiber0);

This is somewhat scary, please make sure we have tests.

[Yusuke Suzuki]

Comment on attachment 363418 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=363418&action=review

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6537
>> +        m_out.storePtr(m_out.constIntPtr(JSString::isRopeInPointer), result, m_heaps.JSRopeString_fiber0);
> 
> This is somewhat scary, please make sure we have tests.

I'm not sure this is reliably testable. The |result| won't be in the stack. So, the only way this happens is,

1. We create JSRopeString 0x0010
2. We do not use 0x0010 because the overflow happens
3. But conservative stack scan accidentally finds 0x0010, (but it is not the pointer to the above JSRopeString)
4. Scan it

We can test JSRopeString(nullptr) case if we add a special function to JSRopeString just for testing (of course, for that JSRopeString, all the operations are broken. This is used just for GC scan test).
I'll add this instead.

[Yusuke Suzuki]

Comment on attachment 363418 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=363418&action=review

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6597
> +        m_out.storePtr(
> +            m_out.bitOr(m_out.zeroExtPtr(flagsAndLength.length), m_out.shl(kids[1], m_out.constInt32(32))),
> +            result, m_heaps.JSRopeString_fiber1);
> +        if (numKids == 2)
> +            m_out.storePtr(m_out.lShr(kids[1], m_out.constInt32(32)), result, m_heaps.JSRopeString_fiber2);
> +        else
> +            m_out.storePtr(m_out.bitOr(m_out.lShr(kids[1], m_out.constInt32(32)), m_out.shl(kids[2], m_out.constInt32(16))), result, m_heaps.JSRopeString_fiber2);

I'll move this part to the prologue of this MakeRope since we would like to ensure that all the 3 fibers are filled before speculative add happens.

[Yusuke Suzuki]

Comment on attachment 363418 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=363418&action=review

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6597
>> +            m_out.storePtr(m_out.bitOr(m_out.lShr(kids[1], m_out.constInt32(32)), m_out.shl(kids[2], m_out.constInt32(16))), result, m_heaps.JSRopeString_fiber2);
> 
> I'll move this part to the prologue of this MakeRope since we would like to ensure that all the 3 fibers are filled before speculative add happens.

No. This is not necessary. When we fill fibers, we already have all the bits and speculation-add already pass.

[Yusuke Suzuki]

Committed r242397: <https://trac.webkit.org/changeset/242397>

[Radar WebKit Bug Importer]

<rdar://problem/48579999>