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)
Tracking
(firefox53 fixed)
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: glandium, Assigned: froydnj)
References
Details
Attachments
(3 files, 3 obsolete files)
4.81 KB,
patch
|
lsalzman
:
review+
|
Details | Diff | Splinter Review |
672 bytes,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
9.30 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•10 years ago
|
||
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → mh+mozilla
Status: NEW → ASSIGNED
Reporter | ||
Comment 2•10 years ago
|
||
Reporter | ||
Updated•10 years ago
|
Attachment #8444829 -
Attachment is obsolete: true
Reporter | ||
Comment 3•10 years ago
|
||
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
Comment 4•10 years ago
|
||
(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.
Comment 5•10 years ago
|
||
(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.
Comment 6•10 years ago
|
||
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.)
Reporter | ||
Comment 7•10 years ago
|
||
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.
Comment 8•9 years ago
|
||
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
Comment 9•9 years ago
|
||
I mean file-scope stub.
Reporter | ||
Comment 10•9 years ago
|
||
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.
Comment 11•9 years ago
|
||
Thanks, yes that makes sense. I've filed Bug 1175546 instead for that.
Comment 12•9 years ago
|
||
This bug doesn't depend on bug 1175546, even though gcc 4.8 < 4.9.
No longer depends on: gcc-4.8
Reporter | ||
Comment 13•9 years ago
|
||
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
Comment 14•9 years ago
|
||
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)
Comment 15•9 years ago
|
||
> 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.
Reporter | ||
Comment 16•9 years ago
|
||
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-
Comment 17•9 years ago
|
||
(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.
Reporter | ||
Comment 18•9 years ago
|
||
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)
Comment 19•8 years ago
|
||
FWIW, after we requires VS2015, upgrading GCC to a newer version would give us many C++14 new features.
Reporter | ||
Comment 20•8 years ago
|
||
(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.
Comment 21•8 years ago
|
||
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.
Assignee | ||
Comment 22•8 years ago
|
||
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
Assignee | ||
Comment 23•8 years ago
|
||
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...
Reporter | ||
Comment 24•8 years ago
|
||
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...
Assignee | ||
Comment 25•8 years ago
|
||
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)
Assignee | ||
Comment 26•8 years ago
|
||
PR 64905 apparently never got backported to 4.9.x, so we still need the
patch for that.
Attachment #8819313 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 27•8 years ago
|
||
Attachment #8819314 -
Flags: review?(mh+mozilla)
Assignee | ||
Updated•8 years ago
|
Attachment #8505280 -
Attachment is obsolete: true
Attachment #8718710 -
Attachment is obsolete: true
Updated•8 years ago
|
Attachment #8819312 -
Flags: review?(lsalzman) → review+
Comment 28•8 years ago
|
||
Upstream Skia bug report: https://skia-review.googlesource.com/c/6200/
Reporter | ||
Updated•8 years ago
|
Attachment #8819313 -
Flags: review?(mh+mozilla) → review+
Reporter | ||
Comment 29•8 years ago
|
||
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+
Comment 30•8 years ago
|
||
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
Comment 31•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/52741577a76d
https://hg.mozilla.org/mozilla-central/rev/ac4575135a2a
https://hg.mozilla.org/mozilla-central/rev/285937702a80
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment 32•8 years ago
|
||
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
Assignee | ||
Comment 33•8 years ago
|
||
(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?
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•