Bug 413253 turned it off because we have too many warnings dealing with it.

So re-enabling this is a Moz2 thing or do you plan to do this for 1.9final still? Perhaps it would be useful to give just a few examples of the warnings in question because not one of them was mentioned in bug 413253.

I wonder if we could improve performance by selectively turning this on where it's not too much work to fix the errors.

Do we have any idea what the perf win would be if we enabled strict aliasing? (I assume it's nontrivial to try and see since GCC will generate invalid code until we fix the warnings.)

I just ran a clean build and collected the strict aliasing warnings. It doesn't look like there are unreasonably many to consider fixing them, especially since in some cases (e.g. breakpad) we can just turn off strict aliasing. Here are the strict aliasing warnings* I'm getting on Linux x64: src/gfx/thebes/gfxContext.cpp:318 src/gfx/thebes/gfxPattern.cpp:114 src/modules/libpr0n/decoders/bmp/nsBMPDecoder.cpp:341 dist/include/jscntxt.h:2202 dist/include/jscntxt.h:2202 dist/include/jscntxt.h:2202 dist/include/jscntxt.h:2202 dist/include/jscntxt.h:2202 dist/include/jscntxt.h:2202 src/dom/base/nsGlobalWindow.cpp:1825 src/dom/base/nsDOMClassInfo.cpp:5772 src/content/canvas/src/WebGLContextGL.cpp:2672 src/content/canvas/src/WebGLContextGL.cpp:2678 src/content/xslt/src/xpath/txXPCOMExtensionFunction.cpp:421 src/content/xslt/src/xpath/txXPCOMExtensionFunction.cpp:469 src/content/xslt/src/xpath/txXPCOMExtensionFunction.cpp:515 src/layout/style/nsCSSDataBlock.cpp:148 src/layout/style/nsCSSDataBlock.cpp:158 src/toolkit/components/places/src/nsMaybeWeakPtr.cpp:73 dist/include/nsCOMPtr.h:1077 src/toolkit/crashreporter/google-breakpad/src/common/linux/http_upload.cc:90 src/toolkit/crashreporter/google-breakpad/src/common/linux/http_upload.cc:102 src/toolkit/crashreporter/google-breakpad/src/common/linux/http_upload.cc:118 src/toolkit/crashreporter/google-breakpad/src/common/linux/http_upload.cc:138 src/toolkit/crashreporter/google-breakpad/src/common/linux/http_upload.cc:149 src/toolkit/crashreporter/google-breakpad/src/common/linux/http_upload.cc:152 src/toolkit/crashreporter/google-breakpad/src/common/linux/http_upload.cc:163 src/toolkit/crashreporter/google-breakpad/src/common/linux/http_upload.cc:167 src/toolkit/crashreporter/google-breakpad/src/common/linux/http_upload.cc:172 src/toolkit/crashreporter/google-breakpad/src/common/linux/http_upload.cc:90 src/toolkit/crashreporter/google-breakpad/src/common/linux/http_upload.cc:102 src/toolkit/crashreporter/google-breakpad/src/common/linux/http_upload.cc:118 src/toolkit/crashreporter/google-breakpad/src/common/linux/http_upload.cc:138 src/toolkit/crashreporter/google-breakpad/src/common/linux/http_upload.cc:149 src/toolkit/crashreporter/google-breakpad/src/common/linux/http_upload.cc:152 src/toolkit/crashreporter/google-breakpad/src/common/linux/http_upload.cc:163 src/toolkit/crashreporter/google-breakpad/src/common/linux/http_upload.cc:167 src/toolkit/crashreporter/google-breakpad/src/common/linux/http_upload.cc:172 src/toolkit/crashreporter/google-breakpad/src/client/linux/crash_generation/crash_generation_server.cc:354 * Actually, this isn't all of them. There are also some errors in the old HTML parser which I already turned off.

With the gcc I have (4.4.3 on Ubuntu 10.04) I'm pretty sure I see a bunch in nsTArray quite frequently.

http_upload.cc isn't even linked into Firefox, only the crash reporter client. I assume most of those are from using dlsym, since you can't cast it's void* return to a function pointer without violating strict aliasing, right?

(In reply to comment #5) > With the gcc I have (4.4.3 on Ubuntu 10.04) I'm pretty sure I see a bunch in > nsTArray quite frequently. Interesting. I just did a full clobber build, and I don't see anything referring to nsTArray. On the other hand, I do see references to mpi/mpcpucache.c, ssl3con.c, sslsnce.c, and jarfile.c. I didn't see any of those before.

Ahh. I do get lots of warnings in nsTArray when I set -Wstrict-aliasing=2. It's not clear that's the right setting, but maybe it is. See http://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#Warning-Options

Just pushed this to try. Seems to work OK on my Linux 64 machine.

Looking at the text of the patch, I now remember that one of the first things I did was fix a strict aliasing error in nsTArray. dbaron, you're not going crazy. :)

I wonder what we should do about making strict-aliasing violations an error. I think they probably should be, but otoh, it can be hard to reproduce locally the conditions which cause the errors. For instance, I'm getting an error at gfx/angle/src/compiler/preprocessor/scanner.c:230 on try that I didn't get on my own machine, even when I used gcc-4.3. The converse may be true as well: I imagine some people's builds will fail due to strict aliasing violations even though our trees are green.

Succeeding on Mac 32/64 debug/opt builds, Linux 32/64 debug builds. Crashing on Linux 32/64 opt builds. Let the fun begin.

Note that strict-aliasing errors often only appear in opt build, because the inlining operations which would cause them to become issues don't happen in debug builds.

These are the valgrind warnings I'm currently getting before FF dies while running the crashtest on Linux-64. ==12304== Thread 2: ==12304== Syscall param socketcall.sendmsg(msg.msg_iov[i]) points to uninitialised byte(s) ==12304== at 0x4E3CF2D: ??? (syscall-template.S:82) ==12304== by 0x5F997C8: IPC::Channel::ChannelImpl::ProcessOutgoingMessages() (ipc_channel_posix.cc:622) ==12304== by 0x5F9A37D: IPC::Channel::ChannelImpl::Send(IPC::Message*) (ipc_channel_posix.cc:678) ==12304== by 0x5F77ED5: MessageLoop::RunTask(Task*) (message_loop.cc:339) ==12304== by 0x5F782FF: MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) (message_loop.cc:347) ==12304== by 0x5F7859C: MessageLoop::DoWork() (message_loop.cc:447) ==12304== by 0x5F933CA: base::MessagePumpLibevent::Run(base::MessagePump::Delegate*) (message_pump_libevent.cc:309) ==12304== by 0x5F780A5: MessageLoop::Run() (message_loop.cc:202) ==12304== by 0x5F83A4B: base::Thread::ThreadMain() (thread.cc:156) ==12304== by 0x5F93B09: ThreadFunc(void*) (platform_thread_posix.cc:26) ==12304== by 0x4E349C9: start_thread (pthread_create.c:300) ==12304== by 0x74156FC: clone (clone.S:112) ==12304== Address 0xe9a78a4 is not stack'd, malloc'd or (recently) free'd ==12304== ==12247== Warning: set address range perms: large range [0x39187000, 0x1b6987000) (defined) ==12247== Warning: set address range perms: large range [0x39187000, 0x1b6987000) (noaccess) ==12247== Warning: set address range perms: large range [0x1b6900000, 0x334100000) (defined) ==12247== Warning: set address range perms: large range [0x1b6900000, 0x334100000) (noaccess) ==12247== Invalid read of size 8 ==12247== at 0x583ACCE: nsINode::ReplaceOrInsertBefore(int, nsINode*, nsINode*) (nsGenericElement.cpp:4075) ==12247== by 0x583617D: nsINode::ReplaceOrInsertBefore(int, nsIDOMNode*, nsIDOMNode*, nsIDOMNode**) (nsGenericElement.cpp:543) ==12247== by 0x58E3F5E: nsHTMLTableRowElement::InsertCell(int, nsIDOMHTMLElement**) (nsHTMLTableRowElement.cpp:314) ==12247== by 0x5C092B4: nsIDOMHTMLTableRowElement_InsertCell(JSContext*, unsigned int, long*) (dom_quickstubs.cpp:19207) ==12247== by 0x69FFED5: js_Interpret (jsops.cpp:2145) ==12247== by 0x6A09AB6: bool Invoke<int (*)(JSContext*, JSObject*, unsigned int, long*, long*)>(JSContext*, JSFunction*, JSScript*, int (*)(JSContext*, JSObject*, unsigned int, long*, long*), js::InvokeArgsGuard const&, unsigned int) (jsinterp.cpp:602) ==12247== by 0x6A09EEB: js_Invoke (jsinterp.cpp:693) ==12247== by 0x6A0A821: js_InternalInvoke (jsinterp.cpp:739) ==12247== by 0x69ABEB9: JS_CallFunctionValue (jsapi.cpp:4850) ==12247== by 0x596FF03: nsJSContext::CallEventHandler(nsISupports*, void*, void*, nsIArray*, nsIVariant**) (nsJSEnvironment.cpp:2224) ==12247== by 0x59AB65C: nsJSEventListener::HandleEvent(nsIDOMEvent*) (nsJSEventListener.cpp:228) ==12247== by 0x58879C4: nsEventListenerManager::HandleEventSubType(nsListenerStruct*, nsIDOMEventListener*, nsIDOMEvent*, nsPIDOMEventTarget*, unsigned int, nsCxPusher*) (nsEventListenerManager.cpp:1094) ==12247== Address 0x30 is not stack'd, malloc'd or (recently) free'd

This probably isn't worth messing with for 4.3, since 4.5 will surely discover new strict aliasing violations. Marking as depends on gcc 4.5.

...especially since the strict aliasing engine has been rewritten for 4.5. http://gcc.gnu.org/gcc-4.5/changes.html

Here's a pretty good reference on strict aliasing: http://replay.web.archive.org/20090421025015/http://www.cellperformance.com/mike_acton/2006/06/understanding_strict_aliasing.html?

Talos numbers are coming in for Mac: http://goo.gl/G1Yqt Right now, looks like a marginal win on mac 64-bit, but I'll report back once all the runs finish.

(In reply to comment #17) > Here's a pretty good reference on strict aliasing: > > http://replay.web.archive.org/20090421025015/http://www.cellperformance.com/mike_acton/2006/06/understanding_strict_aliasing.html? Very interesting to read. :-) In the summary it says: "Be wary of code that requires the use of -fno-strict-aliasing (turns off strict aliasing at any level) in order to work. This is a very good indication that the code relies on aliased memory access and is likely to be dominated by poor memory access patterns." ^^

Keep in mind that the article focuses on the cell processor, which has different properties than x86 or ARM processors.

Full results (comparing four runs of the try push to 4 m-c builds) didn't push the Dromaeo needle too much. There's a 2.3% improvement on Dromaeo V8 on Mac64 that looks like it might be statistically significant, but that's about it. We might see bigger effects on Linux, since we're using a newer version of GCC and since 4.5 has a new strict aliasing engine (whatever that means). At this point, I think the question is: Are we willing to take this change, which adds some ugliness in place of reinterpret_casts and which will cause builds to fail when strict aliasing is violated, in exchange for a marginal performance benefit restricted to Mac, Linux, and Android?

Is there a correctness hazard if we don't fix this bug, ignoring small perf win? /be

(In reply to comment #21) > Full results (comparing four runs of the try push to 4 m-c builds) didn't push > the Dromaeo needle too much. There's a 2.3% improvement on Dromaeo V8 on Mac64 > that looks like it might be statistically significant, but that's about it. Whats about the improvement if we use the newer Clang instead of the older Apple GCC on Mac? Does Clang handle aliasing different than GCC?

Clang has type based alias analysis too. They don't get used as much as in current GCCs and it gives preference to the "points to analysis". In summary, if gcc 4.5 is happy, clang will be happy too.

(In reply to comment #22) > Is there a correctness hazard if we don't fix this bug, ignoring small perf > win? To the contrary, enabling -fstrict-aliasing caries a correctness hazard. The compiler tries to give warnings where its assumptions are violated, but it uses a heuristic. Hopefully the warnings plus our tests would find all the outstanding issues, but of course we have no evidence as to whether or not that's true. I don't know of any correctness issues associated with leaving strict aliasing off.

Here's the latest patch. It passes the tests on MacOS and compiles with GCC 4.5 on Linux. I don't think it's going to run without crashing on Linux, however. We'll see when try comes back.

Latest patch crashes on startup on Linux64 dereferencing a null pointer. #0 0x00007ffff622106d in nsScriptNameSpaceManager::FillHashWithDOMInterfaces (this=0x7fffe10c3ac0) at /build/jlebar/moz/ff-1/src/dom/base/nsScriptNameSpaceManager.cpp:259 #1 0x00007ffff6222c62 in nsScriptNameSpaceManager::Init (this=0x7fffe10c3ac0) at /build/jlebar/moz/ff-1/src/dom/base/nsScriptNameSpaceManager.cpp:413 #2 0x00007ffff61d21df in nsJSRuntime::GetNameSpaceManager () at /build/jlebar/moz/ff-1/src/dom/base/nsJSEnvironment.cpp:3836 #3 0x00007ffff6210ae2 in nsDOMClassInfo::Init () at /build/jlebar/moz/ff-1/src/dom/base/nsDOMClassInfo.cpp:2346 #4 0x00007ffff6215fb8 in NS_GetDOMClassInfoInstance (aID=<value optimized out>) at /build/jlebar/moz/ff-1/src/dom/base/nsDOMClassInfo.cpp:4980 #5 0x00007ffff5f93dc3 in nsDOMParser::QueryInterface (this=<value optimized out>, aIID=<value optimized out>, aInstancePtr=0x7fffffff7840) at /build/jlebar/moz/ff-1/src/content/base/src/nsDOMParser.cpp:133 #6 0x00007ffff6ac37d0 in operator() (this=0x7fffffff7bb0, qi=<value optimized out>, iid=<value optimized out>) at /build/jlebar/moz/ff-1/release-gcc-4.5/xpcom/build/nsCOMPtr.cpp:47 #7 nsCOMPtr_base::assign_from_qi (this=0x7fffffff7bb0, qi=<value optimized out>, iid=<value optimized out>) at /build/jlebar/moz/ff-1/release-gcc-4.5/xpcom/build/nsCOMPtr.cpp:96 #8 0x00007ffff65c1ae1 in operator= (ccx=..., helper=..., Scope=0x7fffe1ebb480, Interface=0x7fffe10c3a60, isGlobal=0, resultWrapper=0x7fffffff7ae0) at ../../../../dist/include/nsCOMPtr.h:681 #9 GetClassInfo (ccx=..., helper=..., Scope=0x7fffe1ebb480, Interface=0x7fffe10c3a60, isGlobal=0, resultWrapper=0x7fffffff7ae0) at /build/jlebar/moz/ff-1/src/js/src/xpconnect/src/xpcprivate.h:3128 #10 XPCWrappedNative::GetNewOrUsed (ccx=..., helper=..., Scope=0x7fffe1ebb480, Interface=0x7fffe10c3a60, isGlobal=0, resultWrapper=0x7fffffff7ae0) at /build/jlebar/moz/ff-1/src/js/src/xpconnect/src/xpcwrappednative.cpp:436 #11 0x00007ffff659ff17 in XPCConvert::NativeInterface2JSObject (lccx=..., d=0x7fffe2cff698, dest=0x0, aHelper=..., iid=0x7fffe6035600, Interface=0x0, allowNativeWrapper=1, isGlobal=0, pErr=0x7fffffff7be0) at /build/jlebar/moz/ff-1/src/js/src/xpconnect/src/xpcconvert.cpp:1283 #12 0x00007ffff658c456 in NativeInterface2JSObject (lccx=..., aScope=0x7fffe1c90528, aCOMObj=0x7fffe10cfb30, aCache=0x0, aIID=0x7fffe6035600, aAllowWrapping=<value optimized out>, aVal=0x7fffe2cff698, aHolder=0x0) at /build/jlebar/moz/ff-1/src/js/src/xpconnect/src/nsXPConnect.cpp:1266 #13 0x00007ffff658c624 in nsXPConnect::WrapNativeToJSVal (this=<value optimized out>, aJSContext=<value optimized out>, aScope=<value optimized out>, aCOMObj=<value optimized out>, aCache=<value optimized out>, aIID=<value optimized out>, aAllowWrapping=1, aVal=0x7fffe2cff698, aHolder=0x0) at /build/jlebar/moz/ff-1/src/js/src/xpconnect/src/nsXPConnect.cpp:1322 #14 0x00007ffff65aab99 in nsJSCID::CreateInstance (this=0x7fffe10cba10, _retval=<value optimized out>) at /build/jlebar/moz/ff-1/src/js/src/xpconnect/src/xpcjsid.cpp:776 #15 0x00007ffff6b2799a in NS_InvokeByIndex_P (that=<value optimized out>, methodIndex=<value optimized out>, paramCount=<value optimized out>, params=<value optimized out>) at /build/jlebar/moz/ff-1/src/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:195 #16 0x00007ffff65c0cad in Invoke (ccx=<value optimized out>, mode=<value optimized out>) at /build/jlebar/moz/ff-1/src/js/src/xpconnect/src/xpcwrappednative.cpp:3141 #17 Call (ccx=<value optimized out>, mode=<value optimized out>) at /build/jlebar/moz/ff-1/src/js/src/xpconnect/src/xpcwrappednative.cpp:2407 and JS stack trace: 0 SRCH_ENG_initFromFile() ["file:///build/jlebar/moz/ff-1/release-gcc-4.5/dist/bin/components/nsSearchService.js":1067] bytes = undefined binaryInStream = undefined doc = undefined domParser = undefined fileInStream = [xpconnect wrapped (nsISupports, nsIInputStream, nsIFileInputStream, nsISeekableStream, nsILineInputStream)] this = [object Object] 1 SRCH_SVC__loadEnginesFromDir(aDir = [xpconnect wrapped nsIFile], 0, [xpconnect wrapped nsIFile]) ["file:///build/jlebar/moz/ff-1/release-gcc-4.5/dist/bin/components/nsSearchService.js":2748] icon = undefined addedEngine = [object Object] dataType = 1 isWritable = false fileExtension = "xml" fileURL = [xpconnect wrapped (nsISupports, nsIURI, nsIURL)] file = [xpconnect wrapped (nsISupports, nsIFile, nsILocalFile)] files = [xpconnect wrapped (nsISupports, nsISimpleEnumerator, nsIDirectoryEnumerator)] isInProfile = false this = [object Object] 2 SRCH_SVC__loadEngines() ["file:///build/jlebar/moz/ff-1/release-gcc-4.5/dist/bin/components/nsSearchService.js":2575] rebuildCache = true cachePaths = /build/jlebar/moz/ff-1/valgrind/dist/bin/searchplugins buildID = "20110428131031" notInToLoad = [function] modifiedDir = [function] toLoad = [xpconnect wrapped nsIFile] chromeFiles = chromeURIs = loadFromJARs = false locations = [xpconnect wrapped nsISimpleEnumerator] loadDirs = [xpconnect wrapped nsIFile] cacheEnabled = true cache = [object Object] this = [object Object] 3 SearchService() ["file:///build/jlebar/moz/ff-1/release-gcc-4.5/dist/bin/components/nsSearchService.js":2416] this = [object Object] 4 anonymous(iid = {8307b8f2-08ea-45b8-96bf-b1dc7688fe3b}, outer = null) ["resource://gre/modules/XPCOMUtils.jsm":256] this = [object Object] 5 XPCU_serviceLambda() ["resource://gre/modules/XPCOMUtils.jsm":187] this = [object Object] 6 anonymous() ["resource://gre/modules/XPCOMUtils.jsm":165] this = [object Object] 7 AHU_loadDefaultSearchEngine() ["file:///build/jlebar/moz/ff-1/release-gcc-4.5/dist/bin/components/nsBrowserContentHandler.js":839] engine = undefined submission = undefined defaultEngine = undefined this = [object Object] 8 anonymous() ["file:///build/jlebar/moz/ff-1/release-gcc-4.5/dist/bin/components/nsBrowserContentHandler.js":542] choice = undefined startPage = undefined ss = undefined haveUpdateSession = false overridePage = "" prefb = [xpconnect wrapped nsIPrefBranch] this = [object Object] 9 dch_handle(cmdLine = [xpconnect wrapped nsICommandLine]) ["file:///build/jlebar/moz/ff-1/release-gcc-4.5/dist/bin/components/nsBrowserContentHandler.js":818] URLlist = undefined curarg = undefined i = 0 count = 0 uri = undefined found = undefined ar = null urilist = this = [object Object]

I spent a while looking at this today. The crash goes away and FF starts successfully if I compile xptiInterfaceInfoManager.cpp without strict aliasing. But unlike the other places where I've had to do this, this file doesn't have any obvious strict aliasing violations. GCC is re-ordering a load and store in xptiInterfaceInfoManager::ArrayAppender and ArrayPrefixAppender; these two functions are the only ones which generate different machine code with and without strict aliasing. I think this has to do with the EntryToInfo function, which gets inlined. It looks like maybe GCC is getting confused by the static cast EntryToInfo does before it returns and the getter_AddRefs in the caller, although I haven't been able to find the strict aliasing violation. In any case, I'm going to disable strict aliasing for this file and push to try to see if Linux64 works with this one change. Linux32 appeared to work fine with the last patch I pushed. We can then look at the numbers and decide whether to keep going with this.

Awesome work, jlebar. Can't wait to hear the Linux outcome. /be

That change was enough to get the Linux-64 builds to pass all the unit tests. Here are the Talos results: http://goo.gl/PxwYJ It's hard to draw strong conclusions, given that there's a lot of variance in the data and that our statistical analysis is not particularly sophisticated. (I ran Talos 7 times and am comparing against as many runs from trunk, but I'm still not sure that's sufficient.) It's also easy to fall into the green jelly beans fallacy [1] when looking at the results. There's a marked improvement in tp4 (2.5% on Linux-32), and perhaps a sunspider improvement (2% on Linux-32 sunspider nochrome and dromaeo_sunspider). It also looks like there's a ts_places_generated_* improvement. But again, jelly beans... I'm pretty sure this doesn't make things slower -- the only mean which moved substantially in the wrong direction was dromaeo DOM on Linux-32, and I think (hope?) we can chalk that up to variance. The one result that seems unquestionably significant is that the variance in the tryserver Talos results is far lower than in the m-c results. This indicates that *something* is different between the two sets of numbers. It's hard to have a lot of confidence in the data until we understand what that is causing this difference. For the purposes of this bug, I think this data indicates that we probably want to take this change. There's some green and no red in the compare talos page, and certainly the general trend of the data reflects an increase in speed. There's a correctness hazard associated with strict aliasing, but I've encountered just a single gotcha in my testing, so it seems unlikely that there are many more invalid optimizations lurking which don't cause test failures. [1] http://xkcd.com/882/

Thanks Justin! What is the impact on code size? I would expect it to get a bit smaller.

I've filed bug 653961 on the Talos variance difference between m-c and try.

(In reply to comment #31) > What is the impact on code size? I would expect it to get a bit smaller. Doesn't look like a significant difference in code size. Builds are at [1] if you want to have a look. [1] http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-9aa7999b4cfa/try-linux/

I want to spend some more time trying to understand the xptiInterfaceInfoManager crash before putting this up for review. But that's going to need to wait a week or two until I receive a new Linux box.

The crash also goes away if I mark EntryToInfo with __attribute__((noinline)).

With EntryToInfo fixed, it now crashes in nsINode::doInsertChildAt. I'm starting to think the correctness hazard here isn't worth the potential speedup. But I'll bisect to see what's going on.

The first bad revision is: changeset: 69015:3abe2f8c592e user: Justin Lebar <jlebar@mozilla.com> date: Thu Apr 28 15:49:16 2011 +0200 summary: Bug 590181 part 2 - Switch default gcc optimize options to -O3. r=ted. Ugh.

Realistically, this is WONTFIX. Life is too short to figure out why -O3 breaks -fstrict-aliasing.

FWIW, I just stumbled across this: http://patrakov.blogspot.com.au/2009/03/dont-use-old-dtoac.html It explains how dtoa.c violates strict aliasing rules, and how to fix it.