Last Comment Bug 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]
: GCC 4.6 Warning on 64-bit linux: nsCheapSets.h:178:43: warning: cast to poin...
Status: RESOLVED FIXED
[build_warning][error w/ --enable-war...
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla12
Assigned To: Daniel Holbert [:dholbert]
:
Mentors:
Depends on:
Blocks: buildwarning 166168
  Show dependency treegraph
 
Reported: 2011-08-17 12:20 PDT by Daniel Holbert [:dholbert]
Modified: 2012-01-13 03:01 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix: add intptr_t cast (878 bytes, patch)
2011-08-17 12:34 PDT, Daniel Holbert [:dholbert]
benjamin: review+
Details | Diff | Review
fix v1 (ifdef 64-bit GCC) (1.04 KB, patch)
2012-01-10 18:12 PST, Daniel Holbert [:dholbert]
benjamin: review+
Details | Diff | Review

Description Daniel Holbert [:dholbert] 2011-08-17 12:20:37 PDT
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] 2011-08-17 12:21:15 PDT
(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] 2011-08-17 12:22:30 PDT
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] 2011-08-17 12:34:59 PDT
Created attachment 553871 [details] [diff] [review]
fix: add intptr_t cast
Comment 4 Daniel Holbert [:dholbert] 2011-08-18 16:13:04 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/196df669baba
Comment 5 Daniel Holbert [:dholbert] 2011-08-18 16:34:43 PDT
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) 2011-08-18 20:07:09 PDT
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] 2011-08-18 20:31:15 PDT
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] 2012-01-10 13:49:04 PST
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] 2012-01-10 14:15:00 PST
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] 2012-01-10 14:36:21 PST
Unassigning as I'm not actively working on this.
Comment 11 Daniel Holbert [:dholbert] 2012-01-10 14:39:27 PST
(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] 2012-01-10 15:25:27 PST
Trying an ifdeffed variant: https://tbpl.mozilla.org/?tree=Try&rev=8e934fe17cf3
Comment 13 Daniel Holbert [:dholbert] 2012-01-10 18:12:58 PST
Created attachment 587563 [details] [diff] [review]
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] 2012-01-11 12:47:55 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/48af5f55c8b8
Comment 15 :Ms2ger 2012-01-12 02:09:35 PST
We've got mozilla/StdInt.h now, which provides intptr_t on platforms that lack it.
Comment 16 Daniel Holbert [:dholbert] 2012-01-12 09:02:55 PST
Oh, nice!  I'll do another Try push using that, and assuming it works, land it as a followup.
Comment 17 Matt Brubeck (:mbrubeck) 2012-01-12 10:56:27 PST
https://hg.mozilla.org/mozilla-central/rev/48af5f55c8b8
Comment 18 Daniel Holbert [:dholbert] 2012-01-12 17:04:30 PST
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

Note You need to log in before you can comment on or make changes to this bug.