Last Comment Bug 662961 - Silence the clang warnings issued because of alignment requirements increase when compiling jsstr.cpp
: Silence the clang warnings issued because of alignment requirements increase ...
Status: RESOLVED FIXED
[fixed-in-tracemonkey]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla7
Assigned To: :Ehsan Akhgari
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-08 16:11 PDT by :Ehsan Akhgari
Modified: 2011-06-20 17:06 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (3.01 KB, patch)
2011-06-08 16:13 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Patch (v2) (3.01 KB, patch)
2011-06-17 15:18 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Patch (v3) (3.01 KB, patch)
2011-06-17 15:37 PDT, :Ehsan Akhgari
jwalden+bmo: review+
Details | Diff | Splinter Review

Description :Ehsan Akhgari 2011-06-08 16:11:52 PDT
These warnings drive me nuts.  We get thousands of them when compiling jsstr.cpp.

/Users/ehsanakhgari/moz/mozilla-central/js/src/jsstr.cpp:3179:16: note: instantiated from:
#define R8(n)  R6(n),  R6((n) + (1 << 6)),   R6((n) + (2 << 6)),   R6((n) + (3 << 6))
               ^
/Users/ehsanakhgari/moz/mozilla-central/js/src/jsstr.cpp:3178:16: note: instantiated from:
#define R6(n)  R4(n),  R4((n) + (1 << 4)),   R4((n) + (2 << 4)),   R4((n) + (3 << 4))
               ^
/Users/ehsanakhgari/moz/mozilla-central/js/src/jsstr.cpp:3177:16: note: instantiated from:
#define R4(n)  R2(n),  R2((n) + (1 << 2)),   R2((n) + (2 << 2)),   R2((n) + (3 << 2))
               ^
/Users/ehsanakhgari/moz/mozilla-central/js/src/jsstr.cpp:3176:16: note: instantiated from:
#define R2(n)  R(n),   R((n) + (1 << 0)),    R((n) + (2 << 0)),    R((n) + (3 << 0))
               ^
/Users/ehsanakhgari/moz/mozilla-central/js/src/jsstr.cpp:3195:7: note: instantiated from:
    { (jschar *)(((char *)(unitStaticTable + (c))) +                          \
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/ehsanakhgari/moz/mozilla-central/js/src/jsstr.cpp:3223:5: warning: cast from 'char *' to 'jschar *' (aka 'unsigned short *') increases required alignment from 1 to
      2 [-Wcast-align]
= { R8(0) };
    ^~~~~
Comment 1 :Ehsan Akhgari 2011-06-08 16:13:00 PDT
Created attachment 538137 [details] [diff] [review]
Patch (v1)
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2011-06-17 11:59:25 PDT
Comment on attachment 538137 [details] [diff] [review]
Patch (v1)

>diff --git a/js/src/jsstr.cpp b/js/src/jsstr.cpp
>-    { (jschar *)(((char *)(unitStaticTable + (c))) +                          \
>+    { (jschar *)(void *)(((char *)(unitStaticTable + (c))) +                  \

>-    { (jschar *)(((char *)(length2StaticTable + (c))) +                       \
>+    { (jschar *)(void *)(((char *)(length2StaticTable + (c))) +               \

>-    { (jschar *)(((char *)(hundredStaticTable + ((c) - 100))) +               \
>+    { (jschar *)(void *)(((char *)(hundredStaticTable + ((c) - 100))) +       \

Wouldn't substituting (void*) for (char*) fix this just as well, and even more concisely?
Comment 3 :Ehsan Akhgari 2011-06-17 13:14:36 PDT
(In reply to comment #2)
> Comment on attachment 538137 [details] [diff] [review] [review]
> Patch (v1)
> 
> >diff --git a/js/src/jsstr.cpp b/js/src/jsstr.cpp
> >-    { (jschar *)(((char *)(unitStaticTable + (c))) +                          \
> >+    { (jschar *)(void *)(((char *)(unitStaticTable + (c))) +                  \
> 
> >-    { (jschar *)(((char *)(length2StaticTable + (c))) +                       \
> >+    { (jschar *)(void *)(((char *)(length2StaticTable + (c))) +               \
> 
> >-    { (jschar *)(((char *)(hundredStaticTable + ((c) - 100))) +               \
> >+    { (jschar *)(void *)(((char *)(hundredStaticTable + ((c) - 100))) +       \
> 
> Wouldn't substituting (void*) for (char*) fix this just as well, and even
> more concisely?

No, because arithmetic on void* pointers is impossible.
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2011-06-17 14:00:28 PDT
Comment on attachment 538137 [details] [diff] [review]
Patch (v1)

Er, right.  What about going through uintptr_t instead of char*?  This macro is messy enough already (and not helped by the non-alignment of its nested components, but I digress).  I'd really rather not add more gunk if I can help it.
Comment 5 :Ehsan Akhgari 2011-06-17 15:18:50 PDT
Created attachment 540146 [details] [diff] [review]
Patch (v2)

Yeah, that would work.
Comment 6 :Ehsan Akhgari 2011-06-17 15:37:16 PDT
Created attachment 540155 [details] [diff] [review]
Patch (v3)
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2011-06-17 15:53:35 PDT
Comment on attachment 540155 [details] [diff] [review]
Patch (v3)

># HG changeset patch
># Parent 3a509617644e2b4530e585a7fa920a2f7a84bb31
># User Ehsan Akhgari <ehsan@mozilla.com>
>Bug 662961 - Silence the clang warnings issued because of alignment requirements increase when compiling jsstr.cpp
>
>
>diff --git a/js/src/jsstr.cpp b/js/src/jsstr.cpp
>--- a/js/src/jsstr.cpp
>+++ b/js/src/jsstr.cpp
>@@ -3187,17 +3187,17 @@ static JSFunctionSpec string_methods[] =
>     (((length) << JSString::LENGTH_SHIFT) | (flags))
> 
> /*
>  * Declare unit strings. Pack the string data itself into the mInlineChars
>  * place in the header.
>  */
> #define R(c) {                                                                \
>     BUILD_LENGTH_AND_FLAGS(1, JSString::STATIC_ATOM_FLAGS),                   \
>-    { (jschar *)(((char *)(unitStaticTable + (c))) +                          \
>+    { (jschar *)(((uintptr_t)(unitStaticTable + (c))) +                       \

There's a minor excess of parentheses in this, some avoidable with a C++-style cast, some by just removing them.  Try this instead:

>-    { (jschar *)(((char *)(unitStaticTable + (c))) +                          \
>+    { (jschar *)(uintptr_t(unitStaticTable + (c)) +                           \


And here, 

> #define R(c) {                                                                \
>     BUILD_LENGTH_AND_FLAGS(2, JSString::STATIC_ATOM_FLAGS),                   \
>-    { (jschar *)(((char *)(length2StaticTable + (c))) +                       \
>+    { (jschar *)(((uintptr_t)(length2StaticTable + (c))) +                    \

Use this:

>-    { (jschar *)(((char *)(length2StaticTable + (c))) +                       \
>+    { (jschar *)(uintptr_t(length2StaticTable + (c)) +                        \

And here,

> #define R(c) {                                                                \
>     BUILD_LENGTH_AND_FLAGS(3, JSString::STATIC_ATOM_FLAGS),                   \
>-    { (jschar *)(((char *)(hundredStaticTable + ((c) - 100))) +               \
>+    { (jschar *)(((uintptr_t)(hundredStaticTable + ((c) - 100))) +            \

Use this:

>-    { (jschar *)(((char *)(hundredStaticTable + ((c) - 100))) +               \
>+    { (jschar *)(uintptr_t(hundredStaticTable + ((c) - 100)) +                \

This is ugly enough to read that you should do a quick build and test for working-ness.  Try server's not necessary if you're pushing to TM and if you do a little smoke-testing yourself first.  (These changes should need very little -- if they're wrong I expect the browser, and JS shell, would fail to run.)  If you have a browser build, build at least far enough to get past the end of the JS engine compilation, then switch to js/src, then run:

python tests/jstests.py $objdir/dist/bin/js --args='-j -m -p'

Five or so minutes later, if that passes, you're good to land in TM.
Comment 8 :Ehsan Akhgari 2011-06-20 06:56:41 PDT
http://hg.mozilla.org/tracemonkey/rev/284ebc48b2cb
Comment 9 Chris Leary [:cdleary] (not checking bugmail) 2011-06-20 17:06:16 PDT
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/284ebc48b2cb

Note You need to log in before you can comment on or make changes to this bug.