text: 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.
  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.
  > we should change the toolchain to build just gecko. This has been suggested countless times. I'm glad this is finally going to happen.
  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... :)
  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.
  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.
  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!
  Thks Ehsan! No, I didn't talk with anyone from CAF... Do we need to?.
  (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!
  We have no active master-based ICS builds so it doesn't matter to us really.
  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?
  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.
  (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.
  Ehsan, ICS can't be killed because we don't have testing running on KK or JB.
  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.
  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) >. 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"
  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: There's no clear indication of what specific changeset fixed it.
  Well, actually, there is one.
  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.
  b2g_ics_strawberry and b2g-4.0.4_r2.1 created on mozilla-b2g/platform_bionic.
  Created attachment 8489108 [details] platform_bionic PRs for both branches: b2g_ics_strawberry and b2g-4.0.4_r2.1.
  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!
  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
  oh! didn't set Mike for review because I thought he was on PTO :)
  Created attachment 8491851 [details] [diff] [review] Set GECKO_TOOLS_PREFIX via path discovery to compile Gecko (for ARM)
  Created attachment 8491852 [details] [diff] [review] Use GECKO_TOOLS_PREFIX variable to compile Gecko
  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.
  Created attachment 8492376 [details] [diff] [review] Use GECKO_TOOLS_PREFIX variable to compile Gecko (v2)
  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)
  Created attachment 8492388 [details] [diff] [review] Set GECKO_TOOLS_PREFIX via path discovery to compile Gecko for ARM (v2)
  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.
  Created attachment 8492397 [details] [diff] [review] Set GECKO_TOOLS_PREFIX via path discovery to compile Gecko for ARM (v3)
  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.
  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.
  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?
  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 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.
  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.
  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.
  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] [2]
  Created attachment 8508931 [details] [review] gonk-misc pull-requests on Github
  Created attachment 8508933 [details] [review] Platform_build pull-request on Github
  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.
  platform_build PR merged.
  Created attachment 8508987 [details] [review] b2g-manifest pull-request on Github
  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.
  > 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.
  Build tests passed! We can merge the final PR (comment #50) and close the bug.
  Master: And a CLOBBER touch to be safe:
  Created attachment 8516220 [details] [review] b2g-manifest pull-request on Github - Build Bustage fix
  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 ;)
  Landed and backed out due to hamachi red.
  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.
  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]
  I have finally come with a reduced testcase for the ICE and submited a bug report to Google Let's see what happens.
  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..
  (In reply to Ralph Giles (:rillian) from comment #63) >. > 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. > :::. Nice, I have added the fix of the offending file to the patch.
  Created attachment 8529334 [details] [diff] [review] 0001-Bug-1056337-Upgrade-toolchain-libopus-gcc-4-8-ICE.v2.patch
  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!
  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.
  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...
  (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 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] [2]
  Has there been any progress on this lately? I can't wait to see this awesome change make its way into the trees! :-)
  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.
  Created attachment 8539831 [details] [review] B2G ICS Emulator only changes to upgrade the toolchain (gcc-4.8)
  Please, checkin the last patch only (attachment 8539831 [details] [review]) :)
  Hi Juan, is the try failure in cpp unitests realted to this checkin ?
  Yes, this is the cppunit test that fails. Bug 1108228 was filled to find out what's happening.
  I agree with Ted in disabling this one last test and figuring out what's happening there in bug 1108228. Wonderful work here, Juan!
  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! Happy Holidays btw! ;)
  I'm about to close bug 1108228, so this patch won't need to land.
  So can we now reland everything here? Ok, tests passed for the emulator. We can land patch: B2G ICS Emulator only changes to upgrade the toolchain (gcc-4.8).
  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) > > 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
  Thanks for seeing this through!
  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).
  Device image builds haven't made the change yet. Reopening to avoid confusion.
  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.
  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.
  Please land only this very last patch. So, as said. If this patch fails (most probably), backit out and send me the errors. :)
  Master: 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.
  (In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #94) Reverted for Hamachi bustage. Master: Looks like it was attempting to use gcc-4.8 :(
  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.
  Master:
  Note, I wonder, does this change mean that people building on mac get to download an additional linux toolchain they don't care about?
  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.
  And we're green on all ICS-based device image builds \m/
  I had to push a bustage fix for Wasabi builds too.
  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?
  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).
  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.
  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.