Implement va_list and va_arg for s390x FFI

[uweigand]

Following the s390x ELF ABI and based on the clang implementation, provide appropriate definitions of va_list in library/core/src/ffi/mod.rs and va_arg handling in compiler/rustc_codegen_llvm/src/va_arg.rs.

Fixes the following test cases on s390x:
src/test/run-make-fulldeps/c-link-to-rust-va-list-fn src/test/ui/abi/variadic-ffi.rs

Fixes #84628.

[rustbot]

r? @davidtwco

(rustbot has picked a reviewer for you, use r? to override)

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[cuviper]

@davidtwco if you're not comfortable looking at s390x, I can take review on this.

[dlrobertson]

This is awesome! Thanks for the work! I don't know a ton about s390x, but is va_end by chance a nop? See this comment.

[uweigand]

This is awesome! Thanks for the work! I don't know a ton about s390x, but is va_end by chance a nop? See this comment.

Yes, va_end is a no-op on s390x as well.

[dlrobertson]

Thanks again for working on this! Looks reasonable after look at clangs implementation followed by a review of this, but note that I have no experience with s390x. Also not that I don't have a box to test this on.

[dlrobertson]

nit: I like the way clang does this with a reg_count_field here (either 0 or 1) followed by something like:

let reg_count = bx.struct_gep(...,reg_count_field))

[uweigand]

Done.

[dlrobertson]

nit: could we use something more clang-like (e.g. scaled_reg_count). I have no real strong opinions, but it would be nice to avoid re-using reg_off.

[uweigand]

Done.

[dlrobertson]

padded_size - unpadded_size is used at least twice, would it make sense to add a variable for this?

[uweigand]

Done.

[uweigand]

Thanks again for working on this! Looks reasonable after look at clangs implementation followed by a review of this, but note that I have no experience with s390x. Also not that I don't have a box to test this on.

Thanks for the review! I've addressed your comments, and also resolved the one FIXME about aggregate handling.

FWIW I'm pretty confident that the s390x ABI parts are correct (I'm the code owner of the IBM Z LLVM back-end, and one of the authors of the platform ABI specification); if there's still a bug somewhere, it's more likely to be about the Rust compiler implementation details, which I'm not very familar with.

I have run the full test suite (./x.py test) natively on a s390x machine, and the patch does fix the two previously failing test cases mentioned above. If there are any further tests you'd like me to run, please let me know.

[dlrobertson]

if there's still a bug somewhere, it's more likely to be about the Rust compiler implementation details, which I'm not very familar with.

Looks like you got those bits right!

FWIW I'm pretty confident that the s390x ABI parts are correct (I'm the code owner of the IBM Z LLVM back-end, and one of the authors of the platform ABI specification);

👍 just wanted to spot check, and learn a bit about s390x 😃

[dlrobertson]

I'm unable to r+ this, but this looks great!

CC @joshtriplett @cuviper

[nikic]

For reference: https://github.com/llvm/llvm-project/blob/e8bcc37fff5bda7dd9326903a2c31e6703b4fe68/clang/lib/CodeGen/TargetInfo.cpp#L7602

[nikic]

@bors r+

[bors]

📌 Commit eb22d70 has been approved by nikic

It is now in the queue for this repository.

[bors]

⌛ Testing commit eb22d70 with merge d6da428...

[bors]

☀️ Test successful - checks-actions
Approved by: nikic
Pushing d6da428 to master...

[rust-timer]

Finished benchmarking commit (d6da428): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.1%	[-1.2%, -1.0%]	2
Improvements ✅ (secondary)	-2.3%	[-2.6%, -2.0%]	6
All ❌✅ (primary)	-1.1%	[-1.2%, -1.0%]	2

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

[dlrobertson]

\o/ Thanks @uweigand for working on this!