Fails to build under Debian powerpcspe

RESOLVED FIXED in Firefox 51

Status

()

Firefox
Build Config
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: sylvestre, Assigned: John Paul Adrian Glaubitz)

Tracking

unspecified
Firefox 51
PowerPC
Linux
Points:
---

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

4 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

4 years ago
Created attachment 8376370 [details] [diff] [review]
fix-powerpcspe.patch

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

2 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

2 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

2 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

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

Comment 7

2 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

2 years ago
yes, what Mike asked
(Assignee)

Comment 9

2 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

2 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

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

Comment 11

2 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

2 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

2 years ago
Created attachment 8776369 [details] [diff] [review]
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
(Reporter)

Updated

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

Updated

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

Updated

2 years ago
Attachment #8376370 - Attachment is obsolete: true
(Assignee)

Comment 14

2 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

2 years ago
Created attachment 8776491 [details] [diff] [review]
Dont-try-to-build-with-Altivec-support-on-powerpcspe.patch

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

2 years ago
Created attachment 8776501 [details] [diff] [review]
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.
Attachment #8776491 - Attachment is obsolete: true
(Reporter)

Comment 17

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

Comment 18

2 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

2 years ago
Created attachment 8776526 [details] [diff] [review]
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).
(Assignee)

Comment 20

2 years ago
Created attachment 8776530 [details] [diff] [review]
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.
Attachment #8776526 - Attachment is obsolete: true
(Assignee)

Comment 21

2 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

2 years ago
Created attachment 8777208 [details] [diff] [review]
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.
Attachment #8776530 - Attachment is obsolete: true
(Assignee)

Comment 23

2 years ago
Created attachment 8777209 [details] [diff] [review]
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.
Attachment #8777208 - Attachment is obsolete: true
(Assignee)

Comment 24

2 years ago
Created attachment 8777729 [details] [diff] [review]
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?
Attachment #8777209 - Attachment is obsolete: true
(Reporter)

Comment 25

2 years ago
Ask for a Review to Mike. See comment#1
(Assignee)

Updated

2 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

2 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

2 years ago
Created attachment 8781490 [details] [diff] [review]
0001-Bug-972945-Add-autoconf-test-for-AltiVec-to-use-it-o.patch
Attachment #8777729 - Attachment is obsolete: true
Attachment #8777729 - Flags: feedback?(jmuizelaar)
Attachment #8781490 - Flags: review?(mh+mozilla)
(Assignee)

Comment 29

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

Comment 30

2 years ago
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.
Attachment #8781490 - Attachment is obsolete: true
Attachment #8781490 - Flags: review?(mh+mozilla)
Attachment #8781521 - Flags: review?(mh+mozilla)
(Assignee)

Comment 31

2 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+
Keywords: checkin-needed
(Assignee)

Comment 34

2 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

2 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

2 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/18992fa73704
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
You need to log in before you can comment on or make changes to this bug.