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

aturon commented 6 years ago

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

joshtriplett commented 3 years ago

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

Grinkers commented 3 years ago

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?

jonas-schievink commented 3 years ago

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 commented 3 years ago

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

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

varkor commented 3 years ago

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

nikomatsakis commented 3 years ago

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

cuviper commented 3 years ago

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

Aaron1011 commented 2 years ago

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?

dlrobertson commented 2 years ago

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.

cuviper commented 2 years ago

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

cuviper commented 2 years ago

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

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

dlrobertson commented 2 years ago

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.

cuviper commented 2 years ago

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

HTGAzureX1212 commented 2 years ago

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

jethrogb commented 2 years ago

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

HTGAzureX1212 commented 2 years ago

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

GabrielMajeri commented 2 years ago

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

Stargateur commented 2 years ago

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.

dlrobertson commented 2 years ago

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.

HTGAzureX1212 commented 2 years ago

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.

GabrielMajeri commented 2 years ago

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.

dlrobertson commented 2 years ago

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

GabrielMajeri commented 2 years ago

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

dlrobertson commented 2 years ago

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?

joshtriplett commented 2 years ago

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.

dlrobertson commented 2 years ago

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.

joshtriplett commented 2 years ago

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

dlrobertson commented 2 years ago

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

nikomatsakis commented 2 years ago

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

jethrogb commented 2 years ago

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

joshtriplett commented 2 years ago

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.

rayanmargham commented 2 years ago

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

jondo2010 commented 1 year ago

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

dlrobertson commented 1 year 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?

Grinkers commented 1 year ago

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?

dlrobertson commented 1 year ago

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.

Jules-Bertholet commented 1 year ago

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

Stargateur commented 1 year ago

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.

Jules-Bertholet commented 1 year ago

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.

nicholasbishop commented 1 year ago

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.

dlrobertson commented 1 year ago

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

nicholasbishop commented 1 year ago

I see, thanks for the clarification!

Grinkers commented 1 year ago

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

tgross35 commented 11 months ago

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

dlrobertson commented 11 months ago

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.

nicholasbishop commented 9 months ago

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

nicholasbishop commented 9 months ago

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.

dlrobertson commented 9 months ago

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.

charmoniumQ commented 5 months ago

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

pervognsen commented 4 months ago

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.

rust-lang / rust

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