"dereferencing type-punned pointer will break strict-aliasing" in nsCOMPtr

RESOLVED FIXED

Status

()

Core
XPCOM
--
minor
RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: Kenneth Herron, Unassigned)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments)

(Reporter)

Description

12 years ago
Gcc 4.1 with optimization emits about 30,000 warnings about dereferencing type-punned pointers in nsCOMPtr.h and nsCOMPtr.cpp, e.g.

.../nsCOMPtr.h: In member function 'void nsCOMPtr<T>::assign_from_qi(nsQueryInterface, const nsIID&) [with T = nsIArray]':
.../nsCOMPtr.h:645:   instantiated from 'nsCOMPtr<T>::nsCOMPtr(nsQuery Interface) [with T = nsIArray]'
...nsCOMPtr.h:593:   instantiated from 'void nsCOMPtr<T>::Assert_NoQueryNeeded() [with T = nsIArray]'
/extra/kherron/moz3/ff/../mozilla/xpcom/glue/nsCOMPtr.h:629:   instantiated from 'nsCOMPtr<T>::nsCOMPtr(T*) [with T = nsIArray]'
.../nsArrayEnumerator.cpp:58:   instantiated from here
.../nsCOMPtr.h:1232: warning: dereferencing type-punned pointer will break strict-aliasing rules

In all gcc complains about 7 sites in nsCOMPtr.cpp and 8 sites in nsCOMPtr.h:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpcom/glue/nsCOMPtr.cpp&rev=1.24&mark=96,105,114,123,132,141,150#92
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpcom/glue/nsCOMPtr.h&rev=1.122&mark=1232,1242,1252,1262,1272,1282,1292,1302#1227
How to reproduce:

1) Install gcc 4.1 (or maybe 3.3 or later)
2) Compile mozilla with optimization -Os, -O2, or -O3.
(Reporter)

Comment 1

12 years ago
Created attachment 236625 [details] [diff] [review]
Use a union

This fixes all of the warnings except line 1302 in nsCOMPtr.h. I couldn't see a way to fix that without making |mRawPtr| a union or adding a temporary variable. In particular the |NS_MAY_ALIAS_PTR| macro didn't help.

Updated

12 years ago
Attachment #236625 - Flags: superreview?(dbaron)
Attachment #236625 - Flags: review?(dbaron)
Comment on attachment 236625 [details] [diff] [review]
Use a union

Some of the changes in nsCOMPtr.cpp are missing the space after the opening "{" of the union declaration.

Could you untabify the lines that you're touching in nsCOMPtr.cpp?  (Replace each tab with 2 spaces, per modeline.)

I don't see anything wrong with a temporary variable in begin_assignment, i.e.:

union { T** mT; void** mVoid; } result;
result.mT = &mRawPtr;
return result.mVoid;

Feel free to add that if you want.


You'll need to be prepared to back this out if it causes codesize/performance regressions on any major platforms.  In particular, please make sure that we're actually running codesize/performance numbers on builds made with the compilers that we actually ship using.
Attachment #236625 - Flags: superreview?(dbaron)
Attachment #236625 - Flags: superreview+
Attachment #236625 - Flags: review?(dbaron)
Attachment #236625 - Flags: review+
(Reporter)

Comment 3

12 years ago
Checked in the patch without the |begin_assignment| change (and with tabs fixed). I'm going to wait for the results of this, and then possibly check in the change to |begin_assignment|.

/cvsroot/mozilla/xpcom/glue/nsCOMPtr.cpp,v  <--  nsCOMPtr.cpp
new revision: 1.25; previous revision: 1.24
/cvsroot/mozilla/xpcom/glue/nsCOMPtr.h,v  <--  nsCOMPtr.h
new revision: 1.124; previous revision: 1.123
(Reporter)

Comment 4

12 years ago
Tinderbox doesn't show an obvious problem so far. Checked in the |begin_assignment| change using the temporary variable (and the space following "{" issues that I forgot to fix, oops).

/cvsroot/mozilla/xpcom/glue/nsCOMPtr.cpp,v  <--  nsCOMPtr.cpp
new revision: 1.26; previous revision: 1.25
/cvsroot/mozilla/xpcom/glue/nsCOMPtr.h,v  <--  nsCOMPtr.h
new revision: 1.125; previous revision: 1.124
Status: NEW → ASSIGNED
(Reporter)

Comment 5

12 years ago
I'm going to resolve this. Tinderbox didn't note any code size changes. Any perf changes seem to be down in the noise.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 6

12 years ago
Comment on attachment 236625 [details] [diff] [review]
Use a union

>+		if ( NS_FAILED( qi(iid, &newRawPtr.mVoid) ) )
According to Wikipedia (not the best reference, I know, but the easiest), a union cannot be safely used for type-punning. The article goes on to say that although gcc will guarantee it in the case where you read and write directly to the members, pointers to union members are still unsafe.

Comment 7

11 years ago
Created attachment 259230 [details] [diff] [review]
Don't use a union

Because even in a simpler case such as this, GCC doesn't guarantee the result of taking the address of a union member:
union a_union { int i; double d; };
int f() { a_union t; int* ip; t.d = 3.0; ip = &t.i; return *ip; }
Attachment #259230 - Flags: superreview?(dbaron)
Attachment #259230 - Flags: review?(dbaron)
Comment on attachment 259230 [details] [diff] [review]
Don't use a union

The new approach looks fine, so r+sr=dbaron, although I'm not convinced that it's worth bothering to change it.  (Was your int/double testcase confused by int and double being different sizes?)
Attachment #259230 - Flags: superreview?(dbaron)
Attachment #259230 - Flags: superreview+
Attachment #259230 - Flags: review?(dbaron)
Attachment #259230 - Flags: review+

Updated

10 years ago
Duplicate of this bug: 472570

Comment 10

10 years ago
Comment on attachment 259230 [details] [diff] [review]
Don't use a union

A request for backporting the patch to make Seamonkey  compatible with gcc-4.3.2 per Fedora request. See bug 472570 for the gory details.
Attachment #259230 - Flags: approval1.8.1.next?

Comment 11

10 years ago
patching nsCOMPtr in stable branches doesn't sound riskless ... I don't think we we want this in the "official" 1.8 trees.
Attachment #259230 - Flags: approval1.8.1.next? → approval1.8.1.next-
Comment on attachment 259230 [details] [diff] [review]
Don't use a union

minusing this request. From the comments your request isn't on the patch that was checked in, and in fact I think this is a patch that assumes the other patch has landed. was it a misplaced request? The other patch wasn't quite what was actually checked in either according to the comments, I think you'd have to reconstruct a patch based on the actual change.
Yes, the request was misplaced, thanks. 

After a discussion with Alexander we're not going to ship the patch in 1.8 for now...
You need to log in before you can comment on or make changes to this bug.