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]

RESOLVED FIXED in mozilla12

Status

()

Core
XPCOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Blocks: 1 bug)

Trunk
mozilla12
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [build_warning][error w/ --enable-warnings-as-errors])

Attachments

(2 attachments)

(Assignee)

Description

6 years ago
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

6 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

6 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

6 years ago
Created attachment 553871 [details] [diff] [review]
fix: add intptr_t cast
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #553871 - Flags: review?(benjamin)

Updated

6 years ago
Attachment #553871 - Flags: review?(benjamin) → review+
(Assignee)

Comment 4

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/196df669baba
Whiteboard: [inbound]
(Assignee)

Comment 5

6 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
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

6 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

6 years ago
Whiteboard: [build_warning]
(Assignee)

Comment 8

6 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.  :(
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

6 years ago
Unassigning as I'm not actively working on this.
Assignee: dholbert → nobody
Status: ASSIGNED → NEW
(Assignee)

Comment 11

6 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

6 years ago
Trying an ifdeffed variant: https://tbpl.mozilla.org/?tree=Try&rev=8e934fe17cf3
(Assignee)

Comment 13

6 years ago
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.)
Attachment #587563 - Flags: review?(benjamin)
(Assignee)

Updated

6 years ago
Whiteboard: [build_warning] → [build_warning][error w/ --enable-warnings-as-errors]

Updated

6 years ago
Attachment #587563 - Flags: review?(benjamin) → review+
(Assignee)

Comment 14

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/48af5f55c8b8
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla12
We've got mozilla/StdInt.h now, which provides intptr_t on platforms that lack it.
(Assignee)

Comment 16

6 years ago
Oh, nice!  I'll do another Try push using that, and assuming it works, land it as a followup.
https://hg.mozilla.org/mozilla-central/rev/48af5f55c8b8
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 18

6 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
Thanks!

https://hg.mozilla.org/mozilla-central/rev/d44e07a5db12
You need to log in before you can comment on or make changes to this bug.