The default bug view has changed. See this FAQ.

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)

(Assignee)

Description

6 years ago
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) };
    ^~~~~
(Assignee)

Comment 1

6 years ago
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?
(Assignee)

Comment 3

6 years ago
(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.
(Assignee)

Comment 5

6 years ago
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)
(Assignee)

Comment 6

6 years ago
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+
(Assignee)

Comment 8

6 years ago
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.