679832 - GCC 4.6 Warning on 64-bit linux: nsCheapSets.h:178:43: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Daniel Holbert [:dholbert]]

nsCheapSets has the same issue as was described in bug 677993  -- 32-bit-value to 64-bit-pointer conversion -- for generic storage in a void* payload -- which makes GCC spam warnings like this:

> In file included from content/html/content/src/nsHTMLSelectElement.h:59:0,
>                 from content/html/content/src/nsHTMLOptionElement.cpp:42:
> dist/include/nsCheapSets.h: In member function ‘void nsCheapInt32Set::SetInt(PRInt32)’:
> dist/include/nsCheapSets.h:178:43: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]

The code in question is:
{
170   /** Get the single integer */
171   PRInt32 GetInt()
172   {
173     return PtrBits(mValOrHash) >> 1;
174   }
175   /** Set the single integer */
176   void SetInt(PRInt32 aInt)
177   {
178     mValOrHash = (void*)((aInt << 1) | 0x1);
179   }

I believe we want to insert an (intptr_t) cast adjacent to the (void*) cast here, to explicitly promote our 32-bit int to a 64-bit int before casting it to a 64-bit pointer.  (on 64-bit systems)  This is what happens under-the-hood anyway -- the cast swill just make that explicit and silence the warning.

(Note that unlike in bug 677993, this code here has sign extension for negative values.  We already get this sign-extension even without the intptr_t cast, though, so this change won't affect behavior.)

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> nsCheapSets has the same issue as was described in bug 677993  --
> 32-bit-value to 64-bit-pointer conversion -- for generic storage in a void*
> payload -- which makes GCC spam warnings like this:

(GCC 4.6, that is)

Comment 2

[Daniel Holbert [:dholbert]]

Meant to include MXR link:
https://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsCheapSets.h?mark=178-178#176

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: fix: add intptr_t cast

Comment 4

[Daniel Holbert [:dholbert]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/196df669baba

Comment 5

[Daniel Holbert [:dholbert]]

Apparently nsCheapSets.h doesn't understand intptr_t on all platforms:
{
In file included from /builds/slave/m-in-linuxqt/build/xpcom/ds/nsCheapSets.cpp:38:0:
/builds/slave/m-in-linuxqt/build/xpcom/ds/nsCheapSets.h: In member function 'void nsCheapInt32Set::SetInt(PRInt32)':
/builds/slave/m-in-linuxqt/build/xpcom/ds/nsCheapSets.h:178:26: error: 'intptr_t' was not declared in this scope
}
http://tbpl.allizom.org/php/getParsedLog.php?id=6023193

Pushed a followup to include stdint.h, which should address that:
http://hg.mozilla.org/integration/mozilla-inbound/rev/902a7c3eb320

Comment 6

[Phil Ringnalda (:philor)]

Backed them both out in http://hg.mozilla.org/integration/mozilla-inbound/rev/54a15fc04437 since Windows rather strongly disapproves of being expected to have a stdint.h (which I presume is why http://mxr.mozilla.org/mozilla-central/search?string=stdint.h includes so much dancing around).

Comment 7

[Daniel Holbert [:dholbert]]

Ack!  I could've sworn I'd seen a stdint.h include (or at least some intptr_t usage) in non-platform-ifdef'd code.

Sorry, & thanks for cleaning up.  I'll just copy whatever dance we use elsewhere to make it work, give it a tryserver run, and re-land.

Comment 8

[Daniel Holbert [:dholbert]]

So to follow up: IIRC, it turned out that the dance we use elsewhere is a bit complex and not suitable for copy-pasting all over the place.

Also: This is now a build *error* for --enable-warnings-as-error builds (with GCC 4.6 on 64-bit systems), since this header is included in /content/html/content and that directory is now FAIL_ON_WARNINGS per Bug 716338.  :(

Comment 9

[Nathan Froyd [:froydnj]]

FWIW, nsCheapSets.h probably needs to be rewritten for bug 700659 anyway; that would be a good time to remove the bit-twiddling games it plays.

Comment 10

[Daniel Holbert [:dholbert]]

Unassigning as I'm not actively working on this.

Comment 11

[Daniel Holbert [:dholbert]]

(In reply to Nathan Froyd (:froydnj) from comment #9)
> FWIW, nsCheapSets.h probably needs to be rewritten for bug 700659 anyway

In the meantime, it could be worth taking the attached patch in an "#ifdef 64-bit-linux-or-mac" chunk, as a fix for the warning now being treated as an error, per comment 8.  (ifdef'd because (a) it's only an issue on 64-bit systems, and (b) we're only treating warnings as errors on Linux & mac)

The fact that the code will need to be rewritten soonish anyway makes me feel less dirty about the possibility of using an ifdef'd hackaround. :)

Comment 12

[Daniel Holbert [:dholbert]]

Trying an ifdeffed variant: https://tbpl.mozilla.org/?tree=Try&rev=8e934fe17cf3

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: fix v1 (ifdef 64-bit GCC)

Nice -- the ifdeffed version passed the Try run (see previous comment)

(technically one platform is still running -- OS X 64 opt -- but it passed on OS X 64 debug, so I fully expect it to pass on opt.)

Comment 14

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/48af5f55c8b8

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

We've got mozilla/StdInt.h now, which provides intptr_t on platforms that lack it.

Comment 16

[Daniel Holbert [:dholbert]]

Oh, nice!  I'll do another Try push using that, and assuming it works, land it as a followup.

Comment 17

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/48af5f55c8b8

Comment 18

[Daniel Holbert [:dholbert]]

Yup -- per comment 15, we can just use the mozilla/StdInt.h-provided intptr_t on all platforms now -- verified on this successful Try push: https://tbpl.mozilla.org/?tree=Try&rev=9e5aae225b00

So, I pushed that followup with carried-forward r=bsmedberg, since (combined with the already-landed patch) it's exactly the r+'d original patch here (with an added #include for StdInt.h).
  https://hg.mozilla.org/integration/mozilla-inbound/rev/d44e07a5db12

Comment 19

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Thanks!

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