Consider folkertdev's c_variadic proposal

[traviscross]

Over in #44930, @folkertdev proposed:

We need c_variadics on stable for zlib-rs, so I'm going to try and move this forward. A draft for a modified API is at

https://gist.github.com/folkertdev/47c79e2f5b03f1138db9d53be5e51ed1

Some highlights

• VaListImpl is private!
• VaList just has a single lifetime parameter now, which makes a lot more sense
• The VaList::with_copy function and a new va_copy! macro can be used to duplicate a VaList

This all relies on a different desugaring of ..., roughly

fn foo(placeholder: i32, mut args: ...) {
    // stuff
}

// --->

fn foo(placeholder: i32) {
    let tag = core::mem::MaybeUninit::<VaListImpl>::uninit();
    unsafe { core::instrinsics::va_start(tag.as_mut_ptr()) }
    let mut args = VaList(tag.assume_init_mut())

    // stuff
}

It seems possible to make that work internally.

---

I think this new API is nicer, because it leaks fewer implementation details. But before we start implementation work, does anyone see issues with this direction?

pinging specifically the c2rust folks @thedataking @ahomescu, you've probably worked with the existing c_variadics most.

Thoughts?

Tracking:

• Tracking issue for RFC 2137: Support defining C-compatible variadic functions in Rust  #44930

cc @rust-lang/lang

[folkertdev]

To explore these ideas, I've implemented my proposal in ea75827.

This is prototype quality at the moment, but I think the examples/tests look much better now that the only user-visible type is VaList. It all seems to work (well, after I implemented #141538 to stop LLVM miscompiling va_arg) and generates the expected (lifetime) errors.

I think my current proposal is more T-libs than T-lang, but c_variadics in general can probably use your discussion: most work on them is from 5+ years ago.

For T-lang specifically:

• are you OK with further use of super let, similar to the pin! macro.
• this sort of touches on 'fn lifetimes, though my proposal does not need it, because the logic now gives the VaList the lifetime of a local binding on the stack
• not part of my proposal, but relevant: should arguments to variadic functions be constrained by VaArgSafe to fix Varargs are completely unchecked if passed as generics #61275
• is VaArgSafe too limited: currently bool, 8-bit and 16-bit integers, and f32 don't implement this trait, because in C these types will (silently) participate in implicit promotion. That does mean that users/tools that want to pass or receive these types have to perform this casting themselves. We already do have good errors for these cases, and and a lint that suggests the right cast.

For T-libs when the time comes

• The va_copy macro requires a doc(hidden) function that does most of the actual work.
• That hidden function does some ugly unsafe stuff so that we don't need to expose VaListTag (formerly VaListImpl, the "tag" mirrors the clang/llvm name). I believe that it's sound, but it does look weird, maybe there is some way to make that nicer that I'm missing

[workingjubilee]

The current proposal does not include i8, u8, etc. as VaArgSafe types on my advisory because of an important detail that may affect both this API and also, depending on what is decided, existing usage of ... by Rust code (namely: calling external functions with it).

• As folkertdev already mentioned, Varargs are completely unchecked if passed as generics #61275 shows that we do not check types are compatible with variable arguments when the type is generic. This allows an end-run around the check that enforces this effective bound, and later parts of the compiler miscompile the result.
• One of the obvious answers for how to solve this would be to move the check into the trait system and check for a trait. The obvious choice would be VaArgSafe, as that would mean we would have one trait, not two, but that militates against allowing it to do implicit "coercions" for smaller types like originally proposed.
• It is still possible to square this circle, allowing both "sides" to be described by a single trait and operate in the same way, if we allow implicit numeric promotion of arguments passed to .... I do not recommend adding numeric promotion to the language, even if it's hard to perceive in this case.
• The same code in C would simply not work for reasons that aren't obvious: even if you passed in a char on one side, you must ask for it as an int when using va_arg! Asking for char is UB because it was promoted to int! For floats, these conversions cannot be "simply" sign-extended and everything is a bit weirder, which is part of what provokes miscompilations.
• I think we should keep it simple and adhere rigidly to "what C allows, without C's conveniences"¹ because while it makes for annoying code in places, the simplicity allows for users to easily understand what happens. That would mean it would be possible to, in the future², allow user-guided implementation of the trait for their own types, especially including repr(C) structs that are ABI-stable enough for sensibly crossing FFI boundaries. These can be passed into ... and taken from va_arg in C, and in some libraries this ability is important.

While having VaList::arg work for structs is considered "out of scope" for the initial stabilization, there is also currently no other plan for how to make this work for VaList::arg in the future due to its trait bound. At least, not without just stripping the bound entirely. I believe adding even tiny amounts of added complexity to how C's variable arguments are handled by the compiler would threaten making the compiler's approach sufficiently illegible that it would be hard to understand when such an implementation would be valid in the future.

Footnotes

1. The "code brutalism" approach. ↩

2. The current va_arg implementation in the compiler isn't up-to-snuff to handle this yet, so this is a forward-looking comment only. ↩

[workingjubilee]

I found something more interesting while reviewing @folkertdev's PR #141538 which is that we currently decide the ABI we handle variable arguments for based on the target.

However, if we want our VaList type to have parity with the extended_varargs_abi_support feature, we need the ability to declare both extern "sysv64" fn(...) and extern "win64" fn(...) in the same program. This means we need those to expand to something that looks like VaList<'f, extern "sysv64"> and VaList<'f, extern "win64"> in the same program. If T-lang wishes us to support this... and I can't see why you wouldn't, as passing and receiving having parity seems valuable to me... then @folkertdev's proposal will have to be elaborated significantly.

So we need at least one more decision here from T-lang, though more of a confirmation:

• Do we need to support receiving C variable arguments, thus a ...-to-VaList desugaring, for every extern "abi" for which a target supports declaring

unsafe extern "abi" {
    fn uses_varargs(...);
}

Note that doing such will bring us afoul of this:

• VaList<'_> does not carry its ABI in its type #141618

[beetrees]

Note that modern C can pass f32/u8/i8/u16/i16 without numeric promotion. For instance, float/short will both be promoted to double/int, whereas _Float32/_BitInt(16) will be passed without numeric promotion.

[workingjubilee]

@beetrees Well, yes, but short isn't the same type as _BitInt(16), and I'm not sure the _Float32 handling isn't a bug. The complexity of such cases seems more like evidence against doing things "for people".

[beetrees]

I don't think the _Float32 handling is a bug AFAICT: the C23 standard states in section 6.5.2.2 (this is from a recent draft as I don't have access to the final standard) "The integer promotions are performed on each trailing argument,
and trailing arguments that have type float are promoted to double . These are called the default
argument promotions. No other conversions are performed implicitly". As _Float32 is a different type in the C type system (the same way that e.g. long and long long are always separate types even if they are the same width), it doesn't get promoted. Specifically, the standard removed default promotions for _FloatN types in a January/February 2022 Virtual Meeting.

The complexity of such cases seems more like evidence against doing things "for people".

I agree that Rust shouldn't be automatically promoting arguments for people.

[workingjubilee]

Fascinating. May have to rethink how this should be approached a bit.

[traviscross]

@rustbot labels -I-lang-nominated +I-lang-radar

We discussed this in the lang call today. People generally liked the idea. Probably it'll need an RFC before stabilization, but we're open to a lang experiment here if it would help. @joshtriplett volunteered to champion it and help guide it along.

[beetrees]

Currently, this proposal doesn't give any way to write the C type va_list * (a pointer to a va_list) in a target independent manner. With this proposal, va_list * would translate to *mut VaList<'_> on targets where va_list is a pointer and to just VaList<'_> on targets where va_list is a single-element array of a struct (although this second translation doesn't allow continuing to use the va_list after passing a pointer to it to a function, which is possible in C). This is because the C array-to-pointer decay only occurs when the va_list is a plain argument of a function, not when it is behind a pointer.

I think a potential solution would be to have the new VaList<'_> be the same as the current VaListImpl<'_>/VaListTag<'_> type (the tag type). This would work without further modifications when the tag is a pointer type. For when the tag is a single-element array of a struct, the only time the VaList<'_> needs to be passed indirectly as a pointer is when it's a direct function argument. This can be handled by modifying the ABI handling code in rustc_target so that the relevant targets always pass VaList<'_> indirectly. This solution would also allow for a Clone impl to copy a VaList<'_> instead of requiring a va_copy macro, as VaList<'_> would now always be the tag type.

This would mean that this:

unsafe extern "C" {
    fn takes_va_list(args: VaList<'_>);
    fn takes_va_list_ref(args: *mut VaList<'_>);
}

unsafe extern "C" fn takes_varargs(mut args: ...) {
    // function body
    unsafe { takes_va_list_ref(&mut args); }
    unsafe { takes_va_list(args); }
}

would roughly desugar into this:

unsafe extern "C" fn takes_varargs() {
    let mut tag = MaybeUninit::<VaList<'_>>::uninit();
    unsafe { va_start(tag.as_mut_ptr()); }
    let mut args = tag.assume_init();
    // function body
    // Passing a pointer to a `VaList<'_>` works as expected.
    unsafe { takes_va_list_reg(&mut args); }
    // On targets where the tag is a pointer, the `va_list` would be passed directly, matching the
    // C ABI.
    unsafe { takes_va_list(args); }
    // On target where the tag is a single-element array of a struct, the `va_list` would be passed
    // indirectly, also matching the C ABI. This would happen using the ABI handling in
    // `rustc_target`.
    unsafe { takes_va_list(&mut args); }
}

and the API would look something like this:

struct VaList<'a> {
    // ...
}

impl Clone for VaList<'_> {
    fn clone(&self) -> Self {
        // ...
    }
}

impl VaList<'_> {
    fn arg<T: VaArgSafe>(&mut self) -> T {
        // ...
    }
}

(I wasn't sure whether to post this here or on the main tracking issue. Please let me know if I've posted it in the wrong place.)

[folkertdev]

Currently, this proposal doesn't give any way to write the C type va_list * (a pointer to a va_list) in a target independent manner.

What do you mean by that exactly? I don't see how that is true when VaList is (supposed to be) exactly the same thing as the target's va_list type. Hence *mut VaList should be equivalent to va_list *, on any target? The property that VaList is va_list is extremely valuable, in my opinion.

I also don't see why having/passing va_list * is particularly important? Certainly not important enough for special ABI handling.

[beetrees]

I don't see how that is true when VaList is (supposed to be) exactly the same thing as the target's va_list type. Hence *mut VaList should be equivalent to va_list *, on any target?

This isn't true of the original proposal. In the original proposal, VaList<'_> is equivalent to va_list when va_list is a pointer and va_list * when va_list is an single-element array of a struct.

For example, on x86_64 Linux, va_list is defined as:

typedef struct {
  unsigned int gp_offset;
  unsigned int fp_offset;
  void *overflow_arg_area;
  void *reg_s;
} va_list[1];

This is equivalent to [VaListTag; 1] in Rust (for instance, sizeof(va_list) == 24). However, C automatically decays arrays to pointers when they are passed to functions. This means that when va_list is an array type (like on x86-64 Linux), this C code:

void foo(va_list va);

void bar(...) {
  va_list va;
  va_start(va);
  foo(va);
  va_end(va);
}

is roughly desugared to this:

void foo(va_list* va);

void bar(...) {
  va_list va;
  va_start(va);
  foo(&va);
  va_end(va);
}

---

In C it is legal to do this:

#include <stdarg.h>

int get_next(va_list* va);

int add2(...) {
  va_list va;
  va_start(va);
  int a = get_next(&va);
  a += get_next(&va);
  va_end(va);
  return a;
}

(To quote footnote 295 from the C23 standard (standard disclaimer that I only have access to a recent draft): "A pointer to a va_list can be created and passed to another function, in which case the original function can make further use of the original list after the other function returns.")

With the original proposal there is no way to convert this code to the equivalent Rust code (for example, using C2Rust). The solution i suggested would make the above C code equivalent to:

unsafe extern "C" {
  fn get_next(va: *mut VaList<'_>) -> c_int;
}

unsafe extern "C" fn add2(mut va: ...) -> c_int {
  unsafe {
    let mut a = get_next(&mut va);
    a += get_next(&mut va);
    a
  }
}

[folkertdev]

I recognize that the single-element array construction is a bit weird, in ways that I admittedly don't fully understand.

For instance, foo and bar generate the same assembly, despite having seemingly fundamentally different input types:

https://godbolt.org/z/n8c4aq5hM

#include <stdarg.h>

extern int foo(va_list va) {
    return va_arg(va, int);
}

extern int bar(va_list *va) {
    return va_arg(*va, int);
}

Looking at that example, rdi must contain a *mut VaListTag in both cases, meaning that somehow va_list ~ va_list *. I think that is just the decay to a pointer that you're describing.

However, looking at this code

#include <stdarg.h>

int get_next(va_list* va);

int add2(...) {
  va_list va;
  va_start(va);
  int a = get_next(&va);
  a += get_next(&va);
  va_end(va);
  return a;
}

With the original proposal there is no way to convert this code to the equivalent Rust code

I mean, you can generate a valid translation:

extern "C" {
    fn get_next(va: *mut VaList);
}

fn add2(va: ...) -> i32 {
    let mut a = unsafe { get_next(&va) };
    a + unsafe { get_next(&va) }
}

Which produces roughly equivalent LLVM IR and assembly:

https://godbolt.org/z/veon5zqja

This isn't true of the original proposal. In the original proposal, VaList<'_> is equivalent to va_list when va_list is a pointer and va_list * when va_list is an single-element array of a struct.

Right, so the relation between va_list and VaList needs a more careful formulation.

• A C function accepting va_list can be declared in rust as accepting VaList
• A C function accepting va_list * can be declared in rust as accepting VaList
• A rust function accepting a VaList can be called from C by passing in a va_list or va_list *
• A rust function accepting a *mut VaList can be called from C by passing va_list **
• it may not be true that sizeof(va_list) equals core::mem::size_of::<VaList>()

That seems perfectly workable to me if it is documented clearly. I don't think we should spend language design budget on accommodating C quirks around when arrays are actually pointers here.

[beetrees]

(As I've just realised I never linked it before, here's a link to the C23 standard draft I've been referencing.)

I recognize that the single-element array construction is a bit weird, in ways that I admittedly don't fully understand.

A brief summary, using a char array as an example:

typedef char array_ty[20];

struct struct_def { array_ty field; };
union union_def { array_ty field; };

array_ty global_var;

void function(array_ty arg) {
    array_ty local_var;
}

The only time that array_ty is essentially the same as array_ty* (except for not needing to be explicitly dereferenced) is when it is passed as an argument. All the other uses of array_ty mean the array itself, not a pointer to it. The argument case is very similar to the way ABIs pass arguments indirectly; the only difference is that any changes to the array in the callee are visible in the calling function in C (usually ABIs make a copy if the argument is used again in the calling function to preserve pass-by-value semantics).

For va_list specifically, the C23 standard states in section 7.16 paragraph 4 "The object ap may be passed as an argument to another function; if that function invokes the va_arg macro with parameter ap, the representation of ap in the calling function is indeterminate and shall be passed to the va_end macro prior to any further reference to ap.²⁹⁵⁾" This essentially prohibits C code from relying on whether va_list is an array or not by prohibiting it from observing whether the callee mutating it's argument also mutates the variable in the caller. (Footnote 295 is the footnote I referenced in my previous comment.)

I mean, you can generate a valid translation:

extern "C" {
    fn get_next(va: *mut VaList);
}

fn add2(va: ...) -> i32 {
    let mut a = unsafe { get_next(&va) };
    a + unsafe { get_next(&va) }
}

Which produces roughly equivalent LLVM IR and assembly:

https://godbolt.org/z/veon5zqja

This is incorrect. In the IR in the Compiler Explorer you linked, the Rust example passes a va_list ** (a pointer to a pointer to the va_list struct) to get_next, whereas the C version passes a va_list *.

• A C function accepting va_list * can be declared in rust as accepting VaList

• A rust function accepting a VaList can be called from C by passing in a va_list or va_list *

• A rust function accepting a *mut VaList can be called from C by passing va_list **

With the original proposal, these are only true when va_list in a single-element array of a struct. Additionally, requiring the user to pass the Rust VaList<'_> type when they want to pass a pointer to the va_list isn't very useful, as the calling function would be unable to use the VaList<'_> after it had been passed to the function as VaList<'_> is (correctly) not Copy.

I don't think we should spend language design budget on accommodating C quirks around when arrays are actually pointers here.

The only time that arrays are pointers in a way that is relevant to Rust is when they are direct arguments to functions. The only alternative to the solution I suggested that's occurred to me is to go back to the design of having two VaList types (one which is the actual (tag) type that is what is used almost everywhere, and one which is only needed when passing a VaList to C ABI functions), which I think is a worse and more complex design. Handling indirect arguments is already something the compiler already does, meaning that making sure VaList is passed correctly would just take a few lines of code for each of the relevant ABIs (some ABIs pass larger structs indirectly already and therefore their ABI handling code would require no modifications).

[folkertdev]

right, I also noticed that later. So, thank you for arguing your point.

To make the issue explicit: to make the C example work in rust like it does in C you'd have to lie about the method signature, and use va_copy! which is 1) annoying and 2) less efficient:

use std::ffi::{va_copy, VaList};

extern "C" {
    fn get_next(va: VaList) -> i32;
}

#[no_mangle]
unsafe extern "C" fn add2(mut va: ...) -> i32 {
    let a = unsafe { get_next(va_copy!(va)) };
    a + unsafe { get_next(va) }
}

---

Some thoughts on the alternative proposal.

It introduces a type that is always passed by memory (and hence uses BackendRepr::Memory). I think that, rather than manually adjusting the ABI algorithm, maybe we can have an internal annotation, e.g. #[rustc_always_pass_by_memory]. The attribute would at least make it clearer that this type behaves in a custom way, and provides a hook for documentation. This functionality may come in handy elsewhere too (though I have no concrete examples).

One potential complication is how an attribute interacts with having multiple vararg ABIs in the same program. It's unclear whether we'll ever actually implement that, but one of the ideas to do it is to add a defaulted generic to VaList. But we cannot change attributes on the basis of a generic. There are other potential solutions for the problem in general though (like using multiple types), or a trait ByMemory marker trait could be added, or at that point we do hardcode VaList into the ABI logic.

I think this attribute semantically means that the VaList type must be !Send and !Sync (the raw pointer fields already handle that). That is because the Drop would still run in the original stack frame. Conveniently I do think this passing mode guarantees that the VaList never leaves that stack frame. It is effectively pinned on the stack.

The VaList just being Clone is very elegant. It removes the need for va_copy! and the super let mess. Big fan.

The next step is probably to get a vibe check from the ABI specialists on whether this idea is feasible and desirable.

[bjorn3]

It introduces a type that is always passed by memory (and hence uses BackendRepr::Memory).

Do you mean PassMode::Indirect { on_stack: false, ... } or PassMode::Indirect { on_stack: true, ... }. Aka will it be passed as pointer or by copying it to a fixed offset on the stack? BackendRepr::Memory will cause the latter to be used for the C ABI on most systems.

[beetrees]

I've created a draft PR of a possible implementation at #141980.

[folkertdev]

I guess actually more the former PassMode::Indirect { on_stack: false, ... } (which is in line with beetrees' PR). This is a bit non-standard because it's really a mutable reference.

[beetrees]

At the backend level, there isn't really any difference between an x with PassMode::Indirect { on_stack: false, ... } and &mut x (except that if x is used again after the call (in Rust this can only occur when x is Copy), a copy of the argument with PassMode::Indirect will be made so that the callee cannot modify the copy of x in the caller. Additionally, when passed by value x will be dropped by the callee, whereas when passing &mut x x won't be dropped). For an example of this, see this compiler explorer.

[folkertdev]

Well because VaList is impl Drop it will (semantically) be used again after the call. That makes this trick unintuitive: it doesn't line up with how move semantics normally work.

Anyway, @bjorn3 are there any issues with that approach that you can see?

[beetrees]

Well because VaList is impl Drop it will (semantically) be used again after the call. That makes this trick unintuitive: it doesn't line up with how move semantics normally work.

Move semantics mean that the drop will occur in the callee, not the caller (I've just realised I forgot to mention this in the summary of the differences between x and &mut x):

extern "C" fn foo(x: VaList<'_>) {
    // `x` is dropped here
}

unsafe extern "C" fn bar(x: ...) {
    foo(x);
}

Regardless, this doesn't matter as the Drop impl of VaList is a no-op.

To clarify, my comment in #141524 (comment) was about what happens at the backend level: from a Rust semantics perspective all arguments are passed by value regardless of PassMode.

[bjorn3]

Anyway, @bjorn3 are there any issues with that approach that you can see?

I guess it would work, but didn't think about it too deeply.

[joboet]

The API as implemented in Bee's PR is still unsound, as it is still possible to move or leak a VaList in safe code – both of which are undefined behaviour according to the C standard. Even though this happens to work out on all current platforms, I think not violating the C standard should be a requirement for std stable API.

[folkertdev]

To be clear, the problem you're describing is that VaList's Drop is a no-op, but should call va_end? Or is there something else?

I wanted to follow up on that. I suspect that it's probably OK these days to annotate the Drop impl with #[inline(always)], but formally the inlining is not guaranteed I think (and the inlining is required for correctness as far as I understand).

[joboet]

To be clear, the problem you're describing is that VaList's Drop is a no-op, but should call va_end? Or is there something else?

I wanted to follow up on that. I suspect that it's probably OK these days to annotate the Drop impl with #[inline(always)], but formally the inlining is not guaranteed I think (and the inlining is required for correctness as far as I understand).

That as well, but it's more generally about the fact that you can move a VaList:

fn example(list: ...) {
    let moved = list;
}

The C standard says that this is implementation-defined behaviour, which means it could be undefined.