-
-
Notifications
You must be signed in to change notification settings - Fork 14.5k
Fix passing/returning structs with the 64-bit SPARC ABI #142680
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
base: main
Are you sure you want to change the base?
Conversation
|
|
|
r? @bjorn3 |
|
Note that there are currently two places that Clang and GCC disagree AFAIK:
|
|
Thank you for doing this! My knowledge in this area is rather limited. I was able to build latest Rust sources with your fix on top on Solaris 11.4 SPARC. Then I used this version to build Firefox on Solaris. There was no problem. Firefox also worked as expected. I also tested some problematic cases which I was handling few years ago (https://github.com/psumbera/rust-sparc64-abi-tests). All seemed to be ok. |
|
I don't have time to properly review this right now. r? compiler |
|
☔ The latest upstream changes (presumably #143521) made this pull request unmergeable. Please resolve the merge conflicts. |
4eb3ce5 to
9e4a7bc
Compare
|
Rebased |
|
☔ The latest upstream changes (presumably #144249) made this pull request unmergeable. Please resolve the merge conflicts. |
9e4a7bc to
b7b3ed7
Compare
|
Hi @workingjubilee, are you still interested in reviewing this? If not, we can reroll for a different reviewer. Thanks! |
|
Looks like there is a Clang fix in llvm/llvm-project#155829 |
|
Sorry about that, currently in catchup mode so I am going to delegate, yes. r? @tgross35 |
b7b3ed7 to
7cba46f
Compare
This comment has been minimized.
This comment has been minimized.
7cba46f to
b2e4ee9
Compare
This comment has been minimized.
This comment has been minimized.
b2e4ee9 to
7df3c25
Compare
This comment has been minimized.
This comment has been minimized.
| let cast_target = match regs { | ||
| [Some(reg), None, ..] => CastTarget::from(reg), | ||
| _ => CastTarget::prefixed(regs, Uniform::new(Reg::i8(), Size::ZERO)), | ||
| }; |
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'm not entirely sure why the special-casing is, but the first case fits in a register no problem, while the second case requires some sort of larger struct that is cast.
This comment has been minimized.
This comment has been minimized.
7df3c25 to
7515aca
Compare
|
@rustbot ready |
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.
Two nits but LGTM, thanks for picking this up. Two comment requests below.
I believe you should technically make yourself the commit author since you're the one submitting it in its final form. Then add beetrees back via a Co-authored-by: trailer.
|
|
||
| arg.cast_to(Uniform::new(Reg::i64(), total)); | ||
| let cast_target = match regs { | ||
| [Some(reg), None, ..] => CastTarget::from(reg), |
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.
Optional nit, sanity check
[Some(reg), None, rest @ ..] => {
debug_assert!(rest.iter().all(|x| x.is_none());
CastTarget::from(reg)
}| // sparc64-unknown-linux-{gnu,musl,uclibc} doesn't ignore ZSTs. | ||
| let passes_zsts = match cx.target_spec().os { | ||
| Os::Linux => matches!(cx.target_spec().env, Env::Gnu | Env::Musl | Env::Uclibc), | ||
| _ => false, | ||
| }; |
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.
What makes this env-dependent? The only supported Linux env seems to be gnu at this time.
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.
We have
sparc64-unknown-linux-gnu
sparc64-unknown-netbsd
sparc64-unknown-openbsd
sparcv9-sun-solaris
so this only matches in if the os is linux, and it's a bit more general so that if a new target (e.g. musl) is added, it does the right thing from the start. There could, I suppose, be a linux env that does ignore ZSTs.
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.
Why would it ever be coupled to the env though? I see that this exists across other arches but it seems a bit bogus.
Doing some archaeology, the pattern kind of follows:
- 65dabd6 which references the SPARC ABI
- 19b8408
- e177207
- More targets, grouping
- Move ZST ABI handling to
rustc_target#125854
The env had to be specified on Windows and it seems like others followed suit. I think this can just be deleted here, probably the same in the other places, under the assumption that we're not likely to get a nightmare env that breaks from the psABI (at least not on Linux).
|
@rustbot author |
7515aca to
9129b31
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
9129b31 to
4373d2f
Compare
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.
Awesome, lgtm
This comment has been minimized.
This comment has been minimized.
4373d2f to
8ebba60
Compare
This comment has been minimized.
This comment has been minimized.
Co-authored-by: beetrees <b@beetr.ee>
8ebba60 to
c9b5c93
Compare
|
@bors r=tgross35 |
… r=tgross35 Fix passing/returning structs with the 64-bit SPARC ABI Fixes the 64-bit SPARC part of rust-lang#115609 by replacing the current implementation with a new implementation modelled on the RISC-V calling convention code ([SPARC ABI reference](https://sparc.org/wp-content/uploads/2014/01/SCD.2.4.1.pdf.gz)). Pinging `sparcv9-sun-solaris` target maintainers: @psumbera @kulikjak Fixes rust-lang#115336 Fixes rust-lang#115399 Fixes rust-lang#122620 Fixes rust-lang#147883 r? @workingjubilee
… r=tgross35 Fix passing/returning structs with the 64-bit SPARC ABI Fixes the 64-bit SPARC part of rust-lang#115609 by replacing the current implementation with a new implementation modelled on the RISC-V calling convention code ([SPARC ABI reference](https://sparc.org/wp-content/uploads/2014/01/SCD.2.4.1.pdf.gz)). Pinging `sparcv9-sun-solaris` target maintainers: @psumbera @kulikjak Fixes rust-lang#115336 Fixes rust-lang#115399 Fixes rust-lang#122620 Fixes rust-lang#147883 r? @workingjubilee
Fixes the 64-bit SPARC part of #115609 by replacing the current implementation with a new implementation modelled on the RISC-V calling convention code (SPARC ABI reference).
Pinging
sparcv9-sun-solaristarget maintainers: @psumbera @kulikjakFixes #115336
Fixes #115399
Fixes #122620
Fixes #147883
r? @workingjubilee