Support powerpc64le-linux platform

RESOLVED FIXED in mozilla30

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: uweigand, Assigned: uweigand)

Tracking

Trunk
mozilla30
PowerPC
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 6 obsolete attachments)

The new little-endian 64-bit PowerPC platform (powerpc64le-linux) is not currently supported by Mozilla.  I'll attach a patch series to add support, consisting of the following parts:

1. Toplevel build/config support
2. NSPR build/config support
3. Javascript build/config support
4. js/src/ctypes - libffi backport
5. xptcall port
6. webrtc config support
7. mfbt config support
Posted patch Part 1: Toplevel build/config (obsolete) — Splinter Review
This patch adds toplevel build/config changes for powerpc64le-linux:
- Update config.guess / config.sub to current upstream versions
- Set CPU_ARCH in configure.in
Posted patch Part 2: NSPR build/config (obsolete) — Splinter Review
This patch adds NSPR build/config changes for powerpc64le-linux:
- Update config.guess / config.sub to current upstream versions
- Correctly set endian macros in pr/include/md/_linux.cfg
This patch adds js/src build/config support for powerpc64le-linux:
- Set CPU_ARCH in configure.in
- Correctly set endian macros in jscpucfg.h
- Correctly set endian macros in assembler/wtf/Platform.h
This patch adds js/src/ctypes/libffi support for powerpc64le-linux:
- Fix libtool shared library handling on powerpc64le-linux
  This backports just the minimal change necessary from upstream libtool into m4/libtool.m4 and the generated files aclocal.m4 and configure.
- Backport powerpc back-end support from upstream libffi
  This backports all changes in src/powerpc, except those that affect solely AIX or Darwin, and except for the following patch (which would require backporting libffi common code changes, and which is not necessary for the built-in copy of libffi in Mozilla since it only implemented back-level compatibility support for a libffi.so shared library):
https://sourceware.org/ml/libffi-discuss/2013/msg00220.html

Overall, non-PowerPC platforms should be completely unaffected by this backport.

Once you upgrade libffi to current mainline, this patch becomes fully obsolete.
Posted patch Part 5: xptcall port (obsolete) — Splinter Review
This patch adds xptcall support for powerpc64le-linux.

This primarily consists of support for the new ELFv2 ABI.  The main changes introduced with this ABI that affect xptcall are the following:

- No more function descriptors
http://gcc.gnu.org/ml/gcc-patches/2013-11/msg01141.html

- Stack frame layout changes
http://gcc.gnu.org/ml/gcc-patches/2013-11/msg01149.html
http://gcc.gnu.org/ml/gcc-patches/2013-11/msg01146.html

In addition, the patch adds fixes to correctly locate "float" arguments on the stack in little-endian mode.

Finally, the patch adds moz.build support to enable the powerpc64le platform.

Note that I've had to rename the assembler files from .s to .S extensions to make sure they are run through the preprocessor.
This patch fixes the endian defines in media/webrtc/trunk/webrtc/typedefs.h.
This patch fixes the mfbt/Endian.h defines for little-endian PowerPC.

This is not strictly necessary on Linux, since there is a generic __GNUC__ clause that will get the endian macros correct when building with GCC, but this patch fixes the defines when building with a non-GCC compiler.
Attachment #8381536 - Flags: review?(benjamin)
Attachment #8381538 - Flags: review?(ted)
Attachment #8381542 - Flags: review?(benjamin)
Attachment #8381546 - Flags: review?(benjamin)
Attachment #8381549 - Flags: review?(nfroyd)
Attachment #8381554 - Flags: review?(rjesup)
Attachment #8381558 - Flags: review?(jwalden+bmo)
Ulrich, should you be listed as the port maintainer?
Comment on attachment 8381536 [details] [diff] [review]
Part 1: Toplevel build/config

Are the config.guess changes just importing a new copy of config.guess from GNU config? r=me if so or re-request review if there's something special.
Attachment #8381536 - Flags: review?(benjamin) → review+
Attachment #8381542 - Flags: review?(benjamin) → review+
Attachment #8381546 - Flags: review?(benjamin) → review+
Comment on attachment 8381549 [details] [diff] [review]
Part 5: xptcall port

Review of attachment 8381549 [details] [diff] [review]:
-----------------------------------------------------------------

This all looks fine.

::: xpcom/reflect/xptcall/src/md/unix/xptcinvoke_asm_ppc64_linux.S
@@ +17,5 @@
>  .set f25,25; .set f26,26; .set f27,27; .set f28,28; .set f29,29
>  .set f30,30; .set f31,31
>  
> +#if _CALL_ELF == 2
> +#define STACK_TOC      28

For my own edification, why does this not match up with the RS6000_TOC_SAVE_SLOT/RS6000_SAVE_TOC macros in http://gcc.gnu.org/ml/gcc-patches/2013-11/msg01146.html ?  28 seems like sort of an odd number in this context, insofar as it's not evenly divisible by the word size.

@@ +70,5 @@
>          #
>          #  | ..128-byte stack frame.. |     | 7 GP | 13 FP | 3 NV |
>          #  |               |(params)........| regs | regs  | regs |
>          # (r1)...........(+112)....(+128)
>          #                               (-23*8).(-16*8).(-3*8)..(r31)

This comment/diagram could probably stand some updating if you felt like it.

@@ +84,5 @@
>          #                           uint32_t paramCount, nsXPTCVariant* s, 
>          #                           uint64_t* d))
>  
>          # r5, r6 are passed through intact (paramCount, params)
>          # r7 (d) has to be r1+112 -- where parameters are passed on the stack.

You may want to update this 112 now?
Attachment #8381549 - Flags: review?(nfroyd) → review+
Comment on attachment 8381558 [details] [diff] [review]
Part 7: mfbt endian defines

Review of attachment 8381558 [details] [diff] [review]:
-----------------------------------------------------------------

Stealing review!
Attachment #8381558 - Flags: review?(jwalden+bmo) → review+
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #8381554 - Flags: review?(rjesup) → review+
Comment on attachment 8381554 [details] [diff] [review]
Part 6: WebRTC endian defines

Review of attachment 8381554 [details] [diff] [review]:
-----------------------------------------------------------------

Note: since this affects upstream code, an Issue should be opened with the upstream for this (webrtc.org: https://code.google.com/p/webrtc/issues/list) and the patch added to the webrtc codereview site.  Once the issue is reported there, please update this bug with the Issue, and link this bug to webrtc_upstream_bugs (bug 864513)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #9)
> Comment on attachment 8381536 [details] [diff] [review]
> Part 1: Toplevel build/config
> 
> Are the config.guess changes just importing a new copy of config.guess from
> GNU config? r=me if so or re-request review if there's something special.

Yes, this just imports a new copy of config.guess / config.sub from GNU config.

Thanks for the review!
(In reply to Nathan Froyd (:froydnj) from comment #10)

> Review of attachment 8381549 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This all looks fine.
> 
> ::: xpcom/reflect/xptcall/src/md/unix/xptcinvoke_asm_ppc64_linux.S
> @@ +17,5 @@
> >  .set f25,25; .set f26,26; .set f27,27; .set f28,28; .set f29,29
> >  .set f30,30; .set f31,31
> >  
> > +#if _CALL_ELF == 2
> > +#define STACK_TOC      28
> 
> For my own edification, why does this not match up with the
> RS6000_TOC_SAVE_SLOT/RS6000_SAVE_TOC macros in
> http://gcc.gnu.org/ml/gcc-patches/2013-11/msg01146.html ?  28 seems like
> sort of an odd number in this context, insofar as it's not evenly divisible
> by the word size.

Oops, that needs to be 24 of course ... not sure how that happened.

Thanks for the thorough review!

I'll submit an updated patch with this fix, and some comment enhancements, shortly.
(In reply to Randell Jesup [:jesup] from comment #12)
> Comment on attachment 8381554 [details] [diff] [review]
> Part 6: WebRTC endian defines
> 
> Review of attachment 8381554 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Note: since this affects upstream code, an Issue should be opened with the
> upstream for this (webrtc.org: https://code.google.com/p/webrtc/issues/list)
> and the patch added to the webrtc codereview site.  Once the issue is
> reported there, please update this bug with the Issue, and link this bug to
> webrtc_upstream_bugs (bug 864513)

The upstream WebRTC version of this file:
https://code.google.com/p/webrtc/source/browse/trunk/webrtc/typedefs.h
does not contain any support for PowerPC at all, this seems to be a private addition in the Mozilla sources to begin with.

Does this mean that there is no need to forward this patch to upstream after all, or were you asking me to submit the whole of PowerPC support (big- and little-endian) upstream?
Posted patch Part 5: xptcall port (obsolete) — Splinter Review
Fixed ELFv2 STACK_TOC definition, and enhanced comments.
Attachment #8381549 - Attachment is obsolete: true
Attachment #8382203 - Flags: review?(nfroyd)
Comment on attachment 8382203 [details] [diff] [review]
Part 5: xptcall port

Review of attachment 8382203 [details] [diff] [review]:
-----------------------------------------------------------------

I think perhaps you forget to |hg qref| or similar?  This looks identical to the previous patch.
Attachment #8382203 - Flags: review?(nfroyd)
Attaching the correct file this time, sorry ...
Attachment #8382203 - Attachment is obsolete: true
Attachment #8382226 - Flags: review?(nfroyd)
Comment on attachment 8382226 [details] [diff] [review]
Part 5: xptcall port

Review of attachment 8382226 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/reflect/xptcall/src/md/unix/xptcinvoke_asm_ppc64_linux.S
@@ +23,5 @@
> +# save area of 8 doublewords (used for vararg routines), followed
> +# by space for parameters passed on the stack.
> +#
> +# We set STACK_TOC to the offset of the TOC pointer save area, and
> +# STACK_PARAMS to the offset of the first on-stack parameter.

This is great, thank you!
Attachment #8382226 - Flags: review?(nfroyd) → review+
I just noticed that there is completely separate "Product" category for NSPR ...  Should I submit the NSPR part as a separate bugzilla in that category instead of just having it as one part here?
typedef.h changes were landed (with a few updates since) as bug 814693; we can let that be the placeholder for upstreaming all of the changes to this file.. thanks
Attachment #8381538 - Flags: review?(ted)
Attachment #8381538 - Attachment is obsolete: true
Depends on: 977685
NSPR changes split off into separate bug now.
Keywords: checkin-needed
Build changes (particularly configure and autoconf) must have review from a build peer - please request review from them one you've fixed the cause of the backout. I imagine :glandium might be your best bet.
(Sorry just noticed the bsmedberg review; had glanced at a hgweb diff for the r= and must have looked at one of the later parts)
(In reply to Ed Morley [:edmorley UTC+0] from comment #24)
> Backed out for failures during configure:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=35422834&tree=Mozilla-
> Inbound

Sorry for the breakage ...

So it seems the difference is that configure is called with "arm-linux-androideabi", which the old config.sub transforms to "arm-linux-android", while the new one transforms it to "arm-unknown-linux-androideabi".  The latter is the standard behaviour of GNU config; the former is due to private patches in Mozilla's config.sub copy, which were introduced by Bug 617115 and later fixed again by Bug 672939.

I guess longer term it would be better to change the rest of the build infrastructure to cope with the now-standard version of the target triple ...   For now, I can add back a private change to have config.sub again return "arm-linux-android".

Investigating the Mercury history further, there is yet another private change to config.sub, to support the -wince-winmo operating system (introduced by Bug 515748).  This is still not supported in upstream GNU config.  Again, I can add back the local change.

Finally, there was also a private change to config.guess to support Darwin on x86_64 (introduced by Bug 515002).  This is now supported in upstream GNU config directly, so the private change shouldn't be necessary any more.

I'll prepare an updated patch with those two private config.sub changes back in.
> ...   For now, I can add back a private change to have config.sub again
> return "arm-linux-android".

Yes, let's do that.

> Investigating the Mercury history further, there is yet another private
> change to config.sub, to support the -wince-winmo operating system
> (introduced by Bug 515748).  This is still not supported in upstream GNU
> config.  Again, I can add back the local change.

This is not important, you don't need to add it back in.
Posted patch Part 1: Toplevel build/config (obsolete) — Splinter Review
Respin of the config.sub patch to add back in two local changes.  See previous comments for discussion ...
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #28)
> > ...   For now, I can add back a private change to have config.sub again
> > return "arm-linux-android".
> 
> Yes, let's do that.
> 
> > Investigating the Mercury history further, there is yet another private
> > change to config.sub, to support the -wince-winmo operating system
> > (introduced by Bug 515748).  This is still not supported in upstream GNU
> > config.  Again, I can add back the local change.
> 
> This is not important, you don't need to add it back in.

Oops, didn't see that in time ...  I'll take them back out and respin another patch.
Attachment #8383792 - Attachment is obsolete: true
Posted patch Part 1: Toplevel build/config (obsolete) — Splinter Review
Respin config.sub patch to add back in the local change for *-linux-android* handling (set vendor to "linux" and OS to "android").
Attachment #8381536 - Attachment is obsolete: true
For reference, the only remaining changes between config.sub as introduced by the latest patch and the upstream GNU config are now:

--- /home/uweigand/config/config.sub    2014-02-28 10:47:47.210000004 -0600
+++ config.sub  2014-02-28 12:17:28.530009090 -0600
@@ -1347,6 +1347,9 @@
        -gnu/linux*)
                os=`echo $os | sed -e 's|gnu/linux|linux-gnu|'`
                ;;
+       *-android*)
+               os=-android
+               ;;
        # First accept the basic system types.
        # The portable systems comes first.
        # Each alternative MUST END IN A *, to match a version number.
@@ -1777,6 +1780,9 @@
                        -vos*)
                                vendor=stratus
                                ;;
+                       -android*)
+                               vendor=linux
+                               ;;
                esac
                basic_machine=`echo $basic_machine | sed "s/unknown/$vendor/"`
                ;;
Attachment #8383803 - Flags: review?(benjamin)
Comment on attachment 8383803 [details] [diff] [review]
Part 1: Toplevel build/config

Feel free to file a followup for using stock config.sub and fixing our configure bits.
Attachment #8383803 - Flags: review?(benjamin) → review+
Keywords: checkin-needed
So there's some progress, the arm-linux-android target is now recognized as it used to.  However, the i386-linux-android target is mis-recognized as i386-pc-android :-( 

This is an unfortunate interaction between the parts of config.sub that were added upstream to properly recognize Android targets, and my port of the Mozilla-private config.sub changes that recognized Android targets in a special (now non-standard) way.

To stay with our original plan of keeping the Mozilla non-standard target triples (arm-linux-android and i386-linux-android), I guess what we have to do is to:
- *remove* the config.sub parts that were added upstream to recognize Android, *and*
- add back the (now unmodified) Mozilla-private hack to recognize Android

I've prepared another iteration of the patch so we might try yet another attempt to land the patch series.  I've verified that this version of config.sub regconizes "arm-linux-android" and "i386-linux-android" the way the rest of Mozilla expects it.
Flags: needinfo?(uweigand)
Here's the diff against upstream config.sub that implements the two changes described in the previous comment:

--- /home/uweigand/config/config.sub    2014-02-28 10:47:47.210000004 -0600
+++ config.sub  2014-03-04 13:25:15.700000007 -0600
@@ -115,7 +115,7 @@
 # Here we must recognize all the valid KERNEL-OS combinations.
 maybe_os=`echo $1 | sed 's/^\(.*\)-\([^-]*-[^-]*\)$/\2/'`
 case $maybe_os in
-  nto-qnx* | linux-gnu* | linux-android* | linux-dietlibc | linux-newlib* | \
+  nto-qnx* | linux-gnu* | linux-dietlibc | linux-newlib* | \
   linux-musl* | linux-uclibc* | uclinux-uclibc* | uclinux-gnu* | kfreebsd*-gnu* | \
   knetbsd*-gnu* | netbsd*-gnu* | \
   kopensolaris*-gnu* | \
@@ -123,10 +123,6 @@
     os=-$maybe_os
     basic_machine=`echo $1 | sed 's/^\(.*\)-\([^-]*-[^-]*\)$/\1/'`
     ;;
-  android-linux)
-    os=-linux-android
-    basic_machine=`echo $1 | sed 's/^\(.*\)-\([^-]*-[^-]*\)$/\1/'`-unknown
-    ;;
   *)
     basic_machine=`echo $1 | sed 's/-[^-]*$//'`
     if [ $basic_machine != $1 ]
@@ -1367,7 +1363,7 @@
              | -udi* | -eabi* | -lites* | -ieee* | -go32* | -aux* \
              | -chorusos* | -chorusrdb* | -cegcc* \
              | -cygwin* | -msys* | -pe* | -psos* | -moss* | -proelf* | -rtems* \
-             | -mingw32* | -mingw64* | -linux-gnu* | -linux-android* \
+             | -mingw32* | -mingw64* | -linux-gnu* \
              | -linux-newlib* | -linux-musl* | -linux-uclibc* \
              | -uxpv* | -beos* | -mpeix* | -udk* \
              | -interix* | -uwin* | -mks* | -rhapsody* | -darwin* | -opened* \
@@ -1506,6 +1502,9 @@
        -dicos*)
                os=-dicos
                ;;
+        -android*)
+               os=-android
+                ;;
        -nacl*)
                ;;
        -none)
@@ -1777,6 +1776,9 @@
                        -vos*)
                                vendor=stratus
                                ;;
+                       *-android*|*-linuxandroid*)
+                               vendor=linux-
+                               ;;
                esac
                basic_machine=`echo $basic_machine | sed "s/unknown/$vendor/"`
                ;;
Next attempt to update config.guess/config.sub.
Attachment #8383803 - Attachment is obsolete: true
Attachment #8385543 - Flags: review?(benjamin)
Comment on attachment 8385543 [details] [diff] [review]
Part 1: Toplevel build/config

Going to bump this to blassey as the android expert.
Attachment #8385543 - Flags: review?(benjamin) → review?(blassey.bugs)
Attachment #8385543 - Flags: review?(blassey.bugs) → review+
Just a note, there are a lot of whitespace only changes in the upstream patches. I don't see too much harm in that since they are upstream patches, but normally I'm not a huge fan of churn for churn's sake.
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.