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.

Sign up for GitHub

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

Jump to bottom

Refactor powerpc64 call ABI handling #128643

Merged
merged 1 commit into from
Aug 13, 2024
Merged

Refactor powerpc64 call ABI handling #128643

merged 1 commit into from
Aug 13, 2024

Conversation

beetrees
Copy link
Contributor

@beetrees beetrees commented Aug 4, 2024

As the specification for the ELFv2 ABI states that returned aggregates are returned like arguments as long as they are at most two doublewords, I've merged the classify_arg and classify_ret functions to reduce code duplication. The only functional change is to fix #128579: the classify_ret function was incorrectly handling aggregates where bits > 64 && bits < 128. I've used the aggregate handling implementation from classify_arg which doesn't have this issue.

@awilfox could you test this on powerpc64-unknown-linux-musl? I'm only able to cross-test on powerpc64-unknown-linux-gnu and powerpc64le-unknown-linux-gnu locally at the moment, and as a tier 3 target powerpc64-unknown-linux-musl has zero CI coverage.

Fixes: #128579

@rustbot
Copy link
Collaborator

rustbot commented Aug 4, 2024

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 Aug 4, 2024
@jieyouxu
Copy link
Member

jieyouxu commented Aug 4, 2024

r? @bjorn3 (feel free to reroll, or, I can open a T-compiler thread asking for a suitable reviewer)

@rustbot rustbot assigned bjorn3 and unassigned jieyouxu Aug 4, 2024
@jieyouxu jieyouxu added the O-PowerPC Target: PowerPC processors label Aug 4, 2024
@jieyouxu
Copy link
Member

jieyouxu commented Aug 4, 2024 •
edited
Loading

cc @ecnelises and @bzEq since you were listed as powerpc64-ibm-aix target maintainers, and this affects your target AFAICT.

@jieyouxu jieyouxu added the A-abi Area: Concerning the application binary interface (ABI). label Aug 4, 2024
@tgross35
Copy link
Contributor

tgross35 commented Aug 4, 2024

Is a codegen regression test possible here?

@workingjubilee
Copy link
Member

An assembly test would be nice.

@awilfox
Copy link
Contributor

awilfox commented Aug 5, 2024

Thank you! This looks right to me. I'll do a test run on my PPC64 musl workstation soon (it is currently doing some LLVM work) and will let you know the result.

@bzEq
Copy link
Contributor

bzEq commented Aug 9, 2024 •
edited
Loading

cc @ecnelises and @bzEq since you were listed as powerpc64-ibm-aix target maintainers, and this affects your target AFAICT.

Thanks for heads up. However, AIX has its own ABI and is not using ELFv1/v2. Anyway, I'll test this PR on AIX.

@bzEq
Copy link
Contributor

bzEq commented Aug 9, 2024

Further, I don't think AIX ABI is a blocker for this PR. If it breaks AIX in our buildbot, we will upstream AIX change.

@beetrees
Copy link
Contributor Author

beetrees commented Aug 9, 2024

I've added an assembly test for the PowerPC64 struct ABI.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@awilfox
Copy link
Contributor

awilfox commented Aug 9, 2024

I've just queued the latest version of this patch (dd2fc07) on my powerpc64-musl build system, and will let you know the result in about two hours when it finishes.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@awilfox
Copy link
Contributor

awilfox commented Aug 10, 2024

All tests pass here on powerpc64-musl (BE)! Including the new tests that the CI bot seems to believe are failing:

test [assembly] tests/assembly/powerpc64-struct-abi.rs#elfv1-be ... ok
test [assembly] tests/assembly/powerpc64-struct-abi.rs#elfv2-be ... ok
test [assembly] tests/assembly/powerpc64-struct-abi.rs#elfv2-le ... ok

This system is using LLVM 18.1.8, so perhaps it is an LLVM 17 issue.

@beetrees beetrees force-pushed the ppc64-abi-fix branch 2 times, most recently from 1c55e73 to 6308d13 Compare August 10, 2024 02:02
@beetrees
Copy link
Contributor Author

I've fixed the test so that it works on LLVM 17.

@bjorn3
Copy link
Member

bjorn3 commented Aug 10, 2024

By the way did you try abi-cafe with these changes? It is pretty good at finding abi divergences between compilers.

@beetrees
Copy link
Contributor Author

beetrees commented Aug 10, 2024 •
edited
Loading

Haven't tried it yet; I'll give it a go on powerpc64-unknown-linux-gnu and powerpc64le-unknown-linux-gnu. @awilfox think you could give abi-cafe a run on powerpc64-unknown-linux-musl? It'll help check if there are any remaining ABI incompatibilities between rustc and gcc/clang.

@bjorn3
Copy link
Member

bjorn3 commented Aug 10, 2024

r=me if abi-cafe passes.

@beetrees
Copy link
Contributor Author

I've ran abi-cafe on powerpc64-unknown-linux-gnu and powerpc64le-unknown-linux-gnu. The only failures were pre-existing failures of some u128 and i128 tests. The root cause of this appears to be that Rust thinks that u128/i128 have an alignment of 8 whereas the ABI specification and gcc/clang think u128/i128 have an alignment of 16. This is essentially another instance of #54341 but on a different platform (cc rust-lang/lang-team#255). Like in that issue, the proper solution here is to fix the LLVM data layout.

@beetrees beetrees mentioned this pull request Aug 11, 2024
Open
@awilfox
Copy link
Contributor

awilfox commented Aug 11, 2024

I see the same on powerpc64-musl here; i128/u128 failures but otherwise abi-cafe passes with this patch applied, both with 1.80.0 and nightly.

@bjorn3
Copy link
Member

bjorn3 commented Aug 11, 2024

Thanls for testing. The u128/i128 issue can be fixed later.

@bors r+

@bors
Copy link
Contributor

bors commented Aug 11, 2024

📌 Commit 6308d13 has been approved by bjorn3

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 Aug 11, 2024
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Aug 11, 2024
@jieyouxu
Rollup merge of rust-lang#128643 - beetrees:ppc64-abi-fix, r=bjorn3
b39e7d5 
Refactor `powerpc64` call ABI handling

As the [specification](https://openpowerfoundation.org/specifications/64bitelfabi/) for the ELFv2 ABI states that returned aggregates are returned like arguments as long as they are at most two doublewords, I've merged the `classify_arg` and `classify_ret` functions to reduce code duplication. The only functional change is to fix rust-lang#128579: the `classify_ret` function was incorrectly handling aggregates where `bits > 64 && bits < 128`. I've used the aggregate handling implementation from `classify_arg` which doesn't have this issue.

`@awilfox` could you test this on `powerpc64-unknown-linux-musl`? I'm only able to cross-test on `powerpc64-unknown-linux-gnu` and `powerpc64le-unknown-linux-gnu` locally at the moment, and as a tier 3 target `powerpc64-unknown-linux-musl` has zero CI coverage.

Fixes: rust-lang#128579
@jieyouxu jieyouxu mentioned this pull request Aug 11, 2024
Closed
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Aug 11, 2024
@jieyouxu
Rollup merge of rust-lang#128643 - beetrees:ppc64-abi-fix, r=bjorn3
806f8ce 
Refactor `powerpc64` call ABI handling

As the [specification](https://openpowerfoundation.org/specifications/64bitelfabi/) for the ELFv2 ABI states that returned aggregates are returned like arguments as long as they are at most two doublewords, I've merged the `classify_arg` and `classify_ret` functions to reduce code duplication. The only functional change is to fix rust-lang#128579: the `classify_ret` function was incorrectly handling aggregates where `bits > 64 && bits < 128`. I've used the aggregate handling implementation from `classify_arg` which doesn't have this issue.

``@awilfox`` could you test this on `powerpc64-unknown-linux-musl`? I'm only able to cross-test on `powerpc64-unknown-linux-gnu` and `powerpc64le-unknown-linux-gnu` locally at the moment, and as a tier 3 target `powerpc64-unknown-linux-musl` has zero CI coverage.

Fixes: rust-lang#128579
@jieyouxu jieyouxu mentioned this pull request Aug 11, 2024
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 11, 2024
@bors
Auto merge of rust-lang#128968 - jieyouxu:rollup-l23j21x, r=jieyouxu
8707132 
Rollup of 5 pull requests

Successful merges:

 - rust-lang#128643 (Refactor `powerpc64` call ABI handling)
 - rust-lang#128873 (Add windows-targets crate to std's sysroot)
 - rust-lang#128916 (Tidy up `dump-ice-to-disk` and make assertion failures dump ICE messages)
 - rust-lang#128929 (Fix codegen-units tests that were disabled 8 years ago)
 - rust-lang#128937 (Fix warnings in rmake tests on `x86_64-unknown-linux-gnu`)

r? `@ghost`
`@rustbot` modify labels: rollup
@jieyouxu
Copy link
Member

jieyouxu commented Aug 11, 2024 •
edited
Loading

Failed in #128968 (comment).
@bors r-

test [assembly] tests/assembly/powerpc64-struct-abi.rs#elfv2-le ... FAILED
test [assembly] tests/assembly/powerpc64-struct-abi.rs#elfv2-be ... FAILED
test [assembly] tests/assembly/powerpc64-struct-abi.rs#elfv1-be ... FAILED

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 11, 2024
@beetrees
Copy link
Contributor Author

I've fixed the test.

@bjorn3
Copy link
Member

bjorn3 commented Aug 11, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Aug 11, 2024

📌 Commit 715728f has been approved by bjorn3

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 11, 2024
@matthiaskrgr matthiaskrgr mentioned this pull request Aug 13, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 13, 2024
@bors
Auto merge of rust-lang#129046 - matthiaskrgr:rollup-9x4xgak, r=matth…
80eb5a8 
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#128643 (Refactor `powerpc64` call ABI handling)
 - rust-lang#128655 (std: refactor UNIX random data generation)
 - rust-lang#128745 (Remove unused lifetime parameter from spawn_unchecked)
 - rust-lang#128841 (bootstrap: don't use rustflags for `--rustc-args`)
 - rust-lang#128983 (Slightly refactor `TargetSelection` in bootstrap)
 - rust-lang#129026 (CFI: Move CFI ui tests to cfi directory)
 - rust-lang#129040 (Fix blessing of rmake tests)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 00d040e into rust-lang:master Aug 13, 2024
6 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Aug 13, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 13, 2024
@rust-timer
Unrolled build for rust-lang#128643
5d8e580 
Rollup merge of rust-lang#128643 - beetrees:ppc64-abi-fix, r=bjorn3

Refactor `powerpc64` call ABI handling

As the [specification](https://openpowerfoundation.org/specifications/64bitelfabi/) for the ELFv2 ABI states that returned aggregates are returned like arguments as long as they are at most two doublewords, I've merged the `classify_arg` and `classify_ret` functions to reduce code duplication. The only functional change is to fix rust-lang#128579: the `classify_ret` function was incorrectly handling aggregates where `bits > 64 && bits < 128`. I've used the aggregate handling implementation from `classify_arg` which doesn't have this issue.

`@awilfox` could you test this on `powerpc64-unknown-linux-musl`? I'm only able to cross-test on `powerpc64-unknown-linux-gnu` and `powerpc64le-unknown-linux-gnu` locally at the moment, and as a tier 3 target `powerpc64-unknown-linux-musl` has zero CI coverage.

Fixes: rust-lang#128579
@beetrees beetrees deleted the ppc64-abi-fix branch August 16, 2024 01:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjorn3 bjorn3 bjorn3 left review comments

Assignees

@bjorn3 bjorn3

Labels
A-abi Area: Concerning the application binary interface (ABI). O-PowerPC Target: PowerPC 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
Milestone
1.82.0
Development

Successfully merging this pull request may close these issues.

tests: FiveU16s extern ABI test fails on big endian PPC64 ELFv2 (musl)
10 participants
@beetrees @rustbot @jieyouxu @tgross35 @workingjubilee @awilfox @bzEq @rust-log-analyzer @bjorn3 @bors