1329194 - Build support for Linux sh4 broken

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[John Paul Adrian Glaubitz]

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.

Comment 1

[John Paul Adrian Glaubitz]

Attachment: 0001-Bug-1329194-mozbuild-Add-SH4-as-target-architecture.patch

Comment 2

[John Paul Adrian Glaubitz]

Attachment: 0002-Bug-1329194-media-webrtc-Add-platform-defines-for-SH.patch

Comment 3

[John Paul Adrian Glaubitz]

Attachment: 0003-Bug-1329194-ipc-chromium-Add-platform-defines-for-SH.patch

Comment 4

[John Paul Adrian Glaubitz]

Attachment: 0007-Bug-1329194-mfbt-tests-Define-RETURN_INSTR-for-SH-in.patch

Comment 5

[John Paul Adrian Glaubitz]

Attachment: 0008-Bug-1329194-js-jit-Add-preprocessor-hacks-to-avoid-R.patch

Comment 6

[John Paul Adrian Glaubitz]

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

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

Comment 7

[John Paul Adrian Glaubitz]

Attachment: 0009-Bug-1329194-js-jit-Use-PowerPC-atomic-operations-on-.patch

Comment 8

[John Paul Adrian Glaubitz]

Attachment: 0001-Bug-1329194-mozbuild-Add-SH4-as-target-architecture.patch

Comment 9

[John Paul Adrian Glaubitz]

Attachment: 0002-Bug-1329194-media-webrtc-Add-platform-defines-for-SH.patch

Comment 10

[John Paul Adrian Glaubitz]

Attachment: 0003-Bug-1329194-ipc-chromium-Add-platform-defines-for-SH.patch

Comment 11

[John Paul Adrian Glaubitz]

Attachment: 0007-Bug-1329194-mfbt-tests-Define-RETURN_INSTR-for-SH-in.patch

Comment 12

[John Paul Adrian Glaubitz]

Attachment: 0008-Bug-1329194-js-jit-Add-preprocessor-hacks-to-avoid-R.patch

Comment 13

[John Paul Adrian Glaubitz]

Attachment: 0009-Bug-1329194-js-jit-Use-PowerPC-atomic-operations-on-.patch

Comment 14

[John Paul Adrian Glaubitz]

Rebased all patches against current master.

Comment 15

[Jan de Mooij [:jandem]]

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?

Comment 16

[John Paul Adrian Glaubitz]

(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

[Michael Karcher]

(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

[Jan de Mooij [:jandem]]

(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?

Comment 19

[John Paul Adrian Glaubitz]

(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/

Comment 20

[John Paul Adrian Glaubitz]

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.

Comment 21

[John Paul Adrian Glaubitz]

(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

[Mike Hommey [:glandium]]

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.

Comment 23

[John Paul Adrian Glaubitz]

(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.

Comment 24

[John Paul Adrian Glaubitz]

Attachment: 0001-Bug-1329194-mozbuild-Add-SH4-as-target-architecture.patch

Updated as requested.

Comment 25

[Mike Hommey [:glandium]]

(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'.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
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

[Phil Ringnalda (:philor)]

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