Closed Bug 1029245 (gcc-4.9) Opened 10 years ago Closed 8 years ago

Update linux builds to use GCC 4.9

Categories

(Firefox Build System :: General, defect)

All
Linux
defect
Not set
normal

Tracking

(firefox53 fixed)

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: glandium, Assigned: froydnj)

References

Details

Attachments

(3 files, 3 obsolete files)

At some point, we'll want to be doing this, although i'm not sure we want to do it right now. Just filing because I had patches ready for other reasons.
Attached patch Switch linux builds to GCC 4.9 (obsolete) — Splinter Review
Assignee: nobody → mh+mozilla
Status: NEW → ASSIGNED
Depends on: 1029325
Depends on: 1029346
Attached patch Switch linux builds to GCC 4.9.1 (obsolete) — Splinter Review
Attachment #8444829 - Attachment is obsolete: true
I won't pretend I'm going to do the final work to get this deployed (involving checking talos results) I'll point out that there are two other tooltool manifests pointing at a different gcc tarball than the one we currently use for gcc 4.7 that would also need investigating: js/src/devtools/rootAnalysis/build/gcc.manifest js/src/devtools/rootAnalysis/build/gcc-b2g.manifest Here is a try build with the patch I just attached: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6fe6e035ad06
Assignee: mh+mozilla → nobody
(In reply to Mike Hommey [:glandium] from comment #3) > I won't pretend I'm going to do the final work to get this deployed > (involving checking talos results) me either, ENOTIME > I'll point out that there are two other tooltool manifests pointing at a > different gcc tarball than the one we currently use for gcc 4.7 that would > also need investigating: > js/src/devtools/rootAnalysis/build/gcc.manifest > js/src/devtools/rootAnalysis/build/gcc-b2g.manifest I'd guess those are for the static analysis plugin thing root analysis builds use, you can ask sfink, but my guess is we probably don't need a separate build at this point.
(In reply to Trevor Saunders (:tbsaunde) from comment #4) > (In reply to Mike Hommey [:glandium] from comment #3) > > I won't pretend I'm going to do the final work to get this deployed > > (involving checking talos results) > > me either, ENOTIME > > > I'll point out that there are two other tooltool manifests pointing at a > > different gcc tarball than the one we currently use for gcc 4.7 that would > > also need investigating: > > js/src/devtools/rootAnalysis/build/gcc.manifest > > js/src/devtools/rootAnalysis/build/gcc-b2g.manifest > > I'd guess those are for the static analysis plugin thing root analysis > builds use, you can ask sfink, but my guess is we probably don't need a > separate build at this point. Maybe. I think the original reason for a separate hazard gcc package was something dumb, like the gcc tarball we were using did not include the plugin headers, and when I tried to compile with the headers pulled from somewhere else the resulting plugin didn't get along with gcc, so I gave up and packaged my own gcc (with headers) and compiled the plugin against that. I'm hoping to be able to use the same package the next time around. And it looks like that may have to be now, due to bug 1171059. So the hazard builds may end up being the first to use gcc 4.9.
Depends on: 1172109
For the record, I have the browser and shell hazards builds working with gcc 4.9 (it required a number of additional fixes, and exposed some latent bugs.) I will switch those builds over as soon as I get review on the prerequisite fix. Deploying that will also fix bug 1162263 for the browser build, which is the real reason I'm doing anything here. I still have to track down a gcc 4.9 for b2g. Bug 1087161 makes it sound easy. I also pushed https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a9f856e0c95 to get talos numbers, though that accidentally included some more of my patches. But I wouldn't expect them to affect timings much at all. (Breaking things completely would not be unexpected, but timings should be ok if it works at all.)
Note we should at least update to 4.9.2 (current tooltool package is 4.9.1), although I'm tempted to go with 5.1.
I'd take anything above 4.8, so we can call static member functions from lambdas without gcc thinking it needs a this pointer for that [1]. Ran into this last week (workaround is a function-scope stub). [1] http://stackoverflow.com/questions/4940259/lambdas-require-capturing-this-to-call-static-member-function
I mean file-scope stub.
Hum, there is a difference between upgrading the compiler we use an upgrading the compiler we *require*. Upgrading the compiler we require is out of scope.
Thanks, yes that makes sense. I've filed Bug 1175546 instead for that.
This bug doesn't depend on bug 1175546, even though gcc 4.8 < 4.9.
No longer depends on: gcc-4.8
Depends on: 1243331
I built GCC 4.9.3 + binutils 2.25.1 on taskcluster (yay) and uploaded it to tooltool. Here is the manifest: [ { "size": 102421980, "visibility": "public", "digest": "f25292aa93dc449e0472eee511c0ac15b5f1a4272ab76cf53ce5d20dc57f29e83da49ae1a9d9e994192647f75e13ae60f75ba2ac3cb9d26d5f5d6cabf88de921", "algorithm": "sha512", "filename": "gcc.tar.xz" } ] It was built from: https://hg.mozilla.org/try/rev/3749ee59d09a
Depends on: 1246772
Attached patch switch to gcc 4.9 (obsolete) — Splinter Review
I believe all the build and test issues are fixed at this point, though I'm still waiting on a pgo try run. Without pgo this seems to significantly improve several talos tests, and only regresses canvas mark. I talked to jrmuizel about canvas mark a little, and it seems like we're probably willing to take that regression since its linux and we don't have other great options. However I'll try and look into that a little to see if there is anything easy we can do. The actual talos numbers are at https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=93591c034042 and the numbers with pgo should come in at https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=1c01c46ff6fc
Attachment #8718710 - Flags: review?(mh+mozilla)
> and the numbers with pgo should come in at > https://treeherder.mozilla.org/perf.html#/ > comparechooser?newProject=try&newRevision=1c01c46ff6fc It looks like with pgo canvas mark is improved, but a couple tests that improved without pgo got worse with it. Jeff semes to think the giant xres regression is not real since that test shouldn't be effected by the compiler.
Comment on attachment 8718710 [details] [diff] [review] switch to gcc 4.9 Review of attachment 8718710 [details] [diff] [review]: ----------------------------------------------------------------- Doing this essentially means we'd likely end up breaking builds with gcc 4.8, which is the version we've settled on being okay upgrading to. I'd be okay building released desktop builds if some of non released builds were separately upgraded to 4.8.
Attachment #8718710 - Flags: review?(mh+mozilla) → review-
(In reply to Mike Hommey [:glandium] from comment #16) > Comment on attachment 8718710 [details] [diff] [review] > switch to gcc 4.9 > > Review of attachment 8718710 [details] [diff] [review]: > ----------------------------------------------------------------- > > Doing this essentially means we'd likely end up breaking builds with gcc > 4.8, which is the version we've settled on being okay upgrading to. I'd be > okay building released desktop builds if some of non released builds were > separately upgraded to 4.8. I don't think this would be the first time we supported building with a older gcc than we used in automation, but I'll agree if we want to support 4.8 using it for a build would be useful. Any preference on which build? I would think either 32 or 64 bit debug is easiest, but I'm not sure which I think is better.
Come to think of it, if we do this, we might as well skip 4.9. So I'd rather a) upgrade everything to 4.8, because that's what we said we'd do (there's a separate bug for that) and b) upgrade cherry-picked builds to 5.3 (which would require making some build types using different tooltool manifests)
FWIW, after we requires VS2015, upgrading GCC to a newer version would give us many C++14 new features.
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #19) > FWIW, after we requires VS2015, upgrading GCC to a newer version would give > us many C++14 new features. see comment 10.
upgrading to gcc 4.9 could help with bug 1308685 and bug 1323531. I know there is not a lot of recent action on this bug and there was talk of gcc 5.3- I am open to testing bug 1323531 with 5.3 if there is an easy way to do that via taskcluster.
I am testing GCC 4.9 builds; using 4.9.4 looks pretty good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=377d97ac6cd968477ed8de9961231ded8ba20eb4 The PGO failure is being addressed with the patch from PR 59448: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59448 and the new GCC with that patch included is being built here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fce2b8610f6a770305ef004e606cf194b5625d21 Once that latter job gets done, I will redo the PGO builds with the new GCC.
Assignee: nobody → nfroyd
Still hitting taskcluster PGO build failures, even with a patched compiler: https://treeherder.mozilla.org/#/jobs?repo=try&revision=53c74c50c0e64d992e69b867bd30bf234c251559 I've seen some evidence searching that this is related to inlining settings, which isn't particularly encouraging. Cranking the -O level on the relevant Skia file might be helpful, as I think Skia essentially assumes you're compiling with -O3. Or we could patch the Skia file to explicitly provide all the necessary arguments, which might satisfy whatever checker is running there...
PGO builds are built with -O3... except the PGO process itself switches cold code to -Os... Tweaking -O flags on some files is not going to change that...
Building Skia inside of mozilla-central with GCC 4.9.4 causes problems: [...]c++/4.9.4/bits/atomic_base.h:581:70: error: failure memory model cannot be stronger than success memory model for '__atomic_compare_exchange' The error stack accompanying this message points at SkEventTracer::GetInstance: SkEventTracer* SkEventTracer::GetInstance() { if (SkEventTracer* tracer = sk_atomic_load(&gUserTracer, sk_memory_order_acquire)) { return tracer; } static SkOnce once; static SkDefaultEventTracer* defaultTracer; once([] { defaultTracer = new SkDefaultEventTracer; }); return defaultTracer; } The only place that compare_exchange_strong could be called here is from SkOnce::operator(): template <typename Fn, typename... Args> void operator()(Fn&& fn, Args&&... args) { auto state = fState.load(std::memory_order_acquire); if (state == Done) { return; } // If it looks like no one has started calling fn(), try to claim that job. if (state == NotStarted && fState.compare_exchange_strong(state, Claimed, std::memory_order_relaxed)) { // Great! We'll run fn() then notify the other threads by releasing Done into fState. fn(std::forward<Args>(args)...); return fState.store(Done, std::memory_order_release); } [...code elided...] where |fState| is an atomic<uint8_t>. The three-argument form of atomic<uint8_t>::compare_exchange_strong is defined as: _GLIBCXX_ALWAYS_INLINE bool compare_exchange_strong(__int_type& __i1, __int_type __i2, memory_order __m = memory_order_seq_cst) noexcept { return compare_exchange_strong(__i1, __i2, __m, __cmpexch_failure_order(__m)); } __cmpexch_failure_order relaxes the given memory_order: // Drop release ordering as per [atomics.types.operations.req]/21 constexpr memory_order __cmpexch_failure_order2(memory_order __m) noexcept { return __m == memory_order_acq_rel ? memory_order_acquire : __m == memory_order_release ? memory_order_relaxed : __m; } constexpr memory_order __cmpexch_failure_order(memory_order __m) noexcept { return memory_order(__cmpexch_failure_order2(__m & __memory_order_mask) | (__m & __memory_order_modifier_mask)); } which then gets us to the four-argument version of compare_exchange_strong: _GLIBCXX_ALWAYS_INLINE bool compare_exchange_strong(__int_type& __i1, __int_type __i2, memory_order __m1, memory_order __m2) noexcept { memory_order __b2 = __m2 & __memory_order_mask; memory_order __b1 = __m1 & __memory_order_mask; __glibcxx_assert(__b2 != memory_order_release); __glibcxx_assert(__b2 != memory_order_acq_rel); __glibcxx_assert(__b2 <= __b1); return __atomic_compare_exchange_n(&_M_i, &__i1, __i2, 0, __m1, __m2); } Despite the constexpr annotation on __cmpexch_failure_order and friends, which ought to imply that they get constant-folded, I think what is happening is that GCC doesn't see |memory_order_relaxed| when it examines __m2. Instead, it seems some internal tree representation for the call to __cmpexch_failure_order. Since this is not an integer constant, GCC treats __m2 as being equivalent to memory_order_seq_cst (see gcc/builtins.c:get_memmodel). And since memory_order_seq_cst is stronger than memory_order_relaxed, we get the above error. In any event, the easiest fix is to simply use the four-argument form of compare_exchange_strong directly, explicitly specifying the failure memory order. I gather that we're not supposed to patch Skia locally, but instead submit patches upstream. Lee, is it possible for you to submit this upstream on my behalf? I tried to write as thorough a description of the problem as I could; if it would help bolster the case, I can modify GCC to print out what it thinks __atomic_compare_exchange is receiving...
Attachment #8819312 - Flags: review?(lsalzman)
PR 64905 apparently never got backported to 4.9.x, so we still need the patch for that.
Attachment #8819313 - Flags: review?(mh+mozilla)
Attachment #8819314 - Flags: review?(mh+mozilla)
Attachment #8505280 - Attachment is obsolete: true
Attachment #8718710 - Attachment is obsolete: true
Blocks: 1322792
Attachment #8819312 - Flags: review?(lsalzman) → review+
Attachment #8819313 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8819314 [details] [diff] [review] part 2 - upgrade builds to use GCC 4.9.4 Review of attachment 8819314 [details] [diff] [review]: ----------------------------------------------------------------- I had to dig in your try pushes to find where this comes from.
Attachment #8819314 - Flags: review?(mh+mozilla) → review+
Pushed by nfroyd@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/52741577a76d part 0 - tweak Skia's SkOnce.h header to work around issues with std::atomic::compare_exchange_strong; r=lsalzman https://hg.mozilla.org/integration/mozilla-inbound/rev/ac4575135a2a part 1 - modify build-gcc.sh to build GCC 4.9.4; r=glandium https://hg.mozilla.org/integration/mozilla-inbound/rev/285937702a80 part 2 - upgrade builds to use GCC 4.9.4; r=glandium
we saw some installer size and build time changes with this: == Change summary for alert #4581 (as of December 21 2016 14:28 UTC) == Regressions: 3% installer size summary linux64 pgo 59803787.58 -> 61438271.5 2% installer size summary linux32 pgo 60370321.42 -> 61699738.33 Improvements: 4% build times summary linux64 opt taskcluster-c4.4xlarge valgrind 1466.67 -> 1406.97 4% build times summary linux64 opt taskcluster-m4.4xlarge valgrind 1637.51 -> 1578.6 1% installer size summary linux32 debug 66358308.17 -> 65554985.58 1% installer size summary linux64 debug 67086703.17 -> 66414589.92 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=4581
(In reply to Joel Maher ( :jmaher) from comment #32) > we saw some installer size and build time changes with this: > == Change summary for alert #4581 (as of December 21 2016 14:28 UTC) == > > Regressions: > > 3% installer size summary linux64 pgo 59803787.58 -> 61438271.5 > 2% installer size summary linux32 pgo 60370321.42 -> 61699738.33 That's...a lot of extra code. More aggressive inlining? Better profile data?
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: