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()
173 return PtrBits(mValOrHash) >> 1;
175 /** Set the single integer */
176 void SetInt(PRInt32 aInt)
178 mValOrHash = (void*)((aInt << 1) | 0x1);
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.)
(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)
Meant to include MXR link:
Created attachment 553871 [details] [diff] [review]
fix: add intptr_t cast
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
Pushed a followup to include stdint.h, which should address that:
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).
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.
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. :(
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.
Unassigning as I'm not actively working on this.
(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. :)
Trying an ifdeffed variant: https://tbpl.mozilla.org/?tree=Try&rev=8e934fe17cf3
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.)
We've got mozilla/StdInt.h now, which provides intptr_t on platforms that lack it.
Oh, nice! I'll do another Try push using that, and assuming it works, land it as a followup.
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).