Upgrade mulet compiler from gcc 4.7.3 to gcc 4.9.3

RESOLVED FIXED in Firefox 47

Status

()

Core
JavaScript: GC
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: sfink, Assigned: sfink)

Tracking

(Blocks: 1 bug)

unspecified
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(2 attachments, 5 obsolete attachments)

The hazard analysis would really like to move to using the same compiler as other builds, and my latest version of the plugin really needs gcc 4.9, so it seems like a good time to upgrade mulet to gcc 4.9 for the b2g hazard builds at least.
Created attachment 8712454 [details] [diff] [review]
Upgrade mulet compiler from gcc 4.7.3 to gcc 4.9.1

I have a very ugly try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=745bab0a287c that should say whether this horribly breaks anything. So far, it has only gotten to failing everything Windows because of the new version of pip (which I really thought I had rebased onto the fix for, but perhaps that's another tree.)

I guess I'll need to either upgrade the browser to 4.9 as well, or continue using my own compiler for the browser hazard job.
Attachment #8712454 - Flags: review?(mh+mozilla)
Blocks: 1243231
Comment on attachment 8712454 [details] [diff] [review]
Upgrade mulet compiler from gcc 4.7.3 to gcc 4.9.1

Review of attachment 8712454 [details] [diff] [review]:
-----------------------------------------------------------------

Please use at least GCC 4.9.3. See bug 1029245.
Attachment #8712454 - Flags: review?(mh+mozilla) → review-
Created attachment 8713762 [details] [diff] [review]
Test ALLOW_COMPILER_WARNINGS instead of WARNINGS_AS_ERRORS, and move it to after it is set

This is a fix to NSS that I need in order to upgrade to gcc 4.9.3. It's really a bugfix for the change from bug 1216318, which glandium pointed out was incorrect.
Attachment #8713762 - Flags: review?(mh+mozilla)
Created attachment 8713763 [details] [diff] [review]
Upgrade mulet compiler from gcc 4.7.3 to gcc 4.9.3

Actually do the upgrade. I haven't done a full try run on this latest set of patches yet, but an earlier one with more hackish stuff was green: https://treeherder.mozilla.org/#/jobs?repo=try&author=sfink%40mozilla.com
Attachment #8713763 - Flags: review?(mh+mozilla)
Attachment #8712454 - Attachment is obsolete: true
Summary: Upgrade mulet compiler from gcc 4.7.3 to gcc 4.9.1 → Upgrade mulet compiler from gcc 4.7.3 to gcc 4.9.3
Comment on attachment 8713762 [details] [diff] [review]
Test ALLOW_COMPILER_WARNINGS instead of WARNINGS_AS_ERRORS, and move it to after it is set

Review of attachment 8713762 [details] [diff] [review]:
-----------------------------------------------------------------

::: config/external/nss/Makefile.in
@@ +340,5 @@
>  
>  include $(topsrcdir)/config/rules.mk
>  
> +ifeq ($(OS_TARGET),Android)
> +ALLOW_COMPILER_WARNINGS := 1

This is already set in moz.build.
Attachment #8713762 - Flags: review?(mh+mozilla) → feedback+
Comment on attachment 8713763 [details] [diff] [review]
Upgrade mulet compiler from gcc 4.7.3 to gcc 4.9.3

Review of attachment 8713763 [details] [diff] [review]:
-----------------------------------------------------------------

::: b2g/dev/config/tooltool-manifests/linux64/releng.manifest
@@ +40,5 @@
> +{
> +"size": 2552852,
> +"digest": "299ab5058f060610a962d3379338f27a63161435909f4fa5559711a18d274f39aa9e48d1be993ea6eb94cd6f566e817f84f799d783b8f3aa98482e62f785f0c1",
> +"algorithm": "sha512",
> +"filename": "sixgill.tar.xz",

How is this built?
Attachment #8713763 - Flags: review?(mh+mozilla)

Updated

2 years ago
Duplicate of this bug: 1246549

Updated

2 years ago
Blocks: 752004
(In reply to Mike Hommey [:glandium] from comment #6)
> Comment on attachment 8713763 [details] [diff] [review]
> Upgrade mulet compiler from gcc 4.7.3 to gcc 4.9.3
> 
> Review of attachment 8713763 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: b2g/dev/config/tooltool-manifests/linux64/releng.manifest
> > +"filename": "sixgill.tar.xz",
> 
> How is this built?

With a script included in my fork of the sixgill repo. I just cleaned up my patch series and uploaded it all to http://hg.mozilla.org/users/sfink_mozilla.com/sixgill

I maintain the sixgill used in the hazard analysis. Sixgill is from Brian Hackett <bhackett@mozilla.com> and I have him review the stuff I am unsure of. Mostly, I try to keep it more or less working with gcc changes and occasional breakage from new constructs used in gecko code.
Created attachment 8717247 [details] [diff] [review]
Test ALLOW_COMPILER_WARNINGS instead of WARNINGS_AS_ERRORS, and move it to after it is set

Good, that makes it nicer.
Attachment #8717247 - Flags: review?(mh+mozilla)
Attachment #8713762 - Attachment is obsolete: true
Attachment #8717247 - Flags: review?(mh+mozilla) → review+

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9f832c0a9f88
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Created attachment 8717964 [details] [diff] [review]
Upgrade mulet compiler from gcc 4.7.3 to gcc 4.9.3

glandium: see comment 8 for how this was built. This new version of sixgill is built with 5b7f97c1b547 from http://hg.mozilla.org/users/sfink_mozilla.com/sixgill

I have tested it with a new taskcluster job and it works, except that it finds a new hazard. (And it's a nasty one, but it's a false positive. I have a very explicit mechanism to avoid it that I guess I'll need to land.)
Attachment #8717964 - Flags: review?(mh+mozilla)
Attachment #8713763 - Attachment is obsolete: true
Oops. Forgot [leave-open].
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
Comment on attachment 8717964 [details] [diff] [review]
Upgrade mulet compiler from gcc 4.7.3 to gcc 4.9.3

Cancelling review request for now. I need to build this in a proper environment that doesn't have stuff that won't be in the docker image. (Such as, say, the docker image.)
Attachment #8717964 - Flags: review?(mh+mozilla)
Created attachment 8718159 [details] [diff] [review]
Upgrade mulet compiler from gcc 4.7.3 to gcc 4.9.3

Ok, this one is working for me. I required an additional change to be compatible with the taskcluster browser hazard build, because its docker container uses a hostname that does not resolve, which confused the server that collects analysis results. I made it fall back to using 'localhost'.

Admittedly, I've only tested this one so far with the regular browser, not mulet. But I'm not asking for review of the correctness of sixgill anyway, just for things like are you ok with having sixgill fetched by tooltool for all builds, not just hazard builds (it'll never be called by other builds, and it's tiny in comparison to the other stuff.)
Attachment #8718159 - Flags: review?(mh+mozilla)
Attachment #8717964 - Attachment is obsolete: true
Comment on attachment 8718159 [details] [diff] [review]
Upgrade mulet compiler from gcc 4.7.3 to gcc 4.9.3

Review of attachment 8718159 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with visibility removed.

::: js/src/devtools/rootAnalysis/build/sixgill.manifest
@@ +8,4 @@
>        "filename" : "sixgill.tar.xz",
> +      "size" : 2684108,
> +      "unpack" : true,
> +      "visibility" : "public"

visibility is only useful for uploads.
Attachment #8718159 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8718159 [details] [diff] [review]
Upgrade mulet compiler from gcc 4.7.3 to gcc 4.9.3

Review of attachment 8718159 [details] [diff] [review]:
-----------------------------------------------------------------

Actually. Why add sixgill on normal mulet builds?

::: js/src/devtools/rootAnalysis/build/sixgill.manifest
@@ +8,4 @@
>        "filename" : "sixgill.tar.xz",
> +      "size" : 2684108,
> +      "unpack" : true,
> +      "visibility" : "public"

visibility is only useful for uploads.
Attachment #8718159 - Flags: review+
Depends on: 1249448
(In reply to Mike Hommey [:glandium] from comment #17)
> Comment on attachment 8718159 [details] [diff] [review]
> Upgrade mulet compiler from gcc 4.7.3 to gcc 4.9.3
> 
> Review of attachment 8718159 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Actually. Why add sixgill on normal mulet builds?

Only because it's easiest. The sixgill download is relatively small (<3MB) and does no harm just sitting there. The alternative would be to either duplicate releng.manifest for the analysis, or run tooltool twice with an additional manifest. The latter is a somewhat larger change to the build scripts (build-mulet-linux.sh calls desktop-setup.sh calls install-packages.sh). But it's totally doable if you'd prefer to avoid the spurious download.

> 
> ::: js/src/devtools/rootAnalysis/build/sixgill.manifest
> @@ +8,4 @@
> >        "filename" : "sixgill.tar.xz",
> > +      "size" : 2684108,
> > +      "unpack" : true,
> > +      "visibility" : "public"
> 
> visibility is only useful for uploads.

Oops, missed another one. My current patch stack does not contain this. (Though my current patch stack no longer uses this particular file at all, but it'll require some more reviews.) My automation for rebuilding sixgill and re-uploading inserts this.
Created attachment 8722147 [details] [diff] [review]
Upgrade mulet compiler from gcc 4.7.3 to gcc 4.9.3,

Actually, doing a separate tooltool run turns out to be quite easy if you don't mind setting TOOLTOOL_MANIFEST and re-rerunning the install-packages.sh script, and I kind of like using it that way. (I'd probably rather have it define a shell function and then have the calling script invoke the function with the proper manifest, but this is good enough.)

It does mean that whenever the compiler version is different between the analysis, firefox browser, and mulet, then I'll need to use a separate sixgill.manifest file. But for now I'd like to move towards getting them on a single one, and forking the file later when it's needed.
Attachment #8722147 - Flags: review?(mh+mozilla)
Attachment #8718159 - Attachment is obsolete: true
Comment on attachment 8722147 [details] [diff] [review]
Upgrade mulet compiler from gcc 4.7.3 to gcc 4.9.3,

Review of attachment 8722147 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not sure why the compiler has to be updated for non hazard mulet builds but meh.
Attachment #8722147 - Flags: review?(mh+mozilla) → review+
(In reply to Mike Hommey [:glandium] from comment #20)
> Comment on attachment 8722147 [details] [diff] [review]
> Upgrade mulet compiler from gcc 4.7.3 to gcc 4.9.3,
> 
> Review of attachment 8722147 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not sure why the compiler has to be updated for non hazard mulet builds
> but meh.

You didn't answer to this.
(In reply to Mike Hommey [:glandium] from comment #23)
> (In reply to Mike Hommey [:glandium] from comment #20)
> > Comment on attachment 8722147 [details] [diff] [review]
> > Upgrade mulet compiler from gcc 4.7.3 to gcc 4.9.3,
> > 
> > Review of attachment 8722147 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I'm not sure why the compiler has to be updated for non hazard mulet builds
> > but meh.
> 
> You didn't answer to this.

It didn't sound like a question. "meh" sounded like you didn't see a reason for it but didn't mind it happening.

There was no strong reason for it. It meant I didn't need to have a separate manifest for mulet hazard browser builds vs mulet browser builds. It also matches where all of the other b2g builds are going right now; see bug 1087161. That could be done in a separate bug, using a separate manifest file, by someone else at a later time.

So if you'd rather I not touch the non-hazard build, let me know, and I'll do a followup patch that creates a separate manifest for the hazard build and reverts regular mulet to gcc 4.7.3 for now. It does mean having more total configurations to manage, but that's your call.
Blocks: 1266294
Blocks: 1266295
Oops, another leave-open leavebehind.
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.