Bug 195234 - [JSC] Store bits for JSRopeString in 3 stores
Summary: [JSC] Store bits for JSRopeString in 3 stores
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-01 16:53 PST by Yusuke Suzuki
Modified: 2019-03-04 15:30 PST (History)
6 users (show)

See Also:


Attachments
Patch (24.46 KB, patch)
2019-03-01 21:01 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (24.75 KB, patch)
2019-03-01 21:04 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (27.23 KB, patch)
2019-03-02 00:16 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (32.66 KB, patch)
2019-03-02 00:32 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (32.66 KB, patch)
2019-03-02 00:36 PST, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>