Closed Bug 1698320 Opened 3 years ago Closed 3 years ago

security/nss/lib/freebl/chacha20poly1305.c:223:9: error: use of unknown builtin '__builtin_cpu_supports' [-Wimplicit-function-declaration]

Categories

(NSS :: Libraries, defect, P1)

Product:
NSS
Component:
Libraries
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: glandium, Assigned: aoeuh)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file)

Bug 1698320 - replace __builtin_cpu_supports("vsx") with ppc_crypto_support() for clang. r=bbeurdouche
48 bytes, text/x-phabricator-request
Details | Review

ppc64el Linux builds fail with:

[task 2021-03-13T23:21:36.474Z] 23:21:36    ERROR -  /builds/worker/checkouts/gecko/security/nss/lib/freebl/chacha20poly1305.c:223:9: error: use of unknown builtin '__builtin_cpu_supports' [-Wimplicit-function-declaration]
[task 2021-03-13T23:21:36.475Z] 23:21:36     INFO -      if (__builtin_cpu_supports("vsx")) {
[task 2021-03-13T23:21:36.475Z] 23:21:36     INFO -          ^
[task 2021-03-13T23:21:36.475Z] 23:21:36  WARNING -  /builds/worker/checkouts/gecko/security/nss/lib/freebl/chacha20poly1305.c:241:32: warning: result of comparison of constant 274877906944 with expression of type 'unsigned int' is always false [-Wtautological-constant-out-of-range-compare]
[task 2021-03-13T23:21:36.475Z] 23:21:36     INFO -      if (sizeof(len) > 4 && len >= (1ULL << (6 + 32))) {
[task 2021-03-13T23:21:36.475Z] 23:21:36     INFO -                             ~~~ ^  ~~~~~~~~~~~~~~~~~~
[task 2021-03-13T23:21:36.475Z] 23:21:36    ERROR -  /builds/worker/checkouts/gecko/security/nss/lib/freebl/chacha20poly1305.c:296:9: error: use of unknown builtin '__builtin_cpu_supports' [-Wimplicit-function-declaration]
[task 2021-03-13T23:21:36.475Z] 23:21:36     INFO -      if (__builtin_cpu_supports("vsx")) {
[task 2021-03-13T23:21:36.476Z] 23:21:36     INFO -          ^
[task 2021-03-13T23:21:36.476Z] 23:21:36  WARNING -  /builds/worker/checkouts/gecko/security/nss/lib/freebl/chacha20poly1305.c:267:42: warning: result of comparison of constant 274877906944 with expression of type 'unsigned int' is always false [-Wtautological-constant-out-of-range-compare]
[task 2021-03-13T23:21:36.476Z] 23:21:36     INFO -      if (sizeof(inputLen) > 4 && inputLen >= (1ULL << (6 + 32))) {
[task 2021-03-13T23:21:36.476Z] 23:21:36     INFO -                                  ~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~
[task 2021-03-13T23:21:36.476Z] 23:21:36    ERROR -  /builds/worker/checkouts/gecko/security/nss/lib/freebl/chacha20poly1305.c:365:9: error: use of unknown builtin '__builtin_cpu_supports' [-Wimplicit-function-declaration]
[task 2021-03-13T23:21:36.476Z] 23:21:36     INFO -      if (__builtin_cpu_supports("vsx")) {
[task 2021-03-13T23:21:36.476Z] 23:21:36     INFO -          ^
[task 2021-03-13T23:21:36.476Z] 23:21:36    ERROR -  /builds/worker/checkouts/gecko/security/nss/lib/freebl/chacha20poly1305.c:421:9: error: use of unknown builtin '__builtin_cpu_supports' [-Wimplicit-function-declaration]
[task 2021-03-13T23:21:36.476Z] 23:21:36     INFO -      if (__builtin_cpu_supports("vsx")) {
[task 2021-03-13T23:21:36.476Z] 23:21:36     INFO -          ^
[task 2021-03-13T23:21:36.476Z] 23:21:36  WARNING -  /builds/worker/checkouts/gecko/security/nss/lib/freebl/chacha20poly1305.c:404:42: warning: result of comparison of constant 274877906944 with expression of type 'unsigned int' is always false [-Wtautological-constant-out-of-range-compare]
[task 2021-03-13T23:21:36.476Z] 23:21:36     INFO -      if (sizeof(inputLen) > 4 && inputLen >= (1ULL << (6 + 32))) {
[task 2021-03-13T23:21:36.477Z] 23:21:36     INFO -                                  ~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~
[task 2021-03-13T23:21:36.477Z] 23:21:36    ERROR -  /builds/worker/checkouts/gecko/security/nss/lib/freebl/chacha20poly1305.c:475:9: error: use of unknown builtin '__builtin_cpu_supports' [-Wimplicit-function-declaration]
[task 2021-03-13T23:21:36.477Z] 23:21:36     INFO -      if (__builtin_cpu_supports("vsx")) {
[task 2021-03-13T23:21:36.477Z] 23:21:36     INFO -          ^
[task 2021-03-13T23:21:36.477Z] 23:21:36  WARNING -  /builds/worker/checkouts/gecko/security/nss/lib/freebl/chacha20poly1305.c:461:42: warning: result of comparison of constant 274877906944 with expression of type 'unsigned int' is always false [-Wtautological-constant-out-of-range-compare]
[task 2021-03-13T23:21:36.477Z] 23:21:36     INFO -      if (sizeof(inputLen) > 4 && inputLen >= (1ULL << (6 + 32))) {
[task 2021-03-13T23:21:36.477Z] 23:21:36     INFO -                                  ~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~
[task 2021-03-13T23:21:36.477Z] 23:21:36     INFO -  4 warnings and 5 errors generated.

https://treeherder.mozilla.org/logviewer?job_id=333148525&repo=try&lineNumber=35908

Looks like clang only supports __builtin_cpu_supports on x86: https://searchfox.org/llvm/search?q=__builtin_cpu_supports&path=&case=true&regexp=false

This should be fixable by use of

#if defined(__VSX__)

see usage example here: https://github.com/google/snappy/pull/126/files#diff-78bef59c652f9c2c0e207f29e5264a94bb0e2d3a5c9eda618a34bb42035193f6

Thanks Glandium!

NSS Beta is on Monday morning and release is on Thursday.
If we can fix and confirm the fix by Wednesday, I’ll keep the patch in release. Otherwise I’ll black-out the patch from 3.63 so that release keeps building on ppc64le.

Assignee: nobody → aoeuh
Severity: -- → S3
Status: NEW → ASSIGNED
Flags: needinfo?(bbeurdouche)
Priority: -- → P1

Possibly the most quick-n-elegant fix would be to adopt the existent code-style and provide a "ppc_vsx_support()", similar to the "ssse3_support() && sse4_1_support() && avx_support()" helpers.

That seem like a good way of fixing this, yes.

I used the already implemented ppc_crypto_support () function because crypto extensions were introduced in ISA v2.07 and my code uses instructions from there. Support for VSX was introduced in ISA v2.06 and it is not enough to test their support.

I also replaced '#include <ppc-asm.h>' with register definitions because clang-7 does not have a ppc-asm.h file.

abebeos thanks!

You're welcome.

@Beurdouche, a question, out of curiosity:

Is this "repo=try" kind of a "full product build", which includes "nss"?

https://treeherder.mozilla.org/logviewer?job_id=333148525&repo=try&lineNumber=35908

(In reply to abebeos from comment #8)

Is this "repo=try" kind of a "full product build", which includes "nss"?

https://treeherder.mozilla.org/logviewer?job_id=333148525&repo=try&lineNumber=35908

It's a Firefox build.

I tried the patch, and that's not enough, at least in the context of Firefox:

[task 2021-03-14T22:56:51.352Z] 22:56:51     INFO -  /builds/worker/fetches/binutils/bin/ld.bfd: chacha20poly1305.o: in function `ChaCha20Xor':
[task 2021-03-14T22:56:51.352Z] 22:56:51     INFO -  /builds/worker/checkouts/gecko/security/nss/lib/freebl/chacha20poly1305.c:224: undefined reference to `chacha20vsx'
[task 2021-03-14T22:56:51.352Z] 22:56:51     INFO -  /builds/worker/fetches/binutils/bin/ld.bfd: chacha20poly1305.o: in function `ChaCha20Poly1305_Seal':
[task 2021-03-14T22:56:51.352Z] 22:56:51     INFO -  /builds/worker/checkouts/gecko/security/nss/lib/freebl/chacha20poly1305.c:297: undefined reference to `Chacha20Poly1305_vsx_aead_encrypt'
[task 2021-03-14T22:56:51.352Z] 22:56:51     INFO -  /builds/worker/fetches/binutils/bin/ld.bfd: chacha20poly1305.o: in function `ChaCha20Poly1305_Open':
[task 2021-03-14T22:56:51.352Z] 22:56:51     INFO -  /builds/worker/checkouts/gecko/security/nss/lib/freebl/chacha20poly1305.c:366: undefined reference to `Chacha20Poly1305_vsx_aead_decrypt'
[task 2021-03-14T22:56:51.352Z] 22:56:51     INFO -  /builds/worker/fetches/binutils/bin/ld.bfd: chacha20poly1305.o: in function `ChaCha20Poly1305_Encrypt':
[task 2021-03-14T22:56:51.352Z] 22:56:51     INFO -  /builds/worker/checkouts/gecko/security/nss/lib/freebl/chacha20poly1305.c:422: undefined reference to `Chacha20Poly1305_vsx_aead_encrypt'
[task 2021-03-14T22:56:51.352Z] 22:56:51     INFO -  /builds/worker/fetches/binutils/bin/ld.bfd: chacha20poly1305.o: in function `ChaCha20Poly1305_Decrypt':
[task 2021-03-14T22:56:51.352Z] 22:56:51     INFO -  /builds/worker/checkouts/gecko/security/nss/lib/freebl/chacha20poly1305.c:476: undefined reference to `Chacha20Poly1305_vsx_aead_decrypt'
[task 2021-03-14T22:56:51.352Z] 22:56:51     INFO -  clang-11: error: linker command failed with exit code 1 (use -v to see invocation)
[task 2021-03-14T22:56:51.353Z] 22:56:51    ERROR -  make[4]: *** [/builds/worker/checkouts/gecko/config/rules.mk:545: libfreeblpriv3.so] Error 1

I presume that's due to target_arch being ppc64 when building inside Firefox. This is arguably a problem with how target_arch is set in Firefox, but this raises the question whether there should also be a check for ppc64 in https://searchfox.org/mozilla-central/source/security/nss/lib/freebl/freebl_base.gypi#98 like in https://searchfox.org/mozilla-central/source/security/nss/lib/freebl/freebl_base.gypi#153. Actually, considering https://searchfox.org/mozilla-central/source/security/nss/lib/freebl/freebl.gyp#417, it seems like it should be the case.

ppc64 is the big-endian (older), ppc64le the little-endian (newer, more supported). [1]

A quick look tells me that you're right,

https://searchfox.org/mozilla-central/source/security/nss/lib/freebl/freebl_base.gypi#98

should become (target_arch=="ppc64" or target_arch=="ppc64le")

But aoeuh should know better, I do not know the code internals.

[1] https://en.wikipedia.org/wiki/Ppc64

This code shouldn't work for big endian. There was no such requirement, and when I wrote it, I decided not to solve a problem that I do not have. I think it won't be difficult to add big endian support in the future if someone really needs it.

I presume that's due to target_arch being ppc64 when building inside Firefox. This is arguably a problem with how target_arch is set in Firefox, but this raises the question whether there should also be a check for ppc64 in https://searchfox.org/mozilla-central/source/security/nss/lib/freebl/freebl_base.gypi#98 like in https://searchfox.org/mozilla-central/source/security/nss/lib/freebl/freebl_base.gypi#153.

Yes, I can add this check and I think it will solve the problem, but I think it should not be so. If firefox uses ppc64 target on ppc64le it's wrong. There is no harm in doing this check, except that whoever reads it will be confused "why did they do this after all this code is only for little endian?". Also, building on big endian will take a few seconds longer due to compilation of an unused module, and the entire compilation may be broken for unpredictable reasons due to an unused module.

Actually, considering https://searchfox.org/mozilla-central/source/security/nss/lib/freebl/freebl.gyp#417, it seems like it should be the case.

Yes, it’s wrong. Should be:

[ 'disable_altivec==0 and target_arch=="ppc64"', {
  'dependencies': [
    'gcm-aes-ppc_c_lib',
    'gcm-sha512-ppc_c_lib',
  ],
}],
[ 'disable_altivec==0 and target_arch=="ppc64le"', {
  'dependencies': [
    'gcm-aes-ppc_c_lib',
    'gcm-sha512-ppc_c_lib',
    'chacha20-ppc_lib',
  ],
}],

but I don't think it will fix the problem in firefox.

Well, I was only assuming the code was supposed to be endian-independant from the fact that it was added to an endian-independent section... if it's not, then obviously the gyp file needs to be fixed, and Firefox will need a separate fix. The Makefile will need to be adjusted too, then. I'm not sure the make-based build system has a distinction for endianness, though...

I'm not sure the make-based build system has a distinction for endianness, though...

OS_TEST would apparently be ppc64le

@glandium Everything that should be fixed is inside NSS build systems, makefile and gyp, correct? OIC

my overall (possibly wrong) understanding is:

the firefox build https://treeherder.mozilla.org/logviewer?job_id=333148525&repo=try&lineNumber=35908 contains:

Job: linux64-other opt Build build-linux-ppc64el/opt ppc64el

=> ppc64el = ppc64le

so, it starts correctly.

The place wheretarget_arch is set-to'/overridden-to/passed-as ppc64 should be the bug to fix.

https://searchfox.org/mozilla-central/search?q=target_arch&redirect=false

https://treeherder.mozilla.org/logviewer?job_id=333148525&repo=try&lineNumber=879 this is the place for the Firefox part of the bug, but there's actually more than ppc to handle there, so it would be better to address in a separate bug. Edit: Filed bug 1698669.

@aoeuh, can you add a commit to this patch to do the freebl gyp change that you proposed above?

Flags: needinfo?(bbeurdouche) → needinfo?(aoeuh)

@glandium what's your opinion on the timing? I am fine with keeping those changes in NSS trunk but I should probably backout from the 3.63 release and the Firefox uplift that goes with it? (I intend to release and uplift on Thursday morning)

Flags: needinfo?(mh+mozilla)

If you can get the fix before then, then I don't think it hurts to have it in the release. Otherwise, yes, backout.

Flags: needinfo?(mh+mozilla)

@beurdouche, done

Flags: needinfo?(aoeuh)

Thanks @aoeuh!
So in the end, since we change Firefox versions on monday, I'll keep the patches on the main NSS branch, but remove them from the 3.63 release to avoid breaking Firefox 78 Beta on PowerPC. I'll land a full update of NSS in mozilla-central early next week, that will contain the changes, so that only Firefox Nightly 79 will be broken on Power until bug 1698669 is fixed.

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Flags: needinfo?(bbeurdouche)
Resolution: --- → FIXED
Flags: needinfo?(bbeurdouche)
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: