1614183 - Check if PPC __has_include(<sys/auxv.h>)

Status: RESOLVED FIXED

(NSS :: Build, defect, P1)

Description

[Giulio Benetti]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/80.0.3987.87 Safari/537.36

Steps to reproduce:

Build on an environment without sys/auxv.h

Actual results:

sys/auxv.h is not present leading to build failure

Comment 1

[Giulio Benetti]

Attachment: 0001-Bug-1614183-Check-if-PPC-__has_include-sys-auxv.h.patch

Comment 2

[Giulio Benetti]

Kindly ping

Comment 3

[Kevin Jacobs [:kjacobs]]

Comment on attachment 9125320 [details] [diff] [review]
0001-Bug-1614183-Check-if-PPC-__has_include-sys-auxv.h.patch

I don't believe is available on GCC <5, and the documentation recommends a check to make sure it's defined first: https://gcc.gnu.org/onlinedocs/cpp/_005f_005fhas_005finclude.html. Could you please add that?

It might be more readable to check this inside the first `defined(__linux__) ...` conditional (rather than prepending two somewhat-unrelated conditions).

Comment 4

[Giulio Benetti]

(In reply to Kevin Jacobs [:kjacobs] from comment #3)

Comment on attachment 9125320 [details] [diff] [review]
0001-Bug-1614183-Check-if-PPC-__has_include-sys-auxv.h.patch

I don't believe is available on GCC <5, and the documentation recommends a
check to make sure it's defined first:
https://gcc.gnu.org/onlinedocs/cpp/_005f_005fhas_005finclude.html. Could you
please add that?
Sure
It might be more readable to check this inside the first defined(__linux__) ... conditional (rather than prepending two somewhat-unrelated conditions).
But this way we assume that on FreeBSD sys/auxv.h always exists.
Is that acceptable?

Comment 5

[Kevin Jacobs [:kjacobs]]

My suggestion was to place the first include guards around the include statement, rather than in the OS checks. Then it will check regardless.

Comment 6

[Giulio Benetti]

(In reply to Kevin Jacobs [:kjacobs] from comment #5)

My suggestion was to place the first include guards around the include statement, rather than in the OS checks. Then it will check regardless.

Ok for this.

Anyway if I use this form:
'''
#if defined __has_include

if __has_include (<sys/auxv.h>)

include <sys/auxv.h>

endif

#endif
'''
it will really include <sys/auxv.h> only if __has_include() is supported, so only with gcc version >= 4.9.2:
https://android.googlesource.com/toolchain/gcc/+/master/gcc-4.9/libcpp/ChangeLog#26

is this acceptable?

Comment 7

[Kevin Jacobs [:kjacobs]]

(In reply to Giulio Benetti from comment #6)

(In reply to Kevin Jacobs [:kjacobs] from comment #5)
....
it will really include <sys/auxv.h> only if __has_include() is supported, so only with gcc version >= 4.9.2:
https://android.googlesource.com/toolchain/gcc/+/master/gcc-4.9/libcpp/ChangeLog#26

is this acceptable?

Given that we still support older GCC versions, we'll probably want to fail the other way (with an else) and simply include it when we're not able to first verify its availability. This maintains the current behavior on those older GCCs.

Comment 8

[Giulio Benetti]

(In reply to Kevin Jacobs [:kjacobs] from comment #7)

(In reply to Giulio Benetti from comment #6)

(In reply to Kevin Jacobs [:kjacobs] from comment #5)
....
it will really include <sys/auxv.h> only if __has_include() is supported, so only with gcc version >= 4.9.2:
https://android.googlesource.com/toolchain/gcc/+/master/gcc-4.9/libcpp/ChangeLog#26

is this acceptable?

Given that we still support older GCC versions, we'll probably want to fail the other way (with an else) and simply include it when we're not able to first verify its availability. This maintains the current behavior on those older GCCs.

But on aarch64 and arm they use this:
https://hg.mozilla.org/projects/nss/file/tip/lib/freebl/blinit.c#l111

so already they assume that if __has_include() is not supported <sys/auxv.h> is not present.
And if I place __has_include() guard only for #include and <sys/auxv.h> is not present and not included,
compiler will fail on getauxval(AT_HWCAP2):
https://hg.mozilla.org/projects/nss/file/tip/lib/freebl/blinit.c#l452
or:
on elf_aux_into():
https://hg.mozilla.org/projects/nss/file/tip/lib/freebl/blinit.c#l455

so this patch would solve nothing IMHO.

So ok for:

#if defined(__powerpc__)

#if defined(__linux__) || (defined(__FreeBSD__) && __FreeBSD__ >= 12)

#ifdef __has_include
#if __has_include(<sys/auxv.h>)
#include <sys/auxv.h>
#endif
#else
#include <sys/auxv.h>
#endif

#elif (defined(__FreeBSD__) && __FreeBSD__ < 12)
#include <sys/sysctl.h>
#endif

but then I need this too:

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

I send a patch like this.

Comment 9

[Giulio Benetti]

(In reply to Giulio Benetti from comment #8)

(In reply to Kevin Jacobs [:kjacobs] from comment #7)

(In reply to Giulio Benetti from comment #6)

(In reply to Kevin Jacobs [:kjacobs] from comment #5)
....
it will really include <sys/auxv.h> only if __has_include() is supported, so only with gcc version >= 4.9.2:
https://android.googlesource.com/toolchain/gcc/+/master/gcc-4.9/libcpp/ChangeLog#26

is this acceptable?

Given that we still support older GCC versions, we'll probably want to fail the other way (with an else) and simply include it when we're not able to first verify its availability. This maintains the current behavior on those older GCCs.

But on aarch64 and arm they use this:
https://hg.mozilla.org/projects/nss/file/tip/lib/freebl/blinit.c#l111

so already they assume that if __has_include() is not supported <sys/auxv.h> is not present.
And if I place __has_include() guard only for #include and <sys/auxv.h> is not present and not included,
compiler will fail on getauxval(AT_HWCAP2):
https://hg.mozilla.org/projects/nss/file/tip/lib/freebl/blinit.c#l452
or:
on elf_aux_into():
https://hg.mozilla.org/projects/nss/file/tip/lib/freebl/blinit.c#l455

so this patch would solve nothing IMHO.

So ok for:

#if defined(__powerpc__)

#if defined(__linux__) || (defined(__FreeBSD__) && __FreeBSD__ >= 12)

#ifdef __has_include
#if __has_include(<sys/auxv.h>)
#include <sys/auxv.h>
#endif
#else
#include <sys/auxv.h>
#endif

#elif (defined(__FreeBSD__) && __FreeBSD__ < 12)
#include <sys/sysctl.h>
#endif

but then I need this too:

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

I send a patch like this.

Oh no, wait, if:

#if defined(__linux__)
#if __has_include(<sys/auxv.h>)
    hwcaps = getauxval(AT_HWCAP2);
#endif
#elif defined(__FreeBSD__)

but __has_include() is not defined, then <sys/auxv.h> will be includeed but getauxval() won't get called.

Mmmh

Comment 10

[Giulio Benetti]

I see only 2 options:

1. treat it like Aarch64/ARM assuming __has_include() is provided by compiler
2. introduce getauxval() and elf_aux_info() pointers similar to what's been already done with Aarch64/ARM:
   https://hg.mozilla.org/projects/nss/file/tip/lib/freebl/blinit.c#l122
   but also case 2) is based on case 1), so it will become a big mess IMHO.

Comment 11

[Kevin Jacobs [:kjacobs]]

I had not seen the #define __has_include(x) 0 ARM code you linked. My request was based on an assumption that it will be more common to compile on an old GCC (that can't check for the include) in an environment with <sys/auxv.h>, than to use such an old GCC and an environment without <sys/auxv.h>... And not wanting to disable HW crypto in the larger subset of builds.

Nevertheless, since we have a precedent for #define __has_include(x) 0, I'm okay adapting that solution here. We'll just need to make sure it's defined in PPC builds as well.

Comment 12

[Giulio Benetti]

Attachment: 0001-Bug-1614183-Check-if-PPC-__has_include-sys-auxv.h.patch

Here is the new patch using the same form Arm used:

#ifndef __has_include
#define __has_include(x) 0
#endif

Comment 13

[Kevin Jacobs [:kjacobs]]

Comment on attachment 9129442 [details] [diff] [review]
0001-Bug-1614183-Check-if-PPC-__has_include-sys-auxv.h.patch

Thanks, Giulio.

Comment 14

[Kevin Jacobs [:kjacobs]]

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