Closed
Bug 1056337
Opened 10 years ago
Closed 9 years ago
Investigate upgrading the toolchain used for B2G ICS builds (and stop using GCC 4.4)
Categories
(Firefox OS Graveyard :: General, defect)
Tracking
(b2g-v2.2 fixed)
RESOLVED
FIXED
2.2 S3 (9jan)
Tracking | Status | |
---|---|---|
b2g-v2.2 | --- | fixed |
People
(Reporter: RyanVM, Assigned: _AtilA_)
References
Details
Attachments
(15 files, 5 obsolete files)
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 | |
53 bytes,
text/x-github-pull-request
|
Details | Review | |
52 bytes,
text/x-github-pull-request
|
Details | Review | |
52 bytes,
text/x-github-pull-request
|
Details | Review | |
3.94 KB,
patch
|
rillian
:
review+
|
Details | Diff | Splinter Review |
52 bytes,
text/x-github-pull-request
|
mwu
:
review+
|
Details | Review |
1.05 KB,
patch
|
kinetik
:
review+
|
Details | Diff | Splinter Review |
52 bytes,
text/x-github-pull-request
|
mwu
:
review+
|
Details | Review |
52 bytes,
text/x-github-pull-request
|
mwu
:
review+
|
Details | Review |
1.48 KB,
text/plain
|
Details |
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.
Comment 1•10 years ago
|
||
I thought this used the android tolls which I thought we used the android toolset t gcc 4.7 minimum anyway,
Comment 2•10 years ago
|
||
(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•10 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•10 years ago
|
Assignee: nobody → atilag
Comment 4•10 years ago
|
||
> we should change the toolchain to build just gecko.
This has been suggested countless times. I'm glad this is finally going to happen.
Assignee | ||
Comment 5•10 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•10 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•10 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•10 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•10 years ago
|
||
Thks Ehsan! No, I didn't talk with anyone from CAF... Do we need to?
Flags: needinfo?(atilag)
Assignee | ||
Comment 10•10 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•10 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)
Comment 12•10 years ago
|
||
We have no active master-based ICS builds so it doesn't matter to us really.
Assignee | ||
Comment 13•10 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•10 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.
Updated•10 years ago
|
Flags: needinfo?(mvines)
Comment 15•10 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•10 years ago
|
||
Ehsan, ICS can't be killed because we don't have testing running on KK or JB.
Reporter | ||
Comment 17•10 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•10 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.
Comment 19•10 years ago
|
||
(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•10 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.
Comment 21•10 years ago
|
||
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.
Comment 22•10 years ago
|
||
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•10 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•10 years ago
|
||
b2g_ics_strawberry and b2g-4.0.4_r2.1 created on mozilla-b2g/platform_bionic.
Assignee | ||
Comment 25•10 years ago
|
||
Attachment #8489108 -
Flags: review?(mwu)
Comment 26•10 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•10 years ago
|
||
Attachment #8490386 -
Flags: review?(ted)
Updated•10 years ago
|
Attachment #8490386 -
Flags: review?(ted) → review?(mh+mozilla)
Assignee | ||
Comment 28•10 years ago
|
||
oh! didn't set Mike for review because I thought he was on PTO :)
Assignee | ||
Comment 29•10 years ago
|
||
Attachment #8491851 -
Flags: review?(mwu)
Assignee | ||
Comment 30•10 years ago
|
||
Attachment #8491852 -
Flags: review?(mwu)
Comment 31•10 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•10 years ago
|
||
Attachment #8491852 -
Attachment is obsolete: true
Attachment #8491852 -
Flags: review?(mwu)
Attachment #8492376 -
Flags: review?(mwu)
Updated•10 years ago
|
Attachment #8492376 -
Flags: review?(mwu) → review+
Assignee | ||
Comment 33•10 years ago
|
||
Attachment #8491851 -
Attachment is obsolete: true
Attachment #8491851 -
Flags: review?(mwu)
Attachment #8492381 -
Flags: review?(mwu)
Assignee | ||
Comment 34•10 years ago
|
||
Attachment #8492381 -
Attachment is obsolete: true
Attachment #8492381 -
Flags: review?(mwu)
Attachment #8492388 -
Flags: review?(mwu)
Comment 35•10 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•10 years ago
|
||
Attachment #8492388 -
Attachment is obsolete: true
Comment 37•10 years ago
|
||
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•10 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•10 years ago
|
||
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•10 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•10 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)
Assignee | ||
Comment 42•10 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•10 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+
Comment 44•10 years ago
|
||
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.
Comment 45•10 years ago
|
||
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•10 years ago
|
||
Assignee | ||
Comment 47•10 years ago
|
||
Comment 48•10 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•10 years ago
|
||
platform_build PR merged. https://github.com/mozilla-b2g/platform_build/commit/70eb0cb0977d6295e7da8896f9efb9f3ca1c13ea
Assignee | ||
Comment 50•10 years ago
|
||
Assignee | ||
Comment 51•10 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.
Comment 52•10 years ago
|
||
> 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 | ||
Comment 53•10 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•10 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 54•10 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
Closed: 10 years ago
status-b2g-v2.2:
--- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S8 (7Nov)
Assignee | ||
Comment 55•10 years ago
|
||
Reporter | ||
Comment 56•10 years ago
|
||
Master: https://github.com/mozilla-b2g/b2g-manifest/commit/6a0e874efc38e560d6f38b7da431798f5e878b44
Comment 57•10 years ago
|
||
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•10 years ago
|
||
Landed and backed out due to hamachi red.
Status: RESOLVED → REOPENED
Flags: needinfo?(mwu)
Resolution: FIXED → ---
Assignee | ||
Comment 59•10 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•10 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
Assignee | ||
Comment 61•9 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•9 years ago
|
||
This is a temporary patch for overcoming the ICE with GCC-4.8 in OPUS library.
Attachment #8529251 -
Flags: review?(giles)
Comment 63•9 years ago
|
||
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•9 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•9 years ago
|
||
Attachment #8529251 -
Attachment is obsolete: true
Attachment #8529334 -
Flags: review?(giles)
Comment 66•9 years ago
|
||
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•9 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•9 years ago
|
Keywords: leave-open
Assignee | ||
Comment 67•9 years ago
|
||
Please land just the last patch: 0001-Bug-1056337-Upgrade-toolchain-libopus-gcc-4-8-ICE.v2.patch
Comment 68•9 years ago
|
||
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•9 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
Comment 71•9 years ago
|
||
(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.
Comment 72•9 years ago
|
||
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
Comment 73•9 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•9 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 :)
Comment 75•9 years ago
|
||
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•9 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=290ba22b2867
Attachment #8539831 -
Flags: review?(mwu)
Updated•9 years ago
|
Attachment #8539831 -
Flags: review?(mwu) → review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 77•9 years ago
|
||
Please, checkin the last patch only (attachment 8539831 [details] [review]) :)
Comment 78•9 years ago
|
||
Hi Juan, is the try failure in cpp unitests realted to this checkin ?
Flags: needinfo?(atilag)
Keywords: checkin-needed
Assignee | ||
Comment 79•9 years ago
|
||
Yes, this is the cppunit test that fails. Bug 1108228 was filled to find out what's happening.
Flags: needinfo?(atilag)
Comment 80•9 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•9 years ago
|
||
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•9 years ago
|
Attachment #8541522 -
Flags: review?(mwu) → review?(kinetik)
Updated•9 years ago
|
Attachment #8541522 -
Flags: review?(kinetik) → review+
Assignee | ||
Comment 82•9 years ago
|
||
I'm about to close bug 1108228, so this patch won't need to land.
Comment 83•9 years ago
|
||
So can we now reland everything here?
Assignee | ||
Comment 84•9 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•9 years ago
|
Keywords: checkin-needed
Comment 85•9 years ago
|
||
Wow, that's great news! Thanks so much for your hard work on this! <breaks out the champagne>
Comment 86•9 years ago
|
||
(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
Comment 87•9 years ago
|
||
Thanks for seeing this through!
Reporter | ||
Comment 88•9 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
Closed: 10 years ago → 9 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 90•9 years ago
|
||
Device image builds haven't made the change yet. Reopening to avoid confusion.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 91•9 years ago
|
||
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•9 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•9 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•9 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•9 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 :(
Assignee | ||
Comment 96•9 years ago
|
||
As per IRC conversation, this is the PR to migrate all important ICS devices.
Attachment #8546920 -
Flags: review?(mwu)
Updated•9 years ago
|
Attachment #8546920 -
Flags: review?(mwu) → review+
Reporter | ||
Comment 97•9 years ago
|
||
Master: https://github.com/mozilla-b2g/b2g-manifest/commit/575867838fa93f8537e683e39a575342a2e2e179
Keywords: leave-open
Comment 98•9 years ago
|
||
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•9 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•9 years ago
|
||
And we're green on all ICS-based device image builds \m/
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 101•9 years ago
|
||
I had to push a bustage fix for Wasabi builds too. https://github.com/mozilla-b2g/b2g-manifest/commit/fd66dfdead833452aa3d8a1024ca9e27ddc206ad
Comment 102•9 years ago
|
||
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•9 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)
Comment 104•9 years ago
|
||
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•9 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)
You need to log in
before you can comment on or make changes to this bug.
Description
•