269823 - Invalid casts prevent compilation on 64-bit platforms with g++ 4.0 (gcc 4) (nsHttpConnectionMgr.cpp)

Status: VERIFIED FIXED

(SeaMonkey :: General, defect)

Description

[falk]

User-Agent:       Mozilla/5.0 (X11; U; Linux alpha; en-US; rv:1.7.5) Gecko/20041114 Firefox/1.0 (Debian package 1.0-2)
Build Identifier: 

C++ does not allow to cast a pointer to an integer of smaller size. There are
several instances of this in Mozilla's source.

Note the NS_FT2_OFFSET macro is still completely unportable (it assumes null
pointers are represented by zero, and invokes undefined behaviour on the
null pointer dereferencing), but I suspect there must be a reason that
somebody went so far out of their way to avoid offsetof from <stdlib.h>.


Reproducible: Always
Steps to Reproduce:
Build mozilla-firefox CVS with g++ 4.0 on a 64-bit platform.

Comment 1

[falk]

Attachment: Patch

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

could you split up this patch in a netwerk/ part and a part for the rest?

Also, I'm really not happy about using a type that's deprecated
(http://lxr.mozilla.org/seamonkey/source/nsprpub/pr/include/prtypes.h#486)

Comment 3

[falk]

(In reply to comment #2)
> could you split up this patch in a netwerk/ part and a part for the rest?

Will do.

> Also, I'm really not happy about using a type that's deprecated
> (http://lxr.mozilla.org/seamonkey/source/nsprpub/pr/include/prtypes.h#486)

Then what do you prefer? size_t?

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

I'd probably pick unsigned long or PRPtrDiff, but I'd like to hear wchang's
opinion on it...

Comment 5

[Wan-Teh Chang]

I agree that the PRWord and PRUword types are
useful.  We often need an integer type that is the
same size as (or large enough to hold) a void*.
What I don't like is their names.  "Word" is very
ambiguous.  I'd rather define new types for this
purpose with descriptive names.  Perhaps PRIntptr
and PRUintptr (modeled after C99's intptr_t and
uintptr_t)?

Until the new types are added, I recommend using
PRSize (or NSPR's own PRUptrdiff) if you need an
unsigned type and PRPtrdiff if you need a signed
type.  But if you want to use PRWord and PRUword,
that's fine, too.

By the way, the best way to elimiate this compiler
warning is to use two typecasts:

-        listTarget->info = (guint)listAtom;
+        listTarget->info = (guint)(PRUword)listAtom;

The first cast makes the compiler let us cast
a pointer to an integer type.  The second cast
eliminates the compiler warning about assigning to
an integer type of a smaller size.

Comment 6

[Wan-Teh Chang]

Re: the suggestion of using unsigned long in comment 4

unsigned long may not be large enough to hold a pointer.
A counterexample is 64-bit Windows, where long is still
32-bit.  So we need a type that is specifically defined
to be the same size as (or large enough to hold) a pointer.

Comment 7

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I would support a PRIntPtr type, yeah.  C# has its own IntPtr for exactly this
purpose, and the C99 types are just like this as well.

I'd probably even review the tree-wide patch to change our PRWord usages!

Comment 8

[falk]

Attachment: Patch for bad casts (netwerk part)

This still uses PRUword to facilitate batch changing to PRIntPtr.

Comment 9

[falk]

Attachment: Patch for bad casts (gtk2 part)

This still uses PRUword to facilitate batch changing to PRIntPtr.

Comment 10

[Darin Fisher]

Comment on attachment 173171 [details] [diff] [review]
Patch for bad casts (netwerk part)

>-    PRUint16 name  = (PRUint32(param) & 0xFFFF0000) >> 16;
>-    PRUint16 value =  PRUint32(param) & 0x0000FFFF;
>+    PRUint16 name  = (PRUword(param) & 0xFFFF0000) >> 16;
>+    PRUint16 value =  PRUword(param) & 0x0000FFFF;

perhaps another choice would be to use the NS_PTR_TO_INT32 macro.

notice that prtypes.h says:

** WARNING: The undocumented data types PRWord and PRUword are
** only used in the garbage collection and arena code.	Do not
** use PRWord and PRUword in new code.

so, perhaps you should head that warning?

Comment 11

[Darin Fisher]

oh, i should have read the rest of the comments in this bug...

wtc, shaver: what do you guys think of NS_PTR_TO_INT32?  that seems to be
exactly what we want here.

Comment 12

[falk]

(In reply to comment #11)
> oh, i should have read the rest of the comments in this bug...
> 
> wtc, shaver: what do you guys think of NS_PTR_TO_INT32?  that seems to be
> exactly what we want here.

Ugh. Firstly, using signed integers here has implementation defined behaviour.
Secondly, this macro is defined as

#define NS_PTR_TO_INT32(x) ((char *)(x) - (char *)0)

which is clearly invalid, and not unlikely to confuse a sufficiently 
advanced optimizer. Oh well.

Comment 13

[Wan-Teh Chang]

The NS_PTR_TO_INT32 macro has a descriptive
name and is exactly what we are doing here.

The current definition (using pointer difference)
of that macro does look ugly.

A macro is nice because as I explained in
comment 5, this may require two casts to
eliminate compiler warnings (I'm using C99
types in the example below):

#define NS_PTR_TO_INT32(x) ((int32_t)(intptr_r)(x))

I found that intptr_t and uintptr_t are
optional data types because an implementation
may not have any integer type large enough
to hold a pointer.  It's very unusual though.

Comment 14

[Wan-Teh Chang]

Looking in CVS history, I found that the
NS_PTR_TO_INT32 macro was added in rev. 1.43
of nscore.h:

1.43 <[e-mail]> 2001-08-13 21:14
Adding NS_PTR_TO_INT32 & NS_INT32_TO_PTR macros to do safe pointer casting on
64-bit platforms. Bug #20860 r=[e-mail]
sr=[e-mail]

Since intptr_t and uintptr_t are only optional
types, I don't think there is a fully portable
definition of NS_PTR_TO_INT32.  So how we want
to define this macro is going to be a matter of
personal experience and preference.

Comment 15

[Darin Fisher]

how about we modify NS_PTR_TO_INT32 and create NS_PTR_TO_UINT32 so that we have
something that works as desired, and then use that instead of PRUword.  Sound
like a plan?

Comment 16

[Wan-Teh Chang]

I agree it's better to hide the casts in a macro.

Comment 17

[Philip K. Warren]

*** Bug 281576 has been marked as a duplicate of this bug. ***

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

*** Bug 284649 has been marked as a duplicate of this bug. ***

Comment 19

[Brian Ryner (not reading)]

Comment on attachment 173172 [details] [diff] [review]
Patch for bad casts (gtk2 part)

minusing per the prior discussion on the bug.

Comment 20

[Henrik Lynggaard Hansen]

Please note that bug 284809 also tries to correct this (and other gcc4.0 +amd64
issues)

They mostly uses "NS_PTR_TO_INT32" as far as I can see.

should one of the bugs be closed as a duplicate ?

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

*** Bug 296792 has been marked as a duplicate of this bug. ***

Comment 22

[Christian :Biesinger (don't email me, ping me on IRC)]

*** Bug 284809 has been marked as a duplicate of this bug. ***

Comment 23

[Christian :Biesinger (don't email me, ping me on IRC)]

*** Bug 298259 has been marked as a duplicate of this bug. ***

Comment 24

[Matthew Daniel]

I wanted to make note that ever since I applied the upcast-downcast patch, my text cursor no longer works inside textarea, input[@type='text'] or the address bar. I don't have any proof (or way of obtaining it short of a checkout of last week's HEAD and building it with these same patches), but found it a strange coincidence.

My .mozconfig hasn't changed in quite a while (since the cairo config change).

Comment 25

[falk]

Attachment: Fix NS_PTR_TO_INT32 and NS_INT32_TO_PTR, add NS_PTR_TO_UINT32

Following comment #15, this avoids undefined behavior in
NS_PTR_TO_INT32 and NS_INT32_TO_PTR. It also makes NS_PTR_TO_INT32
actually return a 32-bit integer instead of a ptrdiff_t, which might
be 64-bit. NS_PTR_TO_UINT32 is added for places where an uint32 is
required to avoid implementation defined int32->uint32 casts.

Comment 26

[falk]

Attachment: Patch for bad casts (netwerk part, using NS_PTR_TO_UINT32)

Comment 27

[falk]

Attachment: Patch for bad casts (gtk2 part, using NS_PTR_TO_UINT32)

Comment 28

[Darin Fisher]

Comment on attachment 187644 [details] [diff] [review]
Fix NS_PTR_TO_INT32 and NS_INT32_TO_PTR, add NS_PTR_TO_UINT32

>-#define NS_PTR_TO_INT32(x) ((char *)(x) - (char *)0)
>+#define NS_PTR_TO_INT32(x)  ((PRInt32)  (PRWord) (x))

C++ allows you to cast a ptrdiff_t to an int, right?  So, why not
modify the macros like so:

  #define NS_PTR_TO_INT32(x) PRInt32((char *)(x) - (char *)0)

?

Comment 29

[falk]

(In reply to comment #28)
> C++ allows you to cast a ptrdiff_t to an int, right?  So, why not
> modify the macros like so:
> 
>   #define NS_PTR_TO_INT32(x) PRInt32((char *)(x) - (char *)0)

Because this is undefined behavior. Pointer difference is only valid for
two pointers into the same array, or one behind the end.

This is not only a hypothetical problem. For example, the compiler can
decuce that x is nonzero here and optimize a further (x != 0) away. So
this code is playing a very dangerous game, for no good reason.

Comment 30

[Darin Fisher]

Comment on attachment 187644 [details] [diff] [review]
Fix NS_PTR_TO_INT32 and NS_INT32_TO_PTR, add NS_PTR_TO_UINT32

OK, thanks for the explanation.  r=darin

Comment 31

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 187647 [details] [diff] [review]
Patch for bad casts (gtk2 part, using NS_PTR_TO_UINT32)

Should this use GPOINTER_TO_UINT instead, since it's all in gtk2-related code? 
That's what I did in my tree, at least.

Comment 32

[falk]

(In reply to comment #31)
> (From update of attachment 187647 [details] [diff] [review] [edit])
> Should this use GPOINTER_TO_UINT instead, since it's all in gtk2-related code? 

I guess it should; I simply didn't know that macro. Can you attach a new patch?

Comment 33

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 187644 [details] [diff] [review]
Fix NS_PTR_TO_INT32 and NS_INT32_TO_PTR, add NS_PTR_TO_UINT32

sr+a=shaver, I'll commit this patch.

Comment 34

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Attachment: Patch for bad casts (gtk2 part, using GPOINTER_TO_UINT)

This is what I'm building with now.  caillon: can you gimme a stamp?

Comment 35

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 187646 [details] [diff] [review]
Patch for bad casts (netwerk part, using NS_PTR_TO_UINT32)

sr+a=shaver, though I don't really understand why we have the masking there as
well as the shift.

I'll commit this ASAP.

Comment 36

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Fixed, woo.