1460791 - -O2 breaks MinGW x86 Opt Build

Status: RESOLVED FIXED

(Firefox Build System :: General: Unsupported Platforms, defect, P5)

Description

[Tom Ritter [:tjr]]

Using esr60 branch:

Broken: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ec676206e34194a177452e42570e2b21013e7a5
Fixed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e9c3d6495bef5c45c21d5a94d7506782d70607a&selectedJob=177975360

> In file included from /builds/worker/workspace/build/src/obj-firefox/dist/include/nsISupportsUtils.h:14:0,
>                  from /builds/worker/workspace/build/src/obj-firefox/dist/include/nsISupports.h:77,
>                  from /builds/worker/workspace/build/src/obj-firefox/dist/include/nsCycleCollectionTraversalCallback.h:12,
>                  from /builds/worker/workspace/build/src/obj-firefox/dist/include/nsCycleCollectionNoteChild.h:13,
>                  from /builds/worker/workspace/build/src/obj-firefox/dist/include/nsTArray.h:28,
>                  from /builds/worker/workspace/build/src/obj-firefox/dist/include/gfxPlatform.h:11,
>                  from /builds/worker/workspace/build/src/obj-firefox/dist/include/gfx2DGlue.h:10,
>                  from /builds/worker/workspace/build/src/image/Image.h:12,
>                  from /builds/worker/workspace/build/src/image/ImageWrapper.h:10,
>                  from /builds/worker/workspace/build/src/image/ImageWrapper.cpp:6,
>                  from /builds/worker/workspace/build/src/obj-firefox/image/Unified_cpp_image1.cpp:2:
> /builds/worker/workspace/build/src/image/SurfaceCache.cpp: In member function 'MozExternalRefCountType mozilla::image::CachedSurface::Release()':
> /builds/worker/workspace/build/src/obj-firefox/dist/include/nsISupportsImpl.h:603:34: error: MozExternalRefCountType mozilla::image::CachedSurface::Release() causes a section type conflict with MozExternalRefCountType mozilla::image::CachedSurface::[e-mail].408()
>    _decl(MozExternalRefCountType) Release(void) __VA_ARGS__ {                  \
>                                   ^
> /builds/worker/workspace/build/src/obj-firefox/dist/include/nsISupportsImpl.h:627:1: note: in expansion of macro 'NS_INLINE_DECL_THREADSAFE_REFCOUNTING_META'
>  NS_INLINE_DECL_THREADSAFE_REFCOUNTING_META(_class, NS_METHOD_, __VA_ARGS__)
>  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> /builds/worker/workspace/build/src/image/SurfaceCache.cpp:128:3: note: in expansion of macro 'NS_INLINE_DECL_THREADSAFE_REFCOUNTING'
>    NS_INLINE_DECL_THREADSAFE_REFCOUNTING(CachedSurface)
>    ^
> /builds/worker/workspace/build/src/obj-firefox/dist/include/nsISupportsImpl.h:603:34: note: 'MozExternalRefCountType mozilla::image::CachedSurface::[e-mail].408()' was declared here
>    _decl(MozExternalRefCountType) Release(void) __VA_ARGS__ {                  \
>                                   ^
> /builds/worker/workspace/build/src/obj-firefox/dist/include/nsISupportsImpl.h:627:1: note: in expansion of macro 'NS_INLINE_DECL_THREADSAFE_REFCOUNTING_META'
>  NS_INLINE_DECL_THREADSAFE_REFCOUNTING_META(_class, NS_METHOD_, __VA_ARGS__)
>  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> /builds/worker/workspace/build/src/image/SurfaceCache.cpp:128:3: note: in expansion of macro 'NS_INLINE_DECL_THREADSAFE_REFCOUNTING'
>    NS_INLINE_DECL_THREADSAFE_REFCOUNTING(CachedSurface)
>    ^

Comment 1

[Tom Ritter [:tjr]]

Probably going to put in a push/pop, but the other option is to just downgrade us to -O1.

Comment 2

[Tom Ritter [:tjr]]

A push/pop compiled; but causes a runtime crash. No useful stack.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2281cbec8e876e0a06f46f1fbef1b452aa141d35

Comment 3

[Tom Ritter [:tjr]]

This still affects mingw-clang. Upstream bug is https://bugs.llvm.org/show_bug.cgi?id=41630

Comment 4

[(Away)]

mingw-clang still has separate toolchains for 32 and 64 bit builds, right? Is it just that the 64 bit one has more address space so it doesn't hit this?

Comment 5

[(Away)]

I'd still be curious to hear about comment 4.

Anyway, a global -O1 is a pretty big hammer. Can we set flags on individual files in moz.build instead? I guess it would be equivalent to push/pop. I would have been interested in debugging the crash from that but the build has aged out.

Comment 6

[Tom Ritter [:tjr]]

Attachment: Bug 1460791 - Turn on -O2 for MinGW as it seems the bug causing problems is fixed r?dmajor

Comment 7

[Tom Ritter [:tjr]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6dcdf1139672a2c48efa0f36dccff1821ac2dab7

Comment 8

[Tom Ritter [:tjr]]

[Tracking Requested - why for this release]: We'd like to backport this for Tor. It only affects MinGW builds.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5cafdf3cf8f9
Turn on -O2 for MinGW as it seems the bug causing problems is fixed r=dmajor

Comment 10

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/5cafdf3cf8f9

Comment 11

[Tom Ritter [:tjr]]

Comment on attachment 9088047 [details]
Bug 1460791 - Turn on -O2 for MinGW as it seems the bug causing problems is fixed r?dmajor

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This will make the browser faster for Tor =)
• User impact if declined: Tor will need to carry the patch
• Fix Landed on Version: 70
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Only affects MinGW build
• String or UUID changes made by this patch:

Comment 12

[Julien Cristau [:jcristau]]

Comment on attachment 9088047 [details]
Bug 1460791 - Turn on -O2 for MinGW as it seems the bug causing problems is fixed r?dmajor

optimize mingw builds, approved for 68.2

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-esr68/rev/1e5260c1256c

Comment 14

[Georg Koppen]

tjr: What actually fixed the underlying issue? Or speaking differently: how do we know we don't hit this issue, when, say, trying to get a chemspill out of the door as quickly as possible?

Comment 15

[Tom Ritter [:tjr]]

(In reply to Georg Koppen from comment #14)

tjr: What actually fixed the underlying issue? Or speaking differently: how do we know we don't hit this issue, when, say, trying to get a chemspill out of the door as quickly as possible?

Hm. Well it also most likely something compiler related. I went and checked the version used in the filed llvm bug and it was clang version 8.0.0 (trunk) (llvm/trunk 348363) and we're currently using clang version 8.0.0 (trunk) (llvm/trunk 348363) .... However a few weeks after I posted that llvm bug I made this commit to put us on the 8 branch. So it seems possible this might come up when we bump to trunk; but when we do that we also have to deal with Bug 1548624