Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fails to build on Linux SPARC #2008

Closed
glaubitz opened this issue Mar 30, 2024 · 10 comments
Closed

Fails to build on Linux SPARC #2008

glaubitz opened this issue Mar 30, 2024 · 10 comments

Comments

@glaubitz
Copy link

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
Copy link

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
Copy link
Owner

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
Copy link
Author

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
Copy link

m-ueberall commented May 20, 2024

[…] 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
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

briansmith commented May 20, 2024

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.

@glaubitz
Copy link
Author

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
Copy link
Owner

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
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants