Closed
Bug 1329194
Opened 8 years ago
Closed 8 years ago
Build support for Linux sh4 broken
Categories
(Firefox Build System :: General, defect)
Tracking
(firefox53 fixed)
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: glaubitz, Unassigned)
Details
Attachments
(5 files, 8 obsolete files)
900 bytes,
patch
|
jesup
:
review+
|
Details | Diff | Splinter Review |
890 bytes,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
851 bytes,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
941 bytes,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
2.37 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:51.0) Gecko/20100101 Firefox/51.0 Build ID: 20161201031821 Steps to reproduce: Trying to build Firefox on Linux/sh4 currently fails with: Adding configure options from /<<PKGBUILDDIR>>/debian/firefox.mozconfig --enable-application=browser --with-app-name=firefox --enable-release --prefix=/usr --enable-default-toolkit=cairo-gtk2 --with-system-zlib --with-system-bz2 --disable-gconf --enable-readline --enable-system-hunspell --disable-strip --disable-install-strip --enable-startup-notification --enable-system-ffi --with-system-libevent --with-system-nspr --with-system-nss --enable-system-sqlite --with-system-libvpx --disable-updater --enable-pie checking for a shell... /bin/sh checking for host system type... Traceback (most recent call last): File "../configure.py", line 94, in <module> sys.exit(main(sys.argv)) File "../configure.py", line 22, in main sandbox.run(os.path.join(os.path.dirname(__file__), 'moz.configure')) File "/<<PKGBUILDDIR>>/python/mozbuild/mozbuild/configure/__init__.py", line 241, in run self._value_for(option) File "/<<PKGBUILDDIR>>/python/mozbuild/mozbuild/configure/__init__.py", line 305, in _value_for return self._value_for_option(obj) File "/<<PKGBUILDDIR>>/python/mozbuild/mozbuild/util.py", line 922, in method_call cache[args] = self.func(instance, *args) File "/<<PKGBUILDDIR>>/python/mozbuild/mozbuild/configure/__init__.py", line 340, in _value_for_option need_help_dependency=False) File "/<<PKGBUILDDIR>>/python/mozbuild/mozbuild/configure/__init__.py", line 297, in _resolve return self._value_for(arg) File "/<<PKGBUILDDIR>>/python/mozbuild/mozbuild/configure/__init__.py", line 302, in _value_for return self._value_for_depends(obj) File "/<<PKGBUILDDIR>>/python/mozbuild/mozbuild/util.py", line 922, in method_call cache[args] = self.func(instance, *args) File "/<<PKGBUILDDIR>>/python/mozbuild/mozbuild/configure/__init__.py", line 328, in _value_for_depends resolved_args = [self._value_for(d) for d in dependencies] File "/<<PKGBUILDDIR>>/python/mozbuild/mozbuild/configure/__init__.py", line 302, in _value_for return self._value_for_depends(obj) File "/<<PKGBUILDDIR>>/python/mozbuild/mozbuild/util.py", line 922, in method_call cache[args] = self.func(instance, *args) File "/<<PKGBUILDDIR>>/python/mozbuild/mozbuild/configure/__init__.py", line 328, in _value_for_depends resolved_args = [self._value_for(d) for d in dependencies] File "/<<PKGBUILDDIR>>/python/mozbuild/mozbuild/configure/__init__.py", line 302, in _value_for return self._value_for_depends(obj) File "/<<PKGBUILDDIR>>/python/mozbuild/mozbuild/util.py", line 922, in method_call cache[args] = self.func(instance, *args) File "/<<PKGBUILDDIR>>/python/mozbuild/mozbuild/configure/__init__.py", line 329, in _value_for_depends return func(*resolved_args) File "/<<PKGBUILDDIR>>/python/mozbuild/mozbuild/configure/__init__.py", line 773, in wrapped return new_func(*args, **kwargs) File "/<<PKGBUILDDIR>>/python/mozbuild/mozbuild/configure/__init__.py", line 518, in wrapper ret = template(*args, **kwargs) File "/<<PKGBUILDDIR>>/build/moz.configure/checks.configure", line 53, in wrapped ret = func(*args, **kwargs) File "/<<PKGBUILDDIR>>/python/mozbuild/mozbuild/configure/__init__.py", line 773, in wrapped return new_func(*args, **kwargs) File "/<<PKGBUILDDIR>>/build/moz.configure/init.configure", line 449, in host return split_triplet(config_sub(shell, host)) File "/<<PKGBUILDDIR>>/python/mozbuild/mozbuild/configure/__init__.py", line 773, in wrapped return new_func(*args, **kwargs) File "/<<PKGBUILDDIR>>/build/moz.configure/init.configure", line 419, in split_triplet cpu=CPU(canonical_cpu), File "/<<PKGBUILDDIR>>/python/mozbuild/mozbuild/util.py", line 1161, in __init__ % (value, self.__class__.__name__)) ValueError: 'sh4' is not a valid value for EnumStringSubclass This is because sh4 has not been properly added to the mozbuild build system. The attached patches fix this particular issue with some additional patches to fix sh4 support in the build system.
Reporter | ||
Comment 1•8 years ago
|
||
Attachment #8824414 -
Flags: review?(mh+mozilla)
Attachment #8824414 -
Flags: review?(martin)
Reporter | ||
Comment 2•8 years ago
|
||
Attachment #8824415 -
Flags: review?(mh+mozilla)
Attachment #8824415 -
Flags: review?(martin)
Reporter | ||
Comment 3•8 years ago
|
||
Attachment #8824416 -
Flags: review?(mh+mozilla)
Attachment #8824416 -
Flags: review?(martin)
Reporter | ||
Comment 4•8 years ago
|
||
Attachment #8824417 -
Flags: review?(mh+mozilla)
Attachment #8824417 -
Flags: review?(martin)
Reporter | ||
Comment 5•8 years ago
|
||
Attachment #8824418 -
Flags: review?(mh+mozilla)
Attachment #8824418 -
Flags: review?(martin)
Reporter | ||
Comment 6•8 years ago
|
||
Patches 0004, 0005 and 0006 are part of bug #382214 [1]. > [1] https://bugzilla.mozilla.org/show_bug.cgi?id=382214
Reporter | ||
Comment 8•8 years ago
|
||
Attachment #8824414 -
Attachment is obsolete: true
Attachment #8824414 -
Flags: review?(mh+mozilla)
Attachment #8824414 -
Flags: review?(martin)
Attachment #8826881 -
Flags: review?(mh+mozilla)
Attachment #8826881 -
Flags: review?(martin)
Reporter | ||
Comment 9•8 years ago
|
||
Attachment #8824415 -
Attachment is obsolete: true
Attachment #8824415 -
Flags: review?(mh+mozilla)
Attachment #8824415 -
Flags: review?(martin)
Attachment #8826882 -
Flags: review?(rjesup)
Attachment #8826882 -
Flags: review?(martin)
Reporter | ||
Comment 10•8 years ago
|
||
Attachment #8824416 -
Attachment is obsolete: true
Attachment #8824416 -
Flags: review?(mh+mozilla)
Attachment #8824416 -
Flags: review?(martin)
Attachment #8826883 -
Flags: review?(nfroyd)
Reporter | ||
Comment 11•8 years ago
|
||
Attachment #8824417 -
Attachment is obsolete: true
Attachment #8824417 -
Flags: review?(mh+mozilla)
Attachment #8824417 -
Flags: review?(martin)
Attachment #8826884 -
Flags: review?(mh+mozilla)
Reporter | ||
Comment 12•8 years ago
|
||
Attachment #8824418 -
Attachment is obsolete: true
Attachment #8824418 -
Flags: review?(mh+mozilla)
Attachment #8824418 -
Flags: review?(martin)
Attachment #8826885 -
Flags: review?(mh+mozilla)
Attachment #8826885 -
Flags: review?(jdemooij)
Reporter | ||
Comment 13•8 years ago
|
||
Attachment #8825814 -
Attachment is obsolete: true
Attachment #8825814 -
Flags: review?(jdemooij)
Attachment #8826886 -
Flags: review?(jdemooij)
![]() |
||
Updated•8 years ago
|
Attachment #8826883 -
Flags: review?(nfroyd) → review+
Updated•8 years ago
|
Attachment #8826882 -
Flags: review?(rjesup) → review+
Updated•8 years ago
|
Attachment #8826886 -
Flags: review?(jdemooij) → review+
Comment 15•8 years ago
|
||
Comment on attachment 8826885 [details] [diff] [review] 0008-Bug-1329194-js-jit-Add-preprocessor-hacks-to-avoid-R.patch Review of attachment 8826885 [details] [diff] [review]: ----------------------------------------------------------------- Why add it to *these* 2 cpp files, there are other files that use R0 and R1 right? We shouldn't duplicate this hack in each of them. Can't we put it in js/src/jit/none/SharedICRegisters-none.h where these functions are defined? Is anyone actually using Firefox on these obscure platforms?
Attachment #8826885 -
Flags: review?(jdemooij)
Reporter | ||
Comment 16•8 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #15) > Why add it to *these* 2 cpp files, there are other files that use R0 and R1 > right? We shouldn't duplicate this hack in each of them. Can't we put it in > js/src/jit/none/SharedICRegisters-none.h where these functions are defined? I have asked Michael Karcher for a statement. He actually had a reason to put the macros into the cpp files instead of the headers and I think it was because the conflict occurs only in these two files and using the macro globally could have side-affects. > Is anyone actually using Firefox on these obscure platforms? sh4 is also known as SuperH is a historic embeddded architecture which has recently been revived as J-Core, a true open source CPU. Firefox has already had partial support for sh4 in the past and we're just reviving it so people can use Firefox on J-Core in the future on Debian, for example.
Comment 17•8 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #15) > Why add it to *these* 2 cpp files, there are other files that use R0 and R1 > right? We shouldn't duplicate this hack in each of them. Can't we put it in > js/src/jit/none/SharedICRegisters-none.h where these functions are defined? The hack *must* be added after the conflicting definition of R0 and R1 by the glibc header files. Including something that indirectly includes <sys/ucontext.h> the first time, like <signal.h> after SharedICRegisters-none.h will break awfully with that hack applied to SharedICRegisters-none.h. So if that hack should be moved into that header file, it also has to include <signal.h> before applying the hack, so <sys/ucontext.h> won't do its things afterwards, as prevented by the include guard. Both adding an artificial dependency on <signal.h> and applying that hack to the .cpp files individually is ugly, and I decided I prefer uglyness in two source files over uglyness in a more widely included header file. It's added to exactly those cpp files, because these are the only files that happen to include <sys/ucontext.h> and <jit/none/SharedICRegisters-none.h> at the same time and make use of R0/R1. On the other hand, instead of applying that patch to firefox to cope with glibc's namespace pollution, a better approach might be to have glibc not clutter the global namespace with the identifiers R0 and R1. They already renamed Rx to REG_Rx on ARM 5 years ago, so that's good precedence to have a similar change applied on SH. See this thread: https://sourceware.org/ml/libc-ports/2011-12/msg00032.html
Comment 18•8 years ago
|
||
(In reply to Michael Karcher from comment #17) > Both adding an artificial dependency on <signal.h> and applying that > hack to the .cpp files individually is ugly, and I decided I prefer uglyness > in two source files over uglyness in a more widely included header file. I agree with the first part, but I'd prefer adding the #include to the header file. The BaselineCacheIRCompiler.cpp file at least changes a lot and I open it all the time, so it seems nicer to have the 15-20 lines of workaround for a tier-3 platform in the tier-3 jit/none/* header file that we rarely look at, so people don't have to scroll past it (and recognize it as "workaround code" etc) all the time. > On the other hand, instead of applying that patch to firefox to cope with > glibc's namespace pollution, a better approach might be to have glibc not > clutter the global namespace with the identifiers R0 and R1. They already > renamed Rx to REG_Rx on ARM 5 years ago, so that's good precedence to have a > similar change applied on SH. See this thread: > https://sourceware.org/ml/libc-ports/2011-12/msg00032.html Yes, I agree that would be the best option :) Just curious, what's the plan for when we'll start requiring Rust to build Firefox in the not too distant future? Or does Rust/LLVM support these architectures already?
Reporter | ||
Comment 19•8 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #18) > Just curious, what's the plan for when we'll start requiring Rust to build > Firefox in the not too distant future? Or does Rust/LLVM support these > architectures already? I hope that Rust will remain optional for the foreseeable future as Rust is currently Tier1-supported on x86 only which is reflected in the regular regressions Rust sees on all the other architectures [1]. Debian currently builds src:rustc on i386 (32-bit x86), amd64 (64-bit x86) and arm64 only. Every other architecture is currently unsupported [2]. Rust upstream will probably have to setup CI for the other main architectures first (arm*, mips*, ppc64el) like Google has done for golang [3], so that other architectures besides x86 can reach Tier1 maturity. If all of that is ignored, Firefox will effectively become an x86-only browser which I'm not sure is a good idea. > [1] http://lists.alioth.debian.org/pipermail/pkg-rust-maintainers/Week-of-Mon-20161226/000758.html > [2] https://buildd.debian.org/status/package.php?p=rustc&suite=sid > [3] http://build.golang.org/
Reporter | ||
Comment 20•8 years ago
|
||
Comment on attachment 8826885 [details] [diff] [review] 0008-Bug-1329194-js-jit-Add-preprocessor-hacks-to-avoid-R.patch I'll remove this patch now and rather get the naming conflict issue resolved in glibc then. Thanks for the suggestion. We just need to have the RETURN_INSTR patch reviewed then.
Attachment #8826885 -
Attachment is obsolete: true
Attachment #8826885 -
Flags: review?(mh+mozilla)
Reporter | ||
Comment 21•8 years ago
|
||
(In reply to John Paul Adrian Glaubitz from comment #20) > I'll remove this patch now and rather get the naming conflict issue resolved > in glibc then. Ok, we have now addressed this issue directly in glibc and Firefox builds without the offensive patch "0008-Bug-1329194-js-jit-Add-preprocessor-hacks-to-avoid-R.patch".
Comment 22•8 years ago
|
||
Comment on attachment 8826881 [details] [diff] [review] 0001-Bug-1329194-mozbuild-Add-SH4-as-target-architecture.patch Review of attachment 8826881 [details] [diff] [review]: ----------------------------------------------------------------- ::: build/moz.configure/init.configure @@ +396,4 @@ > elif cpu.startswith('aarch64'): > canonical_cpu = 'aarch64' > endianness = 'little' > + elif cpu in ('sh4'): This needs to be either "cpu in ('sh4',)" or "cpu == 'sh4'", otherwise, this can't work.
Attachment #8826881 -
Flags: review?(mh+mozilla)
Attachment #8826881 -
Flags: review?(martin)
Updated•8 years ago
|
Attachment #8826884 -
Flags: review?(mh+mozilla) → review+
Reporter | ||
Comment 23•8 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #22) > This needs to be either "cpu in ('sh4',)" or "cpu == 'sh4'", otherwise, this > can't work. Are you sure about this? Because this is the version I'm currently using and it works. But I can send an updated patch using the second variant in any case. I agree it looks a bit weird to search in a list with just one element.
Reporter | ||
Comment 24•8 years ago
|
||
Updated as requested.
Attachment #8826881 -
Attachment is obsolete: true
Attachment #8828237 -
Flags: review?(mh+mozilla)
Comment 25•8 years ago
|
||
(In reply to John Paul Adrian Glaubitz from comment #23) > (In reply to Mike Hommey [:glandium] from comment #22) > > This needs to be either "cpu in ('sh4',)" or "cpu == 'sh4'", otherwise, this > > can't work. > > Are you sure about this? Because this is the version I'm currently using and > it works. > > But I can send an updated patch using the second variant in any case. I > agree it looks a bit weird to search in a list with just one element. Ah, it worked because when cpu is 'sh4', it *is* a substring of 'sh4' (which cpu in ('sh4') does), but it would also match if cpu was 's', 'h', '4', 'h4', or 'sh'.
Updated•8 years ago
|
Attachment #8828237 -
Flags: review?(mh+mozilla) → review+
Comment 26•8 years ago
|
||
Pushed by mh@glandium.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/1380d9d6ee09 mozbuild: Add SH4 as target architecture. r=glandium https://hg.mozilla.org/integration/mozilla-inbound/rev/572856b5083a media:webrtc: Add platform defines for SH. r=jesup https://hg.mozilla.org/integration/mozilla-inbound/rev/5c50cbe11fe3 ipc:chromium: Add platform defines for SH. r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/e62e2c17d75b mfbt:tests: Define RETURN_INSTR for SH in TestPoisonArea. r=glandium https://hg.mozilla.org/integration/mozilla-inbound/rev/e2860876fc43 js:jit: Use PowerPC atomic operations on SH. r=jandem
Comment 27•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1380d9d6ee09 https://hg.mozilla.org/mozilla-central/rev/572856b5083a https://hg.mozilla.org/mozilla-central/rev/5c50cbe11fe3 https://hg.mozilla.org/mozilla-central/rev/e62e2c17d75b https://hg.mozilla.org/mozilla-central/rev/e2860876fc43
Status: UNCONFIRMED → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Updated•8 years ago
|
Attachment #8826882 -
Flags: review?(martin)
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•