Closed Bug 972945 Opened 7 years ago Closed 4 years ago

Fails to build under Debian powerpcspe

Categories

(Firefox Build System :: General, defect)

PowerPC
Linux
defect
Not set
normal

Tracking

(firefox51 fixed)

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: Sylvestre, Assigned: glaubitz)

Details

Attachments

(1 file, 10 obsolete files)

2.73 KB, patch
glandium
: review+
jrmuizel
: feedback+
Details | Diff | Splinter Review
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
Attached 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-
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
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.
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
I think that comment #2 is still valid, right?
(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?
yes, what Mike asked
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 :).
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
Flags: needinfo?(mh+mozilla)
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
(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".
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
Assignee: nobody → glaubitz
Flags: needinfo?(mh+mozilla)
Attachment #8776369 - Flags: review?(mh+mozilla)
Attachment #8376370 - Attachment is obsolete: true
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
Attaching an updated patch, still have to test it though.
Attachment #8776369 - Attachment is obsolete: true
Attachment #8776369 - Flags: review?(mh+mozilla)
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
Good, by the way, you generate the patch from the repo, not on 47.0.1.
(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.
Alright, here's a patch against master (checked out from: https://github.com/mozilla/gecko-dev).
Attaching an updated patch against master with improved wording in the description.
Attachment #8776526 - Attachment is obsolete: true
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
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
Oops, I accidentally sent the old patch again. This is the right one now.
Attachment #8777208 - Attachment is obsolete: true
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
Ask for a Review to Mike. See comment#1
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)
(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.
Attachment #8777729 - Attachment is obsolete: true
Attachment #8777729 - Flags: feedback?(jmuizelaar)
Attachment #8781490 - Flags: review?(mh+mozilla)
I just noticed, I should probably rebase my patch. Just a second.
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)
(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+
(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.
(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.
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
https://hg.mozilla.org/mozilla-central/rev/18992fa73704
Status: NEW → RESOLVED
Closed: 4 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.