Bug 29415 - Byte alignment issue on MIPS
Summary: Byte alignment issue on MIPS
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-09-18 07:36 PDT by Tor Arne Vestbø
Modified: 2010-08-27 10:02 PDT (History)
11 users (show)

See Also:


Attachments
Aligment patch from Gentoo for ARM, SPARC and SH4 platforms (4.30 KB, patch)
2010-03-25 00:34 PDT, Priit Laes (IRC: plaes)
no flags Details | Formatted Diff | Diff
Access one byte at a time for MIPS (1.97 KB, patch)
2010-08-05 17:34 PDT, Chao-ying Fu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tor Arne Vestbø 2009-09-18 07:36:34 PDT
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
Comment 1 Diego Gonzalez 2010-03-16 12:35:32 PDT
The OS should be Other in this bug
Comment 2 Kent Hansen 2010-03-17 04:49:36 PDT
This is not a Qt-specific issue.
+1 for adding CPU(MIPS) to the "going 4 bytes at a time is unsafe" condition, though.
Comment 3 Priit Laes (IRC: plaes) 2010-03-25 00:34:49 PDT
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.
Comment 4 Simon Hausmann 2010-05-06 01:38:36 PDT
Removing Qt keyword and Qt from the title, this is not a Qt specific issue.
Comment 5 Simon Hausmann 2010-05-06 01:39:46 PDT
(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 :)
Comment 6 Chao-ying Fu 2010-08-05 17:34:17 PDT
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
Comment 7 Hajime Morrita 2010-08-11 18:37:43 PDT
Look really a trivial and small change.
Anyone who has MIPS experience is looks good candidate for just giving r+.
Comment 8 Oliver Hunt 2010-08-26 16:51:46 PDT
Comment on attachment 63668 [details]
Access one byte at a time for MIPS

r=me
Comment 9 WebKit Commit Bot 2010-08-27 05:00:52 PDT
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>
Comment 10 WebKit Commit Bot 2010-08-27 05:00:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 WebKit Review Bot 2010-08-27 05:59:47 PDT
http://trac.webkit.org/changeset/66205 might have broken Qt Linux Release
Comment 12 Darin Adler 2010-08-27 10:02:50 PDT
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.