Implement va_list and va_arg for s390x FFI #105381

uweigand · 2022-12-06T16:41:44Z

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 · 2022-12-06T16:41:52Z

r? @davidtwco

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

rustbot · 2022-12-06T16:41:55Z

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 · 2022-12-06T16:47:10Z

@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 · 2022-12-06T17:58:44Z

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 · 2022-12-18T03:25:28Z

compiler/rustc_codegen_llvm/src/va_arg.rs

+    // FIXME: structs with a single element of floating-point type should be
+    // passed in a FPR instead of a GPR.
+    let gpr_type = indirect || target_ty.is_any_ptr() || target_ty.is_integral();
+    let (max_regs, reg_count, reg_save_index, reg_padding) = if gpr_type {


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))

dlrobertson · 2022-12-18T03:34:40Z

compiler/rustc_codegen_llvm/src/va_arg.rs

+    let reg_ptr =
+        bx.struct_gep(va_list_ty, va_list_addr, va_list_layout.llvm_field_index(bx.cx, 3));
+    let reg_ptr_v = bx.load(bx.type_i8p(), reg_ptr, bx.tcx().data_layout.pointer_align.abi);
+    let reg_off = bx.mul(reg_count_v, bx.const_u64(8));


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.

dlrobertson · 2022-12-18T03:40:36Z

compiler/rustc_codegen_llvm/src/va_arg.rs

+    let target_ty_size = bx.cx.size_of(target_ty).bytes();
+    let indirect: bool = target_ty_size > 8 || !target_ty_size.is_power_of_two();
+    let unpadded_size = if indirect { 8 } else { target_ty_size };
+    let padded_size = 8;


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

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 rust-lang#84628.

uweigand · 2022-12-19T20:17:14Z

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 · 2022-12-19T21:13:17Z

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

CC @joshtriplett @cuviper

nikic · 2022-12-20T08:47:54Z

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

nikic · 2022-12-20T08:52:08Z

@bors r+

bors · 2022-12-20T08:52:09Z

📌 Commit eb22d70 has been approved by nikic

It is now in the queue for this repository.

bors · 2022-12-20T17:09:14Z

⌛ Testing commit eb22d70 with merge d6da428...

bors · 2022-12-20T19:50:49Z

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

rust-timer · 2022-12-20T21:12:52Z

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 · 2022-12-21T05:43:56Z

\o/ Thanks @uweigand for working on this!

rustbot assigned davidtwco Dec 6, 2022

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. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 6, 2022

dlrobertson reviewed Dec 6, 2022

View reviewed changes

dlrobertson reviewed Dec 18, 2022

View reviewed changes

uweigand force-pushed the s390x-ffi-vaarg branch from 972833d to eb22d70 Compare December 19, 2022 20:08

dlrobertson approved these changes Dec 19, 2022

View reviewed changes

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 Dec 20, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 20, 2022

bors merged commit d6da428 into rust-lang:master Dec 20, 2022

rustbot added this to the 1.68.0 milestone Dec 20, 2022

uweigand deleted the s390x-ffi-vaarg branch December 22, 2022 09:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement va_list and va_arg for s390x FFI #105381

Implement va_list and va_arg for s390x FFI #105381

uweigand commented Dec 6, 2022

rustbot commented Dec 6, 2022

rustbot commented Dec 6, 2022

cuviper commented Dec 6, 2022

dlrobertson left a comment

uweigand commented Dec 6, 2022

dlrobertson left a comment

dlrobertson Dec 18, 2022

uweigand Dec 19, 2022

dlrobertson Dec 18, 2022

uweigand Dec 19, 2022

dlrobertson Dec 18, 2022

uweigand Dec 19, 2022

uweigand commented Dec 19, 2022

dlrobertson left a comment •

edited

Loading

dlrobertson commented Dec 19, 2022

nikic commented Dec 20, 2022

nikic commented Dec 20, 2022

bors commented Dec 20, 2022

bors commented Dec 20, 2022

bors commented Dec 20, 2022

rust-timer commented Dec 20, 2022

dlrobertson commented Dec 21, 2022

Implement va_list and va_arg for s390x FFI #105381

Implement va_list and va_arg for s390x FFI #105381

Conversation

uweigand commented Dec 6, 2022

rustbot commented Dec 6, 2022

rustbot commented Dec 6, 2022

cuviper commented Dec 6, 2022

dlrobertson left a comment

Choose a reason for hiding this comment

uweigand commented Dec 6, 2022

dlrobertson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uweigand commented Dec 19, 2022

dlrobertson left a comment • edited Loading

Choose a reason for hiding this comment

dlrobertson commented Dec 19, 2022

nikic commented Dec 20, 2022

nikic commented Dec 20, 2022

bors commented Dec 20, 2022

bors commented Dec 20, 2022

bors commented Dec 20, 2022

rust-timer commented Dec 20, 2022

Overall result: ✅ improvements - no action needed

dlrobertson commented Dec 21, 2022

dlrobertson left a comment •

edited

Loading