Bug 29415 – Byte alignment issue on MIPS

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Tor Arne Vestbø]

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

[Diego Gonzalez]

The OS should be Other in this bug

[Kent Hansen]

This is not a Qt-specific issue.
+1 for adding CPU(MIPS) to the "going 4 bytes at a time is unsafe" condition, though.

[Priit Laes (IRC: plaes)]

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.

[Simon Hausmann]

Removing Qt keyword and Qt from the title, this is not a Qt specific issue.

[Simon Hausmann]

(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 :)

[Chao-ying Fu]

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

[Hajime Morrita]

Look really a trivial and small change.
Anyone who has MIPS experience is looks good candidate for just giving r+.

[Oliver Hunt]

Comment on attachment 63668 [details]
Access one byte at a time for MIPS

r=me

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/66205 might have broken Qt Linux Release

[Darin Adler]

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.