Closed Bug 478543 Opened 12 years ago Closed 12 years ago

Fixing warnings about casts between data and function pointers

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: igor, Assigned: igor)

References

Details

(Keywords: fixed1.9.1, Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 2 obsolete files)

Attached patch v1 (obsolete) — Splinter Review
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.
Attachment #362388 - Flags: review?(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
Summary: Fixing warnings about casts between JSObject* and JSPropertyOP → Fixing warnings about casts between data and function pointers
Attached patch v2 (obsolete) — Splinter Review
The new patch uses __extension trick from jsinterp.cpp to silence the warnings. How could I miss it?
Attachment #362388 - Attachment is obsolete: true
Attachment #362897 - Flags: review?(brendan)
Attachment #362388 - Flags: review?(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
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
Attached patch v3Splinter Review
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
Attachment #362897 - Attachment is obsolete: true
Attachment #362948 - Flags: review?(brendan)
Attachment #362897 - Flags: review?(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
Attachment #362948 - Flags: review?(brendan) → review+
landed in TM - http://hg.mozilla.org/tracemonkey/rev/4cf75fc4d196
Whiteboard: fixed-in-tracemonkey
Nominating for 1.9.1 to significantly reduce the warning noise on the branch with GCC.
Flags: wanted1.9.1?
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f97755b4ba32
Flags: wanted1.9.1? → wanted1.9.1+
Keywords: fixed1.9.1
Depends on: 480700
The bug was fixed in mozilla-central on 2009-02-19 - http://hg.mozilla.org/mozilla-central/rev/4cf75fc4d196
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.