Closed Bug 1087161 Opened 6 years ago Closed 4 years ago

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

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: gkw, Assigned: _AtilA_)

References

Details

Attachments

(16 files, 13 obsolete files)

52 bytes, text/x-github-pull-request
mwu
: review+
Details | Review
54 bytes, text/x-github-pull-request
mwu
: review+
Details | Review
52 bytes, text/x-github-pull-request
gerard-majax
: review+
Details | Review
54 bytes, text/x-github-pull-request
gerard-majax
: review+
Details | Review
60 bytes, text/x-github-pull-request
gerard-majax
: review+
Details | Review
52 bytes, text/x-github-pull-request
gerard-majax
: review+
Details | Review
50 bytes, text/x-github-pull-request
gerard-majax
: review+
Details | Review
56 bytes, text/x-github-pull-request
gerard-majax
: review+
Details | Review
52 bytes, text/x-github-pull-request
gerard-majax
: review+
Details | Review
50 bytes, text/x-github-pull-request
gerard-majax
: review+
Details | Review
54 bytes, text/x-github-pull-request
fatseng
: review+
Details | Review
50 bytes, text/x-github-pull-request
gerard-majax
: review+
Details | Review
56 bytes, text/x-github-pull-request
gerard-majax
: review+
Details | Review
55 bytes, text/x-github-pull-request
gerard-majax
: review+
Details | Review
54 bytes, text/x-github-pull-request
gerard-majax
: review+
Details | Review
56 bytes, text/x-github-pull-request
gerard-majax
: review+
Details | Review
+++ 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
The newer toolchain also comes with the newer version of gdb/gdbserver, which fixes a file descriptor leak from gdbserver to the debugee.
See Also: → 1078383
Should we also be looking into this for JB and/or KK devices?  Those are 4.6 or 4.7 currently, it looks like.
4.7. But wouldn't we have the aforementioned partner issues that kept ICS stuck on 4.4 for so long?
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.
I compiled gecko with gcc-4.9 on x86 without error. Maybe it's ARM or gonk only issue?
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
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)
Flags: needinfo?(atilag)
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.
Flags: needinfo?(atilag)
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!
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.
(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)
Both JB && KK are on 4.7 last I checked. Not sure what the L-based builds are using.
I rather retire ICS before spending time on supporting 4.9 on ICS.
Blocks: 845086
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 :(
A lot of that work happened in bug 1137151, are there still a lot more of them?
Oh! think I need to update my Gecko clone :(
Functional-style casting to avoid format error messages
Attachment #8580849 - Flags: review?(ehsan)
Attachment #8580849 - Flags: review?(ehsan) → review+
Assignee: nobody → atilag
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).
{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
Attachment #8614027 - Flags: review?(bent.mozilla)
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 ;)
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 :)
See Also: → 1171931
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?
Flags: needinfo?(xeonchen)
Thank you Juan for reminding, I've opened Bug 1172383 for this issue.
Flags: needinfo?(xeonchen)
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.
Attachment #8614027 - Attachment is obsolete: true
Attachment #8614027 - Flags: review?(bent.mozilla)
Depends on: 1171931
Depends on: 1181117
I'm going to update all our Android base builds, not just ICS.
Summary: Investigate upgrading the toolchain used for B2G ICS builds to GCC 4.9 → Investigate upgrading the toolchain used for B2G ICS/JB/KK/L builds to GCC 4.9
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.
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.
Flags: needinfo?(mwu)
Attachment #8635312 - Flags: review?(mwu) → review+
Attachment #8635313 - Flags: review?(mwu) → review+
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.
Flags: needinfo?(mwu)
Depends on: 1185364
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.
Attachment #8639277 - Flags: review?(mwu)
Same as above, I'll not merge this PR until we get an answer from vendors.
Attachment #8639281 - Flags: review?(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.
Attachment #8639281 - Flags: review?(mwu) → review+
Ok, I see what's happening, I'll fix it.
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
Flags: needinfo?(atilag)
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
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!
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.
Flags: needinfo?(madrid.crespo)
Thanks Juan, I'll contact Adrian and I'll see what can I do for the 2.0 and 2.1 branches :)
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?
Flags: needinfo?(atilag)
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.
Flags: needinfo?(atilag) → needinfo?(mwu)
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
Blocks: 1187826
Flags: needinfo?(mwu)
Flags: needinfo?(madrid.crespo)
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)
Flags: needinfo?(mwu)
Flags: needinfo?(mwu)
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.
Attachment #8580849 - Attachment is obsolete: true
Attachment #8639277 - Flags: review?(mwu) → review?(lissyx+mozillians)
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.
Attachment #8639281 - Flags: review+ → review?(lissyx+mozillians)
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 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 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
Depends on: 1253214
Changed both emulators: arm and x86.
Attachment #8639277 - Attachment is obsolete: true
Attachment #8639277 - Flags: review?(lissyx+mozillians)
Attachment #8726241 - Flags: review?(lissyx+mozillians)
Attachment #8726367 - Flags: review?(lissyx+mozillians)
Attachment #8726370 - Flags: review?(lissyx+mozillians)
Attachment #8726377 - Flags: review?(lissyx+mozillians)
Attachment #8726389 - Flags: review?(lissyx+mozillians)
Blocks: 1245091
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
Attachment #8639281 - Flags: review?(lissyx+mozillians) → review+
What about RPi2 L and Sony AOSP L devices?
Flags: needinfo?(jgomez)
Attachment #8726241 - Flags: review?(lissyx+mozillians) → review+
Attachment #8726367 - Flags: review?(lissyx+mozillians) → review+
Attachment #8726370 - Flags: review?(lissyx+mozillians) → review+
Attachment #8726377 - Flags: review?(lissyx+mozillians) → review+
Attachment #8726389 - Flags: review?(lissyx+mozillians) → review+
Attachment #8729424 - Flags: review?(lissyx+mozillians)
Attachment #8729426 - Flags: review?(lissyx+mozillians)
Attachment #8729428 - Flags: review?(lissyx+mozillians)
Attachment #8729429 - Flags: review?(lissyx+mozillians)
Attachment #8729445 - Flags: review?(lissyx+mozillians)
Attachment #8729446 - Flags: review?(lissyx+mozillians)
Attachment #8729449 - Flags: review?(lissyx+mozillians)
Attachment #8729450 - Flags: review?(lissyx+mozillians)
Attachment #8729451 - Flags: review?(lissyx+mozillians)
Attachment #8729460 - Flags: review?(lissyx+mozillians)
Attachment #8729463 - Flags: review?(lissyx+mozillians)
Attachment #8729465 - Flags: review?(lissyx+mozillians)
Attachment #8729479 - Flags: review?(lissyx+mozillians) → review+
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.
Attachment #8729424 - Flags: review?(lissyx+mozillians) → review+
Attachment #8729426 - Flags: review?(lissyx+mozillians)
Attachment #8729428 - Flags: review?(lissyx+mozillians)
Attachment #8729429 - Flags: review?(lissyx+mozillians)
Attachment #8729445 - Flags: review?(lissyx+mozillians)
Attachment #8729449 - Flags: review?(lissyx+mozillians)
Attachment #8729446 - Flags: review?(lissyx+mozillians)
Attachment #8729450 - Flags: review?(lissyx+mozillians)
Attachment #8729451 - Flags: review?(lissyx+mozillians)
Attachment #8729465 - Flags: review?(lissyx+mozillians)
Attachment #8729463 - Flags: review?(lissyx+mozillians) → review+
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
Attachment #8729460 - Flags: review?(lissyx+mozillians)
Attachment #8729458 - Flags: review?(lissyx+mozillians) → review+
Attachment #8729456 - Flags: review?(lissyx+mozillians) → review+
Attachment #8729452 - Flags: review?(lissyx+mozillians) → review+
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 ?
Flags: needinfo?(jgomez)
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!
Flags: needinfo?(jgomez)
I set all other sony-aosp-l PRs derived devices obsolete.
Attachment #8729426 - Attachment is obsolete: true
Attachment #8729428 - Attachment is obsolete: true
Attachment #8729429 - Attachment is obsolete: true
Attachment #8729445 - Attachment is obsolete: true
Attachment #8729446 - Attachment is obsolete: true
Attachment #8729449 - Attachment is obsolete: true
Attachment #8729450 - Attachment is obsolete: true
Attachment #8729451 - Attachment is obsolete: true
Attachment #8729465 - Attachment is obsolete: true
Attachment #8729651 - Flags: review?(lissyx+mozillians)
Depends on: 1254895
Attachment #8729460 - Attachment is obsolete: true
Attachment #8729651 - Flags: review?(lissyx+mozillians) → review+
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.
Attachment #8729454 - Flags: review?(fatseng) → review+
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.