206599 - GCC 3.3 type-punning/strict-aliasing warnings (in <js*.c>)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[tenthumbs]

So gcc has a new class of warnings. At least it tells you about them.

warning: dereferencing type-punned pointer will break strict-aliasing rules
jsdbgapi.c:449
jsdbgapi.c:459
jsdbgapi.c:487
jsemit.c:1483
jsemit.c:2050
jsfun.c:789
jsfun.c:1160
jsfun.c:1176
jsfun.c:1775
jsobj.c:1283
jsobj.c:1303
jsobj.c:1758
jsobj.c:2057
jsobj.c:2476
jsobj.c:2577
jsparse.c:736
jsxdrapi.c:479
jsxdrapi.c:480

Comment 1

[Phil Schwartau]

Reassigning; cc'ing JS experts -

Comment 2

[tenthumbs]

Many of these occur because of this idiom (jsfun.c:789 is typical):

    JSScopeProperty *sprop;
    ...
    if (!js_LookupProperty(..., (JSProperty **)&sprop)) {...}

I think this is a bit too tricky (and gcc agrees). A union would seem 
appropriate here.

Comment 3

[Brendan Eich [:brendan]]

There's nothing tricky going on, and a union is not the solution.  The parameter
is a JSProperty **, so that's what should be passed.  The caller can then
downcast the resulting JSProperty * to JSScopeProperty *:

    JSProperty *prop;
    JSScopeProperty *sprop;

    ok = OBJ_LOOKUP_PROPERTY(cx, obj, id, &pobj, &prop);
    if (ok && OBJ_IS_NATIVE(pobj)) {
        sprop = (JSScopeProperty *) prop;
        do something peculiar to native objects with pobj and sprop;
    }

I'll fix this.

/be

Comment 4

[tenthumbs]

The tricky part I was referring to is treating a address 
of a JSScopeProperty* as the address of a JSProperty *. A union is one 
way to fix that. You found another. Note that this won't fix all the
warnings but some of them seem silly.

Comment 5

[Brendan Eich [:brendan]]

Right, a union of pointer types would do the trick, but an undiscriminated union
not the right thing, because the downcast is safe only if OBJ_IS_NATIVE(pobj) --
i.e., the arms of the union can't be used equivalently in all code after the
result param propagates out of the call site.  A poor man's dynamic cast, in C,
is bound to be ugly.

bbaetz: isn't this a dup of another bug?  I recall asking you why (in C code)
the compiler has to worry about a JSProperty** out parameter not being
equivalent to a JSScopeProperty ** parameter.

/be

Comment 6

[Bradley Baetz (:bbaetz)]

brandan: Yeah, I think this is a dupe. Foo* and Bar* are compatable if one is a
prefix of teh other, but not Foo**/Bar**.

Comment 7

[Brendan Eich [:brendan]]

Maybe we could make this bug depend on the other bug, make the other bug a metabug.

baetz: in C (not C++), how could the double-indirection matter?

/be

Comment 8

[Bradley Baetz (:bbaetz)]

because the standard says so. Or rather, it doesn't say that they are compatable
types, so they're not

Actually, rereading the standard, c99 6.7.5.1/2 says that two pointer types are
compatible if they point to compatible types. This is a recursive definition, so
maybe they are compatible. Did anyone ever ask the GCC people to confirm these
warnings?

For C++, the requirement is that the two objects have the same dynamic type.
Foo* decays to Bar* given inheritance, but not Foo** to Bar**. Theres no
explciit rule for pointers that I can see. Thats the draft though, so it may
have changed

Comment 9

[Brendan Eich [:brendan]]

Ok, so before I spend time hacking a patch, can someone who knows GCC people
ask? Cc'ing dbaron.

/be

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

bbaetz is more likely to know gcc people than I am.

Comment 11

[tenthumbs]

I think the issue here is the storage size of pointers. JS is implicitly
assuming that all pointers have the same size so it treats them as
interchangeable.  I doubt the standards make that assumption so I can
understand why gcc emits a strictness warning.

The "proper" solution probably is to make JSProperty be a structure or union
of all the possible property structures, but that's a lot of work. The quick
fix is to use a union to explicitly tell gcc that it's ok to have two
pointers occupy the same space.

As a test, I applied this patch which explicitly tells gcc what's happening
and the warning is quelled. Something like this is relatively easy to do.


--- jsfun.c     2003-04-23 10:02:30.000000000 -0400
+++ jsfun.c.new 2003-05-29 06:26:45.000000000 -0400
@@ -768,6 +768,7 @@
     uintN attrs, slot, nslots, spflags;
     jsval *vp, value;
     intN shortid;
+    union { JSScopeProperty *sprop; JSProperty *prop;} uprop;
 
     fp = (JSStackFrame *) JS_GetPrivate(cx, obj);
     if (!fp)
@@ -786,16 +787,16 @@
     if (!atom)
         return JS_FALSE;
     if (!js_LookupProperty(cx, funobj, (jsid)atom, &obj2,
-                           (JSProperty **)&sprop)) {
+                           (JSProperty **)&uprop.prop)) {
         return JS_FALSE;
     }
 
-    if (sprop && OBJ_IS_NATIVE(obj2)) {
-        propid = sprop->id;
-        getter = sprop->getter;
-        attrs = sprop->attrs & ~JSPROP_SHARED;
-        slot = (uintN) sprop->shortid;
-        OBJ_DROP_PROPERTY(cx, obj2, (JSProperty *)sprop);
+    if (uprop.sprop && OBJ_IS_NATIVE(obj2)) {
+        propid = uprop.sprop->id;
+        getter = uprop.sprop->getter;
+        attrs = uprop.sprop->attrs & ~JSPROP_SHARED;
+        slot = (uintN) uprop.sprop->shortid;
+        OBJ_DROP_PROPERTY(cx, obj2, (JSProperty *)uprop.prop);
         if (getter == js_GetArgument || getter == js_GetLocalVariable) {
             if (getter == js_GetArgument) {
                 vp = fp->argv;

Comment 12

[Bradley Baetz (:bbaetz)]

Theres the one-structure-is-a-prefix-of-the-other exception though, which does
make the types compatable.

I guess I'll post to the gcc lists this weekend, then.

Comment 13

[Brendan Eich [:brendan]]

tenthumbs: a union is not necessary or desirable.  A downcast to a separate
variable of the more restrictive type, after testing the discriminant, is ok.

But I thought standard C said all pointers (save function pointers) were of the
same width, or something like that.  We have low-level code in NSPR and JS that
depends on being able to fit a pointer in a long (typedef'd as PRWord
[deprecated] or PRPtrdiff). C99 had some better type, but I don't keep up with
the C standard.  Anyone know the latest?

bbaetz, any reply from the gcc list?

/be

Comment 14

[Bradley Baetz (:bbaetz)]

Hmm, missed this one. I'll send mail tomorrow.

Comment 15

[Brendan Eich [:brendan]]

Ping.  I still claim these are bogus warnings for C, for the pattern in question.

/be

Comment 16

[tenthumbs]

> I still claim these are bogus warnings for C, for the pattern in question.

I'm not sure. The question is whether any group of allowed compiler statement
reorderings will produce bad code. I am sure that it will be very expensive to
verify correct behavior for every code and compiler change. It just seems more
efficient to fix it once and move on to something interesting.

Comment 17

[Brian Ryner (not reading)]

Based what I think I understand form bug 212082, these warnings may be correct.
 Namely, as bbaetz, says, the types JSProperty** and JSScopeProperty** are not
compatible.  So, the requirement is that you only dereference the pointer using
a compatible type.  This means that you can cast JSScopeProperty** to
JSProperty** and be ok, but you have to cast back to JSScopeProperty** before
reading or writing the memory pointed to ( see also
http://bugzilla.mozilla.org/show_bug.cgi?id=212082#c7 ).

In this case, jsfun.c line 789, a JSScopeProperty** is cast to a JSProperty**
and passed to js_LookupProperty.  Now, here at jsobj.c line 2381:

                *propp = NULL;

writes to the memory pointed to using the incompatible type JSProperty**. Also,
at line 2421:

                            if (!ok || *propp)

the memory is read through an incompatible type.  I'm not sure whether anything 
similar happens via the OBJ_LOOKUP_PROPERTY() macro.

One thing I'm not clear on is whether reordering could happen at the jsfun.c
call site based on the compiler seeing that it's passing a JSProperty** to
js_LookupProperty and then later reading sprop as a JSScopeProperty*.  Could it
really see those operations as independent?

cc'ing Wolfgang who seems to understand this much better than anyone :) Can you
please shed some light on the question above?

Comment 18

[Brendan Eich [:brendan]]

Fixed.

/be