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

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

Steps:

Implement the RFC (PR #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.

I'd like to work on this, I already have some prototype

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.

@plietar How goes the implementation? I remember you showing a mostly complete prototype on IRC.

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

Hey,
Sorry I've been busy and then forgot about this. I'll get my prototype back in shape, hopefully by this weekend.

@plietar Any update on this? Do you have a WIP branch I can check out to try / fiddle with this?

Looks like I'm a little late to the party 😄 ... sorry about that

A few questions:

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

@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.

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.

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.

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

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.

@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.

Am I wrong or variadic functions could be used to replace some macros like println / vec etc.?

@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.

@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.

@alexreg
@dlrobertson
Thanks for pointing this out. True I was more looking for Rust native variadic support.
Sorry for the noise

@dlrobertson Do you have any update on this? If someone had the time, would it be possible to provide assistance in any way?

@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 😄

Defining "true" C variadic functions in Rust

Update parsing to allow for implementing C variadic functions
Spoof the VaList argument for Rust defined C variadic functions
Inject va_start
Inject va_end
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: #56475
Attempt to implement VaList::arg in pure Rust: #56489 (comment) also see va_list-rs
Cleanup the cfgs in src/libcore/ffi.rs with the use of cfg_if. Also see: #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!

Posted a WIP PR of my current work. Comments and feedback would be appreciated.

\o/ #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?

@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 immunant/c2rust#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:

@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...

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.

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?

@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.

@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?

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
    ...
}

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?

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.

Ah, I see, fair enough then.

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, ...);
}

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

@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.

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.

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

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.

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.

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) => {
.....

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

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

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.

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!

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.

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.

#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?

@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.

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?

Midway through implementation I talked at a local meetup about it. Would these slides be (at least partially) salvageable.

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.

rust/library/core/src/ffi.rs

Line 309 in a3ed564

impl_va_arg_safe! {i8, i16, i32, i64, usize}

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.

@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?

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?

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?

Ah I guess that ship has sailed, since this is accepted on stable:

extern {
    fn f(#[cfg(NEVER)] ..., named: u8, ...);
}

@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.

I agree that is a bug, possibly even a regression, though who knows when it may have been introduced.

Note that s390x hits an LLVM ERROR in the test for this feature -- #84628.

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 #84628 occurs?

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.

I don't know these details well, but I can volunteer to test things on s390x hardware if needed.

FWIW, here is clang's SystemZABIInfo::EmitVAArg:

https://github.com/llvm/llvm-project/blob/9ad17fe0debb0b48798aaba6d07b65136fcf0664/clang/lib/CodeGen/TargetInfo.cpp#L7467-L7468

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.

Let's take the s390x discussion back to #84628.

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

Isn't EFIAPI not just one of the existing Microsoft calling conventions?

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. 👍

@jethrogb Not exactly. See also this issue with the uefi-rs library: rust-osdev/uefi-rs#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).

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.

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:

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

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.

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.

👍 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 rust-osdev/uefi-rs#302.

@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.

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?

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.

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.

👍 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.

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

@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?

#44930 (comment) 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.

👍 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.

I agree with @joshtriplett that we should stabilize this on a per-target basis.

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();
}

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.

Just commenting to let everyone know that this hasn't been completed just yet

Is there any movement on this? Last comment is from more than 12 months ago.

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?

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

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 @ ...

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.

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.

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 #97971. I've been using c_variadic with x86_64-unknown-uefi and i686-unknown-uefi without issue for some time.

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

I see, thanks for the clarification!

@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.

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

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.

Following up on the concern in #44930 (comment), 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 #44930 (comment) 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.

#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 #116161 open to stabilize extended_varargs_abi_support

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.

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.

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? */
}

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.