Support va_copy with a member of a struct pointer

[Hakuyume]

This PR add support for va_copy which argument is a member of a struct point.

Example C code.

#include <stdarg.h>

struct S {
  va_list args;
};

void foo(va_list args) {
  struct S *s;
  va_copy(s->args, args);
}

Emitted Rust code.

#![allow(dead_code, mutable_transmutes, non_camel_case_types, non_snake_case, non_upper_case_globals, unused_assignments, unused_mut)]
#![register_tool(c2rust)]
#![feature(register_tool)]
pub type __builtin_va_list = [__va_list_tag; 1];
#[derive(Copy, Clone)]
#[repr(C)]
pub struct __va_list_tag {
    pub gp_offset: libc::c_uint,
    pub fp_offset: libc::c_uint,
    pub overflow_arg_area: *mut libc::c_void,
    pub reg_save_area: *mut libc::c_void,
}
pub type va_list = __builtin_va_list;
#[derive()]
#[repr(C)]
pub struct S<'a> {
    pub args: ::core::ffi::VaListImpl::<'a>,
}
#[no_mangle]
pub unsafe extern "C" fn foo(mut args: ::core::ffi::VaList) {
    let mut s: *mut S = 0 as *mut S;
    (*s).args = args.clone();
}

[kkysen]

Hi @Hakuyume! Thanks for the help!

I haven't looked at this part of the transpiler before, so could you tell me what the previous behavior was (an error, mis-transpilation, etc.)? Also, what exactly was the motivating use case for this? The example you gave,

void foo(va_list args) {
  struct S *s;
  va_copy(s->args, args);
}

dereferences an uninitialized pointer. I assume that's just an overly simplified example, but if you could give some more context on what you need this for, that would be great!

That said, I'm not sure why the transpiler only works on only pointers and struct members and not just general expressions. Adding more support would be helpful, but ideally, it'd just work on any expression.

[Hakuyume]

so could you tell me what the previous behavior was (an error, mis-transpilation, etc.)?

c2rust ignores (skips) the function that contains va_copy with a member of struct pointer. In the example above, the function foo is entirely skipped.

I assume that's just an overly simplified example, but if you could give some more context on what you need this for, that would be great!

Yes. As you mentioned, my example is too simplified.
Actually, I would like to transpile graphviz.
graphviz contains va_copy with such arguments.
https://gitlab.com/graphviz/graphviz/-/blob/5.0.0/lib/sfio/sftable.c#L321

I think my patch is ad-hoc. If there is more general way, it would be nice.

[kkysen]

Thanks for the further context! I'll look into it and see if there's a reason it's not more generic. @thedataking, any thoughts, as it says you wrote most of the match_va* code in #92?

[kkysen]

@thedataking, why not do something like this?

Define

// Or maybe call this `__builtin_va_copy`.
fn va_copy(dest: &mut VaListImpl, src: &VaListImpl) {
    dest.clone_from(src); // or `dest = src.clone();`
}

and then transform

va_copy(e1, e2)

into

va_copy(&mut transpile(e1), &transpile(e2));

Why is the specific matching on arguments necessary? Also, I think a mis-translation of invalid C code into similarly invalid Rust code is better than entirely skipping a function, which makes no sense.

[thedataking]

It has been more than three years since I worked on #92 so I'm fuzzy on some of the details. As far as I remember, the challenge is that some uses of va_copy would require (arbitrarily) complex static analysis. I think Andrei and I discovered this while looking at some uses in Apache apr but it might have been something else.

Also, I think a mis-translation of invalid C code into similarly invalid Rust code is better than entirely skipping a function, which makes no sense.

The overall operation of the transpiler is that it either translates the input into Rust that (modulo bugs) is functionally equivalent to the input or bails out with an error. Not doing so could introduce some very subtle and hard to find bugs.

A few other notes:

• I don't see tests for this feature, this PR should test the new code paths. test_varargs.rs might be a good place to do so.
• Handling of variadic functions vary from 32-bit code and 64-bit code and between different architectures. Be very careful.

[kkysen]

Thanks for context, @thedataking! In that case, @Hakuyume, I think an ad-hoc fix like this is okay if we can't easily get a general solution. Please add a test in tests/items/src/test_varargs.rs and tests/items/src/varargs.c that tests this va_copy with a pointer in a struct case, simplified from the graphviz source but not containing any UB. Then it will likely be good to merge after I review it more carefully.

[kkysen]

See the comment; just add a va_copy in the test and then the test should be all good.

[kkysen]

It's not testing va_copy. That might be handled similarly to va_start and va_end, but I think it should still be tested as well. Can you add that? Might as well add the same to the similar valist_struct_member function above.

[Hakuyume]

@kkysen Thank you for reviewing. I updated test cases. Some of CIs failed but the failure does not seem related to my patch.

[kkysen]

I'm just going to rebase and rename that suggestion commit, but otherwise, it's all good and I'll merge this once CI passes. Thank you!

[kkysen]

The CI failure is due to some other flaky tests that we're trying to debug. I re-ran the CI and it should be fine.