Bug 195234

Summary: [JSC] Store bits for JSRopeString in 3 stores
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, mark.lam, msaboff, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch saam: review+

Description Yusuke Suzuki 2019-03-01 16:53:38 PST
Calculate bits before storing.
Comment 1 Yusuke Suzuki 2019-03-01 21:01:47 PST
Created attachment 363406 [details]
Patch
Comment 2 Yusuke Suzuki 2019-03-01 21:04:53 PST
Created attachment 363407 [details]
Patch
Comment 3 Yusuke Suzuki 2019-03-02 00:16:38 PST
Created attachment 363414 [details]
Patch
Comment 4 Yusuke Suzuki 2019-03-02 00:32:54 PST
Created attachment 363416 [details]
Patch
Comment 5 Yusuke Suzuki 2019-03-02 00:36:20 PST
Created attachment 363418 [details]
Patch
Comment 6 Saam Barati 2019-03-04 11:57:50 PST
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.
Comment 7 Yusuke Suzuki 2019-03-04 14:37:26 PST
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.
Comment 8 Yusuke Suzuki 2019-03-04 14:42:39 PST
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.
Comment 9 Yusuke Suzuki 2019-03-04 14:47:32 PST
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.
Comment 10 Yusuke Suzuki 2019-03-04 15:27:58 PST
Committed r242397: <https://trac.webkit.org/changeset/242397>
Comment 11 Radar WebKit Bug Importer 2019-03-04 15:30:09 PST
<rdar://problem/48579999>