1531244 - Use __builtin_bswap64 in crypto_primitives.h

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement)

Description

[Makoto Kato [:m_kato]]

FREEBL_HTONLL dones't use builtin function when using gcc/clang on non-x86_64.

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

Comment 1

[Makoto Kato [:m_kato]]

Attachment: Bug 1531244 - Use __builtin_bswap64 in crypto_primitives.h

FREEBL_HTONLL dones't use builtin function when using gcc/clang on non-x86_64.
If __builtin_bswap64 is available, we should use it.

Comment 2

[Martin Thomson [:mt:]]

Though I've set Bug 15356734 as blocking this, I've landed these changes. They are an improvement and you aren't really responsible for the other bug.

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

Comment 3

[Petr Sumbera]

This change broke GCC build (on SPARC). GCC doesn't implement __has_builtin yet (see: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66970). But it still supports __builtin_bswap64. For now I have workaround it with:

-#define __has_builtin(x) 0
+#define __has_builtin(x) 1

Any suggestion for proper fix?

Comment 4

[Martin Thomson [:mt:]]

Oh yeah, that's unfortunate. We should be using the builtin with GCC too. The fix I can think of is to check for GCC version explicitly:

-#elif __has_builtin(__builtin_bswap64)
+#elif __has_builtin(__builtin_bswap64) || defined(GNUC) && GNUC > 4 || (GNUC == 4 && GNUC_MINOR >= 3)

According to the gcc docs, this builtin was added in GCC 4.3.

m_kato, does that look right to you?

Comment 5

[Makoto Kato [:m_kato]]

(In reply to Martin Thomson [:mt:] from comment #4)

Oh yeah, that's unfortunate. We should be using the builtin with GCC too. The fix I can think of is to check for GCC version explicitly:

-#elif __has_builtin(__builtin_bswap64)
+#elif __has_builtin(__builtin_bswap64) || defined(GNUC) && GNUC > 4 || (GNUC == 4 && GNUC_MINOR >= 3)

According to the gcc docs, this builtin was added in GCC 4.3.

m_kato, does that look right to you?

Yes, right. GCC 4.3+ supports __builtin_bswap64.

Comment 6

[Martin Thomson [:mt:]]

Added a followup: https://hg.mozilla.org/projects/nss/rev/c26bd889775cd57abed5e747a02fd495d52e1cd5

Comment 7

[Petr Sumbera]

There is actually one more occurrence here:
https://searchfox.org/mozilla-central/source/security/nss/lib/freebl/crypto_primitives.c#25