Created attachment 439104 [details] Try to fix WPE build again

Comment on attachment 439104 [details] Try to fix WPE build again Nice!

Comment on attachment 439104 [details] Try to fix WPE build again Thanks for the review!

Committed r283024 (242085@main): <https://commits.webkit.org/242085@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 439104 [details].

<rdar://problem/83477591>

What is "POD" is a fuzzy abstraction of "trivially copyable", so it's sometimes thought wrong. I think originally the intention of the IPC encoding was that encoder wouldn't read padding bits that would alert ASAN/MSAN.

David, do you remember if there's a problem with IPC encoding and memcpy reading arbitrary types? E.g. the types might have fields padded and the padding would be uninitialised and some sanitisers would report read of uninitialised memory?

(In reply to Kimmo Kinnunen from comment #10) > David, do you remember if there's a problem with IPC encoding and memcpy > reading arbitrary types? E.g. the types might have fields padded and the > padding would be uninitialised and some sanitisers would report read of > uninitialised memory? Note that we were already using SimpleArgumentCoder for most of the WebCore types that I modified here (just hard-coded to use `IPC::Encoder`) — the only new ones are some CG structs (namely CGRect, CGPoint, CGSize, CGAffineTransform), which are only comprised of CGFloats that *should* be safe to memcpy. That said, it does look like a couple of webanimation tests are failing.. I'm looking into these to see if they're actually related to the changes here.

(In reply to Wenson Hsieh from comment #11) > That said, it does look like a couple of webanimation tests are failing.. > I'm looking into these to see if they're actually related to the changes > here. Only Length fails is_trivially_constructible, so everything this patch adds should be memcpyable

Created attachment 439163 [details] Related Crash Log

Reverted r283024 for reason: Causes slowdown and crash on EWS Committed r283048 (242108@main): <https://commits.webkit.org/242108@main>

Comment on attachment 439104 [details] Try to fix WPE build again View in context: https://bugs.webkit.org/attachment.cgi?id=439104&action=review > Source/WebKit/Shared/WebCoreArgumentCoders.h:241 > +DEFINE_SIMPLE_ARGUMENT_CODER(WebCore::TransformationMatrix) Using SimpleArgumentCoder for WebCore::TransformationMatrix seems to be the (immediate) cause of the crashes, though it's not obvious to me why.

(In reply to Wenson Hsieh from comment #15) > Comment on attachment 439104 [details] > Try to fix WPE build again > > View in context: > https://bugs.webkit.org/attachment.cgi?id=439104&action=review > > > Source/WebKit/Shared/WebCoreArgumentCoders.h:241 > > +DEFINE_SIMPLE_ARGUMENT_CODER(WebCore::TransformationMatrix) > > Using SimpleArgumentCoder for WebCore::TransformationMatrix seems to be the > (immediate) cause of the crashes, though it's not obvious to me why. Alright, I think I've figured it out. Some logging demonstrates that we sometimes fail to properly decode transformation matrices, due to alignment adjustment logic in IPC::Encoder and IPC::Decoder being inconsistent: ``` Web process: Encoding TransformationMatrix #214 [1.00 0.00 0.00 0.00] [0.00 1.00 0.00 0.00] [0.00 0.00 1.00 0.00] [50.00 0.00 0.00 1.00] Rounding up size: 56 to 64 (for alignment: 16) UI process: Decoding TransformationMatrix #214 Rounding up offset: 0x1373f8500 to 0x1373f8500 (for alignment: 16) got [0.00 1.00 0.00 0.00] [0.00 0.00 1.00 0.00] [0.00 0.00 0.00 1.00] [0.00 50.00 0.00 0.00] ``` On the Encoder side, we round the buffer size (in this particular case, 56) up to 64 to match the 16-byte align of TransformationMatrix: ``` uint8_t* Encoder::grow(size_t alignment, size_t size) { size_t alignedSize = roundUpToAlignment(m_bufferSize, alignment); ``` ..on the Decoder side, however, we don't end up rounding the read offset at all, since we round the _buffer pointer address_ up to the alignment of 16 rather than the offset relative to the start of the buffer. ``` bool Decoder::alignBufferPosition(size_t alignment, size_t size) { const uint8_t* alignedPosition = roundUpToAlignment(m_bufferPos, alignment); if (UNLIKELY(!alignedBufferIsLargeEnoughToContain(alignedPosition, m_buffer, m_bufferEnd, size))) { ``` This diff "fixes" this discrepancy by making Decoder-side rounding logic consistent with Encoder-side rounding logic: ``` diff --git a/Source/WebKit/Platform/IPC/Decoder.cpp b/Source/WebKit/Platform/IPC/Decoder.cpp index 743cb481561b..45a0e7d43cb5 100644 --- a/Source/WebKit/Platform/IPC/Decoder.cpp +++ b/Source/WebKit/Platform/IPC/Decoder.cpp @@ -179,6 +179,11 @@ static inline const uint8_t* roundUpToAlignment(const uint8_t* ptr, size_t align return reinterpret_cast<uint8_t*>((reinterpret_cast<uintptr_t>(ptr) + alignmentMask) & ~alignmentMask); } +static inline size_t roundUpToAlignment(size_t value, size_t alignment) +{ + return ((value + alignment - 1) / alignment) * alignment; +} + static inline bool alignedBufferIsLargeEnoughToContain(const uint8_t* alignedPosition, const uint8_t* bufferStart, const uint8_t* bufferEnd, size_t size) { // When size == 0 for the last argument and it's a variable length byte array, @@ -190,7 +195,9 @@ static inline bool alignedBufferIsLargeEnoughToContain(const uint8_t* alignedPos bool Decoder::alignBufferPosition(size_t alignment, size_t size) { - const uint8_t* alignedPosition = roundUpToAlignment(m_bufferPos, alignment); + size_t currentSize = m_bufferPos - m_buffer; + auto alignedSize = roundUpToAlignment(currentSize, alignment); + auto* alignedPosition = m_buffer + alignedSize; if (UNLIKELY(!alignedBufferIsLargeEnoughToContain(alignedPosition, m_buffer, m_bufferEnd, size))) { // We've walked off the end of this buffer. markInvalid(); ```

(In reply to Wenson Hsieh from comment #16) > (In reply to Wenson Hsieh from comment #15) > > Comment on attachment 439104 [details] > > Try to fix WPE build again > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=439104&action=review > > > > > Source/WebKit/Shared/WebCoreArgumentCoders.h:241 > > > +DEFINE_SIMPLE_ARGUMENT_CODER(WebCore::TransformationMatrix) > > > > Using SimpleArgumentCoder for WebCore::TransformationMatrix seems to be the > > (immediate) cause of the crashes, though it's not obvious to me why. > > Alright, I think I've figured it out. > > Some logging demonstrates that we sometimes fail to properly decode > transformation matrices, due to alignment adjustment logic in IPC::Encoder > and IPC::Decoder being inconsistent: > […] ...let's deal with this separately: filed https://bugs.webkit.org/show_bug.cgi?id=230776

(Adding a few folks who might be familiar with WPE) Would anyone happen to know why attempts to templatize the `encode(Encoder&)` function several WebCore structs (e.g. FloatRect, IntSize, and a few others) could cause linker errors on WPE only? Link to build failure: <https://ews-build.webkit.org/#/builders/8/builds/63759>.

(In reply to Wenson Hsieh from comment #29) > Would anyone happen to know why attempts to templatize the > `encode(Encoder&)` function several WebCore structs (e.g. FloatRect, > IntSize, and a few others) could cause linker errors on WPE only? Guess: it's probably because the code is defining template functions in the C++ source file, which requires special care. If the functions are called from outside the translation unit, they have to be either (a) defined in the header file instead, so they're guaranteed to be available in the translation unit they are called from, or else (b) explicitly instantiated using 'extern template'. This is documented here: https://gcc.gnu.org/onlinedocs/gcc/Template-Instantiation.html

(In reply to Michael Catanzaro from comment #30) > (In reply to Wenson Hsieh from comment #29) > > Would anyone happen to know why attempts to templatize the > > `encode(Encoder&)` function several WebCore structs (e.g. FloatRect, > > IntSize, and a few others) could cause linker errors on WPE only? > > Guess: it's probably because the code is defining template functions in the > C++ source file, which requires special care. If the functions are called > from outside the translation unit, they have to be either (a) defined in the > header file instead, so they're guaranteed to be available in the > translation unit they are called from, or else (b) explicitly instantiated > using 'extern template'. This is documented here: > https://gcc.gnu.org/onlinedocs/gcc/Template-Instantiation.html Hm...so my patch moves the definitions of the template functions to the header file (WebCoreArgumentCoders.h), which I thought would be okay because the header file is included in all the places that currently try to encode or decode these objects. That said, I also tried explicit template instantiation (with and without the `extern` keyword), but that didn't seem to have any effect :/ Maybe the template instantiation needs to be in the CPP source instead of the header, though? Going to try this next, and see if it makes any difference.

Created attachment 440309 [details] Try to fix the WPE build again? (4)

Committed r283617 (242569@main): <https://commits.webkit.org/242569@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 440309 [details].

(In reply to Wenson Hsieh from comment #42) > Created attachment 440309 [details] > Try to fix the WPE build again? (4) After some trial and error yesterday, I figured out that this patch fails for WPE in EWS when the bot used to build the patch is `aperez-wpe-ews`; however, it does not fail when `igalia-wpe-ews` is used instead. As far as I can tell, these both use incremental builds, so it's a bit of a mystery to me why it fails for `aperez-wpe-ews`. Tim Horton also helped confirm that all of the patches labeled "Try to fix the WPE build again? (1-9)" build successfully (locally) against WPE on an Arch Linux machine.

(In reply to Wenson Hsieh from comment #49) > (In reply to Wenson Hsieh from comment #42) > > Created attachment 440309 [details] > > Try to fix the WPE build again? (4) > > After some trial and error yesterday, I figured out that this patch fails > for WPE in EWS when the bot used to build the patch is `aperez-wpe-ews`; > however, it does not fail when `igalia-wpe-ews` is used instead. As far as I > can tell, these both use incremental builds, so it's a bit of a mystery to > me why it fails for `aperez-wpe-ews`. I also am not sure why the build failed there, but I forced a fresh full build by removing the build directory for the EWS and it seems to be chugging along nicely now.