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

RESOLVED FIXED in mozilla7

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

unspecified
mozilla7
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-tracemonkey])

Attachments

(1 attachment, 2 obsolete attachments)

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) };
    ^~~~~
Created attachment 538137 [details] [diff] [review]
Patch (v1)
Assignee: general → ehsan
Status: NEW → ASSIGNED
Attachment #538137 - Flags: review?(jwalden+bmo)
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?
(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 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.
Created attachment 540146 [details] [diff] [review]
Patch (v2)

Yeah, that would work.
Attachment #538137 - Attachment is obsolete: true
Attachment #538137 - Flags: review?(jwalden+bmo)
Attachment #540146 - Flags: review?(jwalden+bmo)
Created attachment 540155 [details] [diff] [review]
Patch (v3)
Attachment #540146 - Attachment is obsolete: true
Attachment #540146 - Flags: review?(jwalden+bmo)
Attachment #540155 - Flags: review?(jwalden+bmo)
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.
Attachment #540155 - Flags: review?(jwalden+bmo) → review+
http://hg.mozilla.org/tracemonkey/rev/284ebc48b2cb
Whiteboard: [fixed-in-tracemonkey]
Target Milestone: --- → mozilla7
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/284ebc48b2cb
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.