Fails to build under Debian powerpcspe

RESOLVED FIXED in Firefox 51

Status

defect
RESOLVED FIXED
5 years ago
6 months ago

People

(Reporter: sylvestre, Assigned: glaubitz)

Tracking

unspecified
mozilla51
PowerPC
Linux

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment, 10 obsolete attachments)

2.73 KB, patch
glandium
: review+
jrmuizel
: feedback+
Details | Diff | Splinter Review
Reporter

Description

5 years ago
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
Reporter

Comment 1

5 years ago
Posted patch fix-powerpcspe.patch (obsolete) — Splinter Review
Patch by Roland Stigge <stigge@antcom.de>
Attachment #8376370 - Flags: review?(mh+mozilla)
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.
Attachment #8376370 - Flags: review?(mh+mozilla) → review-
Assignee

Comment 3

3 years ago
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
Reporter

Comment 4

3 years ago
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.
Assignee

Comment 5

3 years ago
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
Reporter

Comment 6

3 years ago
I think that comment #2 is still valid, right?
Assignee

Comment 7

3 years ago
(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?
Reporter

Comment 8

3 years ago
yes, what Mike asked
Assignee

Comment 9

3 years ago
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 :).
Assignee

Comment 10

3 years ago
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
Reporter

Updated

3 years ago
Flags: needinfo?(mh+mozilla)
Assignee

Comment 11

3 years ago
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
Assignee

Comment 12

3 years ago
(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".
Assignee

Comment 13

3 years ago
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
Reporter

Updated

3 years ago
Assignee: nobody → glaubitz
Flags: needinfo?(mh+mozilla)
Reporter

Updated

3 years ago
Attachment #8776369 - Flags: review?(mh+mozilla)
Reporter

Updated

3 years ago
Attachment #8376370 - Attachment is obsolete: true
Assignee

Comment 14

3 years ago
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
Assignee

Comment 15

3 years ago
Attaching an updated patch, still have to test it though.
Attachment #8776369 - Attachment is obsolete: true
Attachment #8776369 - Flags: review?(mh+mozilla)
Assignee

Comment 16

3 years ago
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.
Attachment #8776491 - Attachment is obsolete: true
Reporter

Comment 17

3 years ago
Good, by the way, you generate the patch from the repo, not on 47.0.1.
Assignee

Comment 18

3 years ago
(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.
Assignee

Comment 19

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

Comment 20

3 years ago
Attaching an updated patch against master with improved wording in the description.
Attachment #8776526 - Attachment is obsolete: true
Assignee

Comment 21

3 years ago
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.
Attachment #8776501 - Attachment is obsolete: true
Assignee

Comment 22

3 years ago
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.
Attachment #8776530 - Attachment is obsolete: true
Assignee

Comment 23

3 years ago
Oops, I accidentally sent the old patch again. This is the right one now.
Attachment #8777208 - Attachment is obsolete: true
Assignee

Comment 24

3 years ago
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?
Attachment #8777209 - Attachment is obsolete: true
Reporter

Comment 25

3 years ago
Ask for a Review to Mike. See comment#1
Assignee

Updated

3 years ago
Attachment #8777729 - Flags: review?(mh+mozilla)
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.
Attachment #8777729 - Flags: review?(mh+mozilla) → feedback?(jmuizelaar)
Assignee

Comment 27

3 years ago
(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.
Assignee

Comment 28

3 years ago
Attachment #8777729 - Attachment is obsolete: true
Attachment #8777729 - Flags: feedback?(jmuizelaar)
Attachment #8781490 - Flags: review?(mh+mozilla)
Assignee

Comment 29

3 years ago
I just noticed, I should probably rebase my patch. Just a second.
Assignee

Comment 30

3 years ago
Updated patch which has been rebased against master.
Attachment #8781490 - Attachment is obsolete: true
Attachment #8781490 - Flags: review?(mh+mozilla)
Attachment #8781521 - Flags: review?(mh+mozilla)
Assignee

Comment 31

3 years ago
(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 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.
Attachment #8781521 - Flags: review?(mh+mozilla)
Attachment #8781521 - Flags: review+
Attachment #8781521 - Flags: feedback?(jmuizelaar)
(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.
Attachment #8781521 - Flags: feedback?(jmuizelaar) → feedback+
Assignee

Comment 34

3 years ago
(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.
Assignee

Comment 35

3 years ago
(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

3 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/18992fa73704
Add autoconf test to enable AltiVec on supported targets only. r=glandium
Keywords: checkin-needed

Comment 37

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/18992fa73704
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Component: Build Config → General
Product: Firefox → Firefox Build System
Target Milestone: Firefox 51 → mozilla51
You need to log in before you can comment on or make changes to this bug.