Closed Bug 1264836 Opened 8 years ago Closed 4 years ago

bigendian support for ICU

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox80 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla80
Tracking Flags:
Tracking Status
firefox80 --- fixed

People

(Reporter: stevensn, Assigned: glandium)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1264836 - Automatically convert the little-endian ICU data file for big-endian builds.
47 bytes, text/x-phabricator-request
Details | Review
Additional patch for 78
526 bytes, patch
Details | Diff | Splinter Review 
See the https://bugzilla.mozilla.org/show_bug.cgi?id=1239083#c32

Since Bug 1239083 builds on my big endian machine (ppc64) have been crashing at startup

  ./dist/bin/firefox --new-instance
[29484] ###!!! ABORT: JS_InitWithFailureDiagnostic: u_init() failed: file /media/nfs/usb_drive_src/firefox/mozilla-central-hg/src/xpcom/build/XPCOMInit.cpp, line 709
[29484] ###!!! ABORT: JS_InitWithFailureDiagnostic: u_init() failed: file /media/nfs/usb_drive_src/firefox/mozilla-central-hg/src/xpcom/build/XPCOMInit.cpp, line 709
Segmentation fault (core dumped)
Huh, we definitely should fail at configure time.
Yeah, I didn't have an endianness check handy when I was writing the patch, but we could expose that now and make this into an error:
https://dxr.mozilla.org/mozilla-central/rev/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/build/autoconf/icu.m4#79

It should just tell you to either use --with-system-icu or --without-intl-api.
We could maybe add an option to just feed a big-endian data file, and still build icu from our code base. Not sure how valuable that would be.
--with-system-icu  on a system with icu 54 doesn't work.  js/src/builtin/Intl.cpp has a few things in it that seem to need a newer ICU (maybe we should update the ICU version  in the configure check)

Things build against a locally installed ICU 56 using --with-system-icu but I suspect this will cause a lot of pain for downstream packagers on platoforms that have the older ICU.

I think Mike's idea of an option to feed in a big endian datafile would be valuable but someone actually involved in packaging firefox for downstream ppc64 distributions should comment.
I am getting the same segfault on an i7 (afaik little endian)


$ /usr/bin/firefox 
[7563] ###!!! ABORT: JS_InitWithFailureDiagnostic: u_init() failed: file /build/firefox-HNVPVM/firefox-48.0~a2~hg20160509r318685/xpcom/build/XPCOMInit.cpp, line 709
[7563] ###!!! ABORT: JS_InitWithFailureDiagnostic: u_init() failed: file /build/firefox-HNVPVM/firefox-48.0~a2~hg20160509r318685/xpcom/build/XPCOMInit.cpp, line 709
Segmentation fault (core dumped)

Using the nightly PPA - http://ppa.launchpad.net/ubuntu-mozilla-daily/firefox-aurora/ubuntu


System
------

kernel: Linux 4.4.0-22-generic #39-Ubuntu SMP Thu May 5 16:53:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
distro: Ubuntu 16.04 LTS
cpu: Intel(R) Core(TM) i7-4712HQ CPU @ 2.30GHz
same here.
The output
> ABORT: JS_InitWithFailureDiagnostic: u_init() failed: file /firefox-48.0~a2~hg20160509r318685/xpcom/build/XPCOMInit.cpp, line 709")
is mentioned in bug 1262731, bug 1264836, bug 1272246, bug 1272209. (In case some might be dups.)
icupkg can create big endian data from little endian data.  But icupkg generates error when converting our data file...
Reported to ICU team.
See Also: → http://bugs.icu-project.org/trac/ticket/12657
I ran into this issue while building FF for Solaris on sparc. And yes icupkg fail. It's failing because of 'confusables' record. So following works:

icupkg -tb -r confusables.cfu icudt58l.dat

At this moment I use this:

--- firefox-52.0b8/build/autoconf/icu.m4.orig   2017-03-01 04:31:04.958804345 +0000
+++ firefox-52.0b8/build/autoconf/icu.m4        2017-03-01 04:38:54.277500982 +0000
@@ -75,10 +75,14 @@
     fi
     MOZ_ICU_VERSION="$version"

-    # TODO: the l is actually endian-dependent
-    # We could make this set as 'l' or 'b' for little or big, respectively,
-    # but we'd need to check in a big-endian version of the file.
     ICU_DATA_FILE="icudt${version}l.dat"
+    case "$CPU_ARCH" in
+    sparc*)
+      # create big-endian data file (v58 fails with 'confusables' record)
+      icupkg -tb -r confusables.cfu $_topsrcdir/config/external/icu/data/$ICU_DATA_FILE
+      ICU_DATA_FILE="icudt${version}b.dat"
+      ;;
+    esac

     dnl We won't build ICU data as a separate file when building
     dnl JS standalone so that embedders don't have to deal with it.

Should we include some patch like this? Or what should be final resolution?
(In reply to Petr Sumbera from comment #10)
> Should we include some patch like this? Or what should be final resolution?

Not enough.  We don't put big endian's dat file into our tree because it is no way to create it on little endian environment.  If http://bugs.icu-project.org/trac/ticket/11046 is fixed, we can generate it and include it.

So if you use big endian CPU, you should use --with-system-icu in .mozconfig now.
We were having problems building FF 52esr on a s390x and ppc64, both big-endian with the same error described in the description. After a couple of weeks of intense research we were able to solve this problem. (NOTE: I develop on a x86_64 box and push to other systems to build.)

We were able to successfully create a icudt58b.dat (big-endian) data file (on a little-endian machine) and plug it into the mozilla-esr52 source (via a similar patch as provided by Petr Sumbera in comment 10) which successfully built on our big-endian machines. According to the ICU documentation these data files are architecture independent except for endianness which can be converted using the icupkg utility.

The problem is trying to convert the icudt58l.dat file provided in the mozilla-esr52 repo. I believe this file is corrupt because it is not the same size as the same file provided by ICU source (http://download.icu-project.org/files/icu4c/58.2/icu4c-58_2-src.tgz) and it produces an error when converting:

$ ~/icu/source/bin/icupkg -tb ~/mozilla-esr52/config/external/icu/data/icudt58l.dat icudt58b.dat
udata_swap(): unknown data format 00.00.00.00 ("")
icupkg: udata_swap(item 249) failed - U_UNSUPPORTED_ERROR
$ echo $?
16

However, using the data file from ICU the conversion works perfect:

$ ~/icu/source/bin/icupkg -tb ~/icu/source/data/in/icudt58l.dat icudt58b.dat
$ echo $?
0
$ ll icudt58*
-rw-r--r-- 1 me users 26213232 May 25 17:26 icudt58b.dat

We had no problems building FF52esr on big-endian machines using this icudt58b.dat data file.

You might want to consider replacing the icudt58l.dat file with the good data file form ICU and applying the patch from Petr Sumbera and it should fix this problem for everyone.
(In reply to Charles Robertson from comment #12)
> You might want to consider replacing the icudt58l.dat file with the good
> data file form ICU and applying the patch from Petr Sumbera and it should
> fix this problem for everyone.

Charles, although I add comment #11, since we have to land ICU Data file into our repository, we need generate bigendian data on littleendian PC such as macOS/x86-64 if we support this.  So if http://bugs.icu-project.org/trac/ticket/11046 is fixed, we can support it without any patches.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #2)
> Yeah, I didn't have an endianness check handy when I was writing the patch,
> but we could expose that now and make this into an error:
> https://dxr.mozilla.org/mozilla-central/rev/
> 21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/build/autoconf/icu.m4#79

I'm honestly a bit shocked to see that such a patch that breaks the build on all big-endian targets was merged without any problems. "I didn't have an endianness check handy" sounds like a very bad excuse to me. This patch shouldn't have been merged in the first place as-is.

Lots of users have run into this issue, this includes Thunderbird users. Debian's Thunderbird have racked their brains about why they weren't able to build Thunderbird on Big-Endian targets anymore.
We don't support any big-endian targets as Tier-1. Sorry. This work fixed a lot of other issues for us so it was worthwhile.
You are making quite a ridiculous argument here. Aside from Mac PowerPC, we have never supported a big-endian platform as Tier-1. We break unsupported platforms all the time in service of fixes that benefit other platforms. Your supposed slippery slope argument doesn't hold water.

People have always been free to build Firefox for unsupported platforms, but that does not mean Mozilla is under any obligation to keep those builds working. Even with this particular breakage you could continue to build Firefox --with-system-icu, so it's not terribly onerous.

I would take a patch to make configure produce an error when targeting big-endian platforms when --with-system-icu has not been specified, but I doubt we're likely to do much more than that.
> You are making quite a ridiculous argument here. Aside from Mac PowerPC, we have never supported a big-endian platform as Tier-1. We break unsupported platforms all the time in service of fixes that benefit other platforms. Your supposed slippery slope argument doesn't hold water.

Such a ridiculous argument that you have to use your admin rights to hide it.

> People have always been free to build Firefox for unsupported platforms, but that does not mean Mozilla is under any obligation to keep those builds working. Even with this particular breakage you could continue to build Firefox --with-system-icu, so it's not terribly onerous.

This is basically what's necessary to fix the build on big-endian architectures. I agree, that's quite a lot of work.

> https://anonscm.debian.org/cgit/pkg-mozilla/iceweasel.git/commit/?h=esr52/master&id=4dd8c5d6dc4d6a712f1c73d5c8e12bc372a331e1
> https://anonscm.debian.org/cgit/pkg-mozilla/iceweasel.git/commit/?h=esr52/master&id=1255e75f0cb491f34467f4c2e684b5ddec91c4ee

> I would take a patch to make configure produce an error when targeting big-endian platforms when --with-system-icu has not been specified, but I doubt we're likely to do much more than that.

You're free to do whatever you want with your own project, of course. But don't be surprised if the community loses interest over time. There have been other projects like OpenOffice and XFree86 where upstream started to ignore the community and we all know where this lead to. Either way, I will probably not contribute to Firefox anymore.
(In reply to John Paul Adrian Glaubitz from comment #18)
> > You are making quite a ridiculous argument here. Aside from Mac PowerPC, we have never supported a big-endian platform as Tier-1. We break unsupported platforms all the time in service of fixes that benefit other platforms. Your supposed slippery slope argument doesn't hold water.
> 
> Such a ridiculous argument that you have to use your admin rights to hide it.

Nobody is using admin rights here; https://wiki.mozilla.org/BMO/comment_tagging#Automatic_Collapsing_of_Comments describes what is going on.  Somebody decided to flag your comment as something an admin should look at, and Bugzilla collapses such comments by default.  All the content is still there.

Ted's correct: we have broken non-Tier 1 builds all the time.  Ask Sparc/PowerPC *BSD maintainers about Skia changes breaking their big-endian platforms.  This is simply a fact of life.  And we have taken patches for such breakages where people have been motivated enough to fix them.
(In reply to John Paul Adrian Glaubitz from comment #18)

> > People have always been free to build Firefox for unsupported platforms, but that does not mean Mozilla is under any obligation to keep those builds working. Even with this particular breakage you could continue to build Firefox --with-system-icu, so it's not terribly onerous.
> 
> This is basically what's necessary to fix the build on big-endian
> architectures. I agree, that's quite a lot of work.

Using sarcasm here wont work. Why not providing those as patches for m-c so that they get merged and everyone is happy ? Tier-3 platforms break regularly, and their respective maintainers have to deal with it, even if that's sad, that's the way things are.

> You're free to do whatever you want with your own project, of course. But
> don't be surprised if the community loses interest over time. There have
> been other projects like OpenOffice and XFree86 where upstream started to
> ignore the community and we all know where this lead to. Either way, I will
> probably not contribute to Firefox anymore.

Yeah, on powerpc, you're likely to get support from chromi^Wwebkit^W.. oh wait. Yeah, there's netsurf :)

 I stopped caring about firefox on OpenBSD/ppc some years ago to concentrate on OpenBSD/amd64+i386, you decide where you put your time if you want a graphical browser on exotic platforms...
Product: Core → Firefox Build System

(In reply to Petr Sumbera from comment #10)

I ran into this issue while building FF for Solaris on sparc. And yes icupkg
fail. It's failing because of 'confusables' record. So following works:

icupkg -tb -r confusables.cfu icudt58l.dat

I've removed the "confusables" record in bug 1569567 and posted two patches to bug 1322212 to add and process big endian ICU data files. I wasn't able to verify the generated data file works correctly (https://phabricator.services.mozilla.com/D40803#1226799), though.

Blocks: big-endian

I hit the missing icudt${ver}b.dat recently (after changes from 1635764). Looks to me that the filename is generated correctly in the buildsystem. So my question is what blocks generating the missing file on runtime with "./mach python intl/icu_sources_data.py ." (as done for example for big endian builds in Fedora)?

The main obstacle is that no one volunteered to write and test a patch. For example I don't have any big endian environments ready for testing, so my patches in bug 1322212 were all only best effort.

And in addition to calling intl/icu_sources_data.py, preferably there also needs to be some way to update the time zone data included in the ICU data file. We're currently using ICU 67, which ships with tzdata 2019c, but the most current tzdata version is 2020a. To update tzdata in ICU, there's the update-tzdata.sh shell script, but that one requires to be called with a compatible icupkg binary. (Some overdue documentation for all this is currently in-progress at bug 1642505.)

If it would help, then I can provide access to a big endian system and naturally test patches. Writing the patch myself would be difficult, because I'm not much familiar with the FF build-system.

FWIW, access to big-endian systems (PPC64 and SPARC64) can be obtained through the GCC compile farm, see:

https://gcc.gnu.org/wiki/CompileFarm

The only requirement is that the account is used for the intent of developing open source software.

The machines gcc202 (SPARC64) and gcc203 (PPC64) are co-maintained by me, so I can install packages if required.

Seems that it's possible to convert ICU data file without an error now:

icupkg -tb config/external/icu/data/icudt67l.dat config/external/icu/data/icudt67b.dat

But I'm not good with Mozilla build system. How can we execute this line? Any suggestion or example?

Also it would be probably better to build icupkg ourself (intl/icu/source/tools/icupkg). Again I don't really know where to start...

But anyway by executing above command I'm able to build Firefox again.

intl/icu/README.md documents all the various relevant commands involved in updating ICU, tzdata, currency information, etc.

I've noted there that we don't have the big-endian version of the file checked in for what are primarily size reasons. Also, those commands currently depend upon a compatible icupkg being provided by the user.

It wouldn't be a crazy nit to fix to build icupkg from the downloaded source and then use that (indeed bug 1449677 would do just that). But it's not really on the radar of the two of us who do most of this work given we can basically just suck it up and do a little extra work.

Assignee: nobody → mh+mozilla
Depends on: 1650299
Depends on: 1650592
Depends on: 1651207
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/d3004626988d
Automatically convert the little-endian ICU data file for big-endian builds. r=firefox-build-system-reviewers,rstewart

For people who would want to cherry-pick this on their copy of esr78, this additional patch is necessary (on top of all the dependencies of this bug).

Flags: needinfo?(mh+mozilla)
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/cfaeba06ee87
Automatically convert the little-endian ICU data file for big-endian builds. r=firefox-build-system-reviewers,rstewart
Pushed by abutkovits@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/65418a2fb344
Automatically convert the little-endian ICU data file for big-endian builds. r=firefox-build-system-reviewers,rstewart

Apologies for troubles that I caused.
It was relanded: https://hg.mozilla.org/integration/autoland/rev/65418a2fb34497160402d09acadeab78974994d3

Flags: needinfo?(mh+mozilla)

https://hg.mozilla.org/mozilla-central/rev/65418a2fb344

Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox80: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla80
Regressions: 1654094
Regressions: 1653909
Blocks: 1673769
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: