1606689 - Fix ppc 32-bit build failure

Yeah, I'm technically still on holiday today, just trying to release-wrangle for tomorrow's release. My review queue is 13 deep at present.. :) will take those on if not tomorrow than next week. Thank you for the patches!

Giulio Benetti

Assignee

Comment 5

•

5 years ago

Don't mind, enjoy your holidays instead!

and best wishes! :-)

vfazio

Comment 6

•

5 years ago

Unfortunately, this patch wont be sufficient. The build system is lacking the granularity needed to properly target PPC Altivec/VSX instructions.

The 64bit e5500 does not support Altivec, so even with this patch, having the below line would cause it to error.

ifeq ($(CPU_ARCH),ppc)
$(OBJDIR)/$(PROG_PREFIX)gcm-ppc$(OBJ_SUFFIX): CFLAGS += -mcrypto -maltivec
endif

cat <<EOF | /opt/xes/devkit/powerpc64-e6500-linux-gnu/bin/powerpc64-e6500-linux-gnu-gcc -o /dev/null -xc -maltivec -mcpu=e5500 -
int main() {  int val = 0; }
EOF  

cc1: error: AltiVec not supported in this target

You could specify -mvsx, but you'll then have instructions emitted that the target CPU may not be able to execute, causing an illegal instruction exception.

In Buildroot's case, there's 2 options

It could probably get away with a compile test to enable -maltivec because the toolchain should be targeting a specific CPU.

so running something like:

cat <<EOF | $(CC) -o /dev/null -xc -maltivec -
#include <altivec.h>
int main() { vector int val = vec_xl(0, (int *)0); }
EOF

Could enable -maltivec if there was no error.

it can key off of BR2_POWERPC_CPU_HAS_ALTIVEC in the libnss.mk file and selectively delete the -maltivec flag. This may get it close enough to working since the rest of the logic can take place in gcm.h/gcm-ppc.c.

For either case:
The USE_PPC_CRYPTO define will need to be guarded by something like __builtin_cpu_supports("arch_3_00") since vec_xl_be is exposed by ISA 3.0 according to the GCC docs

https://gcc.gnu.org/onlinedocs/gcc-8.3.0/gcc/PowerPC-Built-in-Functions.html#PowerPC-Built-in-Functions

I don't have any specific advice for the project itself other than don't enable altivec by default. There may be similar issues with -mcrypto since some of those functions require ISA 2.07 support by the CPU. It may be best to just let them be defined in CFLAGS outside of the build.

Giulio Benetti

Assignee

Comment 7

•

5 years ago

(In reply to vfazio from comment #6)

Unfortunately, this patch wont be sufficient. The build system is lacking the granularity needed to properly target PPC Altivec/VSX instructions.

The 64bit e5500 does not support Altivec, so even with this patch, having the below line would cause it to error.

You're right, now I see the problem we've discussed on Buildroot Mailing list.

ifeq ($(CPU_ARCH),ppc)
$(OBJDIR)/$(PROG_PREFIX)gcm-ppc$(OBJ_SUFFIX): CFLAGS += -mcrypto -maltivec
endif
cat <<EOF | /opt/xes/devkit/powerpc64-e6500-linux-gnu/bin/powerpc64-e6500-linux-gnu-gcc -o /dev/null -xc -maltivec -mcpu=e5500 -
int main() {  int val = 0; }
EOF  

cc1: error: AltiVec not supported in this target
You could specify -mvsx, but you'll then have instructions emitted that the target CPU may not be able to execute, causing an illegal instruction exception.

In Buildroot's case, there's 2 options

It could probably get away with a compile test to enable -maltivec because the toolchain should be targeting a specific CPU.

so running something like:
cat <<EOF | $(CC) -o /dev/null -xc -maltivec -
#include <altivec.h>
int main() { vector int val = vec_xl(0, (int *)0); }
EOF
Could enable -maltivec if there was no error.

it can key off of BR2_POWERPC_CPU_HAS_ALTIVEC in the libnss.mk file and selectively delete the -maltivec flag. This may get it close enough to working since the rest of the logic can take place in gcm.h/gcm-ppc.c.

I think this is the best choice, so need to add a variable to disable Altivec, something like NSS_DISABLE_ALTIVEC. So by default Altivec is enabled and if that variable is =1 then it skips to build gcm-ppc.c And we can emit a macro define to inform gcm.h about it.

What about this?

For either case:
The USE_PPC_CRYPTO define will need to be guarded by something like __builtin_cpu_supports("arch_3_00") since vec_xl_be is exposed by ISA 3.0 according to the GCC docs

This can be a second patch, do you want to create and submit it? Otherwise I can and you could review it.

https://gcc.gnu.org/onlinedocs/gcc-8.3.0/gcc/PowerPC-Built-in-Functions.html#PowerPC-Built-in-Functions

I don't have any specific advice for the project itself other than don't enable altivec by default. There may be similar issues with -mcrypto since some of those functions require ISA 2.07 support by the CPU. It may be best to just let them be defined in CFLAGS outside of the build.

Here it would be a problem since build system should be aware of this. What do you suggest?

Kind regards
Giulio Benetti

vfazio

Comment 8

•

5 years ago

Note that bug 1602386 may be related or a duplicate.

I think you're idea to add a NSS_ define to remove the altivec flags is fine.

I'd suggest the following change:

diff --git a/lib/freebl/gcm.h.bak b/lib/freebl/gcm.h
index ba9d09a..6dd6064 100644
--- a/lib/freebl/gcm.h.bak
+++ b/lib/freebl/gcm.h
@@ -30,7 +30,7 @@
 #include <arm_neon.h>
 #endif
 
-#if __powerpc64__
+#if defined(__powerpc64__) && defined(__POWER9_VECTOR__)
 #include "altivec-types.h"
 
 /* The ghash freebl test tries to use this in C++, and gcc defines conflict. */

Just wrap the block in a check for P9 Vector support, this should cover the case we're interested in. USE_PPC_CRYPTO will only use hardware acceleration if -mcpu=power9 or higher and any following are not specified -mno-vsx -mno-altivec -mno-power8-vector -mno-power9-vector

In the Makefile I'd probably do something like this to explicitly disable Altivec:

diff --git a/lib/freebl/Makefile.bak b/lib/freebl/Makefile
index 98a7c5d..5eb46af 100644
--- a/lib/freebl/Makefile.bak
+++ b/lib/freebl/Makefile
@@ -788,5 +788,9 @@ $(OBJDIR)/$(PROG_PREFIX)gcm-aarch64$(OBJ_SUFFIX): CFLAGS += -march=armv8-a+crypt
 endif
 
 ifeq ($(CPU_ARCH),ppc)
+ifdef $(NSS_DISABLE_ALTIVEC)
+$(OBJDIR)/$(PROG_PREFIX)gcm-ppc$(OBJ_SUFFIX): CFLAGS += -mno-altivec
+else
 $(OBJDIR)/$(PROG_PREFIX)gcm-ppc$(OBJ_SUFFIX): CFLAGS += -mcrypto -maltivec
 endif
+endif

status-firefox71: --- → verified disabled

vfazio

Comment 9

•

5 years ago

I'm still wary of the assumptions being made about CPU support for these features. As I had mentioned in the BR mailing list, PPC CPU tuning is not straightforward with the mix of bitness and ISAs.

There's really no way for the build system to detect the support for cross compile scenarios. It really needs to depend on the caller specifying an appropriate cpu tune (via CFLAGS=-mcpu=<processor>). Setting the proper tune will enable crypto, vsx, altivec, etc as appropriate.

Kevin Jacobs [:kjacobs]

Updated

•

5 years ago

Priority: -- → P1

Giulio Benetti

Assignee

Comment 10

•

5 years ago

(In reply to vfazio from comment #8)

Note that bug 1602386 may be related or a duplicate.

I think you're idea to add a NSS_ define to remove the altivec flags is fine.

I'd suggest the following change:
diff --git a/lib/freebl/gcm.h.bak b/lib/freebl/gcm.h
index ba9d09a..6dd6064 100644
--- a/lib/freebl/gcm.h.bak
+++ b/lib/freebl/gcm.h
@@ -30,7 +30,7 @@
 #include <arm_neon.h>
 #endif
 
-#if __powerpc64__
+#if defined(__powerpc64__) && defined(__POWER9_VECTOR__)
 #include "altivec-types.h"
 
 /* The ghash freebl test tries to use this in C++, and gcc defines conflict. */ 
Just wrap the block in a check for P9 Vector support, this should cover the case we're interested in. USE_PPC_CRYPTO will only use hardware acceleration if -mcpu=power9 or higher and any following are not specified -mno-vsx -mno-altivec -mno-power8-vector -mno-power9-vector

I see vec_xl_be() is available from power >= 7:
https://www.ibm.com/support/pages/adding-support-vecxl-vecxlbe-vecxst-and-vecxstbe-built-functions

So I wouldn't add that guard against POWER9_VECTOR

In the Makefile I'd probably do something like this to explicitly disable Altivec:

diff --git a/lib/freebl/Makefile.bak b/lib/freebl/Makefile
index 98a7c5d..5eb46af 100644
--- a/lib/freebl/Makefile.bak
+++ b/lib/freebl/Makefile
@@ -788,5 +788,9 @@ $(OBJDIR)/$(PROG_PREFIX)gcm-aarch64$(OBJ_SUFFIX): CFLAGS += -march=armv8-a+crypt
 endif
 
 ifeq ($(CPU_ARCH),ppc)
+ifdef $(NSS_DISABLE_ALTIVEC)
+$(OBJDIR)/$(PROG_PREFIX)gcm-ppc$(OBJ_SUFFIX): CFLAGS += -mno-altivec
+else
 $(OBJDIR)/$(PROG_PREFIX)gcm-ppc$(OBJ_SUFFIX): CFLAGS += -mcrypto -maltivec
 endif
+endif

This last sounds good.

vfazio

Comment 11

•

5 years ago

The challenge here is that while the vec_xl_be instruction is technically available as early as ISA 2.06, note that GCC documentation stays it's available as part of ISA 3.0 (-mcpu=power9) presumably to account for the expanded instruction set for big endian loads. so there's no guarantee based on current documentation that it would work if running on a power8 or earlier unless they provided emulation for the instruction.

https://gcc.gnu.org/onlinedocs/gcc-8.3.0/gcc/PowerPC-Built-in-Functions.html#PowerPC-Built-in-Functions

The following built-in functions are also available for the PowerPC family of processors, starting with ISA 3.0 or later (-mcpu=power9). These string functions are described separately in order to group the descriptions closer to the function prototypes:

...snip...

vector signed char vec_xl_be (signed long long, signed char *);
vector unsigned char vec_xl_be (signed long long, unsigned char *);
vector signed int vec_xl_be (signed long long, signed int *);
vector unsigned int vec_xl_be (signed long long, unsigned int *);
vector signed __int128 vec_xl_be (signed long long, signed __int128 *);
vector unsigned __int128 vec_xl_be (signed long long, unsigned __int128 *);
vector signed long long vec_xl_be (signed long long, signed long long *);
vector unsigned long long vec_xl_be (signed long long, unsigned long long *);
vector signed short vec_xl_be (signed long long, signed short *);
vector unsigned short vec_xl_be (signed long long, unsigned short *);
vector double vec_xl_be (signed long long, double *);
vector float vec_xl_be (signed long long, float *);

Regardless, that's not the only instruction we're concerned about here. vpmsumd is also used and that requires P8 vector support as it was introduced in the 2.07 ISA (-mcpu=power8)

BU_CRYPTO_2A (VPMSUMB,          "vpmsumb",        CONST, crypto_vpmsumb)
BU_CRYPTO_2A (VPMSUMH,          "vpmsumh",        CONST, crypto_vpmsumh)
BU_CRYPTO_2A (VPMSUMW,          "vpmsumw",        CONST, crypto_vpmsumw)
BU_CRYPTO_2A (VPMSUMD,          "vpmsumd",        CONST, crypto_vpmsumd)

#define BU_CRYPTO_2A(ENUM, NAME, ATTR, ICODE)                           \
  RS6000_BUILTIN_2 (CRYPTO_BUILTIN_ ## ENUM,            /* ENUM */      \
                    "__builtin_crypto_" NAME,           /* NAME */      \
                    RS6000_BTM_P8_VECTOR,               /* MASK */      \
                    (RS6000_BTC_ ## ATTR                /* ATTR */      \
                     | RS6000_BTC_BINARY),                              \
                    CODE_FOR_ ## ICODE)                 /* ICODE */

So there needs to be at least a POWER8_VECTOR check if you want to ignore the GCC documentation about vec_xl_be. clang also guards the vpmsumd function with POWER8_VECTOR so that should be fine.

Giulio Benetti

Assignee

Comment 12

•

5 years ago

This is a duplicate of:
https://bugzilla.mozilla.org/show_bug.cgi?id=1608151

Giulio Benetti

Assignee

Updated

•

5 years ago

Status: ASSIGNED → RESOLVED

Closed: 5 years ago

Resolution: --- → DUPLICATE

Bugzilla

Fix ppc 32-bit build failure

Categories

(NSS :: Build, defect, P1)

Tracking

(firefox71 verified disabled)

People

(Reporter: giulio.benetti, Assigned: giulio.benetti)

References

Details

Crash Data

Security

(public)

User Story

Attachments

(1 file)

Description

Comment 1

Comment 2

Updated

Comment 3

Comment 4

Comment 5

Comment 6

Comment 7

Comment 8

Comment 9

Updated

Comment 10

Comment 11

Comment 12

Updated

Attachment

General

Description

File Name

Content Type