662961 - Silence the clang warnings issued because of alignment requirements increase when compiling jsstr.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[(no longer active)]

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

[(no longer active)]

Attachment: Patch (v1)

Comment 2

[Jeff Walden [:Waldo]]

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

[(no longer active)]

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

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

[(no longer active)]

Attachment: Patch (v2)

Yeah, that would work.

Comment 6

[(no longer active)]

Attachment: Patch (v3)

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 540155 [details] [diff] [review]
Patch (v3)

># HG changeset patch
># Parent 3a509617644e2b4530e585a7fa920a2f7a84bb31
># User Ehsan Akhgari <[e-mail]>
>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

[(no longer active)]

http://hg.mozilla.org/tracemonkey/rev/284ebc48b2cb

Comment 9

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/284ebc48b2cb