Translation of C's `va_copy` into Rust's `std::ffi::VaList::copy` about c2rust

[joshtriplett]

It seems entirely reasonable to want to iterate over two va_list values in parallel. That wasn't a use case anticipated in the original safe VaList::copy implementation, but it does seem entirely acceptable to do so.

The easiest safe solution would be a copy2 that takes a mutable reference and gives the containing closure two mutable references (including the one passed in). On the other hand, that seems like a niche use case to define a function for. (And I'd want to know that there aren't mor va_copy use cases that we haven't anticipated yet that this wouldn't address.)

Another alternative: we could expose a raw copy function as an unsafe function.

I'm currently looking over the RFC thread to remember why we ended on a closure-taking function rather than a reference-returning function (e.g. VaList::copy taking &self and returning a &mut with the same lifetime so that it can't outlive the VaList).

[dlrobertson]

If we added VaList::raw_copy and VaList::raw_end then we wouldn't have to expose the raw intrinsic which would mean users wouln't need to write ugly cfgs like this and we wouldn't need to export the VaListImpl. We'd have to make sure documentation for the functions was clear that VaLists created with raw_copy must be destroyed with raw_end and other VaLists must not use raw_end. Alternatively, raw_copy could return a wrapper type like VaListCopy that implements raw_end so that a "normal" VaList can't call raw_end

[ahomescu]

Sample code in the Playground here: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=1deb2470984d60fe6ecf4ea10dac17da

pub unsafe extern "C" fn bar(mut ap1: VaList) {
    ap1.copy(|mut ap2| {
        println!("{}=={}", ap1.arg::<u32>(), ap2.arg::<u32>()); // Doesn't compile
    });
}

In this snippet, the closure body cannot call ap1.arg() since the latter mutably borrows ap1, which is already immutably borrowed by ap1.copy().

[dlrobertson]

ap1.arg::<u32>() can be called before the closure with the closure capturing the result.

pub unsafe extern "C" fn bar(mut ap1: VaList) {
    let x = ap1.arg::<u32>();
    ap1.copy(|mut ap2| {
        let y = ap2.arg::<u32>();
        println!("{}=={}", x, y);
    });
}

[dlrobertson]

Would a valid workaround be something like the following:

1. examine the closure and find all the times VaList::arg is called on the copied list.
2. create a variable for each of these
3. replace the calls with the variables

Note that VaList::copy uses an immutable reference, so you can use VaList::copy on the copied VaList in the closure.

[thedataking]

In general, it is not possible to know how many times VaList::arg will be called in a function at translation time, i.e., a call to VaList::arg can be guarded by a variable whose value is only known at run time.

[ahomescu]

I can also see problems in cases where ap1.arg() isn't called directly from the current function, but from some of its callees, e.g., if the current function passes ap1 to vprintf. If the callees have other side-effects, it might not be sound to move them.

[thedataking]

Another alternative: we could expose a raw copy function as an unsafe function.

I can't speak to all use cases here, but in the C translation case, an unsafe copy function that returns a new va_list that have to be va_end'ed manually would work well. Since our translation is driven by the original C code, i.e., we'd translate calls to va_copy and va_end more or less one to one.

[joshtriplett]

@dlrobertson raw_copy and raw_end sound good to me.

Remind me why we couldn't just have copy return a VaList with a lifetime that was a subset of the copied VaList's lifetime?

[dlrobertson]

Remind me why we couldn't just have copy return a VaList with a lifetime that was a subset of the copied VaList's lifetime?

The user would have to use va_end right? Note: The interface was already defined by the time I started working on VaList, so I may not be the best person to ask.

[joshtriplett]

@dlrobertson No, the Drop implementation would call va_end.

[dlrobertson]

No, the Drop implementation would call va_end.

1. I wonder if adding Drop to VaList could complicate the current implementation of C-variadic functions since we "spoof" one in the function signature.
2. I wonder if the current VaList::copy should be be renamed VaList::with_copy.
3. Is there a timeline when this work should be done by (do I need to suspend work on other issues and do this now)?

[thedataking]

I wonder if the current VaList::copy should be renamed VaList::with_copy.

Sounds like a good idea to me.

Is there a timeline when this work should be done by

We are certainly not in a position to make demands on your time. That said, we are very interested in this change since it is the one remaining feature that prevents us from translating large C99 projects. Let us know if we can help.

[dlrobertson]

@joshtriplett

I'm currently looking over the RFC thread to remember why we ended on a closure-taking function rather than a reference-returning function (e.g. VaList::copy taking &self and returning a &mut with the same lifetime so that it can't outlive the VaList).

I remembered a reason why this wasn't done. You'd be returning a reference to data owned by the copy function. For pointer variants, I think you'd be okay, but for structure variants you have to alloc the structure.

[joshtriplett]

On Wed, Mar 20, 2019 at 05:50:41PM +0000, Dan Robertson wrote: > I'm currently looking over the RFC thread to remember why we ended on a closure-taking function rather than a reference-returning function (e.g. VaList::copy taking &self and returning a &mut with the same lifetime so that it can't outlive the VaList). I remembered a reason why this wasn't done. You'd be returning a reference to data owned by the `copy` function. For pointer variants, I think you'd be okay, but for structure variants you have to alloc the structure.

It could still be a VaList<'a> though, right? (An owned structure that nonetheless has a lifetime?)

[joshtriplett]

On Wed, Mar 20, 2019 at 09:00:58AM -0700, Dan Robertson wrote: > No, the Drop implementation would call va_end. 1) I wonder if adding `Drop` to `VaList` could complicate the current implementation of C-variadic functions since we "spoof" one in the function signature.

I don't know. I'll happily leave the approach and solution entirely up to you.

2) I wonder if the current `VaList::copy` should be be renamed `VaList::with_copy`.

That's a good idea, I like that.

3) Is there a timeline when this work should be done by (do I need to suspend work on other issues and do this now)?

In my opinion, this is less important than having VaList support completely working, merged, and tested on all platforms. Once that's done, this would be good to have for full C compatibility as part of the FFI WG.

[dlrobertson]

It could still be a VaList<'a> though, right? (An owned structure that nonetheless has a lifetime?)

I don't think so. For the pointer variants it is fine, but for architectures like Aarch64 and x86_64 the reference contained by the VaList structure would point to a value on the stack of copy.

[ahomescu]

Is there a timeline when this work should be done by (do I need to suspend work on other issues and do this now)?

@dlrobertson If you're busy with other things, we could implement this ourselves (@thedataking & myself) once we all agree on what the API is. This would also help accelerate testing with C2Rust for this feature, since we'd be doing it concurrently with development.

[dlrobertson]

1. I wonder if adding Drop to VaList could complicate the current implementation of C-variadic functions since we "spoof" one in the function signature.

I ran some tests and implementing Drop for VaList doesn't cause any issues with "pure" C-variadic functions.

we could implement this ourselves

@ahomescu @thedataking awesome! I'm on the rustc discussion threads at dlrobertson. Ping me and I have some thoughts on experiments that could be run.

[joshtriplett]

On March 22, 2019 7:39:01 AM PDT, Dan Robertson ***@***.***> wrote: > 1) I wonder if adding `Drop` to `VaList` could complicate the current implementation of C-variadic functions since we "spoof" one in the function signature. I ran some tests and implementing `Drop` for `VaList` doesn't cause any issues with "pure" C-variadic functions.

Awesome!

[ahomescu]

I haven't read the old discussions in their entirety so I'm not aware if this has already been discussed: on all of LLVM's current in-tree architectures, va_end is a no-op, which would explain why the extra Drop hasn't caused any issues.

[dlrobertson]

all of LLVM's current in-tree architectures, va_end is a no-op

Yes

which would explain why the extra Drop hasn't caused any issues.

Sorry, by "hasn't caused any issues" I also mean "generates the correct LLVM IR". With the Drop implementation for VaList + some other changes you still only end up with one call to va_end.

[thedataking]

Closing since rust-lang/rust#59625 was merged.