Calculate bits before storing.
Created attachment 363406 [details] Patch
Created attachment 363407 [details] Patch
Created attachment 363414 [details] Patch
Created attachment 363416 [details] Patch
Created attachment 363418 [details] Patch
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 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 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 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.
Committed r242397: <https://trac.webkit.org/changeset/242397>
<rdar://problem/48579999>