briansmith / ring

Safe, fast, small crypto using Rust
Other
3.65k stars 684 forks source link

Fails to build on Linux SPARC #2008

Closed glaubitz closed 1 month ago

glaubitz commented 3 months ago

Despite the changes in #1555, ring still fails to build on Linux SPARC:

(sid_sparc64-dchroot)glaubitz@stadler:~/ring$ cargo build --release
    Updating crates.io index
  Downloaded untrusted v0.9.0
  Downloaded cfg-if v1.0.0
  Downloaded getrandom v0.2.12
  Downloaded cc v1.0.90
  Downloaded libc v0.2.153
  Downloaded 5 crates (873.1 KB) in 0.66s
   Compiling libc v0.2.153
   Compiling cc v1.0.90
   Compiling cfg-if v1.0.0
   Compiling untrusted v0.9.0
   Compiling ring v0.17.8 (/home/glaubitz/ring)
   Compiling getrandom v0.2.12
The following warnings were emitted during compilation:

warning: Compiler version doesn't include clang or GCC: "cc" "--version"
warning: In file included from /home/glaubitz/ring/include/ring-core/base.h:74,
warning:                  from /home/glaubitz/ring/include/ring-core/mem.h:60,
warning:                  from /home/glaubitz/ring/crypto/curve25519/curve25519.c:22:
warning: /home/glaubitz/ring/include/ring-core/target.h:64:2: error: #error "Unknown target CPU"
warning:    64 | #error "Unknown target CPU"
warning:       |  ^~~~~
warning: In file included from /home/glaubitz/ring/crypto/curve25519/internal.h:20,
warning:                  from /home/glaubitz/ring/crypto/curve25519/curve25519.c:24:
warning: /home/glaubitz/ring/crypto/curve25519/../internal.h:219:2: error: #error "Must define either OPENSSL_32_BIT or OPENSSL_64_BIT"
warning:   219 | #error "Must define either OPENSSL_32_BIT or OPENSSL_64_BIT"
warning:       |  ^~~~~
warning: /home/glaubitz/ring/crypto/curve25519/../internal.h:232:15: error: unknown type name 'crypto_word_t'
warning:   232 | static inline crypto_word_t value_barrier_w(crypto_word_t a) {
warning:       |               ^~~~~~~~~~~~~
warning: /home/glaubitz/ring/crypto/curve25519/../internal.h:232:45: error: unknown type name 'crypto_word_t'
warning:   232 | static inline crypto_word_t value_barrier_w(crypto_word_t a) {
warning:       |                                             ^~~~~~~~~~~~~
warning: /home/glaubitz/ring/crypto/curve25519/../internal.h:244:15: error: unknown type name 'crypto_word_t'
warning:   244 | static inline crypto_word_t constant_time_msb_w(crypto_word_t a) {
warning:       |               ^~~~~~~~~~~~~
warning: /home/glaubitz/ring/crypto/curve25519/../internal.h:244:49: error: unknown type name 'crypto_word_t'
warning:   244 | static inline crypto_word_t constant_time_msb_w(crypto_word_t a) {
warning:       |                                                 ^~~~~~~~~~~~~
warning: /home/glaubitz/ring/crypto/curve25519/../internal.h:249:15: error: unknown type name 'crypto_word_t'
warning:   249 | static inline crypto_word_t constant_time_is_zero_w(crypto_word_t a) {
warning:       |               ^~~~~~~~~~~~~
warning: /home/glaubitz/ring/crypto/curve25519/../internal.h:249:53: error: unknown type name 'crypto_word_t'
warning:   249 | static inline crypto_word_t constant_time_is_zero_w(crypto_word_t a) {
warning:       |                                                     ^~~~~~~~~~~~~
warning: /home/glaubitz/ring/crypto/curve25519/../internal.h:264:15: error: unknown type name 'crypto_word_t'
warning:   264 | static inline crypto_word_t constant_time_is_nonzero_w(crypto_word_t a) {
warning:       |               ^~~~~~~~~~~~~
warning: /home/glaubitz/ring/crypto/curve25519/../internal.h:264:56: error: unknown type name 'crypto_word_t'
warning:   264 | static inline crypto_word_t constant_time_is_nonzero_w(crypto_word_t a) {
warning:       |                                                        ^~~~~~~~~~~~~
warning: /home/glaubitz/ring/crypto/curve25519/../internal.h:269:15: error: unknown type name 'crypto_word_t'
warning:   269 | static inline crypto_word_t constant_time_eq_w(crypto_word_t a,
warning:       |               ^~~~~~~~~~~~~
warning: /home/glaubitz/ring/crypto/curve25519/../internal.h:269:48: error: unknown type name 'crypto_word_t'
warning:   269 | static inline crypto_word_t constant_time_eq_w(crypto_word_t a,

Access to SPARC machines can be obtained through the GCC Compile Farm: https://gcc.gnu.org/wiki/CompileFarm

m-ueberall commented 2 months ago

I had to apply the below patch using cargo patch when building the current libsignal_jni.so (see https://github.com/signalapp/libsignal) – which pulls in ring v0.17.8 – for the sparc64 architecture; all other architectures (amd64, arm64, mips64el, armv7, ppc64, ppc64el, s390x, i686, riscv64) worked out of the box:

diff -U2 -r ring.upstream/include/ring-core/target.h ring/include/ring-core/target.h
--- ring.upstream/include/ring-core/target.h    1973-11-29 22:33:09.000000000 +0100
+++ ring/include/ring-core/target.h     2024-04-28 17:27:31.805358732 +0200
@@ -61,4 +61,10 @@
 #elif defined(__s390x__)
 #define OPENSSL_64_BIT
+#elif defined(__sparc__)
+#if defined(__LP64__)
+#define OPENSSL_64_BIT
+#else
+#define OPENSSL_32_BIT
+#endif
 #else
 #error "Unknown target CPU"
briansmith commented 1 month ago

It would be great if one of you could submit that patch as a PR, and it would be great if you could append mk/install-build-tools.sh and mk/cargo.sh and .github/actions/ci.yml to add this target to the test suite so we ensure it keeps working. I believe we need to also upgrade our dependency of getrandom and probably libc to newer versions.

glaubitz commented 1 month ago

It would be great if one of you could submit that patch as a PR, and it would be great if you could append mk/install-build-tools.sh and mk/cargo.sh and .github/actions/ci.yml to add this target to the test suite so we ensure it keeps working. I believe we need to also upgrade our dependency of getrandom and probably libc to newer versions.

Just opened #2066 which I verified to work on 64-bit SPARC on Debian Linux.

m-ueberall commented 1 month ago

[…] and it would be great if you could append mk/install-build-tools.sh and mk/cargo.sh and .github/actions/ci.yml to add this target to the test suite so we ensure it keeps working. I believe we need to also upgrade our dependency of getrandom and probably libc to newer versions.

Sorry, but we're exclusively using our own internal build environments (with a heavily modified/modernised tool-chain based on Ubuntu 20.04/Focal for the arm64/aarch64, amd64/x86_64, riscv64 architectures; artefacts for other architectures are currently built using cross-compilation); therefore, modifying the above definitions/workflow might be easier for those users/contributors that actually use the GitHub infrastructure (and maybe also work with ring directly instead of pulling it in as a dependency) without having to analyse the available environments first. What I can do is drop our own patch, pull in the above PR (until it is merged), and provide you with feedback whenever future builds start to fail.

briansmith commented 1 month ago

It is easy to modify mk/[install-bulid-tools.sh,cargo.sh}/ci.yml without running them locally exactly because they are run in GitHub Actions in CI. It makes it much easier for me to review the PRs that add the ports when they are run in CI. Basically, I'm asking you to help me help you.

glaubitz commented 1 month ago

It is easy to modify mk/[install-bulid-tools.sh,cargo.sh}/ci.yml without running them locally exactly because they are run in GitHub Actions in CI. It makes it much easier for me to review the PRs that add the ports when they are run in CI. Basically, I'm asking you to help me help you.

OK, sorry, I missed that. I need to have a closer look at the necessary changes.

briansmith commented 1 month ago

I suggested a more general approach to this issue in https://github.com/briansmith/ring/pull/2042#issuecomment-2120898907. I would prefer that approach to be taken because it eliminates the allowlist of target architectures.

glaubitz commented 1 month ago

I suggested a more general approach to this issue in #2042 (comment). I would prefer that approach to be taken because it eliminates the allowlist of target architectures.

Fine with me. Do you still want the CI-related changes?

briansmith commented 1 month ago

Fine with me. Do you still want the CI-related changes?

I would recommend contributing the CI changes if people want any help in the future related to diagnosing any SPARC-related issues and to ensure that SPARC codegen in new versions of Rust doesn't break something here. This will become increasingly important as we add additional QA (tests) for things like side channels.

briansmith commented 1 month ago

Closing as a duplicate of the just-reopened #1512, since there's more recent discussion in that issue.