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

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

Merged
merged 1 commit into from
Nov 4, 2024

Conversation

thejpster
Copy link
Contributor

This fixes #130172 by selecting the correct ELF Machine type for sparc-unknown-none-elf (which has a baseline of SPARC V7).

@rustbot
Copy link
Collaborator

rustbot commented Oct 3, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nnethercote (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 3, 2024
@thejpster
Copy link
Contributor Author

Before:

$ cargo +nightly run --release -Zbuild-std=core
   Compiling compiler_builtins v0.1.123
   Compiling core v0.0.0 (/home/jonathan/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core)
   Compiling rustc-std-workspace-core v1.99.0 (/home/jonathan/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/rustc-std-workspace-core)
   Compiling sparc-demo-rust v0.1.0 (/home/jonathan/Documents/github/sparc-experiments/sparc-demo-rust)
error: linking with `sparc-gaisler-elf-clang` failed: exit status: 1
  |
  = note: LC_ALL="C" PATH="/home/jonathan/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/bin:/home/jonathan/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/bin:/home/jonathan/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/bin:/home/jonathan/Documents/github/sparc-experiments/sparc-demo-rust/tsim-eval/tsim/linux-x64:/home/jonathan/Documents/github/sparc-experiments/sparc-demo-rust/sparc-bcc-2.3.0-llvm/bin:/home/jonathan/.cargo/bin:/usr/local/bin:/usr/bin:/bin:/usr/games:/home/jonathan/.local/bin" VSLANG="1033" "sparc-gaisler-elf-clang" "/tmp/rustc5XTXaN/symbols.o" "/home/jonathan/Documents/github/sparc-experiments/sparc-demo-rust/target/sparc-unknown-none-elf/release/deps/sparc_demo_rust-dda8726ad5b0a340.sparc_demo_rust.d5175a5ce72ae1e3-cgu.0.rcgu.o" "-Wl,--as-needed" "-Wl,-Bstatic" "/home/jonathan/Documents/github/sparc-experiments/sparc-demo-rust/target/sparc-unknown-none-elf/release/deps/libcompiler_builtins-543ae673c75fcb78.rlib" "-Wl,-Bdynamic" "-Wl,-z,noexecstack" "-o" "/home/jonathan/Documents/github/sparc-experiments/sparc-demo-rust/target/sparc-unknown-none-elf/release/deps/sparc_demo_rust-dda8726ad5b0a340" "-Wl,--gc-sections" "-no-pie" "-mcpu=leon3" "-latomic"
  = note: sparc-gaisler-elf-clang: warning: argument unused during compilation: '-nopie' [-Wunused-command-line-argument]
          /home/jonathan/Documents/github/sparc-experiments/sparc-demo-rust/sparc-bcc-2.3.0-llvm/bin/sparc-gaisler-elf-ld: unknown architecture of input file `/tmp/rustc5XTXaN/symbols.o' is incompatible with sparc output
          sparc-gaisler-elf-clang: error: ld command failed with exit code 1 (use -v to see invocation)


error: could not compile `sparc-demo-rust` (bin "sparc-demo-rust") due to 1 previous error

After:

$ cargo +stage1 run --release
   Compiling sparc-demo-rust v0.1.0 (/home/jonathan/Documents/github/sparc-experiments/sparc-demo-rust)
    Finished `release` profile [optimized + debuginfo] target(s) in 0.41s
     Running `tsim-leon3 -c sim-commands.txt target/sparc-unknown-none-elf/release/sparc-demo-rust`

 TSIM3 LEON3 SPARC simulator, version 3.1.11 (evaluation version)

 Copyright (C) 2024, Frontgrade Gaisler - all rights reserved.
 This software may only be used with a valid license.
 For latest updates, go to https://www.gaisler.com/
 Comments or bug-reports to [email protected]

 This TSIM evaluation version will expire 2024-11-03

Number of CPUs: 2
system frequency: 50.000 MHz
icache: 1 * 4 KiB, 16 bytes/line (4 KiB total)
dcache: 1 * 4 KiB, 16 bytes/line (4 KiB total)
Allocated 8192 KiB SRAM memory, in 1 bank at 0x40000000
Allocated 32 MiB SDRAM memory, in 1 bank at 0x60000000
Allocated 8192 KiB ROM memory at 0x00000000
section: .text, addr: 0x40000000, size: 34304 bytes
section: .rodata, addr: 0x40008600, size: 3760 bytes
section: .data, addr: 0x400094b0, size: 1176 bytes
read 431 symbols


  Initializing and starting from 0x40000000
Hello, this is Rust!
     0  1  2  3  4  5  6  7  8  9
 0:  0  0  0  0  0  0  0  0  0  0
 1:  0  1  2  3  4  5  6  7  8  9
 2:  0  2  4  6  8 10 12 14 16 18
 3:  0  3  6  9 12 15 18 21 24 27
 4:  0  4  8 12 16 20 24 28 32 36
 5:  0  5 10 15 20 25 30 35 40 45
 6:  0  6 12 18 24 30 36 42 48 54
 7:  0  7 14 21 28 35 42 49 56 63
 8:  0  8 16 24 32 40 48 56 64 72
 9:  0  9 18 27 36 45 54 63 72 81
PANIC: PanicInfo { message: I am a panic, location: Location { file: "src/main.rs", line: 56, col: 5 }, can_unwind: true, force_no_backtrace: fals

  Program exited normally on CPU 0

@nnethercote
Copy link
Contributor

This seems fine to me but the discussion in #130172 is complex enough that I will defer to...

r? @workingjubilee

@rustbot rustbot assigned workingjubilee and unassigned nnethercote Oct 3, 2024
@workingjubilee
Copy link
Member

workingjubilee commented Oct 4, 2024

I mulled this over for a bit, basically wrote a blog post worth of my thoughts on how silly this case was, then did some research and found something interesting: clang recognizes a lot more than just "v8" and "v9", but fortunately they encode v8 and v9ness.

https://github.com/llvm/llvm-project/blob/876f661dbeb42a76767edfb1f36214baacb27fd4/clang/lib/Basic/Targets/Sparc.cpp#L67-L104

A digression later found to be incorrect

And apparently LLVM actually encodes V8Plus-ness as a feature which they use when doing machine code generation:

https://github.com/llvm/llvm-project/blob/876f661dbeb42a76767edfb1f36214baacb27fd4/llvm/lib/Target/Sparc/MCTargetDesc/SparcTargetStreamer.cpp#L27-L28

And passing this feature is handled in the Clang driver?

https://github.com/llvm/llvm-project/blob/876f661dbeb42a76767edfb1f36214baacb27fd4/clang/lib/Driver/ToolChains/Arch/Sparc.cpp#L39-L186

We can decide we don't want to recognize all these different CPUs, or we can faithfully replicate what clang does, but we should make a decision here.

@workingjubilee
Copy link
Member

workingjubilee commented Oct 4, 2024

Ah apparently that codegen attribute is actually fairly recent: llvm/llvm-project#98713

So not in LLVM 19... nevermind.

Okay, doing some fiddling and more reading, I was missing a detail: LLVM has a fairly... idiosyncratic... way of doing option parsing. So they do recognize all the V8+ CPUs independently, it's not entirely up to Clang. Whew!

@workingjubilee
Copy link
Member

workingjubilee commented Oct 4, 2024

Ah, it's from this, which turns into some generated files of C++ and such: https://github.com/llvm/llvm-project/blob/eaff3a743406ff1636e6328e1ba1bc66318d53cb/llvm/lib/Target/Sparc/Sparc.td#L115-L200

The slightly more complicated pattern-match required for handling the Niagara and UltraSPARC processors doesn't seem too bad.

@thejpster
Copy link
Contributor Author

But if this makes Leon 3 bare metal work and 32-bit on 64-bit SPARC Linux work, that's ok for now right?

@workingjubilee
Copy link
Member

Yes, I'm just saying that it should recognize the other "v9" CPUs as well, which are these:
https://github.com/llvm/llvm-project/blob/eaff3a743406ff1636e6328e1ba1bc66318d53cb/llvm/lib/Target/Sparc/Sparc.td#L148-L160

So, v9, ultrasparc, ultrasparc3, niagara, niagara2, niagara3, niagara4.

@thejpster
Copy link
Contributor Author

thejpster commented Oct 4, 2024

I was unable to find a 32-bit SPARC gcc for Linux (Debian bookworm) so sadly I can't test.

@thejpster
Copy link
Contributor Author

The built in SPARC Linux target says v9 so is this only an issue for anyone who overrides the target-cpu?

@taiki-e
Copy link
Member

taiki-e commented Oct 4, 2024

I was unable to find a 32-bit SPARC gcc for Linux (Debian bookworm) so sadly I can't test.

FWIW, I used to be able to test using g++-multilib-sparc64 on Ubuntu/Debian and -m32 flag (taiki-e/setup-cross-toolchain-action@ea9aca6), but since the nightly a while ago, the build has been broken with the "file in wrong format" error (taiki-e/setup-cross-toolchain-action@8953e6c, probably related to d1d21ed, considering when the error occurred).

@workingjubilee
Copy link
Member

The built in SPARC Linux target says v9 so is this only an issue for anyone who overrides the target-cpu?

Correct.

@thejpster
Copy link
Contributor Author

@glaubitz does this change work for you?

@glaubitz
Copy link
Contributor

glaubitz commented Oct 4, 2024

I was unable to find a 32-bit SPARC gcc for Linux (Debian bookworm) so sadly I can't test.

FWIW, I used to be able to test using g++-multilib-sparc64 on Ubuntu/Debian and -m32 flag (taiki-e/setup-cross-toolchain-action@ea9aca6), but since the nightly a while ago, the build has been broken with the "file in wrong format" error (taiki-e/setup-cross-toolchain-action@8953e6c, probably related to d1d21ed, considering when the error occurred).

Then your toolchain defaults to SPARC V7 which is not what Debian's GCC does.

@glaubitz does this change work for you?

I need to test this first. Please give me the time over the weekend as I'm currently busy with other stuff.

@workingjubilee
Copy link
Member

workingjubilee commented Oct 4, 2024

Auditing Debian's toolchain, it seems Debian carries multiple patches for SPARC that they apparently have yet to submit to GCC. Can you link to the specific patchset your toolchain was built with, so we can be sure this isn't all a problem introduced by Debian's patches in the first place?

@thejpster
Copy link
Contributor Author

Did you have a chance to look at this one, @glaubitz ?

@glaubitz
Copy link
Contributor

Did you have a chance to look at this one, @glaubitz ?

Not yet. I'll try to get it done over the weekend.

@glaubitz
Copy link
Contributor

Working on this now. Sorry, I was on a business trip this week and didn't have any time to look into it.

@workingjubilee workingjubilee added the O-SPARC Target: SPARC processors label Oct 21, 2024
@glaubitz
Copy link
Contributor

So, I tried building the current git master for 32-bit SPARC and it didn't build due to LLVM requiring -latomic.

Does it build for you for Leon with this patch applied?

@workingjubilee
Copy link
Member

So, I tried building the current git master for 32-bit SPARC

which target?

@thejpster
Copy link
Contributor Author

I haven't tried this patch on master, only the exact contents of this PR as per the opening message. I used Gaisler's toolchain and my pre-existing example repository.

I'm hoping you can tell me if I broke SPARC Linux or not because I can't find a SPARC Linux toolchain for my Debian Stable box to do the testing.

@glaubitz
Copy link
Contributor

So, I tried building the current git master for 32-bit SPARC

which target?

That was sparc-unknown-linux-gnu.

@glaubitz
Copy link
Contributor

I haven't tried this patch on master, only the exact contents of this PR as per the opening message. I used Gaisler's toolchain and my pre-existing example repository.

I'm hoping you can tell me if I broke SPARC Linux or not because I can't find a SPARC Linux toolchain for my Debian Stable box to do the testing.

I built the toolchain myself from source but I'm working on enabling the 32-bit SPARC toolchain by default now so it can just be installed from source. This also makes cross-building 32-bit kernels on Debian easier.

@thejpster
Copy link
Contributor Author

I thought sparc-unknown-linux-gnu was only used for making 32-bit applications that run on 64-bit UltraSPARC systems? Not that I suppose it matters here. I just need to know if the symbols.o file still has the right ELF Machine type.

@glaubitz
Copy link
Contributor

I thought sparc-unknown-linux-gnu was only used for making 32-bit applications that run on 64-bit UltraSPARC systems? Not that I suppose it matters here. I just need to know if the symbols.o file still has the right ELF Machine type.

The kernel is usually UltraSPARC, but the userland can be both 32- and 64-bit. Most Linux distributions preferred a 32-bit userland in the past. On Gentoo, you can still chose between the two.

@thejpster
Copy link
Contributor Author

TIL!

@workingjubilee
Copy link
Member

workingjubilee commented Oct 26, 2024

which target?

That was sparc-unknown-linux-gnu.

what linker? specific commit, please.

@thejpster
Copy link
Contributor Author

Can we just merge this and look at resolving any SPARC Linux issues in a subsequent commit?

@taiki-e
Copy link
Member

taiki-e commented Nov 3, 2024

Yes, I'm just saying that it should recognize the other "v9" CPUs as well, which are these: https://github.com/llvm/llvm-project/blob/eaff3a743406ff1636e6328e1ba1bc66318d53cb/llvm/lib/Target/Sparc/Sparc.td#L148-L160

So, v9, ultrasparc, ultrasparc3, niagara, niagara2, niagara3, niagara4.

These all enable the v9 target feature, so I think it would be better to refer to the v9 target feature (like #132472 (comment)) rather than parsing the cpu string.
I opened #132552 as that feature is not currently recognized by rustc.

@workingjubilee
Copy link
Member

workingjubilee commented Nov 4, 2024

Thank you, @taiki-e

As that feature will take care of the problem that "v9" would be an incomplete list of CPUs that should be treated as V8+ (and hinging on -Ctarget-cpu is probably not ideal) I will happily approve this for now. I have opened an issue to track any followups: #132585

@bors r+

@bors
Copy link
Contributor

bors commented Nov 4, 2024

📌 Commit 0f72faa has been approved by workingjubilee

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 4, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 4, 2024
…kingjubilee

Rollup of 4 pull requests

Successful merges:

 - rust-lang#131222 (Generate correct symbols.o for sparc-unknown-none-elf)
 - rust-lang#132423 (remove const-support for align_offset and is_aligned)
 - rust-lang#132565 (Reduce dependence on the target name)
 - rust-lang#132576 (remove attribute ids from hir stats (they're simply not needed))

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f35433e into rust-lang:master Nov 4, 2024
6 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Nov 4, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request 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).
@bors
Copy link
Contributor

bors commented Nov 4, 2024

⌛ Testing commit 0f72faa with merge 56c6a2f...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-SPARC Target: SPARC processors S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sparc-unknown-none-elf target broken
7 participants