Closed Bug 410941 Opened 12 years ago Closed 12 years ago

Type size related compiler warnings on 64-bit Linux machine (size_t, jsuint)

Categories

(Core :: JavaScript Engine, defect, P1)

x86
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta3

People

(Reporter: edwin, Assigned: brendan)

References

Details

(Keywords: 64bit)

Attachments

(1 file, 3 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.11) Gecko/20071127 Firefox/2.0.0.11
Build Identifier: Debian Etch (4.0r0) AMD64

GCC complains about a few comparisons that always are false (or true) due to differences in the ranges of data types.
e.g. sizeof(size_t) is 8; sizeof(jsuint) is 4.

I could file the warnings as seperate bugs, but I think some of them are related.

Ignore the JSArgumentFormatter warnings, those are something completely different, va_list/va_copy related.

Reproducible: Always

Steps to Reproduce:
1. Use a 64-bit Linux machine
2. cd mozilla/js/src
3. make -f Makefile.ref 2>err.log

Actual Results:  
The following is written to stderr when compiling with make -f Makefile.ref

cat: ../../dist/Linux_All_DBG.OBJ/nspr/Version: No such file or directory
cat: ../../dist/Linux_All_DBG.OBJ/nspr/Version: No such file or directory
make[1]: Circular jscpucfg.h <- Linux_All_DBG.OBJ/jsautocfg.h dependency dropped.
make[1]: Circular Linux_All_DBG.OBJ/jsautocfg.h <- Linux_All_DBG.OBJ/jsautocfg.h dependency dropped.
jsapi.c: In function JS_ConvertArgumentsVA:
jsapi.c:272: warning: passing argument 5 of TryArgumentFormatter from incompatible pointer type
jsapi.c: In function JS_PushArgumentsVA:
jsapi.c:375: warning: passing argument 5 of TryArgumentFormatter from incompatible pointer type
jsarray.c: In function array_sort:
jsarray.c:1104: warning: comparison is always false due to limited range of data type
jsarray.c:1206: warning: comparison is always false due to limited range of data type
jsgc.c: In function TraceDelayedChildren:
jsgc.c:1875: warning: right shift count >= width of type
jsgc.c:1875: warning: cast to pointer from integer of different size
jsinterp.c: In function js_Invoke:
jsinterp.c:892: warning: comparison is always true due to limited range of data type
jsobj.c: In function js_ConstructObject:
jsobj.c:2784: warning: comparison is always true due to limited range of data type
jsxml.c: In function XMLArraySetCapacity:
jsxml.c:1072: warning: comparison is always false due to limited range of data type
jsxml.c: In function XMLArrayAddMember:
jsxml.c:1167: warning: comparison is always false due to limited range of data type


Expected Results:  
Compile without warnings
Sizes according to sizeof(type) on 64-bit Linux:

size_t: 8
void *: 8
jsuint: 4
jsval:  8
uint32: 4
uintN:  4
(In reply to comment #0)
> Ignore the JSArgumentFormatter warnings, those are something completely
> different, va_list/va_copy related.

Lack of HAVE_VA_LIST_AS_ARRAY jscpucfg.h macro definition? Oops, I don't see how this can be auto-generated into jsautocfg.h for Unixy platforms. Good catch! Could you please file a separate bug?

> jsinterp.c: In function js_Invoke:
> jsinterp.c:892: warning: comparison is always true due to limited range of data
> type
> jsobj.c: In function js_ConstructObject:
> jsobj.c:2784: warning: comparison is always true due to limited range of data
> type
> jsxml.c: In function XMLArraySetCapacity:
> jsxml.c:1072: warning: comparison is always false due to limited range of data
> type
> jsxml.c: In function XMLArrayAddMember:
> jsxml.c:1167: warning: comparison is always false due to limited range of data
> type

These seem to suggest that ptrdiff_t is an unsigned type (see JS_PUSH_TEMP_ROOT in jscntxt.h -- true?

/be
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch fixes (obsolete) — Splinter Review
Assignee: general → brendan
Status: NEW → ASSIGNED
Attachment #295537 - Flags: review?(igor)
Edwin, could you apply that patch and test? Thanks a ton for filing this bug.

/be
Blocks: 410929
Isn't the right thing here to simply make len and newlen actually _be_ size_t?
Keywords: 64bit
(In reply to comment #0)
> jsgc.c: In function TraceDelayedChildren:
> jsgc.c:1875: warning: right shift count >= width of type
> jsgc.c:1875: warning: cast to pointer from integer of different size

Here is the corresponding code:

JSGCArenaInfo *a, *aprev;
....
jsgc.c:1875: aprev = ARENA_PAGE_TO_INFO(a->prevUntracedPage);

The macro is defined as:

#define ARENA_PAGE_TO_INFO(arenaPage)                                         \
    (JS_ASSERT(arenaPage != 0),                                               \
     JS_ASSERT(((arenaPage) >> (JS_BITS_PER_WORD - GC_ARENA_SHIFT)) == 0),    \
     ARENA_START_TO_INFO((arenaPage) << GC_ARENA_SHIFT))

#define ARENA_START_TO_INFO(arenaStart)                                       \
    (JS_ASSERT(((arenaStart) & (jsuword) GC_ARENA_MASK) == 0),                \
     (JSGCArenaInfo *) ((arenaStart) + ARENA_INFO_OFFSET))

So the warning about the right shift comes from:

JS_ASSERT((a->prevUntracedPage >> (JS_BITS_PER_WORD - GC_ARENA_SHIFT)) == 0)

and warning about the pointer conversion comes from:

(JSGCArenaInfo *) ((a->prevUntracedPage << GC_ARENA_SHIFT) + ARENA_INFO_OFFSET))

where prevUntracedPage is defined as:

struct JSGCArenaInfo {
...
    jsuword         prevUntracedPage :  JS_BITS_PER_WORD - GC_ARENA_SHIFT;
...
}

In principle the firsts warning is justifiable as given the width of the bitfield GCC may figure out that the condition is true. But the second one has no merit as the type of a->prevUntracedPage << GC_ARENA_SHIFT is jsuword, not the anonymous type of the bitfield. On the other hand since ARENA_INFO_OFFSET is uint32 it would be nice if GCC would warn about mixing of operands of different sizes in (a->prevUntracedPage << GC_ARENA_SHIFT) + ARENA_INFO_OFFSET.
Comment on attachment 295537 [details] [diff] [review]
fixes

> #define ARENA_PAGE_TO_INFO(arenaPage)                                         \
>     (JS_ASSERT(arenaPage != 0),                                               \
>-     JS_ASSERT(((arenaPage) >> (JS_BITS_PER_WORD - GC_ARENA_SHIFT)) == 0),    \
>+     JS_ASSERT(!((jsuword)(arenaPage) >> (JS_BITS_PER_WORD-GC_ARENA_SHIFT))), \
>      ARENA_START_TO_INFO((arenaPage) << GC_ARENA_SHIFT))

This would not fix the second warning. A way to address it would be to change ARENA_START_TO_INFO from

#define ARENA_START_TO_INFO(arenaStart)                                       \
    (JS_ASSERT(((arenaStart) & (jsuword) GC_ARENA_MASK) == 0),                \
     (JSGCArenaInfo *) ((arenaStart) + ARENA_INFO_OFFSET))

to

#define ARENA_START_TO_INFO(arenaStart)                                       \
    (JS_ASSERT(((arenaStart) & (jsuword) GC_ARENA_MASK) == 0),                \
     (JSGCArenaInfo *) ((arenaStart) + (jsuword) ARENA_INFO_OFFSET))

as ARENA_INFO_OFFSET is uint32.
(In reply to comment #2)
> (In reply to comment #0)
> > Ignore the JSArgumentFormatter warnings, those are something completely
> > different, va_list/va_copy related.
> 
> Lack of HAVE_VA_LIST_AS_ARRAY jscpucfg.h macro definition? Oops, I don't see
> how this can be auto-generated into jsautocfg.h for Unixy platforms. Good
> catch! Could you please file a separate bug?

I've got this working in a autoconf test, see https://bugzilla.mozilla.org/show_bug.cgi?id=97954 for a discussion about that. I'll try and see what kind of approach would have the least impact on the current build system.

> These seem to suggest that ptrdiff_t is an unsigned type (see JS_PUSH_TEMP_ROOT
> in jscntxt.h -- true?

No, ptrdiff_t is a __kernel_ptrdiff_t, which in turn is an int on i386, but a long on x86-64 linux machines; sizeof(ptrdiff_t) == 8.

(In reply to comment #4)
> Edwin, could you apply that patch and test? Thanks a ton for filing this bug.

The patch doesn't fix the warnings. I still get exactly the same warnings, except for the right shift warning. That one is gone now. So jsarray.c has the same two warnings, jsgc.c now only has the different size warning.
(In reply to comment #5)
> Isn't the right thing here to simply make len and newlen actually _be_ size_t?

No, because those are used as out parameters for ECMA-standard array-length computing functions, and array length is jsuint (uint32). To avoid gratuitous temporaries, cast on both sides of overflow test operator is worth it.

/be
Attached patch fixes, v2 (obsolete) — Splinter Review
This should do it (note the ~(size_t)0 idiom occurs in similar code elsewhere). Edwin, let me know if something remains. Thanks,

/be
Attachment #295537 - Attachment is obsolete: true
Attachment #295565 - Flags: review?(igor)
Attachment #295537 - Flags: review?(igor)
This fixes the warnings about jsgc.c and jscontext.c, but the jsarray.c warnings still remain.
(In reply to comment #11)
> the jsarray.c warnings still remain.

Oh of course! These tests are completely unnecessary on 64-bit targets, precisely because ECMA-262 limits array length to 2^32 - 1. Patching again...

/be
Attached patch fixes, v3 (obsolete) — Splinter Review
Attachment #295565 - Attachment is obsolete: true
Attachment #295568 - Flags: review?(igor)
Attachment #295565 - Flags: review?(igor)
This fixes the warnings in jsarray.c and jsgc.c.
The following remains:

jsxml.c: In function XMLArraySetCapacity:
jsxml.c:1072: warning: comparison is always false due to limited range of data type
jsxml.c: In function XMLArrayAddMember:
jsxml.c:1167: warning: comparison is always false due to limited range of data type
Comment on attachment 295568 [details] [diff] [review]
fixes, v3

>-    if (len > (size_t) -1 / (2 * sizeof(jsval))) {
>+#if JS_BITS_PER_WORD == 32
>+    if ((size_t)len > ~(size_t)0 / (2 * sizeof(jsval))) {

This asks for SIZE_MAX macro from stdint.h or, if SM can not use stdint.h, then its SM equivalent.
(In reply to comment #15)
> (From update of attachment 295568 [details] [diff] [review])
> >-    if (len > (size_t) -1 / (2 * sizeof(jsval))) {
> >+#if JS_BITS_PER_WORD == 32
> >+    if ((size_t)len > ~(size_t)0 / (2 * sizeof(jsval))) {
> 
> This asks for SIZE_MAX macro from stdint.h or, if SM can not use stdint.h, then
> its SM equivalent.

In fact I do not see that the warning is about (size_t) -1. This is well-defined. It is just that GCC is smart enough to figure out that given the type of len its max is less than the right-hand size.  
> 

(In reply to comment #15)
> (From update of attachment 295568 [details] [diff] [review])
> >-    if (len > (size_t) -1 / (2 * sizeof(jsval))) {
> >+#if JS_BITS_PER_WORD == 32
> >+    if ((size_t)len > ~(size_t)0 / (2 * sizeof(jsval))) {
> 
> This asks for SIZE_MAX macro from stdint.h or, if SM can not use stdint.h, then
> its SM equivalent. 

$ fgrep '~(size_t)0' *.[ch]
jsarray.c:    if ((size_t)len > ~(size_t)0 / (2 * sizeof(jsval))) {
jsarray.c:            if ((size_t)newlen > ~(size_t)0 / (4 * sizeof(jsval))) {
jsfun.c:            args_length >= ~(size_t)0 / sizeof(jschar)) {
jsscan.c:        if ((size_t)length >= ~(size_t)0 / sizeof(jschar)) {
jsscan.c:    if ((size_t)offset < newlength && newlength < ~(size_t)0 / sizeof(jschar))
jsstr.c:    if (newlength >= ~(size_t)0 / sizeof(jschar)) {
jsstr.c:    if (taglen >= ~(size_t)0 / sizeof(jschar)) {
jsxml.c:        if ((size_t)capacity > ~(size_t)0 / sizeof(void *) ||
jsxml.c:            if ((size_t)capacity > ~(size_t)0 / sizeof(void *) ||

I'd rather use ~(size_t)0 for now to match elsewhere, and leave the SIZE_MAX change for a later bug.

Last we checked (a couple of years ago), C99 support was lacking on a few supported platforms, so <stdint.h> may be premature. Not for Mozilla 2, though.

> In fact I do not see that the warning is about (size_t) -1. This is
> well-defined. It is just that GCC is smart enough to figure out that given the
> type of len its max is less than the right-hand size.

But per comment 8, casting len to (size_t) did not help. So something else is going on here for GCC to warn about (size_t)-1 / ... but not ~(size_t)0 / ....

/be
(In reply to comment #17)
> But per comment 8, casting len to (size_t) did not help. So something else is
> going on here for GCC to warn about (size_t)-1 / ... but not ~(size_t)0 / ....

Never mind, I'm forgetting my own comment 12. The jsxml.c tests can be #if'd too.

/be
Attached patch fixes, v4Splinter Review
This should do it -- Edwin, make my day ;-).

/be
Attachment #295568 - Attachment is obsolete: true
Attachment #295587 - Flags: review?(igor)
Attachment #295568 - Flags: review?(igor)
The size related warnings are now all gone, so concider your day "made"...
I ran the testsuite before and after the patch, and exactly 185 tests now fail with or without the patch. So either the warnings were harmless and nothing was even broken, or there were no suitable checks.

FYI, I ran:
cd ../tests ; perl jsDriver.pl -L slow-n.tests -L spidermonkey-n.tests -k -e smopt ; cd ../src
Attachment #295587 - Flags: review?(igor) → review+
Edwin, thanks again.

Bob, do we have 64-bit Linux boxes at which we could target regression tests for this bug? Some of these might be tricky to write.

Fixed on trunk:

js/src/jsarray.c 3.145
js/src/jscntxt.h 3.177
js/src/jsgc.c 3.258
js/src/jsxml.c 3.184

/be
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Priority: -- → P1
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M11
We have no 64-bit linux boxes.
Flags: in-testsuite?
on CentOS5 64bit with gcc 4.2.3 I am still getting

jsapi.c: In function ‘JS_ConvertArgumentsVA’:
jsapi.c:272: warning: passing argument 5 of ‘TryArgumentFormatter’ from incompatible pointer type
jsapi.c: In function ‘JS_PushArgumentsVA’:
jsapi.c:380: warning: passing argument 5 of ‘TryArgumentFormatter’ from incompatible pointer type

and in addition:

/usr/bin/ld: warning: i386 architecture of input file `editline/Linux_All_DBG.OBJ/libedit.a(editline.o)' is incompatible with i386:x86-64 output
/usr/bin/ld: warning: i386 architecture of input file `editline/Linux_All_DBG.OBJ/libedit.a(sysunix.o)' is incompatible with i386:x86-64 output

perhaps this message is bug 424555 ?

(In reply to comment #23)
> on CentOS5 64bit with gcc 4.2.3 I am still getting
> 
> jsapi.c: In function ‘JS_ConvertArgumentsVA’:
> jsapi.c:272: warning: passing argument 5 of ‘TryArgumentFormatter’
> from incompatible pointer type
> jsapi.c: In function ‘JS_PushArgumentsVA’:
> jsapi.c:380: warning: passing argument 5 of ‘TryArgumentFormatter’
> from incompatible pointer type
> 

I still see this CentOS5 x86_64 gcc 4.2.3.  reopening.

> and in addition:
> 
> /usr/bin/ld: warning: i386 architecture of input file
> `editline/Linux_All_DBG.OBJ/libedit.a(editline.o)' is incompatible with
> i386:x86-64 output
> /usr/bin/ld: warning: i386 architecture of input file
> `editline/Linux_All_DBG.OBJ/libedit.a(sysunix.o)' is incompatible with
> i386:x86-64 output

was bogus.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Bob, please file a new bug. Patch landed, fixed some problems, is not being backed out. Thanks,

/be
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
filed Bug 431354 
Flags: in-testsuite?
Flags: in-testsuite-
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.