860457 - use uintptr_t instead of unsigned long as our "unsigned integer with the same size as a pointer", in celldata.h

Status: RESOLVED FIXED

(Core :: Layout, defect)

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: use uintptr_t instead of unsigned long in celldata.h

The right thing will happen everywhere except for wacky 64-bit
big-endian platforms with 64-bit pointers and 32-bit longs, I think.
But we should still fix it. :)

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 735941 [details] [diff] [review]
use uintptr_t instead of unsigned long in celldata.h

Could you add a MOZ_STATIC_ASSERT to ensure that the assumption there actually holds?  (i.e. check that sizeof(mOrigCell) == sizeof(mBits) )

You could do this in the CellData constructor, in nsCellMap.cpp.

r=me with that, assuming this compiles. (i.e. assuming it passes a Try build on all platforms.  I've been bitten by missing-uintptr_t-support in the past, I think, though maybe we've got it defined in a header somewhere now.)

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> Could you add a MOZ_STATIC_ASSERT to ensure that the assumption there
> actually holds?  (i.e. check that sizeof(mOrigCell) == sizeof(mBits) )

uintptr_t is defined to be large enough to hold a pointer, so I'm not sure that the assertion is really checking anything useful.

> r=me with that, assuming this compiles. (i.e. assuming it passes a Try build
> on all platforms.  I've been bitten by missing-uintptr_t-support in the
> past, I think, though maybe we've got it defined in a header somewhere now.)

uintptr_t is provided in <stdint.h> on Mac, Linux, and some versions of Windows.  For those versions of Windows where it's not, we have mfbt/MSStdInt.h to fill in the gap.

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to Nathan Froyd (:froydnj) from comment #3)
> (In reply to Daniel Holbert [:dholbert] from comment #2)
> > Could you add a MOZ_STATIC_ASSERT to ensure that the assumption there
> > actually holds?  (i.e. check that sizeof(mOrigCell) == sizeof(mBits) )
> 
> uintptr_t is defined to be large enough to hold a pointer, so I'm not sure
> that the assertion is really checking anything useful.

It is useful. It'd be:
 (a) documenting / verifying our expectation that those variables have the same size
 (b) verifying that we're using a sane uintptr_t definition, and we're not e.g. accidentally getting the wrong typedef from another header which is stomping on mfbt's typedef. (Note that we have 12 different  "typedef [something]  uintptr_t;" statements in the tree, with a variety of different [something] values.)

Comment 5

[Daniel Holbert [:dholbert]]

(hopefully any of the typedefs we might get would be the right size, but there's no harm in explicitly checking it to be on the safe side)

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Daniel Holbert [:dholbert] from comment #4)
>  (b) verifying that we're using a sane uintptr_t definition, and we're not
> e.g. accidentally getting the wrong typedef from another header which is
> stomping on mfbt's typedef. (Note that we have 12 different  "typedef
> [something]  uintptr_t;" statements in the tree, with a variety of different
> [something] values.)

Bleh.  All right, you've convinced me.  (I think all those definitions are correct, but you're right, we might as well verify.)

Comment 7

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/efaa46a0e3e6

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/efaa46a0e3e6