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

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Kenneth Herron]

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.

Comment 1

[Kenneth Herron]

Attachment: 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.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 3

[Kenneth Herron]

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

Comment 4

[Kenneth Herron]

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

Comment 5

[Kenneth Herron]

I'm going to resolve this. Tinderbox didn't note any code size changes. Any perf changes seem to be down in the noise.

Comment 6

[[e-mail]]

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

[[e-mail]]

Attachment: 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; }

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?)

Comment 10

[Jacek Piskozub]

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.

Comment 11

[Alexander Sack]

patching nsCOMPtr in stable branches doesn't sound riskless ... I don't think we we want this in the "official" 1.8 trees.

Comment 12

[Daniel Veditz [:dveditz]]

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.

Comment 13

[Martin Stránský [:stransky] (ni? me)]

Yes, the request was misplaced, thanks. 

After a discussion with Alexander we're not going to ship the patch in 1.8 for now...