Bug 11431

Summary: ARM platform has some byte alignment issues
Product: WebKit Reporter: David Carson <dacarson>
Component: PlatformAssignee: David Carson <dacarson>
Status: RESOLVED FIXED    
Severity: Major CC: zalan
Priority: P2 Keywords: PlatformOnly
Version: 420+   
Hardware: Other   
OS: Other   
Attachments:
Description Flags
Patch for ARM architecture
none
Patch for ARM architecture
none
Corrected patch
darin: review-
Apply comments darin: review+

Description David Carson 2006-10-27 06:19:16 PDT
Build WebKit for ARM exposes some byte alignment issues with AtomicString and NaN in Javascript.
Comment 1 David Carson 2006-10-27 06:42:06 PDT
Created attachment 11239 [details]
Patch for ARM architecture

Because of word boundary issues, AtomicString's equal function and the definition of NaN need to be modified.
Comment 2 Sam Weinig 2006-10-27 08:09:37 PDT
A formating issues I see:

No need for braces around single liners and we generally put spaces around infix operators, 

+        if (memcmp(str->characters(), buf.s, strLength*2) != 0) {
+            return false;
+        }

should be 

+        if (memcmp(str->characters(), buf.s, strLength * 2) != 0)
+            return false;
Comment 3 David Carson 2006-10-27 08:15:21 PDT
Created attachment 11243 [details]
Patch for ARM architecture

Fixed formatting issue raised by Sam
Comment 4 David Carson 2006-10-30 07:29:19 PST
Comments from Nokia via email
______________________________________________ 
From:   Zhang Yongjun
Sent:   Monday, October 30, 2006 9:49 AM 
To:     Bujtas Zalan
Subject:        RE: Could you take a look at it?

I think it is fine.  We used similiar byte arrays for Arm platform in 2.8 Javascript code. In 3.0+, we changed to Symbian Math functions.


_____________________________________________ 
From:   Bujtas Zalan 
Sent:   Friday, October 27, 2006 4:27 PM 
To:     Zhang Yongjun
Subject:        Could you take a look at it?

http://bugs.webkit.org/show_bug.cgi?id=11431

And see if it makes sense? 
Thanks, 
Zalan.
Comment 5 Maciej Stachowiak 2006-10-31 03:53:23 PST
Comment on attachment 11243 [details]
Patch for ARM architecture

Looks ok to me. However, is memcmp really the fastest way to do the comparison on ARM? Hand-coding a loop comparing a UChar at a time might still be faster. Furthermore, the uint32_t optimization could still be useful if ARM at least guarantees 16-bit  alignment (past that it's probably not worth it to code all the edge cases).

Finally, it seems like them issue here isn't CPU architecture so much as aligment guarantees of the malloc implementation.

Ayway, those are my comments, I'm not an ARM expert. Patch seems ok as-is.
Comment 6 Maciej Stachowiak 2006-10-31 04:15:07 PST
Comment on attachment 11243 [details]
Patch for ARM architecture

r=me
Comment 7 David Carson 2006-10-31 10:13:07 PST
(In reply to comment #5)
> (From update of attachment 11243 [details] [edit])
> Looks ok to me. However, is memcmp really the fastest way to do the comparison
> on ARM? Hand-coding a loop comparing a UChar at a time might still be faster.
Excellent point! Will fix the patch

> Furthermore, the uint32_t optimization could still be useful if ARM at least
> guarantees 16-bit  alignment (past that it's probably not worth it to code all
> the edge cases).

The bug title was a bit misleading. The actual problem is that the GCC optimization level is set above 2, and thus strict aliasing is applied.

> 
> Finally, it seems like them issue here isn't CPU architecture so much as
> aligment guarantees of the malloc implementation.

The NaN issue is an alignment issue, but the memcmp is actually strict aliasing issue.

> Ayway, those are my comments, I'm not an ARM expert. Patch seems ok as-is.
> 

Comment 8 Darin Adler 2006-10-31 10:23:52 PST
Comment on attachment 11243 [details]
Patch for ARM architecture

Since Dave says he will fix the patch, clearing the review flag until the new one shows up.
Comment 9 David Carson 2006-10-31 11:27:03 PST
Created attachment 11307 [details]
Corrected patch

Updated the comments to clearly define what the change is about. Also, changed the memcmp to a hand coded loop as mjs suggested.
Comment 10 Maciej Stachowiak 2006-11-01 14:54:50 PST
I don't think WebCore is safe to build with strict-aliasing (JavaScriptCore is, but we haven't done the right cleanup in WebCore). We'd like to use strict-aliasing, but for now I think it would be best to compile with -fno-strict-aliasing. Incidentally if this is not an alignment issue, the right way to fix it would be to use a union instead of a reinterpret_cast to do the type punning. gcc allows this as a loophole to strict aliasing.
Comment 11 Darin Adler 2006-12-11 13:31:20 PST
Comment on attachment 11307 [details]
Corrected patch

+        // -fstrict-aliasing is default in GCC at optimization levels at or above O2

That's not true in the Macintosh version of gcc, so I think this comment is a little too specific and wrong in some cases hence confusing.
Comment 12 Darin Adler 2006-12-11 13:35:52 PST
Comment on attachment 11307 [details]
Corrected patch

Given Maciej's comment, this is review- for the WebCore. The right fix for WebCore is to compile with -fno-strict-aliasing.

+const union {

I think this should be "static const".

Does this create a framework init function on Mac OS X? Have you tried this on OS X? I suspect this may create a framework init function, and if it does, we'll need a different solution for OS X, because we can't have any initializers that involve running code.

So review- on both halves. If we verify that the JavaScriptCore half works on OS X without creating a framework initialization function, then that half is good to go.
Comment 13 David Carson 2006-12-21 09:51:10 PST
Created attachment 11951 [details]
Apply comments

Removed WebCore changes, and are now building with -fno-strict-aliasing
Added static infront of const union.
Tested Mac build, ran run-webkit-tests and noticed no failures in JS, apart from Date
I am not sure how to tell if it is not creating a framework initialization function, though the code change only applies to non-DARWIN builds.
Comment 14 Darin Adler 2006-12-22 17:51:30 PST
Comment on attachment 11951 [details]
Apply comments

I'm quite certain that this does create initialization routines on most platforms. But since it doesn't on OS X, I think it's OK to land it for now.

It would be better to solve this problem without an initialization routine in the future.
Comment 15 Maciej Stachowiak 2006-12-29 00:38:19 PST
I'm not sure why this would create an init routine - are unions not allowed to be in the static data section for some reason, unlike structs, arrays or other POD types?
Comment 16 David Kilzer (:ddkilzer) 2007-01-07 21:24:41 PST
Committed revision 18656.