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

Introduce SPARC64 support #2077

Merged
merged 1 commit into from
May 27, 2024
Merged

Conversation

Richard-Rogalski
Copy link
Contributor

Adds the minimum information to build on sparc64 / sparcv9. Tests pass 100%.

Finally a sore in my conscience gone. Much thanks to the devs from Gentoo and Debian who work very hard to keep these machines in the shape they're in.

For end users: remember to CFLAGS="-mcpu=ultrasparc" CXXFLAGS="-mcpu=ultrasparc" RUSTFLAGS="-C target-cpu=ultrasparc". If you want to build for a higher target, the C/CXX target can be found in man gcc, where for rust you can find it with rustc --print target-cpus.

Closes: #1512

Copy link
Owner

@briansmith briansmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am nervous about rust-lang/rust#115336 since we do use #[repr(transparent)] structures in ring. Especially in conjunction with the -mcpu= issues, where I guess could affect the ABI compatibility.

Do all the tests pass even without the -mcpu= bits?

@@ -60,6 +60,8 @@
#define OPENSSL_32_BIT
#elif defined(__s390x__)
#define OPENSSL_64_BIT
#elif defined(__sparc_v9__)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that __sparc_v9__ is supposed to imply 64-bit. But some versions of LLVM/clang have had bugs where they defined __sparc_v9__ even for 32-bit mode.
So I think we also need to check that __LP64__ is defined too, just in case the user is using an old compiler. See https://reviews.llvm.org/D98574.

mk/cargo.sh Outdated
sparc64-unknown-linux-gnu)
# Ultrasparc is the sparcv9 ISA, without this -mcpu compilers can
# default to lower ISA versions.
export CFLAGS_sparc64_unknown_linux_gnu="--sysroot=/usr/sparc64-linux-gnu -mcpu=ultrasparc"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well...I am a bit unsure about this -mcpu-ultrasparc. If this is something that everybody targeting sparc64 should do, then this should be done in cc-rs instead of here.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The -mcpu=ultrasparc is definitely not necessary. sparc64 defaults to SPARCv9 which is what we want here.

See: https://gcc.gnu.org/onlinedocs/gcc/SPARC-Options.html

Copy link

codecov bot commented May 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.24%. Comparing base (8238c00) to head (4063289).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2077   +/-   ##
=======================================
  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.

@Richard-Rogalski
Copy link
Contributor Author

You're right, sparcv9/ultrasparc is just an ABI and doesn't specify whether the target is 64 or 32 bit. Sparc LEONs are v9 32 bit only CPUs, and many distros ship sparcv9 bins as 32bit for legacy reasons. Huge oversight, apologies

The -mcpu=ultrasparc probably isn't needed on 64bit like glaubitz pointed out, iirc it was for v9 32bit installs.

Updating now

Closes: briansmith#1512
Signed-off-by: Richard Rogalski <[email protected]>
@Richard-Rogalski
Copy link
Contributor Author

Okay. It builds and still passes all tests, regardless of what {C, CXX, RUST}FLAGS are set. It is exclusively for 64 bit sparc.

@glaubitz : On gentoo, we don't package rust for our 32-bit userland profile. (Upstream doesn't provide binary toolchains for sparc at all, and it's a hassle enough as it is for 64bit). Does debian support(or have plans to continue support for) rust programs on 32bit sparc? If so, I can do another PR in the future for sparcv9 32-bit.

Sparcv8 and lower, even I am not insane enough for :b and neither are any linux distros

@Richard-Rogalski
Copy link
Contributor Author

Actually, I just saw the few duplicate issues and PRs, and this comment. To me, this seems like a much better approach-- the only thing I can see this whitelist doing is determining 32/64 bit, which there are definitely better ways to do than listing every possible architecture ever. Can I do a new PR implementing this instead?

@glaubitz
Copy link

Okay. It builds and still passes all tests, regardless of what {C, CXX, RUST}FLAGS are set. It is exclusively for 64 bit sparc.

@glaubitz : On gentoo, we don't package rust for our 32-bit userland profile. (Upstream doesn't provide binary toolchains for sparc at all, and it's a hassle enough as it is for 64bit).

Well, upstream doesn't provide binary toolchains for sparc64 either. However, Debian's SPARC port is fully 64-bit these days.

However, I usually add 32-bit SPARC support when I add 64-bit SPARC support since there is still a commercially supported 32-bit SPARC platform, namely Leon, which is used by ESA, NASA and other high-reliability industry products.

Does debian support(or have plans to continue support for) rust programs on 32bit sparc? If so, I can do another PR in the future for sparcv9 32-bit.

Sparcv8 and lower, even I am not insane enough for :b and neither are any linux distros

32-bit SPARC defaults to SPARCv9 these days, except for Leon which is SPARCv7 with support for atomic operations.

@glaubitz
Copy link

Actually, I just saw the few duplicate issues and PRs, and this comment. To me, this seems like a much better approach-- the only thing I can see this whitelist doing is determining 32/64 bit, which there are definitely better ways to do than listing every possible architecture ever. Can I do a new PR implementing this instead?

Well, how about we merge this commit first so that the package finally builds on sparc64. The improved version can still be implemented later, can't it?

@briansmith briansmith merged commit 0bbf293 into briansmith:main May 27, 2024
138 checks passed
@briansmith
Copy link
Owner

Actually, I just saw the few duplicate issues and PRs, and this comment. To me, this seems like a much better approach-- the only thing I can see this whitelist doing is determining 32/64 bit, which there are definitely better ways to do than listing every possible architecture ever. Can I do a new PR implementing this instead?

First, I just merged this, so that we get the cargo.sh and install-build-tools.sh changes. Those are the most difficult for me to produce myself.

For generalizing target.h away from the allowlist approach, please see PR #2082.

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.

Fails to build on SPARC64
3 participants