972945 - Fails to build under Debian powerpcspe

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Sylvestre Ledru [:Sylvestre]]

As reported on the Debian bug tracker:
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=732368
Firefox fails to build under PowerPC SPE because gcc fails with:
"AltiVec and E500 instructions cannot coexist"

Full build log is available here:
http://buildd.debian-ports.org/status/fetch.php?pkg=iceweasel&arch=powerpcspe&ver=24.3.0esr-1&stamp=1391992866

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Attachment: fix-powerpcspe.patch

Patch by Roland Stigge <[e-mail]>

Comment 2

[Mike Hommey [:glandium]]

Comment on attachment 8376370 [details] [diff] [review]
fix-powerpcspe.patch

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

Note "FTBFS" is Debian terminology. The patch title should better describe what it's doing.

::: gfx/qcms/Makefile.in
@@ +32,5 @@
>  endif
>  else
>  ifeq (ppc,$(findstring ppc,$(CPU_ARCH)))
>  ifdef GNU_CC
> +ifeq ($(shell echo | gcc -x c -c -maltivec - 2>&1),)

Whether -maltivec can be used should be a configure test.

Comment 3

[John Paul Adrian Glaubitz]

Hello!

I have taken over maintainership of powerpcspe in Debian.

As we have now moved over from Iceweasel to Firefox, the powerpcspe buildds have picked up Firefox for building now with the build failing with the same result [1]:

gcc -o transform-altivec.o -c -I/<<PKGBUILDDIR>>/build-browser/dist/system_wrappers -include /<<PKGBUILDDIR>>/config/gcc_hidden.h -DNDEBUG=1 -DTRIMMED=1 -DSTATIC_EXPORTABLE_JS_API -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/<<PKGBUILDDIR>>/gfx/qcms -I/<<PKGBUILDDIR>>/build-browser/gfx/qcms  -I/<<PKGBUILDDIR>>/build-browser/dist/include  -I/usr/include/nspr -I/usr/include/nss       -fPIC  -include /<<PKGBUILDDIR>>/build-browser/mozilla-config.h -DMOZILLA_CLIENT -MD -MP -MF .deps/transform-altivec.o.pp -Wdate-time -D_FORTIFY_SOURCE=2 -Wall -Wempty-body -Wignored-qualifiers -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -fstack-protector-strong -Wformat -Werror=format-security -std=gnu99 -fgnu89-inline -fno-strict-aliasing -ffunction-sections -fdata-sections -fno-math-errno -pthread -pipe  -g -freorder-blocks -Os -fomit-frame-pointer  -Wno-missing-field-initializers -maltivec  /<<PKGBUILDDIR>>/gfx/qcms/transform-altivec.c
transform.o
/<<PKGBUILDDIR>>/gfx/qcms/transform-altivec.c:1:0: error: AltiVec and SPE instructions cannot coexist
 /* vim: set ts=8 sw=8 noexpandtab: */
 ^
/<<PKGBUILDDIR>>/config/rules.mk:924: recipe for target 'transform-altivec.o' failed
make[5]: *** [transform-altivec.o] Error 1

Any chance to get this patch finally merged?

Adrian

> [1] https://buildd.debian.org/status/fetch.php?pkg=firefox&arch=powerpcspe&ver=47.0.1-1&stamp=1469638866

Comment 4

[Sylvestre Ledru [:Sylvestre]]

As I didn't have the capability to test the changes, I dropped the ball here.
However, I would be happy to help to see the patch merged upstream.

Comment 5

[John Paul Adrian Glaubitz]

I would suggest just adding the patch to the Debian package in unstable (provided that the patch still applies cleanly) and have users test the Firefox package. There is quite a number of users of Debian/powerpcspe available now since a company called A-EON has released an PPC-based unofficial successor to the Amiga [1].

Once enough users have reported for the patch to work, we can have it merged upstream.

> [1] http://a-eon.biz/news/News_Release_A1222.pdf

Comment 6

[Sylvestre Ledru [:Sylvestre]]

I think that comment #2 is still valid, right?

Comment 7

[John Paul Adrian Glaubitz]

(In reply to Sylvestre Ledru [:sylvestre] from comment #6)
> I think that comment #2 is still valid, right?

I assume you are talking about moving "-maltivec" testing from Makefile.in to configure.in?

Comment 8

[Sylvestre Ledru [:Sylvestre]]

yes, what Mike asked

Comment 9

[John Paul Adrian Glaubitz]

Ok, I'll have to dig into that. Since Firefox uses a different build system these days, the patch wouldn't apply anymore anyway. I will report back with a hopefully updated patch :).

Comment 10

[John Paul Adrian Glaubitz]

Hmm, I'm a bit stuck as I don't know what would be the best way to set a variable in old-configure.in to be used in gfx/qcms/moz.build.

Checking whether we should use Altivec or not could be done like this, for example:

On regular powerpc:

(sid_powerpc-dchroot)glaubitz@partch:~$ echo  | gcc -x c -c -maltivec -
(sid_powerpc-dchroot)glaubitz@partch:~$ 

While on a powerpcspe machine:

root@atlantis:~> echo  | gcc -x c -c -maltivec -
<stdin>:1:0: error: AltiVec and SPE instructions cannot coexist
root@atlantis:~>

So, one can just check whether the string returned by the command above is empty and set something like HAVE_ALTIVEC and then use that in gfx/qcms/moz.build to set use_altivec. However, I'm not sure whether that would be the preferred way of doing it.

Adrian

Comment 11

[John Paul Adrian Glaubitz]

The rest of the patch remains unchanged:

glaubitz@ikarus:~/upstream/firefox/firefox-47.0.1/gfx/qcms$ diff -u transform.c~ transform.c
--- transform.c~	2016-05-12 19:13:07.000000000 +0200
+++ transform.c	2016-07-28 12:19:44.869155750 +0200
@@ -1296,7 +1296,7 @@
 #endif
 		    } else
 #endif
-#if (defined(__POWERPC__) || defined(__powerpc__))
+#if (defined(__POWERPC__) || defined(__powerpc__)) && !defined(__NO_FPRS__))
 		    if (have_altivec()) {
 			    if (in_type == QCMS_DATA_RGB_8)
 				    transform->transform_fn = qcms_transform_data_rgb_out_lut_altivec;
glaubitz@ikarus:~/upstream/firefox/firefox-47.0.1/gfx/qcms$

So I just need a quick heads-up regarding the first part with moz.build from someone who is more familiar with the Firefox build system and can suggest a clean way of setting HAVE_ALTIVEC/USE_ALTIVEC.

One possible way would be:

glaubitz@ikarus:~/upstream/firefox/firefox-47.0.1$ diff -u old-configure.in~ old-configure.in
--- old-configure.in~	2016-07-28 11:41:49.420697228 +0200
+++ old-configure.in	2016-07-28 12:29:49.097522623 +0200
@@ -1117,9 +1117,8 @@
 
 powerpc | ppc | rs6000)
     CPU_ARCH=ppc
-    if $CC -E -dM -</dev/null | grep -q __ELF__; then
-      CSRCS += transform-altivec.c
-      ALTIVEC_FLAGS=-maltivec
+    if test "$GNU_CC" && test -n "$(echo | $CC -x c -c -maltivec - 2>&1)" ; then
+        USE_ALTIVEC=1
     fi
     ;;
 
glaubitz@ikarus:~/upstream/firefox/firefox-47.0.1$

Adrian

Comment 12

[John Paul Adrian Glaubitz]

(In reply to John Paul Adrian Glaubitz from comment #11)
> +    if test "$GNU_CC" && test -n "$(echo | $CC -x c -c -maltivec - 2>&1)" ;

This should be "test -z".

Comment 13

[John Paul Adrian Glaubitz]

Attachment: Dont-try-to-build-with-Altivec-support-on-powerpcspe.patch

Here's an updated patch against Firefox 47.0.1. I haven't tested it yet.

I think it would be best just to apply the patch to the current Debian package, have the buildds pick up and build the package and then ask the powerpcspe users to test the build.

Adrian

Comment 14

[John Paul Adrian Glaubitz]

Hi!

I realized I probably made a mistake, I think there is some sort of AC_SUBST(USE_ALTIVEC) is still missing. I'll do a test build first.

Adrian

Comment 15

[John Paul Adrian Glaubitz]

Attachment: Dont-try-to-build-with-Altivec-support-on-powerpcspe.patch

Attaching an updated patch, still have to test it though.

Comment 16

[John Paul Adrian Glaubitz]

Attachment: Dont-try-to-build-with-Altivec-support-on-powerpcspe.patch

I just realized that testing for just HAVE_ALTIVEC in gfx/qcms/moz.build should be enough as all the magic is done in old-configure.in.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Good, by the way, you generate the patch from the repo, not on 47.0.1.

Comment 18

[John Paul Adrian Glaubitz]

(In reply to Sylvestre Ledru [:sylvestre] from comment #17)
> Good, by the way, you generate the patch from the repo, not on 47.0.1.

I can also provide a version against Firefox from git/svn. I'll have to check it out first.

Comment 19

[John Paul Adrian Glaubitz]

Attachment: 0001-Bug-972945-Add-autoconf-test-for-AltiVec-to-use-it-o.patch

Alright, here's a patch against master (checked out from: https://github.com/mozilla/gecko-dev).

Comment 20

[John Paul Adrian Glaubitz]

Attachment: 0001-Bug-972945-Add-autoconf-test-for-AltiVec-to-use-it-o.patch

Attaching an updated patch against master with improved wording in the description.

Comment 21

[John Paul Adrian Glaubitz]

Comment on attachment 8776501 [details] [diff] [review]
Dont-try-to-build-with-Altivec-support-on-powerpcspe.patch

Marking this as obsolete, it's got a syntax error anyways:

+elif 'ppc' CONFIG['HAVE_ALTIVEC']:
+    use_altivec = True

We should care about the other patch only which applies against master.

Comment 22

[John Paul Adrian Glaubitz]

Attachment: 0001-Bug-972945-Add-autoconf-test-for-AltiVec-to-use-it-o.patch

Updated patch with a small syntax fix.

I have verified that the version of the patch for Firefox 47.0.1 works, btw. Will test the patch against git master later today.

Comment 23

[John Paul Adrian Glaubitz]

Attachment: 0001-Bug-972945-Add-autoconf-test-for-AltiVec-to-use-it-o.patch

Oops, I accidentally sent the old patch again. This is the right one now.

Comment 24

[John Paul Adrian Glaubitz]

Attachment: 0001-Bug-972945-Add-autoconf-test-for-AltiVec-to-use-it-o.patch

Ok, I have verified that the currently attached version of this patch fixes this particular problem for me on Debian powerpcspe, so I have verified the patch to do the right thing. It also lets AltiVec enabled on other ppc* targets.

How do we proceed to get this merged?

Comment 25

[Sylvestre Ledru [:Sylvestre]]

Ask for a Review to Mike. See comment#1

Comment 26

[Mike Hommey [:glandium]]

Comment on attachment 8777729 [details] [diff] [review]
0001-Bug-972945-Add-autoconf-test-for-AltiVec-to-use-it-o.patch

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

::: gfx/qcms/transform.c
@@ +1296,4 @@
>  #endif
>  		    } else
>  #endif
> +#if (defined(__POWERPC__) || defined(__powerpc__) && !defined(__NO_FPRS__))

IIRC qcms is semi third-party. I don't know how we handle patching it. Jeff, do you know?

::: old-configure.in
@@ +804,5 @@
> +      ;;
> +
> +    ppc*)
> +      AC_MSG_CHECKING(whether we can enable AltiVec support)
> +      if test -z "$(echo | ${CC} -x c -c -maltivec - 2>&1)" ; then

You should use an AC_TRY_COMPILE, saving CFLAGS beforehand, adding -maltivec to CFLAGS and restoring CFLAGS afterwards. Look around in this file for similar patterns.

Comment 27

[John Paul Adrian Glaubitz]

(In reply to Mike Hommey [:glandium] from comment #26)
> You should use an AC_TRY_COMPILE, saving CFLAGS beforehand, adding -maltivec
> to CFLAGS and restoring CFLAGS afterwards. Look around in this file for
> similar patterns.

Alright, just updated the patch to use AC_TRY_COMPILE as you suggested.

I noticed that some instances of AC_TRY_COMPILE in old-configure.in use an additional "if test $result" to set their variables (e.g. HAVE_X86_AVX2) while others set their variable directly within AC_TRY_COMPILE (e.g. HAVE_TOOLCHAIN_SUPPORT_MSSSE3). I opted for the latter.

Attaching my new patch.

Comment 28

[John Paul Adrian Glaubitz]

Attachment: 0001-Bug-972945-Add-autoconf-test-for-AltiVec-to-use-it-o.patch

Comment 29

[John Paul Adrian Glaubitz]

I just noticed, I should probably rebase my patch. Just a second.

Comment 30

[John Paul Adrian Glaubitz]

Attachment: 0001-Bug-972945-Add-autoconf-test-to-enable-AltiVec-on-su.patch

Updated patch which has been rebased against master.

Comment 31

[John Paul Adrian Glaubitz]

(In reply to John Paul Adrian Glaubitz from comment #30)
> Created attachment 8781521 [details] [diff] [review]
> 0001-Bug-972945-Add-autoconf-test-to-enable-AltiVec-on-su.patch
> 
> Updated patch which has been rebased against master.

I have verified that the patch works as expected. On "powerpc", gfx/qcms is still built with AltiVec support.

Comment 32

[Mike Hommey [:glandium]]

Comment on attachment 8781521 [details] [diff] [review]
0001-Bug-972945-Add-autoconf-test-to-enable-AltiVec-on-su.patch

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

The question remains whether gfx/qcms needs some special treatment wrt changes.

Comment 33

[Jeff Muizelaar [:jrmuizel]]

(In reply to Mike Hommey [:glandium] from comment #32)
> Comment on attachment 8781521 [details] [diff] [review]
> 0001-Bug-972945-Add-autoconf-test-to-enable-AltiVec-on-su.patch
> 
> Review of attachment 8781521 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The question remains whether gfx/qcms needs some special treatment wrt
> changes.

Seems reasonable.

Comment 34

[John Paul Adrian Glaubitz]

(In reply to Mike Hommey [:glandium] from comment #32)
> The question remains whether gfx/qcms needs some special treatment wrt
> changes.

I have had a closer look at this part of the code. It actually tests for AltiVec presence on the hardware itself instead of using compiler flags. So, when compiling natively (instead of cross-compiling), I think the additional test for __NO_FPRS__ *might* be redundant.

I have to verify that.

Comment 35

[John Paul Adrian Glaubitz]

(In reply to John Paul Adrian Glaubitz from comment #34)
> I have to verify that.

Ok, my patch is still correct. The test for __NO_FPRS__ here is necessary as otherwise the code in transform.c would still reference qcms_transform_data_rgb_out_lut_altivec and qcms_transform_data_rgba_out_lut_altivec which are in transform-altivec.c but which we are disabling with my patch on unsupported systems.

However, I can try to get my changes merged into qcms upstream if that helps.

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/18992fa73704
Add autoconf test to enable AltiVec on supported targets only. r=glandium

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/18992fa73704