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

sparc-unknown-none-elf target broken #130172

Closed
thejpster opened this issue Sep 9, 2024 · 25 comments · Fixed by #131222
Closed

sparc-unknown-none-elf target broken #130172

thejpster opened this issue Sep 9, 2024 · 25 comments · Fixed by #131222
Labels
O-SPARC Target: SPARC processors P-low Low priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@thejpster
Copy link
Contributor

If I try and build https://github.com/ferrous-systems/sparc-experiments with today's nightly, I get:

/home/jonathan/Downloads/gaisler/sparc-bcc-2.3.0-llvm/bin/sparc-gaisler-elf-ld: unknown architecture of input file `/tmp/rustc4jBo9z/symbols.o' is incompatible with sparc output

Using -Csave-temps, I can see:

$ file /tmp/rustc4jBo9z/symbols.o
/tmp/rustc4jBo9z/symbols.o: ELF 32-bit MSB relocatable, SPARC32PLUS, total store ordering, version 1 (SYSV), not stripped
$ file ./target/sparc-unknown-none-elf/debug/deps/sparc_demo_rust-8c8aedb85be3cab2.0unncljkfm3eoay971moazawq.rcgu.o 
./target/sparc-unknown-none-elf/debug/deps/sparc_demo_rust-8c8aedb85be3cab2.0unncljkfm3eoay971moazawq.rcgu.o: ELF 32-bit MSB relocatable, SPARC, version 1 (SYSV), with debug_info, not stripped

They are not the same.

Doing a bisect, it falls over on 2024-08-08. I suspect it might be d1d21ed, which probably should have picked Architecture::Sparc instead.

diff --git a/compiler/rustc_codegen_ssa/src/back/metadata.rs b/compiler/rustc_codegen_ssa/src/back/metadata.rs
index 0fd9d7fffe8..ea9327dc0f1 100644
--- a/compiler/rustc_codegen_ssa/src/back/metadata.rs
+++ b/compiler/rustc_codegen_ssa/src/back/metadata.rs
@@ -208,7 +208,7 @@ pub(crate) fn create_object_file(sess: &Session) -> Option<write::Object<'static
         "powerpc64" => (Architecture::PowerPc64, None),
         "riscv32" => (Architecture::Riscv32, None),
         "riscv64" => (Architecture::Riscv64, None),
-        "sparc" => (Architecture::Sparc32Plus, None),
+        "sparc" => (Architecture::Sparc, None),
         "sparc64" => (Architecture::Sparc64, None),
         "avr" => (Architecture::Avr, None),
         "msp430" => (Architecture::Msp430, None),

I did a build and my repo now builds and runs.

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Sep 9, 2024
@workingjubilee
Copy link
Member

@glaubitz Can you explain why Sparc32Plus is the "proper" architecture?

@glaubitz
Copy link
Contributor

glaubitz commented Sep 9, 2024

@glaubitz Can you explain why Sparc32Plus is the "proper" architecture?

It's been the default baseline for 32-bit SPARC on Linux for a long time already as it was the only 32-bit baseline for a long time to fully support atomic operations until Leon came around.

If you need to support a 32-bit SPARC baseline with EM_SPARC, we should find a way to differentiate between 32-bit SPARC on Linux and the sparc-unknown-none-elf target.

@lolbinarycat lolbinarycat added regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. O-SPARC Target: SPARC processors labels Sep 9, 2024
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Sep 9, 2024
@lolbinarycat lolbinarycat removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Sep 9, 2024
@workingjubilee
Copy link
Member

workingjubilee commented Sep 9, 2024

I think we should probably conditionalize this on Linux, then (or on "none" as an OS, whatevr)

@bjorn3
Copy link
Member

bjorn3 commented Sep 9, 2024

Do the linux and none targets set a different target cpu? If so maybe switching based on the target cpu would work?

@workingjubilee
Copy link
Member

Switching based on the target cpu seems incorrect?

@bjorn3
Copy link
Member

bjorn3 commented Sep 9, 2024

If you have a target cpu which doesn't support the sparc32plus isa, then using regular sparc32 as elf machine is correct and if the target cpu does support it, LLVM will likely emit instructions that require it and thus sparc32plus is the correct elf machine. Or am I misunderstanding the difference between the two elf machine ids?

@workingjubilee
Copy link
Member

...wtf, they made their ISA extension level part of the ABI

@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

Also referencing the Zulip discussion.

@rustbot label -I-prioritize +P-low

@rustbot rustbot added P-low Low priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Sep 10, 2024
@glaubitz
Copy link
Contributor

It's unfortunate that the Leon designers introduced a new 32-bit SPARC baseline without changing the triplet name.

Classic 32-bit SPARC is definitely not suitable for modern software due to the lack of atomic operations which is why SPARC V8+ was created at some point. No idea though why a new ELF target machine was introduced for that.

@thejpster
Copy link
Contributor Author

Cross-posting from Zulip:

Why does that line only affect symbols.o and not any other object file created by Rust?

And do you have any ideas on how we fix this so it works for Rust on Leon 3 (which you can buy today, definitely has users, and is growing) and people using Rust on Solaris/Linux on 32-bit SPARC (which I think is fair to call a legacy platform because they stopped making those machines in the mid 90s)?

@glaubitz
Copy link
Contributor

glaubitz commented Sep 10, 2024

As someone who has invested a lot of time and effort in the past ten years to keep open source on SPARC afloat, I disagree with your snarky comment. 32-bit SPARC on Linux and Solaris is still a valid multilib target on 64-bit SPARC since not all applications have been ported to 64-bit SPARC and especially since using 32-bit pointers can dramatically improve performance where necessary.

And the proper last 64-bit SPARC CPU was released in 2017 (SPARC M8), not in the mid-90s.

@thejpster
Copy link
Contributor Author

thejpster commented Sep 10, 2024

Your point about 32 bit applications on 64 bit systems is fair. As the former user of an Ultra 5, the current owner on an Armv3 machine, and the founder of the #RustRetro channel on Matrix, I wasn't intending to be snarky. I apologise.

@bjorn3
Copy link
Member

bjorn3 commented Sep 10, 2024

Why does that line only affect symbols.o and not any other object file created by Rust?

Because symbols.o and lib.rmeta are the only object files created by rustc itself rather than the codegen backend. The LLVM backend creates the object files for all codegen units and already knows to pick the right elf machine.

@thejpster
Copy link
Contributor Author

So always picking sparc32plus here is wrong then because that's not what LLVM chooses for this target.

I guess I can do a quick test to try all the SPARC targets and see what llvm does but I'm assuming that this works for 32 bit Linux and so LLVM is picking sparc32plus there.

@thejpster
Copy link
Contributor Author

thejpster commented Sep 11, 2024

A little more digging tells me that sparc32plus is basically 64-bit SPARC V9 (for the UltraSPARC) but in 32-bit mode.

https://docs.oracle.com/cd/E19205-01/819-5267/bkazb/index.html

That's definitely wrong for sparc-unknown-elf and wrong for anyone who wants to use Rust on a SuperSPARC (a small group I suspect). Perhaps the 32-bit V8+ targets should be called sparc32plus-unknown-linux-gnu to avoid confusion? Or sparcv8plus-, perhaps.

Can this function use the full target name to pick the right EM type here?

I also spoke to Gaisler who confirmed their toolchain expects EM_SPARC.

@thejpster
Copy link
Contributor Author

This seems like it might do the trick, but I can't test on Apple Silicon macOS unless I rebuild rustc inside an Rosetta 2 emulated Docker container (*), and I'm not doing that today.

diff --git a/compiler/rustc_codegen_ssa/src/back/metadata.rs b/compiler/rustc_codegen_ssa/src/back/metadata.rs
index 6215616e510..1707094b0ea 100644
--- a/compiler/rustc_codegen_ssa/src/back/metadata.rs
+++ b/compiler/rustc_codegen_ssa/src/back/metadata.rs
@@ -208,7 +208,13 @@ pub(crate) fn create_object_file(sess: &Session) -> Option<write::Object<'static
         "powerpc64" => (Architecture::PowerPc64, None),
         "riscv32" => (Architecture::Riscv32, None),
         "riscv64" => (Architecture::Riscv64, None),
-        "sparc" => (Architecture::Sparc32Plus, None),
+        "sparc" => if sess.target.options.cpu == "v9" {
+            // Target uses V8+, aka EM_SPARC32PLUS, aka 64-bit V9 but in 32-bit mode
+            (Architecture::Sparc32Plus, None)
+        } else {
+            // Target uses V7 or V8, aka EM_SPARC
+            (Architecture::Sparc, None)
+        }
         "sparc64" => (Architecture::Sparc64, None),
         "avr" => (Architecture::Avr, None),
         "msp430" => (Architecture::Msp430, None),

* because most SPARC toolchains are only available on x86 Linux

@workingjubilee
Copy link
Member

Can this function use the full target name to pick the right EM type here?

sounds good to me. while some solutions are more principled here, I don't think there is any intrinsic reason not to use any of the information available to us to correct our target output, whether it's target-cpu or any part of the target string.

@bjorn3
Copy link
Member

bjorn3 commented Oct 3, 2024

The full target name can be effectively anything in the case of custom target spec json files. Only matching on individual fields in the target spec is guaranteed to work. If necessary we can add a new field to the target spec to determine if EM_SPARC32 or EM_SPARC32PLUS should be used, but if matching on the target cpu works fine, that would probably be better. That leaves the question though if -Ctarget-cpu=v9 on sparc-unknown-elf should use EM_SPARC32 or EM_SPARC32PLUS.

@thejpster
Copy link
Contributor Author

Well I guess can basically pick at random and the first person to try it can tell us we got it wrong. But I suspect no-one will try it because the existing sparc-unknown-none-elf is aimed at the SPARC V8 LEON3, and the use-case outlined above of the v9 targets is 32-bit code running under a 64-bit OS, not bare-metal.

@workingjubilee
Copy link
Member

workingjubilee commented Oct 3, 2024

The full target name can be effectively anything in the case of custom target spec json files.

deliberately combining "sparc-unknown-none-elf" with EM_SPARC32PLUS code while using a target-spec.json sounds like such an exquisitely specific choice that I kind of want to meet that person.

@workingjubilee
Copy link
Member

For note: EM_SPARC and EM_SPARC32PLUS files should be able to be linked together, and should produce an EM_SPARC32PLUS file.

@glaubitz Was there a practical concern about emitting Sparc objects with an incorrect ELF machine?

@bjorn3
Copy link
Member

bjorn3 commented Oct 4, 2024

EM_SPARC and EM_SPARC32PLUS files should be able to be linked together, and should produce an EM_SPARC32PLUS file.

If that is true, using EM_SPARC unconditionally for symbols.o should be fine, right? symbols.o doesn't contain any machine code, so it could have used EM_ANY if such a thing existed.

@glaubitz
Copy link
Contributor

glaubitz commented Oct 4, 2024 via email

@workingjubilee
Copy link
Member

workingjubilee commented Oct 4, 2024

@glaubitz Well it seems you broke taiki's SPARC testing, or a nearby commit did: taiki-e/setup-cross-toolchain-action@8953e6c

The V8+ Technical Specification states that EM_SPARC and EM_SPARC32PLUS should merge. If a linker can't support the published technical spec for a 30 year old architecture, and that's the only linker we have for that target, I don't see why we should bother with the target.

@thejpster
Copy link
Contributor Author

The reason why I did not consider anything else but V8+ is because Rust itself did not work on 32-bit SPARC before all of the recent fixes that I submitted.If you look at the commit history, you’ll see a lot of fixes by me to enable Rust on 32-bit SPARC in the first place. So, I just assumed that I was the only person to have tested it.

When I added sparc-unknown-none-elf back in September 2023 to coincide with an ESA software summit, everything was working great. If that was after your changes went in then I thank you for making it so easy for me.

Rust on RTEMS on SPARC is in the RTEMS user manual (https://docs.rtems.org/branches/master/user/rust/bare-metal.html#build-and-run-on-sparc), although they don't observe this breakage because they build a C binary with a Rust static library linked in.

workingjubilee added a commit to workingjubilee/rustc that referenced this issue Nov 4, 2024
…=workingjubilee

Generate correct symbols.o for sparc-unknown-none-elf

This fixes rust-lang#130172 by selecting the correct ELF Machine type for sparc-unknown-none-elf (which has a baseline of SPARC V7).
@bors bors closed this as completed in 0f72faa Nov 4, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Nov 4, 2024
Rollup merge of rust-lang#131222 - thejpster:fix-sparc-v7-symbol-o, r=workingjubilee

Generate correct symbols.o for sparc-unknown-none-elf

This fixes rust-lang#130172 by selecting the correct ELF Machine type for sparc-unknown-none-elf (which has a baseline of SPARC V7).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-SPARC Target: SPARC processors P-low Low priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants