-
Notifications
You must be signed in to change notification settings - Fork 715
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
Conversation
There was a problem hiding this 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?
include/ring-core/target.h
Outdated
@@ -60,6 +60,8 @@ | |||
#define OPENSSL_32_BIT | |||
#elif defined(__s390x__) | |||
#define OPENSSL_64_BIT | |||
#elif defined(__sparc_v9__) |
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
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 Updating now |
Closes: briansmith#1512 Signed-off-by: Richard Rogalski <rrogalski@tutanota.com>
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 |
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, 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.
32-bit SPARC defaults to SPARCv9 these days, except for Leon which is SPARCv7 with support for atomic operations. |
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? |
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. |
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 inman gcc
, where for rust you can find it withrustc --print target-cpus
.Closes: #1512