Bug 11431 – ARM platform has some byte alignment issues

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[David Carson]

Build WebKit for ARM exposes some byte alignment issues with AtomicString and NaN in Javascript.

[David Carson]

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.

[Sam Weinig]

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;

[David Carson]

Created attachment 11243 [details]
Patch for ARM architecture

Fixed formatting issue raised by Sam

[David Carson]

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.

[Maciej Stachowiak]

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.

[Maciej Stachowiak]

Comment on attachment 11243 [details]
Patch for ARM architecture

r=me

[David Carson]

(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.
> 

[Darin Adler]

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.

[David Carson]

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.

[Maciej Stachowiak]

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.

[Darin Adler]

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.

[Darin Adler]

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.

[David Carson]

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.

[Darin Adler]

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.

[Maciej Stachowiak]

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?

[David Kilzer (:ddkilzer)]

Committed revision 18656.