This bug report originated from issue QTBUG-3991 <http://bugreports.qt.nokia.com/browse/QTBUG-3991> --- Description --- We have found an issue in the following function: static inline bool equal(StringImpl* string, const UChar* characters, unsigned length) in AtomicString.cpp. The issue is that the variables 'stringCharacters' and 'bufferCharacters' are uint32 pointers but in fact point to UChar* variables. This means that the starting addresses might be not aligned to 4 bytes. On MIPS an unaligned exception is raised and in our case is handled by the kernel, but, depending on the kernel, it can also lead to the kernel sending SIGBUS to the application. The kernel exception handling is very costly and below is a solution to this problem; basically we define a structure with the 'packed' attribute which will make the compiler to generate code capable of handling unaligned accesses (on MIPS this means replacing 'lw' with a pair: 'lwl' and 'lwr'). I saw that Q_PACKED is defined only for the GCC and Intel compiler which should make our change safe for all cases. Another solution is to simply add || PLATFORM(MIPS) where the check for ARM is made. Patch: --- a/src/3rdparty/webkit/WebCore/platform/text/AtomicString.cpp +++ b/src/3rdparty/webkit/WebCore/platform/text/AtomicString.cpp @@ -96,6 +96,10 @@ struct UCharBuffer { unsigned length; }; +struct unaligned_struct { + uint32_t i; +} Q_PACKED; + static inline bool equal(StringImpl* string, const UChar* characters, unsigned length) { if (string->length() != length) @@ -111,17 +115,21 @@ static inline bool equal(StringImpl* string, const UChar* characters, unsigned l #else /* Do it 4-bytes-at-a-time on architectures where it's safe */ - const uint32_t* stringCharacters = reinterpret_cast<const uint32_t*>(string->characters()); - const uint32_t* bufferCharacters = reinterpret_cast<const uint32_t*>(characters); + const struct unaligned_struct *stringCharacters = + reinterpret_cast<const struct unaligned_struct*>(string->characters()); + const struct unaligned_struct *bufferCharacters = + reinterpret_cast<const struct unaligned_struct*>(characters); unsigned halfLength = length >> 1; - for (unsigned i = 0; i != halfLength; ++i) { - if (*stringCharacters++ != *bufferCharacters++) + for (unsigned i = 0; i != halfLength; ++i, stringCharacters++, bufferCharacters++) { + if (stringCharacters->i != bufferCharacters->i) return false; } - if (length & 1 && *reinterpret_cast<const uint16_t*>(stringCharacters) != *reinterpret_cast<const uint16_t*>(bufferCharacters)) + if (length & 1 && + *reinterpret_cast<const uint16_t*>(stringCharacters) != *reinterpret_cast<const uint16_t*>(bufferCharacters)) { return false; + } return true; #endif
The OS should be Other in this bug
This is not a Qt-specific issue. +1 for adding CPU(MIPS) to the "going 4 bytes at a time is unsafe" condition, though.
Created attachment 51602 [details] Aligment patch from Gentoo for ARM, SPARC and SH4 platforms This is the patch we are using on Gentoo to fix alignment issues on SPARC and SH4 platforms.
Removing Qt keyword and Qt from the title, this is not a Qt specific issue.
(In reply to comment #3) > Created an attachment (id=51602) [details] > Aligment patch from Gentoo for ARM, SPARC and SH4 platforms > > This is the patch we are using on Gentoo to fix alignment issues on SPARC and > SH4 platforms. Priit, can you submit your patch with ChangeLog, diffed against tip of tree and with review? set? See also http://webkit.org/coding/contributing.html Thanks :)
Created attachment 63668 [details] Access one byte at a time for MIPS To fix this issue, we follow ARM and SH4 to access one byte at a time. This improves the performance on MIPS, due to no kernel involvement to fix unaligned accesses. In the future, we still can use lwl/lwr to improve the performance. Thanks! Regards, Chao-ying
Look really a trivial and small change. Anyone who has MIPS experience is looks good candidate for just giving r+.
Comment on attachment 63668 [details] Access one byte at a time for MIPS r=me
Comment on attachment 63668 [details] Access one byte at a time for MIPS Clearing flags on attachment: 63668 Committed r66205: <http://trac.webkit.org/changeset/66205>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/66205 might have broken Qt Linux Release
Maybe we should reverse this check. Maintaining list of platforms that allow unaligned access might be better than a list of platforms that do not.