478543 - Fixing warnings about casts between data and function pointers

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

Attachment: v1

With the default options when I compile SpiderMonkey on Linux with GCC 4.2 the compiler generates a few warnings like:

ISO C++ forbids casting between pointer-to-function and pointer-to-object

With few exception in jstracer.cpp these warnings come from the casts between JSPropertyOP and JSObject* when code works with getter or setter operations. The warnings are not mere nuisance that can be ignored as they show the real problem with code that assumes that sizeof(void*) == sizeof(void (*)()). This will not be true on 64-bit Linux with so-called medium programming model with 64-bit data pointer an 32-bit code.

So it would be nice to silence the warnings while explicitly enforcing the restriction that SM assumes sizeof(JSObject*) == sizeof(JSPropertyOp).

The attached patch does just that with a helper union that is used to store getter and setter in JSScipeProperty.

Comment 1

[Brendan Eich [:brendan]]

Instead why not move JS_EXTENSION (and JS_EXTENSION_) from jsinterp.cpp to some place like jstypes.h at the bottom, and use them in jstracer.cpp as we do in the interpreter?

/be

Comment 2

[Igor Bukanov]

Attachment: v2

The new patch uses __extension trick from jsinterp.cpp to silence the warnings. How could I miss it?

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 362897 [details] [diff] [review]
v2

>+                                        OBJECT_TO_JSVAL(
>+                                            js_CastAsObject(wp->setter)),

This recurs, how about a js_CastAsObjectJSVal inline helper to shorten all the nested calls?

>+/*
>+ * Helper macros to convert between function and data pointers assuming that
>+ * they have the same size.
>+ */
>+JS_STATIC_ASSERT(sizeof(void *) == sizeof(void (*)()));
>+
>+#ifdef __GNUC__
>+# define JS_FUNCTION_TO_POINTER(type, fun) (__extension__ (type) (fun))
>+# define JS_POINTER_TO_FUNCTION(type, ptr) (__extension__ (type) (ptr))
>+#else
>+/* Use an extra (void *) cast for MSVC */
>+# define JS_FUNCTION_TO_POINTER(type, fun) ((type) (void *) (fun))
>+# define JS_POINTER_TO_FUNCTION(type, ptr) ((type) (void *) (ptr))
>+#endif
>+
> #endif /* jsprvtd_h___ */

This seems better in jstypes.h, as it is not a set of private typedefs, rather generally useful type-related macros.

> #define INS_CONST(c)    addName(lir->insImm(c), #c)
> #define INS_CONSTPTR(p) addName(lir->insImmPtr((void*) (p)), #p)
>+#define INS_CONSTFUNPTR(p) addName(lir->insImmPtr(JS_FUNCTION_TO_POINTER(void*, p)), #p)

Feel free to align the prior two line's macro bodies to start in the same column as the third macro's body.

>         JSFunction *fun = js_NewFunction(cx,
>                                          NULL,
>-                                         (JSNative) builtinFunctionInfo[index].tn,
>+                                         JS_POINTER_TO_FUNCTION(JSNative, builtinFunctionInfo[index].tn),
1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890

This line goes beyond the new textwidth world order. how about

>+                                         JS_POINTER_TO_FUNCTION(JSNative,
>+                                                                builtinFunctionInfo[index].tn),

although now I wonder if, given JS_INT32_TO_PTR, JS_PTR_TO_INT32, and the UINT32 variants, we should not avoid overlong names and use JS_PTR_TO_FUNCTION or even JS_PTR_TO_FUNC. A compromise that may be too pedantic: JS_DATA_TO_FUNC_PTR and JS_FUNC_TO_DATA_PTR. Thoughts?

/be

Comment 4

[Brendan Eich [:brendan]]

I'm in favor of JS_DATA_TO_FUNC_PTR and JS_FUNC_TO_DATA_PTR, since C and C++ make a distinction between function type and function-pointer type.

/be

Comment 5

[Igor Bukanov]

Attachment: v3

The new version uses JS_DATA_TO_FUNC_PTR and JS_FUNC_TO_DATA_PTR as the macros names and adds js_CastAsObjectJSVal.

I haven't moved the macros from jsprvtd.h as I prefer to keep JS_STATIC_ASSERT. Currently the assertion is defined in jsutil.h which depends on types from jstypes.h. So to solve this what about moving JS_STATIC_ASSERT into jstypes.h together with the new macros? At the end, the types are also a form of static assertions.

In principle the static asserts can be moved from jsutil.h into jstypes.h as well as the types is a form of static asserts

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 362948 [details] [diff] [review]
v3

(In reply to comment #5)
> Created an attachment (id=362948) [details] 
> I haven't moved the macros from jsprvtd.h as I prefer to keep JS_STATIC_ASSERT.
> Currently the assertion is defined in jsutil.h which depends on types from
> jstypes.h. So to solve this what about moving JS_STATIC_ASSERT into jstypes.h
> together with the new macros? At the end, the types are also a form of static
> assertions.

To me this suggests a problem with static asserts in .h files. We've had similar problems previously. In theory they are ok and better situated near related definitions, but in practice they drag in too much. Putting "too much" in jstypes.h is not the answer and could mess up other things, so let's leave this for a separate bug (file at your discretion). Thanks,

/be

Comment 7

[Igor Bukanov]

landed in TM - http://hg.mozilla.org/tracemonkey/rev/4cf75fc4d196

Comment 8

[Igor Bukanov]

Nominating for 1.9.1 to significantly reduce the warning noise on the branch with GCC.

Comment 9

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f97755b4ba32

Comment 10

[Igor Bukanov]

The bug was fixed in mozilla-central on 2009-02-19 - http://hg.mozilla.org/mozilla-central/rev/4cf75fc4d196