558410 - Silence GCC strict-aliasing warnings about LazilyConstructed<AutoLockGC> etc.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

This sort of thing:

In file included from ../jshashtable.h:46,
                 from ../jscntxt.h:55,
                 from ../jscntxt.cpp:55:
../jstl.h: In member function ‘T& js::LazilyConstructed<T>::asT() [with T = js::AutoLockGC]’:
../jstl.h:273:   instantiated from ‘js::LazilyConstructed<T>::~LazilyConstructed() [with T = js::AutoLockGC]’
../jstl.h:307:   instantiated from here
../jstl.h:269: warning: dereferencing type-punned pointer will break strict-aliasing rules
../jscntxt.h: In function ‘JSContext* js_ContextIterator(JSRuntime*, JSBool, JSContext**)’:
../jscntxt.h:1953: warning: dereferencing pointer ‘<anonymous>’ does break strict-aliasing rules
../jstl.h:286: note: initialized from here

Patch coming.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: v1

I tried several things. The only thing that seems to silence the warning is not doing that. So this patch specializes LazilyConstructed<Auto{Lock,Unlock}GC> to avoid the char[] buffer and the cast.

Comment 2

[Luke Wagner [:luke]]

This would be a second, and much grosser, hack introduced to silence these over-zealous and unsilenceable strict-aliasing warnings.  And this is only going to continue.  Perhaps (a) we can silence the warning and (b) I can ask a GCC standard C++ library devs or Boost devs how they don't run into this issue.

Comment 3

[Robert Sayre]

hmm, did Waldo crank up the strict-aliasing warning level recently?

Comment 4

[Luke Wagner [:luke]]

Of interest, this stuff is hitting std:: libraries

  http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39390

The testcase in the summary still gives me a warning on 4.4.1 with -O3 -Wall.

Comment 5

[Luke Wagner [:luke]]

(In reply to comment #3)
> hmm, did Waldo crank up the strict-aliasing warning level recently?

The higher warning levels are supposed to help (less false positives), although I think the warning level was already at the highest by default (from the "-Wstrict-aliasing=n" section in the gcc man page).

Comment 6

[Jason Orendorff [:jorendorff]]

Yeah - I don't like having 30 lines of code that we really shouldn't need. Luke, it would be awesome if you could get advice from stdlib/boost developers.

I thought maybe I could get rid of the warning by declaring the 'constructed' bit as a separate bool in LazilyConstructed, putting it in a 2-element struct with the buffer; but it didn't help.

Comment 7

[Luke Wagner [:luke]]

I built and tried gcc trunk, but its no different.  However, I also found out that the pragma

  #pragma GCC diagnostic ignored "-Wstrict-aliasing"

actually works.  Previously, I tried selectively turning this on and off around offending code, but to no avail.  Playing with a minimal test case which reproduces the warning, it seems that the only thing that matters with this pragma is that it is in the 'ignored' state at the end of the file.  Nothing else matters.  So, as best I can see, using this pragma means turning off the warning for the entire translation unit.  I'm going to use the minimal test case to file a GCC bug.

Comment 9

[Luke Wagner [:luke]]

Attachment: patch

So, I was pointed to boost::optional.  Its implementation has the same underlying buf[sizeof(T)] and casting, but no warning.  The only difference is, being Boost, there are like 40 layers of abstraction (purely compile-time, of course) between public member functions and the data.  So I played around a little with similar-minded obfuscations and was able to make the warning go away.  Better yet, it also works on js::Vector, so I can consolidate kludges.  Better yet still, the kludge is halfway useful in its own right (js::AlignedStorage<N>)

Comment 10

[Luke Wagner [:luke]]

On the subject, it has been observed that js::LazilyConstructed is not the most intuitive name on the planet.  While I'm touching this, perhaps I could just merge js::Conditionally into js::LazilyConstructed and rename LazilyConstructed to be Conditionally?  Or js::Option?  js::Maybe?  Thoughts?  Wrong bug for this discussion?  Too many questions?

Comment 11

[Jeff Walden [:Waldo]]

LazilyConstructed seems fine to me; beyond it being fine on its own merits, I can't think of a better name anyway.

Comment 12

[Jason Orendorff [:jorendorff]]

Comment on attachment 438661 [details] [diff] [review]
patch

Great!

Comment 13

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/5b1c7bc8783e

Comment 14

[Luke Wagner [:luke]]

Fix Windows bustage
http://hg.mozilla.org/tracemonkey/rev/b94a937fcd96

Comment 15

[Luke Wagner [:luke]]

Brendan pointed out that uint64 was more appropriate than JSUint64
http://hg.mozilla.org/tracemonkey/rev/18d60992ab7c

Comment 16

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/5b1c7bc8783e