265174 - gcc warnings when building the JS engine (x86_64)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Johnny Stenback (:jst)]

When building the JS engine on AMD x86_64 running 64-bit linux I get a bunch of
compiler warnings. They look harmless to me, but they're annoying. Mostly from
code that casts pointers to 32-bit values (intentionally), we'll need an
intermediate cast to jsword (or unsigned long) to eliminate those warnings.
Patch coming up.

Comment 1

[Johnny Stenback (:jst)]

Attachment: Fix warnings.

Add more casts, fix a few warnings about uninitialized variables (could be
bogus), and some other random warning fixes.

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 162650 [details] [diff] [review]
Fix warnings.

> #ifdef HAVE_VA_LIST_AS_ARRAY
>-#define JS_ADDRESSOF_VA_LIST(ap) (ap)
>+#define JS_ADDRESSOF_VA_LIST(ap) ((void *)ap)

This is kinda scary -- is it right?  Why isn't (ap) good enough?

>     if (attrs & JSPROP_INDEX) {
>-        id = INT_TO_JSID((jsint)name);
>+        id = INT_TO_JSID((jsint)(jsword)name);

Ok, so jsword is signed, and that doesn't matter here, but it's consistent with
the outer (jsint) cast.

>-#define HASH_OBJECT(o)  ((JSHashNumber)(o) >> JSVAL_TAGBITS)
>+#define HASH_OBJECT(o)  ((JSHashNumber)(jsword)(o) >> JSVAL_TAGBITS)

But JSHashNumber is uint32, so maybe use jsuword here?	It's not going to
affect the computation, but it may help readers see more consistency.

Maybe at this point we need a JS_PTR_TO_INT32(p) macro ;-).  Actually, not
kidding -- and we would want a JS_PTR_TO_UINT32(p) macro too, I bet.

> #define ALE_ATOM(ale)   ((JSAtom *) (ale)->entry.key)
>-#define ALE_INDEX(ale)  ((jsatomid) (ale)->entry.value)
>+#define ALE_INDEX(ale)  ((jsatomid) (jsword)(ale)->entry.value)

Ditto uint32/jsuword comment, or use the macro.

> JS_STATIC_DLL_CALLBACK(JSDHashNumber)
> resolving_HashKey(JSDHashTable *table, const void *ptr)
> {
>     const JSResolvingKey *key = (const JSResolvingKey *)ptr;
> 
>-    return ((JSDHashNumber)key->obj >> JSVAL_TAGBITS) ^ key->id;
>+    return ((JSDHashNumber)(jsword)key->obj >> JSVAL_TAGBITS) ^ key->id;
> }

jsword note.

> JS_PUBLIC_API(JSDHashNumber)
> JS_DHashVoidPtrKeyStub(JSDHashTable *table, const void *key)
> {
>-    return (JSDHashNumber)key >> 2;
>+    return (JSDHashNumber)(unsigned long)key >> 2;

Why (unsigned long) here, vs. jsword noted above?  jsuword^H^H^H^H^H^H^H^Hmacro
time.

Don't fix those uninitialized variable warnings, ok?  Don't want js_Interpret
perf to degrade pointlessly, even by a cycle; ditto jsdtoa.c.

/be

Comment 3

[Johnny Stenback (:jst)]

Attachment: Macros, more macros.

This I believe fixes all of those issues, including adding those macros
(following how NS_PTR_TO_INT() et al are written).

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 168378 [details] [diff] [review]
Macros, more macros.

>-#define HASH_OBJECT(o)  ((JSHashNumber)(o) >> JSVAL_TAGBITS)
>+#define HASH_OBJECT(o)  ((JSHashNumber)(JS_PTR_TO_UINT32(o) >> JSVAL_TAGBITS))
> #define HASH_INT(i)     ((JSHashNumber)(i))
> #define HASH_DOUBLE(dp) ((JSHashNumber)(JSDOUBLE_HI32(*dp) ^ JSDOUBLE_LO32(*dp)))

0123456789012345678901234567890123456789012345678901234567890123456789012345678
9

WHOOP! WHOOP! 80th column violation warning! ;-)

Not sure these are worth wrapping.  Leave it to me at some later date, I guess.
 Or lose the (JSHashNumber) cast for those expressions that are uint32 already
(second and fourth, the long ones).

> #define ALE_ATOM(ale)   ((JSAtom *) (ale)->entry.key)
>-#define ALE_INDEX(ale)  ((jsatomid) (ale)->entry.value)
>+#define ALE_INDEX(ale)  ((jsatomid) JS_PTR_TO_UINT32((ale)->entry.value))

Cool, PTR_TO_UINT32.

> #define ALE_JSOP(ale)   ((JSOp) (ale)->entry.value)
> #define ALE_VALUE(ale)  ((jsval) (ale)->entry.value)
> #define ALE_NEXT(ale)   ((JSAtomListElement *) (ale)->entry.next)
> 
> #define ALE_SET_ATOM(ale,atom)  ((ale)->entry.key = (const void *)(atom))
>-#define ALE_SET_INDEX(ale,index)((ale)->entry.value = (void *)(index))
>+#define ALE_SET_INDEX(ale,index)((ale)->entry.value = (void *)(jsuword)(index))
> #define ALE_SET_JSOP(ale,op)    ((ale)->entry.value = (void *)(jsuword)(op))

Hmm, why no UINT32_TO_PTR here for index at least, if not for index and op?

>-#define IS_SHARP(he)    ((jsatomid)(he)->value & SHARP_BIT)
>-#define MAKE_SHARP(he)  ((he)->value = (void*)((jsatomid)(he)->value|SHARP_BIT))
>-#define IS_BUSY(he)     ((jsatomid)(he)->value & BUSY_BIT)
>-#define MAKE_BUSY(he)   ((he)->value = (void*)((jsatomid)(he)->value|BUSY_BIT))
>-#define CLEAR_BUSY(he)  ((he)->value = (void*)((jsatomid)(he)->value&~BUSY_BIT))
>+#define IS_SHARP(he)    (JS_PTR_TO_UINT32((he)->value) & SHARP_BIT)
>+#define MAKE_SHARP(he)  ((he)->value = JS_UINT32_TO_PTR(JS_PTR_TO_UINT32((he)->value)|SHARP_BIT))
>+#define IS_BUSY(he)     (JS_PTR_TO_UINT32((he)->value) & BUSY_BIT)
>+#define MAKE_BUSY(he)   ((he)->value = JS_UINT32_TO_PTR(JS_PTR_TO_UINT32((he)->value)|BUSY_BIT))
>+#define CLEAR_BUSY(he)  ((he)->value = JS_UINT32_TO_PTR(JS_PTR_TO_UINT32((he)->value)&~BUSY_BIT))

This stuff was crammed to just fit at 80 columns, and it may not be any better
wrapped.  So I'd say unless you have a pretty way to wrap it, what the patch
does is ok by me.

Thanks for doing this, r+sr=me with any fiddling per above nits that seems good
to you.

/be

Comment 5

[Johnny Stenback (:jst)]

Fix landed. I didn't end up re-wrapping anything, but I did eliminate some
JSHashNumber casts where they weren't needed to get things to fit in 80 columns.
The rest I couldn't easily wrap in any really neat way...