Build support for Linux sh4 broken

RESOLVED FIXED in Firefox 53

Status

()

Core
Build Config
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: John Paul Adrian Glaubitz, Unassigned)

Tracking

51 Branch
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(5 attachments, 8 obsolete attachments)

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
(Reporter)

Description

11 months ago
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

11 months ago
Created attachment 8824414 [details] [diff] [review]
0001-Bug-1329194-mozbuild-Add-SH4-as-target-architecture.patch
Attachment #8824414 - Flags: review?(mh+mozilla)
Attachment #8824414 - Flags: review?(martin)
(Reporter)

Comment 2

11 months ago
Created attachment 8824415 [details] [diff] [review]
0002-Bug-1329194-media-webrtc-Add-platform-defines-for-SH.patch
Attachment #8824415 - Flags: review?(mh+mozilla)
Attachment #8824415 - Flags: review?(martin)
(Reporter)

Comment 3

11 months ago
Created attachment 8824416 [details] [diff] [review]
0003-Bug-1329194-ipc-chromium-Add-platform-defines-for-SH.patch
Attachment #8824416 - Flags: review?(mh+mozilla)
Attachment #8824416 - Flags: review?(martin)
(Reporter)

Comment 4

11 months ago
Created attachment 8824417 [details] [diff] [review]
0007-Bug-1329194-mfbt-tests-Define-RETURN_INSTR-for-SH-in.patch
Attachment #8824417 - Flags: review?(mh+mozilla)
Attachment #8824417 - Flags: review?(martin)
(Reporter)

Comment 5

11 months ago
Created attachment 8824418 [details] [diff] [review]
0008-Bug-1329194-js-jit-Add-preprocessor-hacks-to-avoid-R.patch
Attachment #8824418 - Flags: review?(mh+mozilla)
Attachment #8824418 - Flags: review?(martin)
(Reporter)

Comment 6

11 months ago
Patches 0004, 0005 and 0006 are part of bug #382214 [1].

> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=382214

Updated

11 months ago
Component: Untriaged → Build Config
Product: Firefox → Core
(Reporter)

Comment 7

11 months ago
Created attachment 8825814 [details] [diff] [review]
0009-Bug-1329194-js-jit-Use-PowerPC-atomic-operations-on-.patch
Attachment #8825814 - Flags: review?(jdemooij)
(Reporter)

Comment 8

11 months ago
Created attachment 8826881 [details] [diff] [review]
0001-Bug-1329194-mozbuild-Add-SH4-as-target-architecture.patch
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

11 months ago
Created attachment 8826882 [details] [diff] [review]
0002-Bug-1329194-media-webrtc-Add-platform-defines-for-SH.patch
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

11 months ago
Created attachment 8826883 [details] [diff] [review]
0003-Bug-1329194-ipc-chromium-Add-platform-defines-for-SH.patch
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

11 months ago
Created attachment 8826884 [details] [diff] [review]
0007-Bug-1329194-mfbt-tests-Define-RETURN_INSTR-for-SH-in.patch
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

11 months ago
Created attachment 8826885 [details] [diff] [review]
0008-Bug-1329194-js-jit-Add-preprocessor-hacks-to-avoid-R.patch
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

11 months ago
Created attachment 8826886 [details] [diff] [review]
0009-Bug-1329194-js-jit-Use-PowerPC-atomic-operations-on-.patch
Attachment #8825814 - Attachment is obsolete: true
Attachment #8825814 - Flags: review?(jdemooij)
Attachment #8826886 - Flags: review?(jdemooij)
(Reporter)

Comment 14

11 months ago
Rebased all patches against current master.
Attachment #8826883 - Flags: review?(nfroyd) → review+

Updated

11 months ago
Attachment #8826882 - Flags: review?(rjesup) → review+

Updated

11 months ago
Attachment #8826886 - Flags: review?(jdemooij) → review+

Comment 15

11 months 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

11 months 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

11 months 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

11 months 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

11 months 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

11 months 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

11 months 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 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

11 months ago
Attachment #8826884 - Flags: review?(mh+mozilla) → review+
(Reporter)

Comment 23

11 months 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

11 months ago
Created attachment 8828237 [details] [diff] [review]
0001-Bug-1329194-mozbuild-Add-SH4-as-target-architecture.patch

Updated as requested.
Attachment #8826881 - Attachment is obsolete: true
Attachment #8828237 - Flags: review?(mh+mozilla)
(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

11 months ago
Attachment #8828237 - Flags: review?(mh+mozilla) → review+

Comment 26

11 months 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

11 months 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
Last Resolved: 11 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Updated

10 months ago
Attachment #8826882 - Flags: review?(martin)
You need to log in before you can comment on or make changes to this bug.