1029245 - (gcc-4.9) Update linux builds to use GCC 4.9

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

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.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Switch linux builds to GCC 4.9

Comment 2

[Mike Hommey [:glandium]]

Attachment: Switch linux builds to GCC 4.9.1

Comment 3

[Mike Hommey [:glandium]]

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

Comment 4

[Trevor Saunders (:tbsaunde)]

(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

[Steve Fink [:sfink] [:s:]]

(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

[Steve Fink [:sfink] [:s:]]

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.)

Comment 7

[Mike Hommey [:glandium]]

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

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

I mean file-scope stub.

Comment 10

[Mike Hommey [:glandium]]

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

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Thanks, yes that makes sense. I've filed Bug 1175546 instead for that.

Comment 12

[Steve Fink [:sfink] [:s:]]

This bug doesn't depend on bug 1175546, even though gcc 4.8 < 4.9.

Comment 13

[Mike Hommey [:glandium]]

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

[Trevor Saunders (:tbsaunde)]

Attachment: switch to gcc 4.9

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

Comment 15

[Trevor Saunders (:tbsaunde)]

> 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 16

[Mike Hommey [:glandium]]

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.

Comment 17

[Trevor Saunders (:tbsaunde)]

(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.

Comment 18

[Mike Hommey [:glandium]]

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

[Xidorn Quan [:xidorn] UTC+11]

FWIW, after we requires VS2015, upgrading GCC to a newer version would give us many C++14 new features.

Comment 20

[Mike Hommey [:glandium]]

(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

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 22

[Nathan Froyd [:froydnj]]

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.

Comment 23

[Nathan Froyd [:froydnj]]

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...

Comment 24

[Mike Hommey [:glandium]]

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...

Comment 25

[Nathan Froyd [:froydnj]]

Attachment: part 0 - tweak Skia's SkOnce.h header to work around issues with std::atomic::compare_exchange_strong

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...

Comment 26

[Nathan Froyd [:froydnj]]

Attachment: part 1 - modify build-gcc.sh to build GCC 4.9.4

PR 64905 apparently never got backported to 4.9.x, so we still need the
patch for that.

Comment 27

[Nathan Froyd [:froydnj]]

Attachment: part 2 - upgrade builds to use GCC 4.9.4

Comment 28

[Lee Salzman [:lsalzman]]

Upstream Skia bug report: https://skia-review.googlesource.com/c/6200/

Comment 29

[Mike Hommey [:glandium]]

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.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/52741577a76d
https://hg.mozilla.org/mozilla-central/rev/ac4575135a2a
https://hg.mozilla.org/mozilla-central/rev/285937702a80

Comment 32

[Joel Maher ( :jmaher ) (UTC -8)]

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

Comment 33

[Nathan Froyd [:froydnj]]

(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?