briansmith / ring

Safe, fast, small crypto using Rust
Other
3.64k stars 683 forks source link

Add support for arch csky #2042

Closed Dirreke closed 1 month ago

Dirreke commented 2 months ago

Add support for arch csky. Ref: https://github.com/rust-lang/rust/blob/master/src/doc/rustc/src/platform-support/csky-unknown-linux-gnuabiv2.md

briansmith commented 1 month ago

@Dirreke Please:

  1. Make a mk/install-build-tools-unsupported.sh that is in the same form of mk/install-build-tools.sh, which adds support for installing the csky toolchain on an Ubuntu system, starting from the state where qemu-csky2 is not installed.
  2. Make a mk/cargo-unsupported.sh that mirrors mk/cargo.sh which uses the csky toolchain to run cargo commands in qemu-sky2.
  3. Update .github/workflows/ci.yml to add the csky target to the test matrix.
Dirreke commented 1 month ago

Actually, I'm not sure if we should add this target to CI, because it's a Tier 3 target of rust and there's no prebuilt toolchain. We have to build rust/cargo from source (also including llvm/clang, because there's no prebuild llvm with csky target too), which will take lots of time. In addition, there are really few people using this target. Therefore, I think we can just keep it without CI and I will maintain it for the csky target if it's broken.

If you still insist that we need CI, please tell me and I will try to finish it.

briansmith commented 1 month ago

Actually, I'm not sure if we should add this target to CI, because it's a Tier 3 target of rust and there's no prebuilt toolchain. We have to build rust/cargo from source (also including llvm/clang, because there's no prebuild llvm with csky target too), which will take lots of time.

First, I don't think you'd need to build cargo from source because we'd be cross-compiling from host x86_64-unknown-linux-gnu. But, if it isn't even in LLVM yet then I agree that seems like quite a burden.

Here's an alternative: Change build.rs so that it contains some logic that adds a define for OPENSSL_32BIT if target_pointer_width = 32 or OPENSSL_64BIT if target_pointer_width = 64, replace this section that exablishes the allowlist for targets that we support that BoringSSL doesn't:

- #elif defined(__MIPSEL__) && !defined(__LP64__)
- #define OPENSSL_32_BIT
- #elif defined(__MIPSEL__) && defined(__LP64__)
- #define OPENSSL_64_BIT
- #elif defined(__MIPSEB__) && !defined(__LP64__)
- #define OPENSSL_32_BIT
- #elif defined(__PPC64__) || defined(__powerpc64__)
- #define OPENSSL_64_BIT
- #elif (defined(__PPC__) || defined(__powerpc__)) && defined(_BIG_ENDIAN)
- #define OPENSSL_32_BIT
- #elif defined(__s390x__)
- #define OPENSSL_64_BIT
+ #elif defined(OPENSSL_64_BIT) || defined(OPENSSL_32_BIT)
  #else
  #error "Unknown target CPU"
  #endif

And also add this to the bottom of the block of assertions in base.h:

#if defined(OPENSSL_64_BIT)
OPENSSL_STATIC_ASSERT(sizeof(size_t) == 8, "size_t isn't 64 bits on a 64-bit target.");
#endif
#if defined(OPENSSL_32_BIT)
OPENSSL_STATIC_ASSERT(sizeof(size_t) == 4, "size_t isn't 32 bits on 32-bit target.");
#endif
Dirreke commented 1 month ago

Or how about using __WORDSIZE?

briansmith commented 1 month ago

Or how about using __WORDSIZE?

Based on https://bugs.chromium.org/p/nativeclient/issues/detail?id=1214, https://github.com/llvm/llvm-project/issues/48906, https://sourceware.org/bugzilla/show_bug.cgi?id=27574, and other things, I think __WORDSIZE isn't reliable.

Also, it seems to be part of the sysroot, so it isn't always defined. For example clang -dM -E -x c /dev/null doesn't output a definition for it.

I think there are not many 32-bit targets left to add. Since I merged #2082, I suggest you just add csky to the allowlist for 32-bit targets at the end. You don't need to do any other extra work. But it would be useful to see the output of clang --target=<your-target> -dM -E -x c /dev/null and the equivalent for GCC, in the commit messae.

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 97.24%. Comparing base (ed5b2a8) to head (68c3309).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #2042 +/- ## ======================================= Coverage 97.24% 97.24% ======================================= Files 144 144 Lines 19995 19995 Branches 228 228 ======================================= Hits 19444 19444 Misses 525 525 Partials 26 26 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.