Closed
Bug 679832
Opened 13 years ago
Closed 13 years ago
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]
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
mozilla12
People
(Reporter: dholbert, Assigned: dholbert)
References
(Blocks 1 open bug)
Details
(Whiteboard: [build_warning][error w/ --enable-warnings-as-errors])
Attachments
(2 files)
878 bytes,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
1.04 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
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.)
Assignee | ||
Comment 1•13 years ago
|
||
(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)
Assignee | ||
Comment 2•13 years ago
|
||
Meant to include MXR link:
https://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsCheapSets.h?mark=178-178#176
Assignee | ||
Comment 3•13 years ago
|
||
Updated•13 years ago
|
Attachment #553871 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 4•13 years ago
|
||
Whiteboard: [inbound]
Assignee | ||
Comment 5•13 years ago
|
||
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•13 years ago
|
||
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).
Whiteboard: [inbound]
Assignee | ||
Comment 7•13 years ago
|
||
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.
Updated•13 years ago
|
Whiteboard: [build_warning]
Assignee | ||
Comment 8•13 years ago
|
||
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•13 years ago
|
||
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.
Assignee | ||
Comment 10•13 years ago
|
||
Unassigning as I'm not actively working on this.
Assignee: dholbert → nobody
Status: ASSIGNED → NEW
Assignee | ||
Comment 11•13 years ago
|
||
(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. :)
Assignee | ||
Comment 12•13 years ago
|
||
Trying an ifdeffed variant: https://tbpl.mozilla.org/?tree=Try&rev=8e934fe17cf3
Assignee | ||
Comment 13•13 years ago
|
||
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.)
Attachment #587563 -
Flags: review?(benjamin)
Assignee | ||
Updated•13 years ago
|
Whiteboard: [build_warning] → [build_warning][error w/ --enable-warnings-as-errors]
Updated•13 years ago
|
Attachment #587563 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 14•13 years ago
|
||
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla12
Comment 15•13 years ago
|
||
We've got mozilla/StdInt.h now, which provides intptr_t on platforms that lack it.
Assignee | ||
Comment 16•13 years ago
|
||
Oh, nice! I'll do another Try push using that, and assuming it works, land it as a followup.
Comment 17•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 18•13 years ago
|
||
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•13 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•