1602386 - Fix build on FreeBSD/powerpc*

Status: RESOLVED FIXED

(NSS :: Build, defect)

Description

[Piotr Kubaj]

Attachment: patch-lib_freebl_blinit.c

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Firefox/68.0

Steps to reproduce:

On FreeBSD/powerpc64 box:

1. portsnap fetch extract
2. make -C /usr/ports/security/nss install clean

Actual results:

blinit.c:417:27: error: use of undeclared identifier 'hwcaps'
ppc_crypto_support_ = hwcaps & PPC_FEATURE2_VEC_CRYPTO && disable_hw_crypto == NULL;
^
1 error generated.

Expected results:

NSS should install.

Comment 1

[Piotr Kubaj]

I copied from a wrong window, the actual error is:
blinit.c:410:19: error: implicit declaration of function 'getauxval' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
long hwcaps = getauxval(AT_HWCAP2);
^

Comment 2

[Kevin Jacobs [:kjacobs]]

Comment on attachment 9114492 [details] [diff] [review]
patch-lib_freebl_blinit.c

Thanks - the fix looks ok, but I don't have such a machine to confirm. We should probably include other BSDs: https://searchfox.org/mozilla-central/source/config/external/nspr/prcpucfg.h#21

Piotr: could you please attach an exported patch rather than just the diff, or (preferably) use [phabricator](https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html)? This way you'll get credit for the fix :).

Comment 3

[Jan Beich]

Comment on attachment 9114492 [details] [diff] [review]
patch-lib_freebl_blinit.c

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

Contains a bug. Please, fix and resubmit.

FWIW, attachment 9087350 [details] [diff] [review] has an example how to fix FreeBSD < 12 which lacks <sys/auxv.h>.

::: lib/freebl/blinit.c.orig
@@ +411,3 @@
>      long hwcaps = getauxval(AT_HWCAP2);
> +#elif defined(__FreeBSD__)
> +    long hwcaps;

IMPORTANT: Initialize to `0` in case `elf_aux_info` fails e.g., when run via qemu-user.

NOTE: `AT_HWCAP2` returns `unsigned long`, not `long`. Affects Linux as well.

Comment 4

[Piotr Kubaj]

Attachment: Fix build on FreeBSD/powerpc platforms.

FreeBSD has elf_aux_info instead of getauxval, but only since FreeBSD 12. Previous versions (11 is still supported) don't have any equivalent and users need to query sysctl manually.

Comment 5

[Jan Beich]

Comment on attachment 9115139 [details]
Fix build on FreeBSD/powerpc platforms.

(Sorry, I can't comment on Phabricator due to bug 1536716)

1. sysctlbyname requires <sys/sysctl.h> and maybe <sys/types.h>
2. AT_HWCAP2 returns hw.cpu_features2, not hw.cpu_features
3. unsigned long change should be in a separate commit and applied to AT_HWCAP as well
4. <sys/auxv.h> doesn't exist on OpenBSD and NetBSD, so move hwcaps outside of #if conditional

#if defined(__linux__) || (defined(__FreeBSD__) && __FreeBSD__ >= 12)
#include <sys/auxv.h>
#elif (defined(__FreeBSD__) && __FreeBSD__ < 12)
#include <sys/sysctl.h>
#endif
...
    unsigned long hwcaps = 0;
#if defined(__linux__)
    hwcaps = getauxval(AT_HWCAP2);
#elif defined(__FreeBSD__)
#  if __FreeBSD__ >= 12
    elf_aux_info(AT_HWCAP2, &hwcaps, sizeof(hwcaps));
#  else
    size_t len = sizeof(hwcaps);
    sysctlbyname("hw.cpu_features2", &hwcaps, &len, NULL, 0);
#  endif
#endif

Comment 6

[Jan Beich]

Comment on attachment 9115139 [details]
Fix build on FreeBSD/powerpc platforms.

+#if defined(__FreeBSD__) && __FreeBSD__ < 12 && defined(__powerpc__)
+#include <sys/types.h>
+#include <sys/sysctl.h>
+#endif
...
 #include <sys/auxv.h> 

Won't build on FreeBSD < 12 (what you're trying to fix) and NetBSD/OpenBSD due to missing /usr/include/sys/auxv.h. Why move includes to the top only to add defined(__powerpc__)?

Comment 7

[Piotr Kubaj]

Corrected.

I didn't notice that sys/auxv.h is included down there and since includes are generally put at the top, I put them there.

Comment 8

[Jan Beich]

Comment on attachment 9115139 [details]
Fix build on FreeBSD/powerpc platforms.

+#if defined(__FreeBSD__) && __FreeBSD__ < 12
+#include <sys/types.h>
+#include <sys/sysctl.h>
+#else
 #include <sys/auxv.h>
+#endif

Missing <sys/auxv.h> would still break NetBSD, OpenBSD and maybe Darwin, AIX. comment 5 had an example fix but if you didn't like then let respective maintainers take care. I was asked to review only FreeBSD-specific code. ;)

Comment 9

[Jan Beich]

Comment on attachment 9115139 [details]
Fix build on FreeBSD/powerpc platforms.

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

gcm.c and rijndael.c dispatch into SIMD only when CPU supports it. Building them with -maltivec may lead to a crash on older CPUs. There's probably a better way...

Can you show more context of the error e.g., command line used to build gcm-ppc.c that failed? I can't reproduce when using #include e.g.,

$ ssh ref12-ppc64.freebsd.org

$ cat a.c
#include "altivec-types.h"
vec_u64 foo(void) { }

$ gcc8 -maltivec -c a.c
$ echo $?
0

$ gcc8 -maltivec -c altivec-types.h
altivec-types.h:19:1: error: use of 'long long' in AltiVec types is invalid without '-mvsx'
 typedef __vector unsigned long long vec_u64;
 ^~~~~~~
altivec-types.h:20:1: error: use of 'long long' in AltiVec types is invalid without '-mvsx'
 typedef __vector signed long long vec_s64;
 ^~~~~~~

Comment 10

[Piotr Kubaj]

Error for gcm.c:
gcc9 -o FreeBSD12.1_OPT.OBJ/FreeBSD_SINGLE_SHLIB/gcm.o -c -std=c99 -O2 -pipe -I/usr/local/include/nspr -fstack-protector-strong -Wl,-rpath=/usr/local/lib/gcc9 -fno-strict-aliasing
-fPIC -Wall -Wno-switch -DFREEBSD -DHAVE_STRERROR -DHAVE_BSD_FLOCK -Wall -Wshadow -Werror -DXP_UNIX -UDEBUG -DNDEBUG -D_THREAD_SAFE -D_REENTRANT -DNSS_NO_INIT_SUPPORT -DUSE_UTIL_DIR
ECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -I./../dist/FreeBSD12.1_OPT.OBJ/include -I./../dist/public/ -I./../dist/private/ -fPIC -Wall -Wno-switch -DFR
EEBSD -DHAVE_STRERROR -DHAVE_BSD_FLOCK -Wall -Wshadow -Werror -DXP_UNIX -UDEBUG -DNDEBUG -D_THREAD_SAFE -D_REENTRANT -DNSS_NO_INIT_SUPPORT -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -D
SSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -I../../dist/FreeBSD12.1_OPT.OBJ/include -I../../dist/public/ -I../../dist/private/ -fPIC -Wall -Wno-switch -DFREEBSD -DHAVE_STRERROR -DHAVE_BSD_FLOCK -Wall -Wshadow -Werror -DXP_UNIX -DSHLIB_SUFFIX="so" -DSHLIB_PREFIX="lib" -DSHLIB_VERSION="3" -DSOFTOKEN_SHLIB_VERSION="3" -DRIJNDAEL_INCLUDE_TABLES -UDEBUG -DNDEBUG -D_THREAD_SAFE -D_REENTRANT -DNSS_NO_INIT_SUPPORT -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -DNSS_USE_64 -DFREEBL_LOWHASH -DNSS_NO_INIT_SUPPORT -DHAVE_INT128_SUPPORT -DMP_API_COMPATIBLE -I../../../dist/FreeBSD12.1_OPT.OBJ/include -I../../../dist/public/nss -I../../../dist/private/nss -Impi -Iecl -Iverified -fPIC
-Wall -Wno-switch -DFREEBSD -DHAVE_STRERROR -DHAVE_BSD_FLOCK -Wall -Wshadow -Werror -DXP_UNIX -DSHLIB_SUFFIX="so" -DSHLIB_PREFIX="lib" -DSHLIB_VERSION="3" -DSOFTOKEN_SHLIB_VERSION="3" -DRIJNDAEL_INCLUDE_TABLES -UDEBUG -DNDEBUG -D_THREAD_SAFE -D_REENTRANT -DNSS_NO_INIT_SUPPORT -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -DNSS_USE_64 -DFREEBL_LOWHASH -DNSS_NO_INIT_SUPPORT -DHAVE_INT128_SUPPORT -DMP_API_COMPATIBLE -I../../../dist/FreeBSD12.1_OPT.OBJ/include -I../../../dist/public/nss -I../../../dist/private/nss -Impi -Iecl -Iverified gcm.c
In file included from altivec-types.h:11,
from gcm.h:34,
from gcm.c:12:
/usr/local/lib/gcc9/gcc/powerpc64-portbld-freebsd12.1/9.2.0/include/altivec.h:34:2: error: #error Use the "-maltivec" flag to enable PowerPC AltiVec support
34 | #error Use the "-maltivec" flag to enable PowerPC AltiVec support
| ^~~~~
In file included from gcm.h:34,
from gcm.c:12:
altivec-types.h:19:1: error: use of 'long long' in AltiVec types is invalid without '-mvsx'
19 | typedef __vector unsigned long long vec_u64;
| ^~~~~~~
altivec-types.h:20:1: error: use of 'long long' in AltiVec types is invalid without '-mvsx'
20 | typedef __vector signed long long vec_s64;
| ^~~~~~~

Note that it's actually a bit inaccurate, adding -mvsx alone is actually enough (VSX probably implies AltiVec).

Comment 11

[Jan Beich]

Piotr, can you try enabling #define USE_PPC_CRYPTO from lib/freebl/gcm.h on big-endian then test build and runtime on FreeBSD powerpc64? -mcrypto -maltivec only need to be passed if USE_PPC_CRYPTO is set.

Comment 13

[J.C. Jones [:jcj] (he/they)]

I've posted a couple of questions in the Phabricator review, but I do want to get this PPC fix into tree.

Comment 14

[Jan Beich]

(In reply to Piotr Kubaj from comment #10)

In file included from altivec-types.h:11,
from gcm.h:34,
from gcm.c:12:
/usr/local/lib/gcc9/gcc/powerpc64-portbld-freebsd12.1/9.2.0/include/altivec.h:34:2: error: #error Use the "-maltivec" flag to enable PowerPC AltiVec support

Try pragma target (a la sse2 in the same file) then revert lib/freebl/Makefile changes.

#pragma GCC push_options
#pragma GCC target("altivec")
#include "altivec-types.h"
#pragma GCC pop_options

Clang has attribute pragma but it doesn't seem to work with headers. Maybe Clang should port https://github.com/llvm/llvm-project/commit/9fc7fb274e53 to <altivec.h> instead.

Comment 15

[J.C. Jones [:jcj] (he/they)]

Just to reiterate, I'm happy to accept a patch, via any means!

Comment 16

[J.C. Jones [:jcj] (he/they)]

https://hg.mozilla.org/projects/nss/rev/f2ac5e318886e39965d90e422ba20c8dcb9feca7

Comment 17

[J.C. Jones [:jcj] (he/they)]

https://hg.mozilla.org/projects/nss/rev/4bf79c4d29544d5eed2ecd2f59b2ef1a0ae29486