1590676 - Fix build if arm doesn't support NEON

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/78.0.3904.70 Safari/537.36

Steps to reproduce:

Here is the build log using Buildroot:
http://autobuild.buildroot.net/results/1342d305d1aeebef7af54a83afc094fda12421e2/build-end.log
on ARM architecture without NEON support.

Actual results:

Not every ARM architecture support NEON extension so need to take care about it.

Comment 1

[Giulio Benetti]

Attachment: 0001-Bug-1590676-Fix-build-if-arm-doesn-t-support-NEON.patch

Comment 2

[Giulio Benetti]

Sorry but this patch is buggy.
I submit the corrected one.

Comment 3

[Giulio Benetti]

Attachment: v2-0001-Bug-1590676-Fix-build-if-arm-doesn-t-support-NEON.patch

Comment 4

[Giulio Benetti]

Attachment: v3-0001-Bug-1590676-Fix-build-if-arm-doesn-t-support-NEON.patch

Sorry again, previous patch lack a paranthesis.
Now it builds correctly.

Comment 5

[Kevin Jacobs [:kjacobs]]

Comment on attachment 9104677 [details] [diff] [review]
v3-0001-Bug-1590676-Fix-build-if-arm-doesn-t-support-NEON.patch

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

Thanks for the patch. There's one issue here that needs to be resolved before this can land.

::: lib/freebl/rijndael.c
@@ +20,5 @@
>  #include "gcm.h"
>  #include "mpi.h"
>  
> +#if (!defined(IS_LITTLE_ENDIAN) && !defined(NSS_X86_OR_X64)) || \
> +    (!defined(__ARM_NEON) && !defined(__ARM_NEON__))

The new OR condition here will evaluate to true on anything except ARM (NEON). This disables native AES on Linux x86_64, among others.

I'd recommend conditioning it on `defined(__arm__)`. It wouldn't hurt to add its negation to the first line, either.

Comment 6

[Giulio Benetti]

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

Comment on attachment 9104677 [details] [diff] [review]
v3-0001-Bug-1590676-Fix-build-if-arm-doesn-t-support-NEON.patch

Review of attachment 9104677 [details] [diff] [review]:

Thanks for the patch. There's one issue here that needs to be resolved
before this can land.

::: lib/freebl/rijndael.c
@@ +20,5 @@

#include "gcm.h"
#include "mpi.h"

+#if (!defined(IS_LITTLE_ENDIAN) && !defined(NSS_X86_OR_X64)) || \

• (!defined(__ARM_NEON) && !defined(ARM_NEON))

The new OR condition here will evaluate to true on anything except ARM
(NEON). This disables native AES on Linux x86_64, among others.

Oops that's right.

I'd recommend conditioning it on defined(__arm__).

Ok

It wouldn't hurt to add its negation to the first line, either.

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

Comment on attachment 9104677 [details] [diff] [review]
v3-0001-Bug-1590676-Fix-build-if-arm-doesn-t-support-NEON.patch

Review of attachment 9104677 [details] [diff] [review]:

Thanks for the patch. There's one issue here that needs to be resolved
before this can land.

::: lib/freebl/rijndael.c
@@ +20,5 @@

#include "gcm.h"
#include "mpi.h"

+#if (!defined(IS_LITTLE_ENDIAN) && !defined(NSS_X86_OR_X64)) || \

• (!defined(__ARM_NEON) && !defined(ARM_NEON))

The new OR condition here will evaluate to true on anything except ARM
(NEON). This disables native AES on Linux x86_64, among others.

Oops, you're right.

I'd recommend conditioning it on defined(__arm__). It wouldn't hurt to add
its negation to the first line, either.

Something like this?

#if (!defined(arm) && !defined(IS_LITTLE_ENDIAN) && !defined(NSS_X86_OR_X64)) ||
(defined(arm) && !defined(__ARM_NEON) && !defined(ARM_NEON))

Comment 7

[Kevin Jacobs [:kjacobs]]

Actually, we need to leave the first line as-is (we rely on it to disable ARM HW AES if big-endian. I was mistaken in thinking that check was done elsewhere).

The second line is exactly what I had in mind, just note it's __arm__.

Comment 8

[Giulio Benetti]

Attachment: v4-0001-Bug-1590676-Fix-build-if-arm-doesn-t-support-NEON.patch

Here is the patch with correct #ifdef condition.

Best regards

Comment 9

[Kevin Jacobs [:kjacobs]]

Attachment: v5-0001-Bug-1590676-Fix-build-if-arm-doesn-t-support-NEON.patch

Looks good to me. Attaching a clang-formatted version of the same patch. Thanks again.

https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=6cf6eaf951143e00528e3e9ef3682b3c55266c63

Comment 10

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

https://hg.mozilla.org/projects/nss/rev/58f2471ace3b33d8a055b9e37921da8b960136e3