Skip to content

implement va_arg for powerpc #141622

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

Merged
merged 2 commits into from
Jun 1, 2025
Merged

implement va_arg for powerpc #141622

merged 2 commits into from
Jun 1, 2025

Conversation

folkertdev
Copy link
Contributor

@folkertdev folkertdev commented May 26, 2025

tracking issue: #44930

The llvm va_arg implementation is well-known to have serious limitations. Some planned changes to rust's VaList make it much more likely that LLVM miscompiles va_arg, so this PR adds support for the various powerpc targets. Now at least the targets that core has explicit support for will continue to work.

For powerpc (the 32-bit variant) this implementation also fixes a bug where only up to 20 variadic arguments were supported.

Locally (with qemu), these targets now pass the tests in https://github.com/rust-lang/rust/blob/master/tests/run-make/c-link-to-rust-va-list-fn/checkrust.rs. That test does not actually run for the powerpc targets in CI though.

The implementation is based on clang:

cc @daltenty (target maintainer)
r? @workingjubilee
@rustbot label: +F-c_variadic

@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. F-c_variadic `#![feature(c_variadic)]` labels May 26, 2025
@workingjubilee
Copy link
Member

cc @programmerjake I don't know if you give a damn but

@programmerjake

This comment was marked as resolved.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 27, 2025
…r=workingjubilee

use custom types to clarify arguments to `emit_ptr_va_arg`

tracking issue: rust-lang#44930

split out of rust-lang#141622

r? `@workingjubilee`
`@rustbot` label: +F-c_variadic
@rust-log-analyzer

This comment has been minimized.

compiler-errors added a commit to compiler-errors/rust that referenced this pull request May 27, 2025
…r=workingjubilee

use custom types to clarify arguments to `emit_ptr_va_arg`

tracking issue: rust-lang#44930

split out of rust-lang#141622

r? ``@workingjubilee``
``@rustbot`` label: +F-c_variadic
rust-timer added a commit that referenced this pull request May 27, 2025
Rollup merge of #141623 - folkertdev:va-arg-explicit-types, r=workingjubilee

use custom types to clarify arguments to `emit_ptr_va_arg`

tracking issue: #44930

split out of #141622

r? ``@workingjubilee``
``@rustbot`` label: +F-c_variadic
@bors
Copy link
Collaborator

bors commented May 30, 2025

☔ The latest upstream changes (presumably #141753) made this pull request unmergeable. Please resolve the merge conflicts.

@rustbot rustbot added the A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. label May 30, 2025
@workingjubilee
Copy link
Member

r=me with the assert! to make sure whoever wants the rest of support actually implements it.

@bors delegate+

@bors
Copy link
Collaborator

bors commented May 31, 2025

✌️ @folkertdev, you can now approve this pull request!

If @workingjubilee told you to "r=me" after making some further change, please make that change, then do @bors r=@workingjubilee

@workingjubilee workingjubilee 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-review Status: Awaiting review from the assignee but also interested parties. labels May 31, 2025
This actually fixes a bug where before only 20 arguments could be passed. As far as I can tell, an arbitrary number of arguments is now supported
@folkertdev
Copy link
Contributor Author

@bors r=@workingjubilee

@bors
Copy link
Collaborator

bors commented Jun 1, 2025

📌 Commit be13ce3 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 1, 2025
bors added a commit that referenced this pull request Jun 1, 2025
Rollup of 9 pull requests

Successful merges:

 - #140370 (Improve diagnostics for usage of qualified paths within tuple struct exprs/pats)
 - #141224 (terminology: allocated object → allocation)
 - #141622 (implement `va_arg` for `powerpc`)
 - #141666 (source_span_for_markdown_range: fix utf8 violation)
 - #141789 (Exclude `CARGO_HOME` from `generate-copyright` in-tree determination)
 - #141823 (Drive-by refactor: use `OnceCell` for the reverse region SCC graph)
 - #141834 (Add unimplemented `current_dll_path()` for WASI)
 - #141846 (Fix TLS model on bootstrap for cygwin)
 - #141852 (resolve if-let-chain FIXME on bootstrap)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit cf4fbc0 into rust-lang:master Jun 1, 2025
9 checks passed
@rustbot rustbot added this to the 1.89.0 milestone Jun 1, 2025
rust-timer added a commit that referenced this pull request Jun 1, 2025
Rollup merge of #141622 - folkertdev:powerpc-va_arg, r=workingjubilee

implement `va_arg` for `powerpc`

tracking issue: #44930

The llvm `va_arg` implementation is well-known to have serious limitations. Some planned changes to rust's `VaList` make it much more likely that LLVM miscompiles `va_arg`, so this PR adds support for the various powerpc targets. Now at least the targets that `core` has explicit support for will continue to work.

For `powerpc` (the 32-bit variant) this implementation also fixes a bug where only up to 20 variadic arguments were supported.

Locally (with qemu), these targets now pass the tests in https://github.com/rust-lang/rust/blob/master/tests/run-make/c-link-to-rust-va-list-fn/checkrust.rs. That test does not actually run for the powerpc targets in CI though.

The implementation is based on clang:

- handling of big endian architectures https://github.com/llvm/llvm-project/blob/3c8089d1ea53232d5a7cdc33f0cb43ef7d6f723b/clang/lib/CodeGen/ABIInfoImpl.cpp#L191-L193
- 64-bit https://github.com/llvm/llvm-project/blob/3c8089d1ea53232d5a7cdc33f0cb43ef7d6f723b/clang/lib/CodeGen/Targets/PPC.cpp#L969
- 32-bit https://github.com/llvm/llvm-project/blob/3c8089d1ea53232d5a7cdc33f0cb43ef7d6f723b/clang/lib/CodeGen/Targets/PPC.cpp#L430

cc `@daltenty` (target maintainer)
r? `@workingjubilee`
`@rustbot` label: +F-c_variadic
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Jun 3, 2025
Rollup of 9 pull requests

Successful merges:

 - rust-lang/rust#140370 (Improve diagnostics for usage of qualified paths within tuple struct exprs/pats)
 - rust-lang/rust#141224 (terminology: allocated object → allocation)
 - rust-lang/rust#141622 (implement `va_arg` for `powerpc`)
 - rust-lang/rust#141666 (source_span_for_markdown_range: fix utf8 violation)
 - rust-lang/rust#141789 (Exclude `CARGO_HOME` from `generate-copyright` in-tree determination)
 - rust-lang/rust#141823 (Drive-by refactor: use `OnceCell` for the reverse region SCC graph)
 - rust-lang/rust#141834 (Add unimplemented `current_dll_path()` for WASI)
 - rust-lang/rust#141846 (Fix TLS model on bootstrap for cygwin)
 - rust-lang/rust#141852 (resolve if-let-chain FIXME on bootstrap)

r? `@ghost`
`@rustbot` modify labels: rollup
github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request Jun 3, 2025
…llaumeGomez

Rollup of 9 pull requests

Successful merges:

 - rust-lang#140370 (Improve diagnostics for usage of qualified paths within tuple struct exprs/pats)
 - rust-lang#141224 (terminology: allocated object → allocation)
 - rust-lang#141622 (implement `va_arg` for `powerpc`)
 - rust-lang#141666 (source_span_for_markdown_range: fix utf8 violation)
 - rust-lang#141789 (Exclude `CARGO_HOME` from `generate-copyright` in-tree determination)
 - rust-lang#141823 (Drive-by refactor: use `OnceCell` for the reverse region SCC graph)
 - rust-lang#141834 (Add unimplemented `current_dll_path()` for WASI)
 - rust-lang#141846 (Fix TLS model on bootstrap for cygwin)
 - rust-lang#141852 (resolve if-let-chain FIXME on bootstrap)

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. F-c_variadic `#![feature(c_variadic)]` 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.

6 participants