1087161 - Investigate upgrading the toolchain used for B2G ICS/JB/KK/L builds to GCC 4.9

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

+++ This bug was initially created as a clone of Bug #1056337 +++

In bug 1056337 comment 45, I suggested upgrading to GCC 4.9, it has ASan for GCC on ARM, which is likely to be useful for finding security issues. [1]

Plus, it's already available on Android NDK version 10c. [2]

[1] https://gcc.gnu.org/gcc-4.9/changes.html
[2] https://developer.android.com/tools/sdk/ndk/index.html#Revisions

Comment 1

[Cervantes Yu [:cyu] [:cervantes]]

The newer toolchain also comes with the newer version of gdb/gdbserver, which fixes a file descriptor leak from gdbserver to the debugee.

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Should we also be looking into this for JB and/or KK devices?  Those are 4.6 or 4.7 currently, it looks like.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

4.7. But wouldn't we have the aforementioned partner issues that kept ICS stuck on 4.4 for so long?

Comment 4

[Juan Gomez [:_AtilA_] (CET/CEST)]

Ok, my first attemp compiling Gecko with gcc-4.9 threw some errors complaining about the existance of public destructors in Reference-counted classes... so, it looks like migrating to gcc-4.9 is going to take a bunch of patches.

Comment 5

[Kan-Ru Chen [:kanru] (UTC+9)]

I compiled gecko with gcc-4.9 on x86 without error. Maybe it's ARM or gonk only issue?

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Not sure how to nominate this bug going forward (I couldn't set feature-b2g, but I think that's restricted to some other workflow.

There's another version 10d of the NDK which may or may not help in this regard. [1]

[1] https://developer.android.com/tools/sdk/ndk/index.html#Revisions

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Juan, now that bug 1056337 is freshly fixed, do you mind seeing if you can get this easily working too? (i.e. whether it's low hanging fruit or not)

Comment 8

[Juan Gomez [:_AtilA_] (CET/CEST)]

Hi Gary, it's not fixed yet! :) I need to land the rest of devices, but as soon as it gets closed. I'll work on it.

Comment 9

[Juan Gomez [:_AtilA_] (CET/CEST)]

Hi Gary,
I'm afraid I'll need to work on other bug and it will probably take me some time to fix it, so I will give some pointers here just in case someone else would like to work in this one.
 
Ok, with all the work done in bug 1056337, testing new Android toolchain is far more easy.
We just need to edit the device manifest file and replace this line (repository):

<project path="prebuilts/gcc/linux-x86/arm/arm-linux-androideabi-4.8" name="platform/prebuilts/gcc/linux-x86/arm/arm-linux-androideabi-4.8" revision="aosp-new/master"/>

by this one:

<project path="prebuilts/gcc/linux-x86/arm/arm-linux-androideabi-4.9" name="platform/prebuilts/gcc/linux-x86/arm/arm-linux-androideabi-4.9" revision="aosp-new/master"/>

Then use the classic cycle of: Compile, fix build error (mostly Gecko), compile, fix error, compile...

One thing I just saw is that there are _A LOT_ of potential public destructors that the compiler wants them to be private for safety. So please the compiler and make it happy :)

Sorry!

Good luck ;)
Feel free to needinfo/ping me if you get stuck at some point!

Comment 10

[Ryan VanderMeulen [:RyanVM]]

FWIW, I feel like it'd probably time better spent getting other B2G builds on to 4.8 across the board first before worrying about getting ICS to 4.9.

Comment 11

[Mike Hommey [:glandium]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #10)
> FWIW, I feel like it'd probably time better spent getting other B2G builds
> on to 4.8 across the board first before worrying about getting ICS to 4.9.

What b2g builds we do are not using 4.8? (besides the issue of the host compiler, which is separate and has a patch already, that I just need to land)

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Both JB && KK are on 4.7 last I checked. Not sure what the L-based builds are using.

Comment 13

[Michael Wu [:mwu]]

I rather retire ICS before spending time on supporting 4.9 on ICS.

Comment 14

[Juan Gomez [:_AtilA_] (CET/CEST)]

Because of bug 845086, I'm building Gecko with gcc-4.9 and started patching. So, I'm gonna take this work and prepare a big patch. 98% of the fixes are because of public destructors used in ref-counted classes... and there are a lot of them :(

Comment 15

[(not currently active) Ted Mielczarek]

A lot of that work happened in bug 1137151, are there still a lot more of them?

Comment 16

[Juan Gomez [:_AtilA_] (CET/CEST)]

Oh! think I need to update my Gecko clone :(

Comment 17

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: Explicit casting to avoid Werror=format build breaks

Functional-style casting to avoid format error messages

Comment 18

[Juan Gomez [:_AtilA_] (CET/CEST)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=67aaa2a46834

Comment 19

[Juan Gomez [:_AtilA_] (CET/CEST)]

https://hg.mozilla.org/integration/b2g-inbound/rev/5fdb92dc4374

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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

Comment 21

[Juan Gomez [:_AtilA_] (CET/CEST)]

If there's no objection, I'd like to take and close this issue, because it's a must if we want to go for hardfp ABI (bug 845086).

Comment 22

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: 0001-Bug-1087161-Fix-unused-function-build-error-with-gcc.patch

{anonymous}::IsMainProcess() is defined but not used in b2g (this warning turned into an error in gcc-4.9). But this function is already implemented in XRE [1], so I just deleted the definition of the function and used the one in XRE.

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#4522

Comment 23

[Mike Hommey [:glandium]]

As mentioned on irc, you should do that for *all* the IsMainProcess in the tree (and you might as well also do it for the other XRE_GetProcessType() == GeckoProcessType_Default tests ;)

Comment 24

[Juan Gomez [:_AtilA_] (CET/CEST)]

I filed a new bug to follow the issue (and other related) mentioned by Mike in comment 23.
As this issue is not a blocker, I've assigned it to me and will work on it as soon as I fix this bug first :)

Comment 25

[Juan Gomez [:_AtilA_] (CET/CEST)]

Sorry, bug 1171931.

Comment 26

[Juan Gomez [:_AtilA_] (CET/CEST)]

gecko/netwerk/dns/mdns/libmdns/nsDNSServiceDiscovery.cpp:30:1: error: 'void mozilla::net::{anonymous}::StopService()' defined but not used [-Werror=unused-function]

Ok, this function is not being used (it's not disabled because of any #define). Requesting permission for:
1º - Removing it
2º - Add unused attribute

What do you think Gary Chen?

Comment 27

[Gary Chen [:xeonchen]]

Thank you Juan for reminding, I've opened Bug 1172383 for this issue.

Comment 28

[Juan Gomez [:_AtilA_] (CET/CEST)]

Comment on attachment 8614027 [details] [diff] [review]
0001-Bug-1087161-Fix-unused-function-build-error-with-gcc.patch

I moved these patch into a bigger one from bug 1171931.

Comment 29

[Juan Gomez [:_AtilA_] (CET/CEST)]

I'm going to update all our Android base builds, not just ICS.

Comment 30

[Juan Gomez [:_AtilA_] (CET/CEST)]

As per IRC conversation, we are going to upgrade all JB devices first. Meanwhile we should talk to vendors so they can ensure us that there's no problem by upgrading to a newer toolchain.

Comment 31

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: PR: Add gcc-4.9 toolchain to base-jb.xml manifest

Comment 32

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: PR: Add GECKO_TOOLS_PREFIX to JB Emulator manifest in order to compile with gcc-4.9 toolchain

Comment 33

[Juan Gomez [:_AtilA_] (CET/CEST)]

Michael, I didn't add the toolchain for Darwin because I don't have a Mac to test with.
Anyway, if we support Mac compiling (I think we do), let me know and I'll add the Darwin toolchain so anyone with a Mac could test the build.

Comment 34

[Michael Wu [:mwu]]

I think you should add the Darwin toolchains anyway. It might not be enough by itself (I don't know), but it won't hurt.

Comment 35

[Juan Gomez [:_AtilA_] (CET/CEST)]

https://github.com/mozilla-b2g/platform_build/commit/660169a3d7e034a892359e39135e8c2785a6ad6f

Comment 36

[Juan Gomez [:_AtilA_] (CET/CEST)]

https://github.com/mozilla-b2g/b2g-manifest/commit/2103b2f89f07f9c8a999ef2a08971cf405b70449

Comment 37

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: PR: Add GECKO_TOOLS_PREFIX to KK Emulator manifest in order to compile with gcc-4.9 toolchain

I'll not merge this patch until Michael allows me to do it, because it's probably that vendors wouldn't like to use gcc-4.9 toolchain.

Comment 38

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: PR: Add gcc-4.9 toolchain to base-kk.xml manifest

Same as above, I'll not merge this PR until we get an answer from vendors.

Comment 39

[Michael Wu [:mwu]]

Comment on attachment 8639281 [details] [review]
PR: Add gcc-4.9 toolchain to base-kk.xml manifest

Checks are failing though, so we probably need to setup a mirror for this.

Comment 40

[Juan Gomez [:_AtilA_] (CET/CEST)]

Ok, I see what's happening, I'll fix it.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

Looks like this broke b2g37.
https://treeherder.mozilla.org/logviewer.html#?job_id=162194&repo=mozilla-b2g37_v2_2
https://treeherder.mozilla.org/logviewer.html#?job_id=162190&repo=mozilla-b2g37_v2_2
https://treeherder.mozilla.org/logviewer.html#?job_id=162191&repo=mozilla-b2g37_v2_2

Comment 42

[Juan Gomez [:_AtilA_] (CET/CEST)]

Ok, looks like I needed to PR b2g-manifest v2.2 and v2.1s as well (as per IRC convesation).
PRd and merged:
https://github.com/mozilla-b2g/b2g-manifest/commit/5ce8f16e394c40adfe481fff4fca7bfe66f09ce7
https://github.com/mozilla-b2g/b2g-manifest/commit/bfa0bcf27181f16ccefab0c861ec1c5e746e93d9

Comment 43

[Ryan VanderMeulen [:RyanVM]]

That hit other Gecko bustage, so we decided to just pin to the last-good revision of platform_build instead of fighting with this on legacy branches.

v2.2:  https://github.com/mozilla-b2g/b2g-manifest/commit/25089f19b9c9fb636d8bf753e0957f4bc6192dbd
v2.1s: https://github.com/mozilla-b2g/b2g-manifest/commit/c91aa80a41fc7da0a99df8ad721d43422e3ab593

Comment 44

[Penserbjorne]

Hi, please fix this for all the old devices like Hamachi, Inari, Keon :/ 

Im trying to compile for that devices but I get the "Only GCC 4.7 or newer supported" error, in my toolchain the selected compiler is 4.4.x so I guess that is causing the problem.

Thanks!

Comment 45

[Juan Gomez [:_AtilA_] (CET/CEST)]

Hamachi, Inari and Keon are ICS devices, and we already upgraded them to gcc-4.8 based toolchain time ago. That upgrade was only applied on versions greater than v2.2. AFAIK older versions are not officially supported anymore, so if you are trying to compile an older version, we would need to patch them in order to work.
Adrian Crespo already fixed some of this branches, so let's see if he can help you and fix older versions.

Comment 46

[Penserbjorne]

Thanks Juan, I'll contact Adrian and I'll see what can I do for the 2.0 and 2.1 branches :)

Comment 47

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Juan, now that various patches have landed, bugs fixed, I see that the last mentioned bug seems to be bug 1172383 that now has a patch this week.

I was just wondering (not rushing or anything), is that one of the last blockers for this bug?

Comment 48

[Juan Gomez [:_AtilA_] (CET/CEST)]

Hey Gary, the plan was to upgrade JB devices first and then KK and finally Lollipop ones, gradually. Before we can upgrade KK devices, we need to know if vendors will agree with this change. I don't really know if there has been some sort of conversations with them, maybe Michael could sched some light on this.

Comment 49

[Adam Farden [:adfad666]]

FYI for (potential) aarch64 devices on L we currently have the 4.8 toolchain, but it's not being picked up, it complains about a lack of 4.9 toolchain. This is probably going to be mandatory for those devices.

/bin/bash: prebuilts/gcc/linux-x86/aarch64/aarch64-linux-android-4.9/bin/aarch64-linux-android-gcc: No such file or directory


https://github.com/mozilla-b2g/b2g-manifest/blob/master/base-l.xml#L36

Comment 50

[Juan Gomez [:_AtilA_] (CET/CEST)]

I think that we can move this bug forward as we don't have the partner barrier anymore. Michael, is it ok to start by upgrading old devices first? (JB, KK, and L)

Comment 51

[Juan Gomez [:_AtilA_] (CET/CEST)]

We are facing ICEs with Gecko being built with gcc-4.7 based toolchains (bug 1251717), so it's time to move forward this bug.
I'm going to start upgrading all KK devices.

Comment 52

[Juan Gomez [:_AtilA_] (CET/CEST)]

Comment on attachment 8639281 [details] [review]
PR: Add gcc-4.9 toolchain to base-kk.xml manifest

I did a little modification, because the previous commit was failing with dolphin devices. I just needed to add the remote attribute to the gcc-4.9 project tags.

Comment 53

[Juan Gomez [:_AtilA_] (CET/CEST)]

Comment on attachment 8635312 [details] [review]
PR: Add gcc-4.9 toolchain to base-jb.xml manifest

This was merged time ago:
https://github.com/mozilla-b2g/platform_build/commit/660169a3d7e034a892359e39135e8c2785a6ad6f

Comment 54

[Juan Gomez [:_AtilA_] (CET/CEST)]

Comment on attachment 8635312 [details] [review]
PR: Add gcc-4.9 toolchain to base-jb.xml manifest

Sorry this is the commit of the merge:
https://github.com/mozilla-b2g/b2g-manifest/commit/2103b2f89f07f9c8a999ef2a08971cf405b70449

Comment 55

[Juan Gomez [:_AtilA_] (CET/CEST)]

Comment on attachment 8635313 [details] [review]
PR: Add GECKO_TOOLS_PREFIX to JB Emulator manifest in order to compile with gcc-4.9 toolchain

This is the commit of the merge:
https://github.com/mozilla-b2g/platform_build/commit/660169a3d7e034a892359e39135e8c2785a6ad6f

Comment 56

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: PR: Add GECKO_TOOLS_PREFIX to KK Emulator manifest in order to compile with gcc-4.9 toolchain

Changed both emulators: arm and x86.

Comment 57

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: PR to change shinano based devices toolchain

Comment 58

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: PR to change Flame-kk device toolchain to gcc-4.9

Comment 59

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: PR to change Nexus-4-kk device toolchain to gcc-4.9

Comment 60

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: PR to change Nexus 5 device toolchain to gcc-4.9

Comment 61

[:gerard-majax]

Comment on attachment 8639281 [details] [review]
PR: Add gcc-4.9 toolchain to base-kk.xml manifest

Make sure to wait for the mirror to be ready before merging this

Comment 62

[:gerard-majax]

What about RPi2 L and Sony AOSP L devices?

Comment 63

[Juan Gomez [:_AtilA_] (CET/CEST)]

https://github.com/mozilla-b2g/platform_build/commit/964d9fa4eabe9eb473ef9101ca2a690880f28547
https://github.com/mozilla-b2g/device-shinano-common/commit/53dcf88a4314c05dae3464f7647138ad420398e6
https://github.com/mozilla-b2g/device-flame/commit/71adaecff0d7ca51f9ee9d58b39b6957344d1478
https://github.com/mozilla-b2g/device-hammerhead/commit/14e489b1fa92e86f280dcb22fee19a9f1be27ed7
https://github.com/mozilla-b2g/device-mako/commit/f1f2af075a376c44aa672a3e5ca8b22ba90a55fa

All KK devices upgraded to gcc-4.9 (reminder: *just* Gecko toolchain, Gonk is still being compiled with the official one from it's Android base).

Comment 64

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: PR to add gcc-4.9 toolchain for Lollipop devices

Comment 65

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: PR to change Gecko toolchain to gcc-4.9 on Flamingo device

Comment 66

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: PR to change Gecko toolchain to gcc-4.9 on Tianchi device

Comment 67

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: PR to change Gecko toolchain to gcc-4.9 on Amami device

Comment 68

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: PR to change Gecko toolchain to gcc-4.9 on Honami device

Comment 69

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: PR to change Gecko toolchain to gcc-4.9 on Sirius device

Comment 70

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: PR to change Gecko toolchain to gcc-4.9 on Scorpion device

Comment 71

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: PR to change Gecko toolchain to gcc-4.9 on Leo-l device

Comment 72

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: PR to change Gecko toolchain to gcc-4.9 on Seagull device

Comment 73

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: PR to change Gecko toolchain to gcc-4.9 on RPI2 device

Comment 74

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: PR to change Gecko toolchain to gcc-4.9 on Nexusplayer device

Comment 75

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: PR to change Gecko toolchain to gcc-4.9 on Nexus6 device

Comment 76

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: PR to change Gecko toolchain to gcc-4.9 on Nexus-5-L device

Comment 77

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: PR to change Gecko toolchain to gcc-4.9 on Flame-L device

Comment 78

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: PR to change Gecko toolchain to gcc-4.9 on FP2 device

Comment 79

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: PR to change Gecko toolchain to gcc-4.9 on Aries-L device

Comment 80

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: PR to change Gecko toolchain to gcc-4.9 on Emualtors-l device

Comment 81

[:gerard-majax]

Comment on attachment 8729424 [details] [review]
PR to add gcc-4.9 toolchain for Lollipop devices

maybe we don't need to specify the name of the remote.

Comment 82

[:gerard-majax]

Comment on attachment 8729460 [details] [review]
PR to change Gecko toolchain to gcc-4.9 on Flame-L device

Flame L got dropped, I don't think we should even update it

Comment 83

[:gerard-majax]

I would prefer we leverage device-sony-common repo for upgrading the toolchain of the Sony AOSP L based ports: the intent is exactly to avoid having to deal with 16 reviews when dealing with generic stuff for those.

I guess we could export the variable from https://github.com/mozilla-b2g/device-sony-common/blob/sony-aosp-l/CommonConfig.mk ?

Comment 84

[Juan Gomez [:_AtilA_] (CET/CEST)]

Oh! I liked the granularity and coherence of modifying each device separately, like all other non-sony devices, so for example in the future you want to change or test with another toolchain, it's clear and fast to detect where to make the change. On the other hand, any new Sony device will inherit the Gecko toolchain without changing anything.

Let's PR the common!

Comment 85

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: PR to change Gecko toolchain to gcc-4.9 on all Sony-L devices

I set all other sony-aosp-l PRs derived devices obsolete.

Comment 86

[Juan Gomez [:_AtilA_] (CET/CEST)]

Almost all merged!
https://github.com/mozilla-b2g/b2g-manifest/commit/c29c764b04350bda5bec87ec29c059dd33153e12
https://github.com/mozilla-b2g/device-rpi2b/commit/883f1ad31487f2d1d9bda47134ff2f91b87017b8
https://github.com/mozilla-b2g/device-shamu/commit/5786a029adeecc0290063437af1985324af692b4
https://github.com/mozilla-b2g/device-hammerhead/commit/acbb7a8914059426180c9059fc9419e57668a478
https://github.com/mozilla-b2g/device-fairphone2/commit/2188aa0ee854f90aee166447bd72c02078910ff0

I'll wait for bug 1254895 to be solved and merge the platform_build PR for the emulators.

Comment 87

[Juan Gomez [:_AtilA_] (CET/CEST)]

Emulators upgrade merged.
https://github.com/mozilla-b2g/platform_build/commit/aee7ff3dba262a037559d360b62af429b62cb876

Comment 88

[Juan Gomez [:_AtilA_] (CET/CEST)]

Sony common devices merged.
https://github.com/mozilla-b2g/device-sony-common/commit/a425d08255d3d4135fed995fe6d28ea096747910

Comment 89

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8729454 [details] [review]
PR to change Gecko toolchain to gcc-4.9 on Nexusplayer device

I tested it on Nexus Player. It is fine.

Comment 90

[Juan Gomez [:_AtilA_] (CET/CEST)]

Thanks Farmer, merged:
https://github.com/mozilla-b2g/device-asus-fugu/commit/9bf2bdf7afe9abecd032e88100f63decef949d59

Comment 91

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.