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

Add support for arch csky #2042

Merged
merged 1 commit into from
May 31, 2024
Merged

Add support for arch csky #2042

merged 1 commit into from
May 31, 2024

Conversation

Dirreke
Copy link
Contributor

@Dirreke Dirreke commented May 2, 2024

@briansmith
Copy link
Owner

briansmith commented May 18, 2024

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

Dirreke commented May 20, 2024

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

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 Dirreke force-pushed the support-csky branch 3 times, most recently from f8bfa98 to 4aef2bf Compare May 25, 2024 07:28
@Dirreke Dirreke changed the title Add support for arch csky Change the logic of OPENSSL_XX_BIT definition and support more arch May 25, 2024
@Dirreke
Copy link
Contributor Author

Dirreke commented May 29, 2024

Or how about using __WORDSIZE?

@briansmith
Copy link
Owner

Or how about using __WORDSIZE?

Based on https://bugs.chromium.org/p/nativeclient/issues/detail?id=1214, llvm/llvm-project#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.

@Dirreke Dirreke changed the title Change the logic of OPENSSL_XX_BIT definition and support more arch Add support for arch csky May 30, 2024
Copy link

codecov bot commented May 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@           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           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@briansmith briansmith merged commit d3f508b into briansmith:main May 31, 2024
138 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants