If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Investigate upgrading the toolchain used for B2G ICS builds (and stop using GCC 4.4)

RESOLVED FIXED in Firefox OS v2.2

Status

Firefox OS
General
RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: RyanVM, Assigned: _AtilA_)

Tracking

unspecified
2.2 S3 (9jan)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(b2g-v2.2 fixed)

Details

Attachments

(15 attachments, 5 obsolete attachments)

108 bytes, text/plain
mwu
: review+
Details
815 bytes, patch
glandium
: review-
Details | Diff | Splinter Review
2.18 KB, patch
mwu
: review+
Details | Diff | Splinter Review
1.29 KB, patch
Details | Diff | Splinter Review
9.46 KB, patch
mwu
: review+
Details | Diff | Splinter Review
49 bytes, text/x-github-pull-request
Details | Review | Splinter Review
53 bytes, text/x-github-pull-request
Details | Review | Splinter Review
52 bytes, text/x-github-pull-request
Details | Review | Splinter Review
52 bytes, text/x-github-pull-request
Details | Review | Splinter Review
3.94 KB, patch
rillian
: review+
Details | Diff | Splinter Review
52 bytes, text/x-github-pull-request
mwu
: review+
Details | Review | Splinter Review
1.05 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
52 bytes, text/x-github-pull-request
mwu
: review+
Details | Review | Splinter Review
52 bytes, text/x-github-pull-request
mwu
: review+
Details | Review | Splinter Review
1.48 KB, text/plain
Details
(Reporter)

Description

3 years ago
B2G ICS builds are built with GCC 4.4, by far the oldest compiler we still support. It causes ongoing pain for developers and adds complexity to our codebase having to support it. Per discussion in #b2g today, it may be possible to move ICS builds to a newer toolchain which would allow us to drop 4.4 support. We should investigate if this is feasible and and consider doing so if it is.
I thought this used the android tolls which I thought we used the android toolset
t gcc 4.7  minimum anyway,
(In reply to Bill Gianopoulos [:WG9s] from comment #1)
> I thought this used the android tolls which I thought we used the android
> toolset
> t gcc 4.7  minimum anyway,

Unfortunately it doesn't.

I'm sure there's already an existing bug about this.
(Assignee)

Comment 3

3 years ago
As gcc 4.4 is the official toolchain to build ICS on Android (so Gonk), we should change the toolchain to build just gecko. We are mainly using CAF repositories, so I will get the lastest toolchain there: arm-linux-androideabi-4.8. It should work without any patches, but if for any reason it cannot be done, I'll go for the next lowest version (arm-linux-androideabi-4.7), and so on.
(Assignee)

Updated

3 years ago
Assignee: nobody → atilag
> we should change the toolchain to build just gecko.

This has been suggested countless times. I'm glad this is finally going to happen.
Blocks: 1056407
(Assignee)

Comment 5

3 years ago
Update:
I'm trying few things in parallel:
1 - Compile Gonk with it's own toolchain (4.4.x) and Gecko with arm-linux-androideabi-4.8.
Gonk compiled as expected, but Gecko failed while linking libmozglue.so because of this:
/home/jgomez/b2g/build.emulator.ics/prebuilts/gcc/linux-x86/arm/arm-linux-androideabi-4.8/bin/../lib/gcc/arm-linux-androideabi/4.8/../../../../arm-linux-androideabi/bin/ld: error: read-only segment has dynamic relocations
collect2: error: ld returned 1 exit status
make[6]: *** [libmozglue.so] Error 1
Now I'm making sure that Gonk is being compiled with proper options (like -fpic / -fPIC) for all the libreries that are linked with Gecko. AFAIK this should fix the problem.

2 - Compile the whole system with arm-linux-androideabi-4.8
I had some compile errors in bionic, but didn't patch them because they were fixed in later versiones. So I tried to update bionic, but then it didn't compile because this newer version of Bionic needs more configuration data from the device, so I stoped this way so far.

3 - Compile the whole system with arm-linux-androideabi-4.7
I've had to fix some weird error on Android build system (build/core/TARGET_linux-arm.mk), because for some reason it wasn't compiling with crt* stuff (crtend/crtbegin) so the __dso_handle symbol wasn't being resolved. After fixing it, I needed to patch libicu4c because of a well known problem with gcc >= 4.6... and is still compiling while writting this, so I will update again in a few minutes.

I'll keep fighting... :)
(Assignee)

Comment 6

3 years ago
Update:
Compiling the whole system (Gonk+Gecko) with arm-linux-androideabi-4.7 drives us to the same "read-only segment has dynamic relocation" error. I realized that libmozglue is not linking against any Gonk library, so we have the porlem with Gecko. Looks like the objects being linked with libMozGlue are: cpuacct.o, BionicGlue.o, arm.o and Nuwa.o. I'm lookigin at their ELF sections and try to figure out where is the problem.
(Assignee)

Comment 7

3 years ago
Update:
I could compile Gecko with arm-linux-androideabi-4.6 toolchain.
Anyway, I'll keep trying with 4.7 and later with 4.8.

Comment 8

3 years ago
Great work here, Juan!

I'm curious, have you been in touch with anyone from CAF?  Do you know if our plans for stopping to use gcc 4.4 is OK with them?  Thanks!
Flags: needinfo?(atilag)
(Assignee)

Comment 9

3 years ago
Thks Ehsan!
No, I didn't talk with anyone from CAF... Do we need to?
Flags: needinfo?(atilag)
(Assignee)

Comment 10

3 years ago
Ok, I could compile Gecko with gcc-4.7 and gcc-4.8. 
I just needed to remove the "-z,text" flag. This linker flag was preventing us from having dynamic rellocations in a read-only segment. I suppose that we have set this flag for security reasons, so I'm not really sure if this could be the a valid change.
Maybe Mike Hommey could shed some light on this matter.
Flags: needinfo?(mh+mozilla)

Comment 11

3 years ago
(In reply to Juan Gomez [:_AtilA_] from comment #9)
> Thks Ehsan!
> No, I didn't talk with anyone from CAF... Do we need to?

Yes, I think so.  Michael, do you know if CAF still relies on using gcc 4.4 for ICS based Firefox OS builds?  Thanks!
Flags: needinfo?(mvines)
We have no active master-based ICS builds so it doesn't matter to us really.
(Assignee)

Comment 13

3 years ago
Ok, but if we are planning to upgrade the toolchain for building Gonk too, it's ok for gcc-4.6 and gcc-4.7, but not for gcc-4.8 (so far). I can work on it but, is it really what we want? Build both Gonk+Gecko or just Gecko?
(Reporter)

Comment 14

3 years ago
I don't think Gonk's compiler is really holding us back on anything of consequence. Gecko is where gcc 4.4 is a significant limitation. I think Gonk can just ride the Android trains.
Flags: needinfo?(mvines)

Comment 15

3 years ago
(In reply to Michael Vines [:m1] [:evilmachines] from comment #12)
> We have no active master-based ICS builds so it doesn't matter to us really.

That is great news!

Andreas, do you who we can talk to in order to figure out whether we still need to do ICS builds on trunk?  It would be tremendously helpful if we manage to kill ICS on trunk, because that means that we won't have to keep making our code build with gcc 4.4.
Flags: needinfo?(gal)

Comment 16

3 years ago
Ehsan, ICS can't be killed because we don't have testing running on KK or JB.
(Reporter)

Comment 17

3 years ago
This bug is about decoupling GCC 4.4 from ICS builds, so whether or not we should kill the builds entirely seems like an orthogonal question.

I think Juan has already clearly shown that we can update GCC to at least 4.7 without much difficulty and it appears there is no partner need to stay on the older version anymore either, so this bug can be completed as-filed without needing to answer that question here.

Comment 18

3 years ago
Yes, my point is that for our infra test runs on the emulator, we can build Gecko with gcc 4.7+ and the rest with gcc 4.4.  If our partners do not depend on ICS any more, then we can fix this bug, and bump up the Gecko compiler requirements.
(In reply to Juan Gomez [:_AtilA_] from comment #10)
> Ok, I could compile Gecko with gcc-4.7 and gcc-4.8. 
> I just needed to remove the "-z,text" flag. This linker flag was preventing
> us from having dynamic rellocations in a read-only segment. I suppose that
> we have set this flag for security reasons, so I'm not really sure if this
> could be the a valid change.
> Maybe Mike Hommey could shed some light on this matter.

Yeah, you don't want to remove -z,text. I guess this is the good old "crt_begin.o contains text relocations bug".
Search the block that adds -nostartfiles in configure.in, and try to make it run on gonk (remove the condition for testing), and check what configure says for "whether the CRT objects have text relocations"
Flags: needinfo?(mh+mozilla)
(Assignee)

Comment 20

3 years ago
Nailed it. Looks like we have text relocations in our crtbegin_so.o:

Relocation section '.rel.fini_array' at offset 0x408 contains 1 entries:
 Offset     Info    Type            Sym.Value  Sym. Name
00000004  00000102 R_ARM_ABS32       00000000   .text

CRT stuff comes from bionic, so before patching/forking anything, I'm going to try upgrading it as our first option.
FWIW, the corresponding upstream bug: https://code.google.com/p/android/issues/detail?id=23203
There's no clear indication of what specific changeset fixed it.
Well, actually, there is one.
https://code.google.com/p/android-source-browsing/source/detail?spec=svn.platform--bionic.0836a7ffb13890558fe568119b023007cce66373&repo=platform--bionic&r=9d40326830c2bd407427889c554adeb915ee6b4a
(Assignee)

Comment 23

3 years ago
Upgrading bionic to a working/fixed version (the one from JellyBean) is not possible, it depends on new device configurations that in turn depends on a newer version of the platform_build repository, so I've finally applied the necessary patches to Bionic current ICS version. With these patches applied and a little modification in configure.in to overcome the infamous __dso_handle symbol problems, I managed to compile Gecko with the google toolchain based on gcc-4.8.

I'll post non-Bionic related patches to be reviewed here ASAP, but we should first fork Bionic from: git://codeaurora.org/platform/bionic (tag: android-4.0.4_r2.1). When forked I'll make a Pull-Request on github to apply the patches, so with this fixed version of Bionic we can freely apply/land the other patches.

Comment 24

3 years ago
b2g_ics_strawberry and b2g-4.0.4_r2.1 created on mozilla-b2g/platform_bionic.
(Assignee)

Comment 25

3 years ago
Created attachment 8489108 [details]
platform_bionic PRs for both branches: b2g_ics_strawberry and b2g-4.0.4_r2.1.
Attachment #8489108 - Flags: review?(mwu)

Comment 26

3 years ago
Comment on attachment 8489108 [details]
platform_bionic PRs for both branches: b2g_ics_strawberry and b2g-4.0.4_r2.1.

I think bugzilla only really supports a single PR per attachment.. oh well close enough. Thanks!
Attachment #8489108 - Flags: review?(mwu) → review+
(Assignee)

Comment 27

3 years ago
Created attachment 8490386 [details] [diff] [review]
Add gcc-4.7 and gcc-4.8 to the list of compilers that need to define __dso_handle symbol with the correct visibility
Attachment #8490386 - Flags: review?(ted)

Updated

3 years ago
Attachment #8490386 - Flags: review?(ted) → review?(mh+mozilla)
(Assignee)

Comment 28

3 years ago
oh! didn't set Mike for review because I thought he was on PTO :)
(Assignee)

Comment 29

3 years ago
Created attachment 8491851 [details] [diff] [review]
Set GECKO_TOOLS_PREFIX via path discovery to compile Gecko (for ARM)
Attachment #8491851 - Flags: review?(mwu)
(Assignee)

Comment 30

3 years ago
Created attachment 8491852 [details] [diff] [review]
Use GECKO_TOOLS_PREFIX variable to compile Gecko
Attachment #8491852 - Flags: review?(mwu)

Comment 31

3 years ago
Comment on attachment 8491851 [details] [diff] [review]
Set GECKO_TOOLS_PREFIX via path discovery to compile Gecko (for ARM)

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

::: core/combo/TARGET_linux-arm.mk
@@ +48,5 @@
>  endif
>  
> +ifeq ($(strip $(GECKO_TOOLS_PREFIX)),)
> +    # Use the Gonk toolchain by default to compile Gecko
> +    GECKO_TOOLS_PREFIX := $(TARGET_TOOLS_PREFIX)

I don't think we need a default - gonk-misc needs to fall back on TARGET_TOOLS_PREFIX if GECKO_TOOLS_PREFIX isn't available. Otherwise this will break on JB/KK.

@@ +50,5 @@
> +ifeq ($(strip $(GECKO_TOOLS_PREFIX)),)
> +    # Use the Gonk toolchain by default to compile Gecko
> +    GECKO_TOOLS_PREFIX := $(TARGET_TOOLS_PREFIX)
> +    # ... but look for the existence of a better toolchain
> +    gcc_toolchain_path := prebuilts/gcc/$(HOST_PREBUILT_TAG)/arm/arm-linux-androideabi-4.8/bin

Can we use a wildcard to select the GCC version? Then we can use any version in the future without touching this, as long as we don't put more than one toolchain in the directory.
(Assignee)

Comment 32

3 years ago
Created attachment 8492376 [details] [diff] [review]
Use GECKO_TOOLS_PREFIX variable to compile Gecko (v2)
Attachment #8491852 - Attachment is obsolete: true
Attachment #8491852 - Flags: review?(mwu)
Attachment #8492376 - Flags: review?(mwu)

Updated

3 years ago
Attachment #8492376 - Flags: review?(mwu) → review+
(Assignee)

Comment 33

3 years ago
Created attachment 8492381 [details] [diff] [review]
0001-Bug-1056337-Upgrade-the-toolchain-GONK-MISC.patch Set GECKO_TOOLS_PREFIX via path discovery to compile Gecko for ARM (v2)
Attachment #8491851 - Attachment is obsolete: true
Attachment #8491851 - Flags: review?(mwu)
Attachment #8492381 - Flags: review?(mwu)
(Assignee)

Comment 34

3 years ago
Created attachment 8492388 [details] [diff] [review]
Set GECKO_TOOLS_PREFIX via path discovery to compile Gecko for ARM (v2)
Attachment #8492381 - Attachment is obsolete: true
Attachment #8492381 - Flags: review?(mwu)
Attachment #8492388 - Flags: review?(mwu)

Comment 35

3 years ago
Comment on attachment 8492388 [details] [diff] [review]
Set GECKO_TOOLS_PREFIX via path discovery to compile Gecko for ARM (v2)

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

::: core/combo/TARGET_linux-arm.mk
@@ +50,5 @@
> +ifeq ($(strip $(GECKO_TOOLS_PREFIX)),)
> +    # Look for the existence of a better toolchain
> +    gcc_toolchain_path := $(firstword $(wildcard prebuilts/gcc/$(HOST_PREBUILT_TAG)/arm/arm-linux-androideabi*/bin))
> +    ifneq ($(strip $(wildcard $(gcc_toolchain_path))),)
> +        GECKO_TOOLS_PREFIX = $(gcc_toolchain_path)/arm-linux-androideabi-

Use := here.
Attachment #8492388 - Flags: review?(mwu) → review+
(Assignee)

Comment 36

3 years ago
Created attachment 8492397 [details] [diff] [review]
Set GECKO_TOOLS_PREFIX via path discovery to compile Gecko for ARM (v3)
Attachment #8492388 - Attachment is obsolete: true
Comment on attachment 8490386 [details] [diff] [review]
Add gcc-4.7 and gcc-4.8 to the list of compilers that need to define __dso_handle symbol with the correct visibility

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

While it was kind of okayish to do that for a few old versions b2g is stuck with, it's not okay to do that with more widespread recent versions. In particular, this would affect linux desktop builds, as well as android builds that don't have the dso_handle problem.
It's time to switch to detecting whether the problem exists instead of relying on a gcc version, since the crt files are not tied to the gcc version on android/gonk.
Attachment #8490386 - Flags: review?(mh+mozilla) → review-
(Assignee)

Comment 38

3 years ago
Ok, I just realized that we are compiling crt stuff with the GLOBAL/DEFAULT visibility for __dso_handle symbols because ICS uses gcc-4.4 and this is the default. So we don't really need to add any check to configure.in. Anyway, JB/KK Gonk are compiled with gcc-4.6 or greater and here the default visibility is HIDDEN/GLOBAL so for those use-cases we would still need the autodetect mechanism that Mike is proposing, but maybe we could handle this issue in another bug and try to fix ICS builds so far. 
Of course, I'd love to work in that bug too.
(Assignee)

Comment 39

3 years ago
Created attachment 8496603 [details] [diff] [review]
Updated ICS manifests to point new bionic branches and gcc-4.8 prebuilts

There are other ICS devices that uses other remotes to download their Bionic, like: Tarako, Fugu and Pandaboard. The latter gets its copy from CAF, but uses another revision (aosp-new/ics-plus-aosp).
What should we do with them?
Attachment #8496603 - Flags: review?(mwu)

Comment 40

3 years ago
Pandaboard can be forced to use the bionic we have here. It's also not an important device to support - we only added support since we were looking into using them for testing.

Fugu is deprecated and no products shipped from that - we can drop that manifest entirely.

Tarako is a bit tricky.. we can probably create a fork just for that.

Comment 41

3 years ago
Comment on attachment 8496603 [details] [diff] [review]
Updated ICS manifests to point new bionic branches and gcc-4.8 prebuilts

Per IRC, waiting for a new patch.
Attachment #8496603 - Flags: review?(mwu)
Blocks: 1077549
No longer blocks: 1056407
(Assignee)

Comment 42

3 years ago
Comment on attachment 8496603 [details] [diff] [review]
Updated ICS manifests to point new bionic branches and gcc-4.8 prebuilts

As talked on IRC, we can update devices later so let's update these ones first: Emulator/s, Galaxy Nexus, Galaxy S2,Hamachi, Helix, Leo, Nexus-S 4g and Nexus-S.
As soon as Geeksphone guys merge the PR I launched them (to update their platfom_build fork), I will update Peak and Keon too.
So only 3 devices left: Tarako (the only who really matters), Fugu, and Pandaboard.
Attachment #8496603 - Flags: review?(mwu)

Comment 43

3 years ago
Comment on attachment 8496603 [details] [diff] [review]
Updated ICS manifests to point new bionic branches and gcc-4.8 prebuilts

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

r=me if you also add the OSX version of the toolchain.
Attachment #8496603 - Flags: review?(mwu) → review+
btw, the Firefox for Android build machine still uses gcc 4.7 (bug 899405), but could be updated to use gcc 4.8 (bug 900797) with a little work.
I guess it's a little too early to suggest GCC 4.9, but it does have 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
(Assignee)

Comment 46

3 years ago
Created attachment 8508931 [details] [review]
gonk-misc pull-requests on Github
(Assignee)

Comment 47

3 years ago
Created attachment 8508933 [details] [review]
Platform_build pull-request on Github

Comment 48

3 years ago
gonk-misc and bionic changes merged. Haven't merged the platform_build changes since they're a little messy looking - I think we can fold them together before merging.

https://github.com/mozilla-b2g/gonk-misc/commit/314f305d3163cc094e6fe7701d95a98fc180b639
https://github.com/mozilla-b2g/platform_bionic/commit/d3e9d73b6bed5363b440bd9d1871d0a87b1f0813
https://github.com/mozilla-b2g/platform_bionic/commit/40eb0ecb54c75b055af14d017892c0aefdc64d4a

Comment 49

3 years ago
platform_build PR merged.

https://github.com/mozilla-b2g/platform_build/commit/70eb0cb0977d6295e7da8896f9efb9f3ca1c13ea
(Assignee)

Comment 50

3 years ago
Created attachment 8508987 [details] [review]
b2g-manifest pull-request on Github
(Assignee)

Comment 51

3 years ago
Gary,
We can fill a new bug to update to gcc-4.9. 
With this bug landed, it will be fairly easy to test and update.
Blocks: 1087161
> We can fill a new bug to update to gcc-4.9. 
> With this bug landed, it will be fairly easy to test and update.

Thanks, filed bug 1087161.
(Assignee)

Updated

3 years ago
Depends on: 1087192
(Assignee)

Updated

3 years ago
Depends on: 1089194
(Assignee)

Comment 53

3 years ago
Build tests passed!
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=eb36b4f8c541

We can merge the final PR (comment #50) and close the bug.
Flags: needinfo?(mwu)
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Reporter)

Comment 54

3 years ago
Master: https://github.com/mozilla-b2g/b2g-manifest/commit/53a09b32c170d171e484cc8d14d1e87645cc2389

And a CLOBBER touch to be safe:
https://hg.mozilla.org/integration/b2g-inbound/rev/7aca153e69f0
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-b2g-v2.2: --- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S8 (7Nov)
(Assignee)

Comment 55

3 years ago
Created attachment 8516220 [details] [review]
b2g-manifest pull-request on Github - Build Bustage fix
(Reporter)

Comment 56

3 years ago
Master: https://github.com/mozilla-b2g/b2g-manifest/commit/6a0e874efc38e560d6f38b7da431798f5e878b44
So, for the record, what version of GCC are the B2G builds using now? Also, perhaps there should be a PSA about this on mozilla.dev.platform as a heads up for the people who were waiting for GCC 4.4 to die ;)

Comment 58

3 years ago
Landed and backed out due to hamachi red.
Status: RESOLVED → REOPENED
Flags: needinfo?(mwu)
Resolution: FIXED → ---
(Assignee)

Comment 59

3 years ago
UPDATE: 
* I'm trying to figure out why 3 cppunitests are crashing with gcc-4.8 (emulator build)
* Hamachi build are working ok locally, so it looks like we need to install a package that is not being installed into the build servers.
(Assignee)

Comment 60

3 years ago
Well, looks like Hamachi builds are not working anymore with gcc-4.8. Ineed, I'm hitting a compiler bug [1] that was fixed and landed time ago in the gcc mainstream, but looks like the google/caf toolchain that I'm using didn't get the fix.
The bug was induced by Libopus (media/libopus),so I'll try to make a patch to overcome the code that is causing the compiler bug until the toolchain gets fixed (I'll report it).

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56797

Updated

3 years ago
Blocks: 1102195
(Assignee)

Comment 61

3 years ago
I have finally come with a reduced testcase for the ICE and submited a bug report to Google https://code.google.com/p/android/issues/detail?id=80723

Let's see what happens.
(Assignee)

Comment 62

3 years ago
Created attachment 8529251 [details] [diff] [review]
0001-Bug-1056337-Upgrade-toolchain-libopus-gcc-4-8-ICE.patch

This is a temporary patch for overcoming the ICE with GCC-4.8 in OPUS library.
Attachment #8529251 - Flags: review?(giles)
Comment on attachment 8529251 [details] [diff] [review]
0001-Bug-1056337-Upgrade-toolchain-libopus-gcc-4-8-ICE.patch

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

Approach is fine. Thanks for reducing the problem to the specific routine the compiler fails on. Please address comments and ask for review again.

::: media/libopus/gcc-4.8-ICE.patch
@@ +18,5 @@
> +    return codedBands;
> + }
> + 
> ++#if !defined(__clang__) && defined(__GNUC__) && \
> ++    __GNUC__ == 4 && __GNUC_MINOR__ == 8

Is there some way to limit this to arm, b2g or ICS builds? It's a shame to slow down other targets where gcc 4.8 works fine.

::: media/libopus/update.sh
@@ +74,4 @@
>  python gen-sources.py $1
>  
>  # apply outstanding local patches
> +patch -p3 < $1/gcc-4.8-ICE.patch

patch should be in the source directory, so you want ./ or $(dirname $0)/ instead of $1, which is the upstream source directory.

You also need to apply the patch to the in-tree opus code, either manually, or ideally by running update.sh against an opus checkout of the revision we're currently using (v1.1 tag).

update.sh is about future-proofing your change; you still have to make the change and commit it to the tree for it to take effect now.
Attachment #8529251 - Flags: review?(giles)
(Assignee)

Comment 64

3 years ago
(In reply to Ralph Giles (:rillian) from comment #63)
> Comment on attachment 8529251 [details] [diff] [review]
> 0001-Bug-1056337-Upgrade-toolchain-libopus-gcc-4-8-ICE.patch
> 
> Review of attachment 8529251 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Approach is fine. Thanks for reducing the problem to the specific routine
> the compiler fails on. Please address comments and ask for review again.
> 
> ::: media/libopus/gcc-4.8-ICE.patch
> @@ +18,5 @@
> > +    return codedBands;
> > + }
> > + 
> > ++#if !defined(__clang__) && defined(__GNUC__) && \
> > ++    __GNUC__ == 4 && __GNUC_MINOR__ == 8
> 
> Is there some way to limit this to arm, b2g or ICS builds? It's a shame to
> slow down other targets where gcc 4.8 works fine.
> 

We will get the ICE in all ARM platforms using this gcc-4.8 version, so I think that the most appropiate tune will be add the arm architecture to the condition.

> ::: media/libopus/update.sh
> @@ +74,4 @@
> >  python gen-sources.py $1
> >  
> >  # apply outstanding local patches
> > +patch -p3 < $1/gcc-4.8-ICE.patch
> 
> patch should be in the source directory, so you want ./ or $(dirname $0)/
> instead of $1, which is the upstream source directory.
> 
> You also need to apply the patch to the in-tree opus code, either manually,
> or ideally by running update.sh against an opus checkout of the revision
> we're currently using (v1.1 tag).
> 
> update.sh is about future-proofing your change; you still have to make the
> change and commit it to the tree for it to take effect now.

Nice, I have added the fix of the offending file to the patch.
(Assignee)

Comment 65

3 years ago
Created attachment 8529334 [details] [diff] [review]
0001-Bug-1056337-Upgrade-toolchain-libopus-gcc-4-8-ICE.v2.patch
Attachment #8529251 - Attachment is obsolete: true
Attachment #8529334 - Flags: review?(giles)
Comment on attachment 8529334 [details] [diff] [review]
0001-Bug-1056337-Upgrade-toolchain-libopus-gcc-4-8-ICE.v2.patch

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

Looks good to me. Thanks for the fix!
Attachment #8529334 - Flags: review?(giles) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Keywords: leave-open
(Assignee)

Comment 67

3 years ago
Please land just the last patch:  0001-Bug-1056337-Upgrade-toolchain-libopus-gcc-4-8-ICE.v2.patch
Pushed the last patch to inbound after fixing the commit message.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d1cbbd46b0d
Keywords: checkin-needed
(Assignee)

Comment 69

3 years ago
I wanted to paste the try-server link before pushing, but I don't know why it took A LOT more time to complete as usual...

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f08b168c851b
https://hg.mozilla.org/mozilla-central/rev/1d1cbbd46b0d
(In reply to Juan Gomez [:_AtilA_] from comment #69)
> I wanted to paste the try-server link before pushing, but I don't know why
> it took A LOT more time to complete as usual...

We had infrastructure problems for most of today. No jobs were running.
Blocks: 1104663
Comment on attachment 8529334 [details] [diff] [review]
0001-Bug-1056337-Upgrade-toolchain-libopus-gcc-4-8-ICE.v2.patch

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

::: media/libopus/gcc-4.8-ICE.patch
@@ +17,5 @@
> + }
> + 
> ++#if !defined(__clang__) && defined(__GNUC__) && defined(__arm__) && \
> ++    __GNUC__ == 4 && __GNUC_MINOR__ == 8
> ++#warning "OPUS library causes an internal compiler error for gcc-4.8 based toolchain in arm"

btw, the gcc bug report [1] suggests this compiler bug was fixed in upstream gcc 4.8.2, so you might be able to amend this version check to include `__GNUC_PATCHLEVEL__ >= 2` so this deoptimization automatically disables itself when compiling with a future, fixed gcc 4.8.x. :)

Also, a patch [2] on bug 1104663 suggests -Os may avoid the compiler bug without falling back to -O0.

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58394
[2] https://bugzilla.mozilla.org/attachment.cgi?id=8528890&action=diff
(Assignee)

Updated

3 years ago
Depends on: 1108228

Comment 73

3 years ago
Has there been any progress on this lately?  I can't wait to see this awesome change make its way into the trees! :-)
(Assignee)

Comment 74

3 years ago
Yeah sorry! I've been very busy these days :(
Right now, I'm trying to figure out why one of the cpp unit tests is failing witch gcc-4.8 (and gcc-4.7): test_tone (gecko/media/libcubeb/tests/)
As bug 1108228 shows, Bionic is segfaulting when the program finally exits.
I've tried with Valgrind as well but nothing wrong seems to be happening...
I'm running out of ideas!! So any help would be appreciated :)
If you're hung up on a single C++ unit test, I'd recommend just disabling it for B2G and investigating afterwards. It's not fantastic, but this change is blocking a lot of other work so I think it's a worthwhile tradeoff.
(Assignee)

Comment 76

3 years ago
Created attachment 8539831 [details] [review]
B2G ICS Emulator only changes to upgrade the toolchain (gcc-4.8)

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=290ba22b2867
Attachment #8539831 - Flags: review?(mwu)

Updated

3 years ago
Attachment #8539831 - Flags: review?(mwu) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Comment 77

3 years ago
Please, checkin the last patch only (attachment 8539831 [details] [review]) :)
Hi Juan,

is the try failure in cpp unitests realted to this checkin ?
Flags: needinfo?(atilag)
Keywords: checkin-needed
(Assignee)

Comment 79

3 years ago
Yes, this is the cppunit test that fails. Bug 1108228 was filled to find out what's happening.
Flags: needinfo?(atilag)
Blocks: 1114703

Comment 80

3 years ago
I agree with Ted in disabling this one last test and figuring out what's happening there in bug 1108228.  Wonderful work here, Juan!
(Assignee)

Comment 81

3 years ago
Created attachment 8541522 [details] [diff] [review]
Disable test_tone cpptest for B2G ICS devices

As Ted mentioned in bug 1108228 - comment #6 , this patch will disable test_tone for B2G/ICS devices. Thanks Ted!

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=79c553afe212

Happy Holidays btw! ;)
Attachment #8541522 - Flags: review?(mwu)

Updated

3 years ago
Attachment #8541522 - Flags: review?(mwu) → review?(kinetik)
Blocks: 1115910
Blocks: 1058561
Attachment #8541522 - Flags: review?(kinetik) → review+
(Assignee)

Comment 82

3 years ago
I'm about to close bug 1108228, so this patch won't need to land.

Comment 83

3 years ago
So can we now reland everything here?
(Assignee)

Comment 84

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f9a41dd22da
Ok, tests passed for the emulator.
We can land patch:  B2G ICS Emulator only changes to upgrade the toolchain (gcc-4.8).
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
Wow, that's great news! Thanks so much for your hard work on this!

<breaks out the champagne>
(In reply to Juan Gomez [:_AtilA_] from comment #84)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f9a41dd22da
> Ok, tests passed for the emulator.
> We can land patch:  B2G ICS Emulator only changes to upgrade the toolchain
> (gcc-4.8).

landed this in https://github.com/mozilla-b2g/b2g-manifest/commit/5bce19fd07b01f2f66cd64f5387764b07842ac48
Thanks for seeing this through!
(Reporter)

Comment 88

3 years ago
And a clobber touch (since at least the first ICS emulator opt job I've seen on b-i still shows 4.4 being used).
https://hg.mozilla.org/integration/b2g-inbound/rev/30e4856f503e
Flags: needinfo?(gal)
Keywords: checkin-needed, leave-open
Target Milestone: 2.1 S8 (7Nov) → 2.2 S3 (9jan)
https://hg.mozilla.org/mozilla-central/rev/30e4856f503e
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
(Reporter)

Comment 90

3 years ago
Device image builds haven't made the change yet. Reopening to avoid confusion.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 91

3 years ago
Created attachment 8546802 [details] [review]
Update Hamachi manifest to use new gcc-4.8 based toolchain

This PR modifies the Hamachi manifest only. I cannot launch try-server jobs for devices, so if it burns the tree, just backit out and if I'm not online, please paste/send me the result.
I sucessfully build in my local machine with a fresh build.
Attachment #8546802 - Flags: review?(mwu)

Comment 92

3 years ago
Comment on attachment 8546802 [details] [review]
Update Hamachi manifest to use new gcc-4.8 based toolchain

Ok, but don't close this bug until every ICS device is switched over.
Attachment #8546802 - Flags: review?(mwu) → review+
(Assignee)

Comment 93

3 years ago
Please land only this very last patch.
So, as said. If this patch fails (most probably), backit out and send me the errors. :)
Keywords: checkin-needed, leave-open
(Reporter)

Comment 94

3 years ago
Master: https://github.com/mozilla-b2g/b2g-manifest/commit/d038fbc528a1326da53a75f3d829a0834f017b46

To those playing from home, we're still waiting for a patch to change over all the other device image builds still. This only changes Hamachi builds.
Keywords: checkin-needed
(Reporter)

Comment 95

3 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #94)

Reverted for Hamachi bustage.
Master: https://github.com/mozilla-b2g/b2g-manifest/commit/0ebf1a21491c1c323e828a405b2a0ce543a7b77a

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=1132723&repo=b2g-inbound

Looks like it was attempting to use gcc-4.8 :(
(Reporter)

Updated

3 years ago
Depends on: 1119997
(Assignee)

Comment 96

3 years ago
Created attachment 8546920 [details] [review]
PR to migrate all ICS devices

As per IRC conversation, this is the PR to migrate all important ICS devices.
Attachment #8546920 - Flags: review?(mwu)

Updated

3 years ago
Attachment #8546920 - Flags: review?(mwu) → review+
(Reporter)

Comment 97

3 years ago
Master: https://github.com/mozilla-b2g/b2g-manifest/commit/575867838fa93f8537e683e39a575342a2e2e179
Keywords: leave-open
Note, I wonder, does this change mean that people building on mac get to download an additional linux toolchain they don't care about?

Comment 99

3 years ago
On ICS, you always had to download every version of every toolchain on every platform. All the toolchains plus some other things were stuffed into the prebuilt repo. So yes, but the addition of this toolchain doesn't hurt much on ICS.

The prebuilt repo was removed after ICS and replaced with individual toolchain repos in the prebuilts directory.
(Reporter)

Comment 100

3 years ago
And we're green on all ICS-based device image builds \m/
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
(Reporter)

Comment 101

3 years ago
I had to push a bustage fix for Wasabi builds too.
https://github.com/mozilla-b2g/b2g-manifest/commit/fd66dfdead833452aa3d8a1024ca9e27ddc206ad
Created attachment 8547096 [details]
Build failing with Keon

Juan, thanks for your work. It looks like Keon build is currently broken. I get that you want to move Peak and Keon devices to GCC 4.8 also, do you have any ETA on this?
Flags: needinfo?(atilag)
(Assignee)

Comment 103

3 years ago
Hi Alex, Geeksphone has it's own fork of platform_build, so they must merge my changes in order to build. Anyway, I'm going to facilitate their job and PR they repos with my changes (I did it time ago, but they didn't respond). As soon as they merge the changes, I'll update the manifests (Keon and Peak).
Flags: needinfo?(atilag)
I've had issues contacting GP in the past so I got in touch with them now through the same channel that worked for me last time.

And now as it happens, their platform_build repo has been updated with your patch.
Flags: needinfo?(atilag)
(Assignee)

Comment 105

3 years ago
Oh! ok, they didn't cherry-pick my commit hehehe.
There's also a bug compiling Peak (I don't know about Keon, but probably) that I'll fix here: bug 1120239 ASAP.
Flags: needinfo?(atilag)
Duplicate of this bug: 1104663
Blocks: 1372751
You need to log in before you can comment on or make changes to this bug.