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

Opened by Aaron Turon at 2024-11-18 07:50:13

This is a tracking issue for the RFC "Support defining C-compatible variadic functions in Rust" (rust-lang/rfcs#2137).

Steps:

[x] Implement the RFC (cc @rust-lang/compiler -- can anyone write up mentoring instructions?)
[ ] Adjust documentation (see instructions on forge)
[ ] Stabilization PR (see instructions on forge)

Unresolved questions:

"When implementing this feature, we will need to determine whether the compiler can provide an appropriate lifetime that prevents a VaList from outliving its corresponding variadic function."
Continuing bikeshed on the ... syntax.

This is a tracking issue for the RFC "Support defining C-compatible variadic functions in Rust" (rust-lang/rfcs#2137).

Steps:
- [x] Implement the RFC (PR https://github.com/rust-lang/rust/pull/57760)
- [ ] Adjust documentation (see instructions on forge)
- [ ] Stabilization PR (see instructions on forge)
Unresolved questions:
- "When implementing this feature, we will need to determine whether the compiler can provide an appropriate lifetime that prevents a VaList from outliving its corresponding variadic function."
- Continuing bikeshed on the ... syntax.
- Ensure that even when this gets stabilized for regular functions, it is still rejected on const fn.
- What even is the semantics of this? What exactly is allowed and disallowed? Would be good to have a Miri implementation.
- borrowck problems: https://github.com/rust-lang/rust/issues/125431
- What exactly are the ABI requirements around variadic calls? What are the ABI compatibility rules? Do we have to be concerned about LLVM target features affecting variadic ABI?
- Soundness bug: https://github.com/rust-lang/rust/issues/61275
Alexander Regueiro at 2019-02-28 18:20:24
I'd like to work on this, I already have some prototype
Paul Liétar at 2017-09-29 18:15:03
Awesome @plietar! It'd probably be good to bring this up in the "middle-end" compiler working group channel, which would also be a good place to get any help you might need.
Aaron Turon at 2017-09-29 18:19:55
@plietar How goes the implementation? I remember you showing a mostly complete prototype on IRC.
Josh Triplett at 2017-11-23 16:15:54
@plietar any news to share? This is a blocker for teams working on C to Rust transpilers, so this addition would be very welcome. (I'm part of one such team).
Per Larsen at 2018-02-14 22:06:07
Hey, Sorry I've been busy and then forgot about this. I'll get my prototype back in shape, hopefully by this weekend.
Paul Liétar at 2018-02-22 22:53:48
@plietar Any update on this? Do you have a WIP branch I can check out to try / fiddle with this?
Alec Theriault at 2018-03-16 18:01:01
Looks like I'm a little late to the party :smile: ... sorry about that

A few questions:
1. How would functions that use a va_list multiple times work without the ability to explicitly use va_start and va_end? Are they expected to use copy? E.g. execl typically loops through the arguments to to get argc, creats a array argv of size argc, loops through the list again populating argv, and finally call execv.
2. The structure of a va_list varies greatly between the architectures. The intrinsic functions work with the architecture specific structure bitcast to an i8*, but AFAIK we'll still need to define the structure. Which architectures will be expected to be supported in the first iteration? Or am I mistaken that we'll have to define the structure?
@plietar if you don't have the time to work on this any more or if there is any way I could help out, I'd be more than happy to do so. I haven't worked on rustc much, but I'd be happy to help however I can with the implementation of this.
Dan Robertson at 2018-03-20 14:04:02
@dlrobertson

Seems likely that @plietar doesn't have much time, though they can speak for themselves.

I've not really looked closely at what would be needed to implement this, but if you need any help, please ping me, or reach out on gitter/IRC.
Niko Matsakis at 2018-03-20 16:55:32
I've been working on this for the past two week and have
- Added va_list_kind to the target specification which closely mirrors clangs TargetInfo::BuiltinVaListKind
- Added Type::va_list which builds the correct structure based on the targets va_list_kind
I'm struggling a bit with understanding how to write something in libcore and link that to Type::va_list in trans. I'm currently attempting to add #[lang = "va_list"] so that I can check the def.did against the lang_items().va_list_impl() id. Does this seem correct?

Also how would you like me to break this up into PRs? My current plan was to submit a PR once I got VaList implemented (meaning functions like vprintf could be defined and tested). Then submit a second PR for implementing support for functions like printf.
Dan Robertson at 2018-04-04 14:04:45
Now that the VaList structure is implemented and merged into master, I'm moving on to implementing variadic functions.

When any issues with the implementation of VaList are found, please ping me.
Dan Robertson at 2018-11-29 22:29:18
@dlrobertson That's super. Are you implementing the ... syntax given that seems to be the consensus so far, and bikeshedding hasn't revealed a better one? (Unless I'm behind on things.)
Alexander Regueiro at 2018-12-09 02:37:25
Are you implementing the ... syntax given that seems to be the consensus so far, and bikeshedding hasn't revealed a better one? (Unless I'm behind on things.)

Yeah, that is what I'm working on at the moment. Just trying to figure out the best way to automagically insert va_start/va_end and generate the correct type etc.
Dan Robertson at 2018-12-09 20:26:09
@dlrobertson Yeah, that doesn't sound trivial... pop onto Discord or Zulip if you need advice though, and I'm sure someone will be able to give some tips.
Alexander Regueiro at 2018-12-09 22:26:10
Am I wrong or variadic functions could be used to replace some macros like println / vec etc.?
Natalie Perret at 2018-12-19 22:47:25
@ehouarn-perret Yes, they certainly could be. That may happen after they're implemented and stabilised... but anyway this feature is slightly different: it's about C-compatible variadic functions (VaList), not Rust-native variadics. The intention is to implement both in time.
Alexander Regueiro at 2018-12-19 22:55:27
@ehouarn-perret as @alexreg mentioned this work is purely for C variadic functions, so it is only valid for extern "C" functions. I recently started researching variadic generics. I think that is what you're looking for.
Dan Robertson at 2018-12-20 00:16:43
@alexreg @dlrobertson Thanks for pointing this out. True I was more looking for Rust native variadic support. Sorry for the noise
Natalie Perret at 2018-12-22 22:44:57
@dlrobertson Do you have any update on this? If someone had the time, would it be possible to provide assistance in any way?
Dan Kolsoi at 2019-01-14 20:06:20
@TheDan64 thanks for checking up on this, and there is definitely enough work to share! I'll use your prompt as a chance to give a general post on the status of my current work and the general state of things :smile:

Defining "true" C variadic functions in Rust
- [x] Update parsing to allow for implementing C variadic functions
- [x] Spoof the VaList argument for Rust defined C variadic functions
- [x] Inject va_start
- [x] Inject va_end
- [x] Call Rust defined C variadic functions in Rust
I plan to post a very WIP PR shortly.

NB: There are three know issues with my current WIP codegen code:
- The creation of the va_list allocates a VaList, which compiles to a i64*. It should 1) allocate a VaListImpl 2) get the pointer to the allocated VaList 3) call va_start on 2.
- The calculation of the argument name that va_end should be called on is incorrect.
- The "spoofed" VaList causes calls to Rust defined C variadic functions from Rust to be incorrect.
Other work unrelated to my current work
- [ ] Fix Aarch64 support: https://github.com/rust-lang/rust/issues/56475
- [ ] Attempt to implement VaList::arg in pure Rust: https://github.com/rust-lang/rust/issues/56489#issuecomment-444142735 also see va_list-rs
- [ ] Cleanup the cfgs in src/libcore/ffi.rs with the use of cfg_if. Also see: https://github.com/rust-lang/rust/issues/57446
- [ ] Test on various architectures and OSes
- [ ] Improve documentation
If anyone is interested in working on these, please feel free to, and let me know how I can help!

Dan Robertson at 2019-01-14 21:03:57
Posted a WIP PR of my current work. Comments and feedback would be appreciated.
Dan Robertson at 2019-01-19 17:25:21
\o/ https://github.com/rust-lang/rust/pull/57760 has been merged thanks to some awesome reviewing and help I received from @alexreg, @oli-obk, @matthewjasper, @varkor, and many others. The c_variadic feature now enables both core::ffi::VaList and Rust defined C-variadic functions. The work is still a long way from stable and there are still a few open issues, but I think technically the RFC has been implemented now. Could someone with the right privileges update the issue and tags?
Dan Robertson at 2019-02-28 18:16:59
@dlrobertson We are working to use the c_variadics feature in the C2Rust translator. Things were going great until we took a closer look at the design of VaList::copy. It makes a lot of sense to ensure that the copy of the va_list can only be used inside the closure such that copy can call va_end before returning the result of the closure. However, this design makes syntax-directed translation from C really difficult, if not impossible, in some situations. See issue https://github.com/immunant/c2rust/issues/43 for more detail.

On the other hand, exposing the va_copy and va_end intrinsics would make a translation of arbitrary C code straightforward. We'd be grateful for your thoughts on this. I think this is essentially what @joshtriplett and @eddyb already suggested in these comments on the RFC:
- https://github.com/rust-lang/rfcs/pull/2137#issuecomment-328621331
- https://github.com/rust-lang/rfcs/pull/2137#issuecomment-328885252
Per Larsen at 2019-03-15 19:50:56
@thedataking Drop isn't getting used here for va_end. If you want those intrinsics to be exposed, we may be able to do that...
Alexander Regueiro at 2019-03-15 21:41:02
However, this design makes syntax-directed translation from C really difficult

It does take a bit more care and thought to convert code, and I'm not entirely sure how to do this automatically like c2rust is designed to do. It will take a lot to convince me that the API should be changed because I think the current API promotes a relatively safer use of va_list.

On the other hand, exposing the va_copy and va_end intrinsics would make a translation of arbitrary C code straightforward.

Opinions changed as we worked on it (see this comment). Also, the intrinsics are not useful without a API change because we don't export the underlying implementation etc.

I'll post on the immunant issue and if there really is no other way, further discussion should be moved to the RFC PR as the people who worked on it would probably have some input.
Dan Robertson at 2019-03-15 21:41:08
Hi, guys. Sorry in advance, if it's the wrong place to ask. I'm currently trying to write a Rust function that accepts a vararg of C structs, but VaArgSafe trait, which is implemented only for a handful of types, doesn't allow me to do this. I believe that C language doesn't have such restrictions. Also, I've come across this comment, which raises a similar point.

Is it something that is going to be addressed in the future?
Oleh Palianytsia at 2019-05-27 20:48:20
@OlegTheCat Actually I believe the C standard does specify that this is undefined behaviour, though I don't have the relevant section at hand. (Incidentally I'm curious why f32 isn't supported.) I know @dlrobertson is busy currently, but hopefully he can clarify when he's back.
Alexander Regueiro at 2019-05-28 00:20:07
@alexreg

I'm curious why f32 isn't supported.

float is not allowed as stated in the C specification.

@OlegTheCat Once you get into arbitrary aggregate types things get much more complex. More testing and stabilization of the current implementation needs to be done to open VaList::arg up to more types. Are you working with an aggregate type or is there a primitive type that you're hitting this with?
Dan Robertson at 2019-05-28 01:50:24
I believe the C standard does specify that this is undefined behaviour

@alexreg I've just had a glance at C standard and couldn't find any info regarding struct types and UB. As far as I can see, the only thing that is UB in varargs is when the type of the passed value and the type passed to va_arg don't match. Maybe I'm looking into the wrong section.

I'm curious why f32 isn't supported. float is not allowed as stated in the C specification.

As far as I understand, the thing is that f32 is being promoted to a double (f64) when passed as a vararg. Therefore there's no actual possibility to retrieve an f32 value from a va_list because of the promotion.

@dlrobertson Yeah, I'm working with simple aggregate types and all of them have a #[repr(C)] tag. Something like this:
```
#[repr(C)]
struct Foo {
    x: i32
}

#[repr(C)]
struct Bar {
    foo: Foo
}


unsafe extern fn vararg_test(n: usize, mut args: ...) {
    ...
    args.arg::<Bar>(); //this cannot be compiled
    ...
}
```
Oleh Palianytsia at 2019-05-28 09:04:42
As far as I understand, the thing is that f32 is being promoted to a double (f64) when passed as a vararg. Therefore there's no actual possibility to retrieve an f32 value from a va_list because of the promotion.

This is what I understood too. In that case, there's no possibility of retrieving anything other than a C int, unsigned int, or double... so why the impls for the other types?
Alexander Regueiro at 2019-05-28 21:33:09
On May 28, 2019 2:34:10 PM PDT, Alexander Regueiro notifications@github.com wrote:

As far as I understand, the thing is that f32 is being promoted to a double (f64) when passed as a vararg. Therefore there's no actual possibility to retrieve an f32 value from a va_list because of the promotion.

This is what I understood too. In that case, there's no possibility of retrieving anything other than a C int, unsigned int, or double... so why the impls for the other types?

Precisely because those impls are supposed to handle the promotion behavior correctly.
Josh Triplett at 2019-05-29 06:09:21
Ah, I see, fair enough then.
Alexander Regueiro at 2019-05-29 16:52:05
One thing we need to not lose track of is: VaList shouldn't be leaking into the ty::FnSig of functions that don't pass VaList as a regular argument (but rather they're variadic and use VaList internally).

I think the way we should handle this is:
- AST: whatever matches the syntax best
  - e.g. ast::TyKind::CVarArg is probably fine
- HIR/MIR: mark the signature as C-variadic instead of having a type for the last argument
  - the argument list inside the body would be one longer than the signature, but this is fine
  - anything dealing with those arguments lists can detect this from the signature being C-variadic
  - rustc_{typeck,mir} can give that argument pattern the type VaList, as it's a lang item
  - codegen can special-case the argument (see also: the spread_arg field of mir::Body)
From the outside, a Rust fn definition that's C-variadic would look no different than:
```
extern {
    fn foo(a: A, b: B, c: C, ...);
}
```
Eduard-Mihai Burtescu at 2019-05-30 01:12:56
Is there any interest towards implementing this RFC for non-native extern ABIs? I'm primarily interested in "win64" ABIs, as that's what is used under UEFI (there are some platform APIs that have variadic args and they're required to implement certain driver functionality).
Ryszard Knop at 2019-06-14 11:39:16
@DragoonAethis Do you have a link to a spec or good implementation? I found some basic info, but nothing solid. This could be done. I don't think win64 is covered by LLVM, so I think we'd need to implement va_arg etc.
Dan Robertson at 2019-06-16 15:44:41
It's documented by Microsoft here, although quick googling also suggests LLVM doesn't support this. (I'm also not an expert, so I'd be happy to be wrong :)

It looks like varargs on Windows are just loaded as next arguments with some exceptions like "float args are always converted to doubles". There's a test suite from .NET Core available here that tries to check quite a few cases for correctness, so one could browse these to see if there are any special gotchas in there.
Ryszard Knop at 2019-06-16 16:33:14
with some exceptions like "float args are always converted to doubles"

Pretty sure that's enshrined in the C standard (the promotion happens in the call, before ABI).
Eduard-Mihai Burtescu at 2019-06-16 17:04:51
Pretty sure that's enshrined in the C standard (the promotion happens in the call, before ABI).

Yeah floats are not allowed, so that would have to happen before.
Dan Robertson at 2019-06-16 22:24:03
Variadic in C are maybe the definition of implemented behavior, well maybe bitfield are...

Relevant section of C11 (I don't have C17, I don't think it has changed):
6.5.2.2.6 If the expression that denotes the called function has a type that does not include a prototype, the integer promotions are performed on each argument, and arguments that have type float are promoted to double. These are called the default argument promotions. If the number of arguments does not equal the number of parameters, the behavior is undefined. If the function is defined with a type that includes a prototype, and either the prototype ends with an ellipsis (, ...) or the types of the arguments after promotion are not compatible with the types of the parameters, the behavior is undefined. If the function is defined with a type that does not include a prototype, and the types of the arguments after promotion are not compatible with those of the parameters after promotion, the behavior is undefined, except for the following cases:
- one promoted type is a signed integer type, the other promoted type is the corresponding unsigned integer type, and the value is representable in both types;
- both types are pointers to qualified or unqualified versions of a character type or void.
7 If the expression that denotes the called function has a type that does include a prototype, the arguments are implicitly converted, as if by assignment, to the types of the corresponding parameters, taking the type of each parameter to be the unqualified version of its declared type. The ellipsis notation in a function prototype declarator causes argument type conversion to stop after the last declared parameter. The default argument promotions are performed on trailing arguments.

8 No other conversions are performed implicitly; in particular, the number and types of arguments are not compared with those of the parameters in a function definition that does not include a function prototype declarator.
Have fun for the next one:
6.3.1.11 Every integer type has an integer conversion rank defined as follows:
- No two signed integer types shall have the same rank, even if they have the same representation.
- The rank of a signed integer type shall be greater than the rank of any signed integer type with less precision.
- The rank of long long int shall be greater than the rank of long int, which shall be greater than the rank of int, which shall be greater than the rank of short int, which shall be greater than the rank of signed char.
- The rank of any unsigned integer type shall equal the rank of the corresponding signed integer type, if any.
- The rank of any standard integer type shall be greater than the rank of any extended integer type with the same width.
- The rank of char shall equal the rank of signed char and unsigned char.
- The rank of _Bool shall be less than the rank of all other standard integer types.
- The rank of any enumerated type shall equal the rank of the compatible integer type (see 6.7.2.2).
- The rank of any extended signed integer type relative to another extended signed integer type with the same precision is implementation-defined, but still subject to the other rules for determining the integer conversion rank.
- For all integer types T1, T2, and T3, if T1 has greater rank than T2 and T2 has greater rank than T3, then T1 has greater rank than T3.
2 The following may be used in an expression wherever an int or unsigned int may be used:
- An object or expression with an integer type (other than int or unsigned int) whose integer conversion rank is less than or equal to the rank of int and unsigned int.
- A bit-field of type _Bool, int, signed int, or unsigned int.
If an int can represent all values of the original type (as restricted by the width, for a bit-field), the value is converted to an int; otherwise, it is converted to an unsigned int. These are called the integer promotions.58) All other types are unchanged by the integer promotions.

3 The integer promotions preserve value including sign. As discussed earlier, whether a ''plain'' char is treated as signed is implementation-defined.
The stdarg.h definition.

So, AFAIK, "the integer promotions are performed on each argument" say that a structure type is also rule by the integer promotion so a structure that only contain a char should be promote to int in size, enum too and union too. The only exception is floating number.

But, I never seen a (serious) code that push a structure in a variadic list. Also, technically, the user should not worry about all of that, the only thing to do is to always match type in va_arg call, the compiler will do the rest. I'm unsure rust can handle every C implementation about this.

Note: clang produce a warning for it: warning: passing object of class type 'struct foo' through variadic function [-Wclass-varargs] but its compile fine with -pedantic-errors and std=c11(gcc compile too) so I think it's a fair warning about strange code but standard allow it.
Stargateur at 2019-06-21 02:14:33

Just FYI. One problem I ran into is the rust macro cannot deal with "..." If I wanted to create a rust macro for hook

hook! {
    unsafe fn prinf(format: *const c_char, args:...) -> c_int => my_prrintf {
        if let Ok(path) = std::str::from_utf8(std::ffi::CStr::from_ptr(path).to_bytes()) {
            println!("printf(\"{}\")", format);
        } else {
            println!("printf(...)");
    }
}

The goal of the macro is to expand to a larger rust function that also takes c_variadic.

if hook macro was defined as follows, it cant capture "..." as a type and errors out.

unsafe fn $real_fn:ident ( $($v:ident : $t:ty),* ) -> $r:ty => $hook_fn:ident $body:block) => {
.....

Saravanan "Sarvi" Shanmugham at 2020-07-25 00:15:46

One more issue. C_variadics only works as functions not as methods. The following function works

#![feature(c_variadic)]
extern crate libc;
use libc::{c_char,c_int};
#[no_mangle]
pub unsafe extern "C" fn printf(_format: *const c_char, mut args: ...) -> c_int  {
    10
}

Done as a method, it fails

#![feature(c_variadic)]

extern crate libc;

// #[macro_use]
extern crate redhook;

use libc::{c_char,c_int};


#[allow(non_camel_case_types)]
pub struct printf {__private_field: ()}
#[allow(non_upper_case_globals)]
static printf: printf = printf {__private_field: ()};


impl printf {
    #[no_mangle]
    pub unsafe extern "C" fn printf(_format: *const c_char, mut args: ...) -> c_int  {
        10
    }
}

The non c_variadic version of the above code compiles fine

#![feature(c_variadic)]

extern crate libc;

// #[macro_use]
extern crate redhook;

use libc::{c_char,c_int};


#[allow(non_camel_case_types)]
pub struct printf {__private_field: ()}
#[allow(non_upper_case_globals)]
static printf: printf = printf {__private_field: ()};


impl printf {
    #[no_mangle]
    pub unsafe extern "C" fn printf(_format: *const c_char) -> c_int  {
        10
    }
}

Saravanan "Sarvi" Shanmugham at 2020-07-25 03:24:01

That's not a method, but an associated function. I do think associated functions should support this feature though.
jethrogb at 2020-07-25 16:53:58
That's not a method, but an associated function. I do think associated functions should support this feature though.

Yeah, associated functions do not support this feature at the moment. Associated functions were not in the original RFC, but I agree that they should be supported. Will post a PR in a sec.
Dan Robertson at 2020-07-25 19:52:37
On Sat, Jul 25, 2020 at 12:52:51PM -0700, Dan Robertson wrote:

That's not a method, but an associated function. I do think associated functions should support this feature though.

Yeah, associated functions do not support this feature at the moment. Associated functions were not in the original RFC, but I agree that they should be supported. Will post a PR in a sec.

Thanks!
Josh Triplett at 2020-07-25 22:41:03
Apart from #74765, what is the status of this work? Is this something we could move towards stabilization? Maybe someone would be interested in driving that? Nominating for @rust-lang/lang meeting.
Niko Matsakis at 2020-07-27 17:53:07
We discussed this in the @rust-lang/lang meeting today. We feel like this may be ready for a stabilization report and stabilization PR. Associated function support just went in, and it doesn't feel like it would be worth the trouble to split the feature gate for that, so we could instead wait one release for that to bake.
Josh Triplett at 2020-07-27 19:40:29
https://github.com/rust-lang/rust/pull/73655 adds aarch64 support, but I have not been able to test on many architectures. Are there certain architectures/OSes that we'd like to have tested better before stabilization?
Dan Robertson at 2020-07-27 20:32:33
@dlrobertson You might consider porting some of the more complex tests from libffi (testsuite/va_*.c) to Rust. Those test lots of corner cases. If we have those, and they pass on a new architecture, and that architecture's support goes in at the beginning of a new development cycle (right after beta branches), I wouldn't have any concerns.
Josh Triplett at 2020-07-29 06:42:34
By the way, I would like us to get in the habit of posting IntoRust blog posts if we'd like folks to experiment -- maybe somebody wants to write a IntoRust blog post highlighting that we plan to stabilize this feature and encouraging folks to tinker with it?
Niko Matsakis at 2020-07-29 15:59:06
Midway through implementation I talked at a local meetup about it. Would these slides be (at least partially) salvageable.
Dan Robertson at 2020-07-29 19:35:01
I, like probably many others that rely heavily on c, would really like to see this stabilized.

As a recommended next step, I started porting over some of libffi's more complex tests, but quickly ran into a limitation with #[repr(C)] struct's. As of right now, we're limited to only these types, even though C can pass structures. https://github.com/rust-lang/rust/blob/a3ed564c130ec3f19e933a9ea31faca5a717ce91/library/core/src/ffi.rs#L309

I have some availability to work on this, but am rather unfamiliar with contributing to Rust. So I thought I'd at least bring up the issue here for discussion/advice.
Kevin Nakamura at 2021-02-02 18:08:47
@Grinkers I'd like to see support for passing structs by value, but for now, could you port over the tests that don't depend on that functionality, or stub out the struct passing?
Josh Triplett at 2021-02-02 21:33:38
Sure, just to be clear, I assume you mean the va_*.c https://github.com/libffi/libffi/tree/master/testsuite/libffi.call which are mostly the same and testing structs. Without the structs it's mostly just testing various sized c types, which is still very good to test on various architectures, but it seems like we cover quite a few.

Is there any automated tests different architectures or is that work that needs to be done?
Kevin Nakamura at 2021-02-03 14:25:53
Currently it looks like ... is syntactically allowed anywhere in the parameter list. Is that intentional, or should it be restricted to the last parameter only?
Jonas Schievink at 2021-03-17 15:43:12
Ah I guess that ship has sailed, since this is accepted on stable:
```
extern {
    fn f(#[cfg(NEVER)] ..., named: u8, ...);
}
```
Jonas Schievink at 2021-03-17 15:44:48
@jonas-schievink: this seems like a bug according to the RFC.

This must appear as the last argument of the function, and the function must have at least one argument before it.

I don't imagine this behaviour is being relied upon, so we can probably fix it now.
varkor at 2021-03-20 16:26:31
I agree that is a bug, possibly even a regression, though who knows when it may have been introduced.
Niko Matsakis at 2021-03-22 14:22:27
Note that s390x hits an LLVM ERROR in the test for this feature -- #84628.
Josh Stone at 2021-04-27 20:24:42
Is anything blocking the stabilization of this? s390x is a tier-two target, so could we just emit a warning/error on LLVM versions (currently all of them) where https://github.com/rust-lang/rust/issues/84628 occurs?
Aaron Hill at 2021-09-29 17:03:13
s390x should fall through to LLVM's va_arg instruction, so there might be an issue there. I can't remember off the top of my head, but I think s390x uses the void pointer variant of a va_list, so I wonder if using our emit_ptr_va_arg like we do for x86 and windows would fix some of the issues seen on s390x. I haven't tested on s390x, so I wouldn't know for sure though.
Dan Robertson at 2021-09-29 18:28:34
I don't know these details well, but I can volunteer to test things on s390x hardware if needed.
Josh Stone at 2021-09-29 18:30:27
FWIW, here is clang's SystemZABIInfo::EmitVAArg:

https://github.com/llvm/llvm-project/blob/9ad17fe0debb0b48798aaba6d07b65136fcf0664/clang/lib/CodeGen/TargetInfo.cpp#L7467-L7468
Josh Stone at 2021-09-29 20:18:33
Thanks for the link! There is definitely something wrong with our implementation. We're emitting a void pointer like va_list but it looks like a complex structure is expected. We'll need to change the intrinsic type VaListImpl for the architecture. It would be interesting to see if just changing the structure for s390x fixes this or if we do indeed need to implement a custom va_arg as well.
Dan Robertson at 2021-09-29 20:45:00
Let's take the s390x discussion back to #84628.
Josh Stone at 2021-09-30 16:28:25
I see a potential use case for efiapi calling convention (screenshot from UEFI specification PDF document, version 2.9):

Example implementation:
```
pub type EFI_INSTALL_MULTIPLE_PROTOCOL_INTERFACES = extern "efiapi" fn(
    Handle: *mut VOID,
    ...
) -> EFI_STATUS;
```
Is this within of the scope of this RFC, or should a new RFC be opened to extend this feature (considering the RFC has already been implemented)?
HTGAzureX1212. at 2022-01-28 04:33:03
Isn't EFIAPI not just one of the existing Microsoft calling conventions?
jethrogb at 2022-01-28 09:25:38
Isn't EFIAPI not just one of the existing Microsoft calling conventions?

I think so.

Probably, but I was thinking about would extern "C" work here - as it stands, I am working with EFI, not C.......?

Please do correct and clarify if I am mistaken. Corrections are appreciated. 👍
HTGAzureX1212. at 2022-01-28 09:34:04
@jethrogb Not exactly. See also this issue with the uefi-rs library: https://github.com/rust-osdev/uefi-rs/issues/302

The basic idea is: for the GNU C/C++ compilers, you can choose a calling convention (extern "C" or just no extern), and you can also choose an ABI using a custom function attribute (ms_abi vs sysv_abi).

Rust doesn't have this distinction. There's no extern "C" with ms_abi or extern "C" with gnu_abi; because of this, we're kind of forced to either use extern "C" everywhere (and the ABI cannot be selected, it depends on whatever settings the Rust compiler has for that target; which is risky) or use extern "efiapi" (which will enforce the MS ABI, but doesn't currently work with variadics, as mentioned in the issue).
Gabriel Majeri at 2022-01-28 11:37:58
I am working with EFI, not C.......?

C standard don't state any ABI requirement. Rust extern "C" is "use the classic C ABI for this target used by most C compiler", it's not guarantee to work all the time.
Stargateur at 2022-01-28 21:19:15
Perhaps we should also allow sym::efi_api in check_variadic_type? extern efi_api didn't exist when I implemented it, which is the only reason I didn't add it in the original implementation. I don't immediately see a reason we shouldn't allow extern "efiapi". I could take a crack at an implementation attempt if this is a reasonably common use case. Issues that I think we'll hit:
1. I highly doubt we'd emit the correct structure for the VaListImpl. clang emits a void pointer va list, but I'm pretty sure for x86_64 and aarch64 we'd emit the structure used by both. I don't think this would be too hard to fix. We could just check target_os, which should be "uefi". We don't currently handle that properly, which is a bug.
2. clang uses emitVoidPointerVaArg in this case, so I'd assume we'd need to do the same and add a case to our llvm backend.
Questions:
- I'm not up to speed with extern "efiapi". Would there be a case where "efiapi" was used but target_os != "uefi"? We figure out the right va_list structure and code to emit based on the target_os and target_arch, so to get this to work, I think we'll need target_os == "uefi" any time we expect to use that ABI.
- Is there a test case for this? I have minimal uefi experience.
Dan Robertson at 2022-01-28 22:29:40
I could take a crack at an implementation attempt if this is a reasonably common use case.

I am not sure whether it would be a common use case for everything; but it would indeed be a huge demand for support in low-level development, specifically when working with the UEFI specification and even OSDev with Rust in general.

I'm not up to speed with extern "efiapi". Would there be a case where "efiapi" was used but target_os != "uefi".

Possibly, but as far as I am aware - you'll most likely end up with using target_os = "uefi" when using extern "efiapi". Feel free to correct me on this one.
HTGAzureX1212. at 2022-01-29 03:31:14
Would there be a case where "efiapi" was used but target_os != "uefi"? We figure out the right va_list structure and code to emit based on the target_os and target_arch, so to get this to work, I think we'll need target_os == "uefi" any time we expect to use that ABI.

Yes, unfortunately... Besides directly targeting UEFI and using the extern "efiapi" functions with the native MS ABI on UEFI targets, it's also possible that your Rust app is actually an OS kernel (targeting ELF with the GNU ABI for example), but you want to call some firmware functions which are declared with extern "efiapi" (MS ABI calling convention).

Proposed solution: I'd think it's enough to force "ms_abi" on all extern "efiapi" functions (since I'm pretty sure that's how that calling convention is defined by the spec - unlike regular extern "C" functions which could be ms_abi or gnu_abi dependening on the target OS) and add support for C variadics for them, just like for regular extern "C" functions.
Gabriel Majeri at 2022-01-29 06:43:09
:+1: I think this enough info to get started on a implementation. To start, inspecting the LLVM IR emitted by simple standalone examples should be enough, but to move from WIP to something merge-able, I'd like a real test case. For that I might need some pointers and/or some help. Is there a commonly used IRC channel or chat mechanism for the rust-osdev group? If not, I may move discussions about test cases to https://github.com/rust-osdev/uefi-rs/issues/302.
Dan Robertson at 2022-01-29 15:34:59
@dlrobertson There's a gitter but it's not so used anymore. I'd encourage you to leave a comment on that uefi-rs issue, and maybe @timrobertsdev (who originally encountered and reported this problem with Rust's implementation of variadics) could provide us with a minimal repro.
Gabriel Majeri at 2022-01-29 18:35:38
Did a bit of work on this and x86_64-unknown-uefi emits the correct LLVM now, but I've realized we hit issues with specific cases for extern "efiapi". We currently emit one VaListImpl as a language item in library/core/ffi.rs based on the OS and architecture. This causes issues for the following example:
```
#![no_std]
#![feature(c_variadic)]
#![feature(abi_efiapi)]

pub unsafe extern "efiapi" fn my_example_fn(int: u64, mut args: ...) -> u64 {
    if int > 0 {
        args.arg::<u64>()
    } else {
        0
    }
}
```
If you compile with rustc --target=x86_64-unknown-linux-gnu --crate-type=lib --emit=llvm-ir <file>, you can find something like the following
```
%"core::ffi::VaListImpl" = type { <va list structure here> }
...
define win64cc i64 ...
  ...
  ; this isn't right. inside this function the VaListImpl should be a void pointer
  %args = alloca %"core::ffi::VaListImpl", align 8
  %2 = bitcast %"core::ffi::VaListImpl"* %args to i8*
  call void @llvm.va_start(i8* %2)
  ...
  <emit_ptr_va_arg code here>
```
So currently defining new C variadic functions with extern "efiapi" doesn't really work when the target doesn't match. I think I have a solution though. The fix might not be the easiest, but I think what we can do is create a MsVaListImpl that is a type that always equals the void pointer variant. Then if the function is extern "efiapi" use that language item instead of the VaListImpl. The downside is that we're adding a language item and potential complexity. Any thoughts, questions, or input before I make an attempt at this?
Dan Robertson at 2022-02-02 13:28:39
Nominating this for the next @rust-lang/lang meeting, so that we can make a decision about what targets need to be supported before we stabilize this.

In the past, for things like i128/u128, we added and stabilized a mechanism without waiting for every last target to implement it. I think we need to do something similar here, rather than waiting for every target to completely implement .... We could, for instance, mark VaListImpl with cfg so that code using it on an unsupported target doesn't compile.

Separately, we also need to decide if we can forwards-compatibly handle variable-argument functions for different ABIs.
Josh Triplett at 2022-02-09 19:03:27
Nominating this for the next https://github.com/orgs/rust-lang/teams/lang meeting, so that we can make a decision about what targets need to be supported before we stabilize this.

:+1: Let me know if you could use any info before the meeting.

Separately, we also need to decide if we can forwards-compatibly handle variable-argument functions for different ABIs.

I think I'll post the much smaller pull request with just the fix for target_os = "uefi" so we can get that in ASAP.
Dan Robertson at 2022-02-09 20:02:52
@dlrobertson It'd help to have a rough summary of the current status. Which targets does it work completely on, without any holes as far as we know? Which targets does it work partially on, but still needs fixes?

Also, is there a sketch for how to handle multiple calling conventions on the same target, in a forwards-compatible way? We can stabilize this on a target without supporting any non-default calling conventions on the target, but we need to feel confident that we can expand to support non-default calling conventions in the future without breaking changes. If we need to change something before stabilization to leave ourselves room for evolution here, we should. (For instance, does VaList need to have an opaque template parameter for ABI?)
Josh Triplett at 2022-02-12 02:42:03
@joshtriplett

It'd help to have a rough summary of the current status. Which targets does it work completely on, without any holes as far as we know? Which targets does it work partially on, but still needs fixes?

AFAIK all tier 1 targets are supported for {i,u}{8,16,32,64,size} and f64. I have not tested targets beyond that enough to claim reasonable support, and as a result there are bugs to be found there. The targets I know there are issues with are target_os = "uefi" and target_arch = "s390x". FWIW a good way to test if we can support a target is to run the c-link-to-rust-va-list-fn test in run-make-fulldeps. If that test passes, I'm comfortable enough with our support of the target to maintain it.

Also, is there a sketch for how to handle multiple calling conventions on the same target, in a forwards-compatible way?

https://github.com/rust-lang/rust/issues/44930#issuecomment-1027941470 adding a language item that is always the pointer variant is the only way I can currently see us supporting the target + the MS ABI.

We can stabilize this on a target without supporting any non-default calling conventions on the target, but we need to feel confident that we can expand to support non-default calling conventions in the future without breaking changes.

:+1: I agree

If we need to change something before stabilization to leave ourselves room for evolution here, we should. (For instance, does VaList need to have an opaque template parameter for ABI?)

I'm not entirely sure I follow here. I don't know how we could get the appropriate layout in codegen from this, but I'm likely missing something. Since we really need two implementations of variadics for a given libcore build, I think the easiest would be to add another lang item, and based on the function ABI inject the correct type. That being said, I also would like to avoid adding another lang item if possible. So if using an opaque template parameter would work, I think it would be preferred.

Are there any other language items that have a variable layout depending on the ABI? Another implementation that could serve as a guide would be hugely helpful.
Dan Robertson at 2022-02-13 21:36:41
I agree with @joshtriplett that we should stabilize this on a per-target basis.
Niko Matsakis at 2022-02-14 14:40:03

I don't know if the compiler would be able to extract the calling convention from a type like this, but here's a way that at least lets you use the type system to specify a calling convention and only has one lang item.

#[lang_item]
pub struct VaList<T: CallingConvention = C> {
    convention: PhantomData<T>,
}

#[unstable(perma-unstable)]
pub trait CallingConvention {
    type F;
}

pub enum C {}

impl CallingConvention for C {
    type F = extern "C" fn();
}

pub enum Win64 {}

impl CallingConvention for Win64 {
    type F = extern "win64" fn();
}

pub enum Sysv64 {}

impl CallingConvention for Sysv64 {
    type F = extern "sysv64" fn();
}

jethrogb at 2022-02-14 15:58:17

Tagging as ready-to-stabilize based on previous lang discussion.

The author mentioned wanting to add some more tests. Assuming those tests pass, this is ready for stabilization; if any fail, this should move to impl-incomplete until they pass.
Josh Triplett at 2022-03-16 17:28:09
Just commenting to let everyone know that this hasn't been completed just yet
rayanmargham at 2022-06-24 21:10:41
Is there any movement on this? Last comment is from more than 12 months ago.
John Hughes at 2023-05-03 13:08:25
Is there any movement on this? Last comment is from more than 12 months ago.

I haven't had any free time to work on this. Is there a particular open issue that is part of stabilizing Rust defined C-variadics you're experiencing issues with?
Dan Robertson at 2023-05-05 15:02:31
I just tested my limited use case, (tier1 only; linux and windows), without issues.

https://github.com/rust-lang/rust/tree/master/tests/run-make/c-link-to-rust-va-list-fn Are there more tests that are required before moving to stabilization?
Kevin Nakamura at 2023-05-05 18:01:15
I just tested my limited use case, (tier1 only; linux and windows), without issues.

https://github.com/rust-lang/rust/tree/master/tests/run-make/c-link-to-rust-va-list-fn Are there more tests that are required before moving to stabilization?

I'd like to add support for defining functions with an EFI API within a x86_64, aarch64, or powerpc target, but not sure if this is needed to be completed before stabilization.
Dan Robertson at 2023-05-05 21:42:17
Bikeshed: the ellipsis in mut args: ... has no precedent anywhere else in the language. The syntax could be something more reminiscent of pattern matching, like mut args @ ...
Jules Bertholet at 2023-05-17 02:13:42
Bikeshed: the ellipsis in mut args: ... has no precedent anywhere else in the language. The syntax could be something more reminiscent of pattern matching, like mut args @ ...

I'm pretty sure that would be a bad thing, pattern matching is already a thing in function argument; you can deconstruct a tuple for example. And something like fn foo(args @ ..: &[u8]) { } is already illegal. The proposition to add a special type ... make more sense. vaarg is not a pattern it's a type.
Stargateur at 2023-05-17 02:31:00
pattern matching is already a thing in function argument

Exactly, so we should consider following that precedent.

vaarg is not a pattern it's a type.

When you are calling the variadic function, you provide a comma-separated set of values—which is exactly what .. matches in patterns.
Jules Bertholet at 2023-05-17 02:43:55
I'd like to add support for defining functions with an EFI API within a x86_64, aarch64, or powerpc target, but not sure if this is needed to be completed before stabilization.

@dlrobertson I may be misunderstanding you, but I think variadics already work fine with efiapi? Probably from https://github.com/rust-lang/rust/pull/97971. I've been using c_variadic with x86_64-unknown-uefi and i686-unknown-uefi without issue for some time.
Nicholas Bishop at 2023-05-17 16:03:44
@dlrobertson I may be misunderstanding you, but I think variadics already work fine with efiapi? Probably from #97971. I've been using c_variadic with x86_64-unknown-uefi and i686-unknown-uefi without issue for some time.

Great point! This isn't quite what I'm getting at though. See this rust example and note that the VaList structure used by the "efiapi" function is actually the ABI of the host (in this case amd64). AFAIK the ABI of the VaList in the example should be the void pointer variant. I'm not entirely sure how often such a case would exist in the wild.

Also see this comment (It's a little outdated, but I think there are parts of it that are still somewhat relevant).
Dan Robertson at 2023-05-17 16:49:57
I see, thanks for the clarification!
Nicholas Bishop at 2023-05-17 17:10:16
@dlrobertson I may be misunderstanding you, but I think variadics already work fine with efiapi? Probably from #97971. I've been using c_variadic with x86_64-unknown-uefi and i686-unknown-uefi without issue for some time.

Great point! This isn't quite what I'm getting at though. See this rust example and note that the VaList structure used by the "efiapi" function is actually the ABI of the host (in this case amd64). AFAIK the ABI of the VaList in the example should be the void pointer variant. I'm not entirely sure how often such a case would exist in the wild.

Also see this comment (It's a little outdated, but I think there are parts of it that are still somewhat relevant).

There was previous discussion about stabilization on a per-target basis. Would gating "efiapi" for now be sufficient (while still leaving room for the future)?

I feel it's a shame to have this stuck in limbo for something that may not have a use case. Especially when this is a blocker for rust adoption in many cases on tier 1 targets. Or, if this target issue is problematic, I think it would be nice to have this in the Steps on the first post.
Kevin Nakamura at 2023-05-17 21:56:58
Is there a way to make VaArgSafe visible in docs so it is easy to find allowed types? I am not sure if this is possible for sealed traits, but it would be nice if it was quick to find this information
Trevor Gross at 2023-09-06 03:47:38
Is there a way to make VaArgSafe visible in docs so it is easy to find allowed types? I am not sure if this is possible for sealed traits, but it would be nice if it was quick to find this information

Would it be reasonable add the documentation to the VaList. We currently only state:

A wrapper for a va_list.

Perhaps we should add more info about the types allowed.
Dan Robertson at 2023-09-06 10:40:55
Following up on the concern in https://github.com/rust-lang/rust/issues/44930#issuecomment-1551747344, I think that may not matter for stabilizing this because it's gated by a different feature: extended_varargs_abi_support.

To summarize my understanding of the concern, the problem occurs when you have an extern "efiapi" variadic function, and you compile for a target that uses a different variadic ABI (e.g. x86_64-unknown-linux-gnu). See https://github.com/rust-lang/rust/issues/44930#issuecomment-1024849683 for an explanation of when that would come up.

But that case cannot currently compile: error: only foreign or `unsafe extern "C"` functions may be C-variadic. There are a number of tests for this as well.

https://github.com/rust-lang/rust/issues/100189 tracks the extended_varargs_abi_support feature which would enable ABIs other than "C". And personally that is something I would like to see in the future, but I don't think it needs to block stabilization of c_variadic.

@dlrobertson in case I've misunderstood anything above :) @Soveu FYI since you have https://github.com/rust-lang/rust/pull/116161 open to stabilize extended_varargs_abi_support
Nicholas Bishop at 2023-11-04 20:13:05
Of course, now that I've typed that, I see I did miss something :) The example in https://github.com/rust-lang/rust/issues/44930#issuecomment-1027941470 won't compile because it uses extern "efiapi" with ..., but the rust-playground example in https://github.com/rust-lang/rust/issues/44930#issuecomment-1551747344 does compile because it uses VaList directly, and produces wrong code depending on the host target.
Nicholas Bishop at 2023-11-04 20:33:34
Of course, now that I've typed that, I see I did miss something :) The example in #44930 (comment) won't compile because it uses extern "efiapi" with ..., but the rust-playground example in #44930 (comment) does compile because it uses VaList directly, and produces wrong code depending on the host target.

Yeah, this is tricky to fix... Honestly, this case is probably trickier to fix than the c_variadic case. I'm uncertain how much of an edge case this is, so I don't know if this is a blocker or not.
Dan Robertson at 2023-11-06 23:57:12

Is there a way to "forward" the variadic arguments to another variadic function, supposing I know the size in bytes of the arguments?

Use-case: Implementing library interposition (aka LD_PRELOAD wrapper) around a C interface with variadic functions. For a (rough) example:

pub unsafe extern "C" fn printf(fmt: *const libc::c_char, mut args: ...) -> usize {
    let real_printf = dlsym(RTLD_NEXT, "printf");
    let size = /*
        compute size of args based on %-codes in fmt until reaching null byte
    */;
    real_printf(fmt, *args) /* How to forward args to real_printf? */
}

Sam Grayson at 2024-03-18 20:51:10

Is there a way to "forward" the variadic arguments to another variadic function, supposing I know the size in bytes of the arguments?

Use-case: Implementing library interposition (aka LD_PRELOAD wrapper) around a C interface with variadic functions. For a (rough) example:
```
pub unsafe extern "C" fn printf(fmt: *const libc::c_char, mut args: ...) -> usize {
    let real_printf = dlsym(RTLD_NEXT, "printf");
    let size = /*
        compute size of args based on %-codes in fmt until reaching null byte
    */;
    real_printf(fmt, *args) /* How to forward args to real_printf? */
}
```
I need to do this too. Between reading the RFC (which appears outdated since the current implementation now has a split between VaList and VaListImpl) and looking at the scant API docs, I came up with this example code which passes my basic tests: https://play.rust-lang.org/?version=nightly&mode=release&edition=2021&gist=a536485d2935d6138b6bd8425d5f43a0. However, it would be nice to have confirmation from someone who understands the intended semantics.
Per Vognsen at 2024-04-14 08:54:38
We're talking about slightly different things.
<table> <tr> <td> <td>Function we are defining <td>Function we are calling <tr> <td>My request <td>variadic (e.g., `printf_wrapper`) <td>variadic (e.g., `printf`) <tr> <td>Your request <td>variadic (e.g., `printf_wrapper`) <td>not variadic (e.g., `vprintf`) </table>
In GCC, forwarding variadic functions to a variadic function can be done using __builtin_apply. I don't think there is a Rust equivalent of this.
Sam Grayson at 2024-04-14 11:57:04
We're talking about slightly different things. Function we are defining Function we are calling My request variadic (e.g., printf_wrapper) variadic (e.g., printf) Your request variadic (e.g., printf_wrapper) not variadic (e.g., vprintf)

In GCC, forwarding variadic functions to a variadic function can be done using __builtin_apply. I don't think there is a Rust equivalent of this.

Ah. It doesn't even look like clang supports __builtin_apply, which does not make me hopeful there would be a 1:1 equivalent in LLVM. Since the '...' in 'args: ...' currently stands for a platform/ABI-specific opaque VaListImpl, you can get 'naive forwarding' to pass Rust's type checks and it misleadingly passes basic tests as long as you stay within the ABI argument limit that stays in registers, but it's revealed as incorrect when you exercise a longer argument list: https://play.rust-lang.org/?version=nightly&mode=release&edition=2021&gist=3f7e586b0d35e50626cf290097731d6a. I'm assuming you got at least this far in your own investigation; I'm just leaving this here in case anyone else comes across this thread.
Per Vognsen at 2024-04-14 13:58:18
Something to note in the unresolved questions:

libcore currently hard codes the layout of va_list that llvm uses for each individual target. This probably won't work with other backends. How should we handle the target and backend dependent VaListImpl layout?
bjorn3 at 2024-05-24 08:49:52
Bikeshed: the ellipsis in mut args: ... has no precedent anywhere else in the language. The syntax could be something more reminiscent of pattern matching, like mut args @ ...

untrue, it is already used when declaring extern variadic functions.

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=724901e4535cd7c5981de0eff52a978f
lolbinarycat at 2024-05-31 01:15:45
For declaring variadic functions the ... is not in the type position like with mut foo: ..., instead it is a special case only valid inside function declarations.
bjorn3 at 2024-05-31 10:05:31
presumably the syntax for defining variadic functions would also be a special case.

regardless, my point is that it does have precedent, and indeed is already associated with variadics.
lolbinarycat at 2024-05-31 21:51:04
Something to note in the unresolved questions:

libcore currently hard codes the layout of va_list that llvm uses for each individual target. This probably won't work with other backends. How should we handle the target and backend dependent VaListImpl layout?

@bjorn3 The layout of va_list is part of the C ABI for a target, so it doesn't vary by backend. For instance, on x86-64 Unix, it is defined to always be:
```
typedef struct {
    unsigned int gp_offset;
    unsigned int fp_offset;
    void *overflow_arg_area;
    void *reg_save_area;
} va_list[1];
```
in the ABI specification.
beetrees at 2024-06-01 06:17:24
For C-SKY the abi specification doesn't seem to dictate any specific layout for va_list. It suggests a possible implementation for va_start and va_arg in section 2.2.4, but does not say that this is the only valid implementation. And the Hexagon abi specification doesn't even mention va_list, va_start or va_arg. It only explains the calling convention in section 5.2.

You are right that many other abi specifications do explicitly mention the layout. I wasn't aware of this.

References: C-SKY: https://github.com/c-sky/csky-doc/blob/master/C-SKY_V2_CPU_Applications_Binary_Interface_Standards_Manual.pdf Hexagon: https://lists.llvm.org/pipermail/llvm-dev/attachments/20190916/21516a52/attachment-0001.pdf
bjorn3 at 2024-06-01 10:55:43
I see what you mean. In the cases of C-SKY and Hexagon, this seems to be a case of the ABI specification under-specifying; as va_list can be passed between C functions (such as when calling vprintf), compilers that can link to each others code (e.g. clang and gcc) must agree on the representation of va_list.
beetrees at 2024-06-01 11:15:01
On May 28, 2019 2:34:10 PM PDT, Alexander Regueiro @.***> wrote: > As far as I understand, the thing is that f32 is being promoted to a double (f64) when passed as a vararg. Therefore there's no actual possibility to retrieve an f32 value from a va_list because of the promotion. This is what I understood too. In that case, there's no possibility of retrieving anything other than a C int, unsigned int, or double... so why the impls for the other types? Precisely because those impls are supposed to handle the promotion behavior correctly.

I do not believe this should be done. The Rust compiler does not even correctly check passing varargs currently when generic types become involved, see https://github.com/rust-lang/rust/issues/61275 about that. When combined with accepting these types, this would make it very difficult to reason correctly about the type conversions involved, because we would be adding in our own special rules on top of the already Byzantine rules for C's variable arguments. This would make it much harder to correctly add support for passing e.g. structs over variadic arguments, down the line[^0]. I don't believe our current implementations for variadic arguments are adequately or thoroughly tested. We have had a bad habit of not testing many edge-cases that could potentially get around naive checks.

In other words, if you extract a different type from the va_list than the types that are passed into the variable arguments (barring some very specific equivalencies that are preserved between types of the same size), it's UB in C. It should be UB in Rust, too.

I have found way too many bugs in how we handle low-level compilation details, including ABI problems, to believe this is a good idea.

[^0]: This is partly "because we might accidentally introduce behaviors that conflict with other ABI handling details", and also partly because our energies for handling ABI correctly are extremely limited.
Jubilee at 2024-06-24 09:01:20
It also seems incoherent to provide a demotion behavior for all of those types and then refuse to demote f32, I would think, as there is an expected lossless translation from f32 to f64 for non-NAN bitpatterns and a lossy translation from f64 to f32 for non-NAN bitpatterns that provides the rounded inverse of that. Importantly, if someone passed an f32, they'd get the same f32 back (except NANs might get weird, but we can attempt to define the promotion/demotion behavior to preserve NAN bitpatterns in the case where there's no optimizations).

Or f16, for that matter.

I still think we shouldn't do this because it invites confusion and misunderstanding what the Rust compiler is doing (because people don't really understand what the C compilers do, either).
Jubilee at 2024-06-27 02:51:03
@programmerjake has informed me we cannot even rely anymore on things like "all C types will get promoted to at least a certain size" (even relative to the C ABI!) casting further doubt on that idea.
Jubilee at 2024-06-29 09:20:48
yes, afaict _BitInt(8) doesn't get promoted to int but instead gets passed as an 8-bit integer (verified using clang -emit-llvm), this also applies to FP types -- _Float32 doesn't get promoted to double (didn't check what clang does). just the older C types get promoted: e.g. signed char to int and float to double.
Jacob Lifshay at 2024-06-30 05:24:05
In other words, if you extract a different type from the va_list than the types that are passed into the variable arguments (barring some very specific equivalencies that are preserved between types of the same size), it's UB in C. It should be UB in Rust, too.

Definitely fully agreed. Whether an f32 gets promoted to something else or not should not be the programmer's concern. Especially if that is already how C works (if the argument has type f32, it needs to be extracted from the va_list at type f32), then we should do the same in Rust.

It follows that the entire discussion about what gets promoted is unnecessary. It's the backends responsibility to handle this correctly.
Ralf Jung at 2024-06-30 07:17:52
Whether an f32 gets promoted to something else or not should not be the programmer's concern.

I think Rust shouldn't do promotions for f32 since there are multiple corresponding C types: float and _Float32. float gets promoted but _Float32 does not, so I think the programmer just has to manually promote to f64 if that's what the function expects, Rust doesn't know wether float or _Float32 is what the API expects so can't do promotions for you.
Jacob Lifshay at 2024-06-30 07:32:39
As an example of the kind of "fun" one can have with this, see https://github.com/rust-lang/rust/issues/71915. The man page says the argument has type mode_t, which is unsigned short, but the actual function signature is a variadic so on the ABI level the argument is passed as int... and if you pass this as a mode_t from Rust you risk UB.
Ralf Jung at 2024-06-30 07:45:19
maybe a good solution would be to warn when passing or accepting types that are usually subject to promotion since _BitInt(N) and _Float32 are much rarer than char, short, and float. when you actually need to pass/accept _BitInt(8) or _Float32 you can use an attribute to silence the warning, maybe #[uncommon_c_types]:
```
unsafe extern "C" {
    unsafe fn f(c_int, ...);
}

pub g(v: f32) {
    unsafe {
        f(0, #[uncommon_c_types] v)
    }
}
```
Jacob Lifshay at 2024-06-30 07:59:03
why are you suggesting a special attribute for silencing a warning, we already have allow. such a warning does seem like a good idea though, mixing up u8 and char in signatures does seem like it could be problematic.
lolbinarycat at 2024-06-30 17:42:07
why are you suggesting a special attribute for silencing a warning, we already have allow.

because iirc a lot of people don't want rustc to emit warnings on code when it's the only valid way to implement something and you can't rewrite it to be warning-free.
Jacob Lifshay at 2024-06-30 18:10:55
because iirc a lot of people don't want rustc to emit warnings on code when it's the only valid way to implement something and you can't rewrite it to be warning-free.

this is still doing that, just providing an alias for allow(whatever) doesn't change that. i would argue such a warning should probably be part of clippy instead. also the attribute feels badly named, since it doesn't mention the important part, is which is that this has to do with variadics and promotion.
lolbinarycat at 2024-06-30 20:18:20
Definitely fully agreed. Whether an f32 gets promoted to something else or not should not be the programmer's concern. Especially if that is already how C works (if the argument has type f32, it needs to be extracted from the va_list at type f32), then we should do the same in Rust.

To be clear, @RalfJung, this is actually part of the issue: in C, you are expected to have memorized the arcane promotion rules, and thus the author of the variadic function has to specify va_arg to name exactly the type that it has been promoted to. To do otherwise is UB.

This is because the C promotion rules actually are not about the ABI. As demonstrated by _BitInt(8), it is legal, in some cases, to pass smaller-than-c_int args. Instead, it has to do with C source code semantics: all of these transitions are handled by clang's frontend, before it ever reaches LLVM!

So, I think that in Rust, we should not try to squirm out from under this. We should make apparent the bizarreness of the results.

The example you cite, in any case, actually would not compile if you had tried to make the change: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=26aa043833877711d0aba4e26abdebae
Jubilee at 2024-06-30 22:24:11
this is still doing that, just providing an alias for allow(whatever) doesn't change that.

I guess...

i would argue such a warning should probably be part of clippy instead.

I strongly think it should be in rustc because >99.9% of the time the warning is correct and C APIs are almost always documented as you just pass the smaller type, but rust needs the argument to be cast to the promoted type.

rustc already has lints that will be incorrect more often than this, e.g.: temporary_cstring_as_ptr

also the attribute feels badly named, since it doesn't mention the important part, is which is that this has to do with variadics and promotion.

yeah, picking a different name is fine with me.
Jacob Lifshay at 2024-06-30 23:39:39
in C, you are expected to have memorized the arcane promotion rules, and thus the author of the variadic function has to specify va_arg to name exactly the type that it has been promoted to. To do otherwise is UB.

Fun!

The example you cite, in any case, actually would not compile if you had tried to make the change: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=26aa043833877711d0aba4e26abdebae

Ah, neat. I had not seen that as I was just working on the other end -- implementing open in Miri. The error message itself is not very clear about why I need to do this though... Interestingly, the detailed explanation for that error then says

Certain Rust types must be cast before passing them to a variadic function, because of arcane ABI rules dictated by the C standard.

which seems to contradict a little your statement that this is not about ABI.

also the attribute feels badly named, since it doesn't mention the important part, is which is that this has to do with variadics and promotion.

Note that Rust also has a concept called "promotion", and it is entirely unrelated. So we need to be careful with how we use that term in documentation.
Ralf Jung at 2024-07-01 05:38:49
I don't think the expanded error message is correct, no.

It was written by someone who had far more SAN remaining than me.
Jubilee at 2024-07-01 09:25:35
The missing safety contracts on the documentation for VaList really irk me.
Balt at 2024-11-18 03:04:18
Then maybe make a PR that adds some? :) Otherwise, your comment isn't going to achieve anything I am afraid. We all know that the feature is quite incomplete.
Ralf Jung at 2024-11-18 07:49:09
On another note, there was some recent discussion of varargs on Zulip which should be helpful on writing a spec for the ABI compatibility requirements of this feature.
Ralf Jung at 2024-11-18 07:50:13

Defining "true" C variadic functions in Rust

Other work unrelated to my current work