Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mishandle of variadic function with wrapped static functions #2498

Closed
Urgau opened this issue Apr 11, 2023 · 4 comments · Fixed by #2499
Closed

Mishandle of variadic function with wrapped static functions #2498

Urgau opened this issue Apr 11, 2023 · 4 comments · Fixed by #2499

Comments

@Urgau
Copy link
Member

Urgau commented Apr 11, 2023

Input C/C++ Header

static inline void foo(int flags, ...) {
    // ...
}

Bindgen Invocation

$ bindgen --experimental --wrap-static-fns input.h

Actual Results

extern "C" {
    #[link_name = "foo__extern"]
    pub fn foo(flags: ::std::os::raw::c_int, ...);
}

and

#include "input.h"

// Static wrappers

void foo__extern(int flags) { foo(flags); } // At least missing `...`

Expected Results

I expect bindgen to either handle correctly (what ever that would be) or to ignore those bind of functions.

cc @pvdrz

@pvdrz
Copy link
Contributor

pvdrz commented Apr 11, 2023

@Urgau I couldn't find a way to forward a call to a variadic function in C. For now I opened a PR that will skip any variadic functions that are also static if the --wrap-static-fns option is enabled.

@Urgau
Copy link
Member Author

Urgau commented Apr 11, 2023

I couldn't find a way to forward a call to a variadic function in C.

Neither could I.

However in my situation and I suppose many others, those variadic static inline are just helpers that call an underline va_list function. See this example:

int vlc_stream_vaControl(stream_t *s, int query, va_list args);

static inline int vlc_stream_Control(stream_t *s, int query, ...)
{
    va_list ap;
    int ret;

    va_start(ap, query);
    ret = vlc_stream_vaControl(s, query, ap);
    va_end(ap);
    return ret;
}

If we could have a way to "generate" this intermediate code, maybe with a callback that takes the function name and returns an option of the va_list fn name to delegate to?

@pvdrz
Copy link
Contributor

pvdrz commented Apr 11, 2023

That sounds like something that ParseCallbacks could certainly do but I wonder how common is this operation considering that you could append that code to the C file generated by bindgen.

@Urgau
Copy link
Member Author

Urgau commented Apr 12, 2023

That sounds like something that ParseCallbacks could certainly do but I wonder how common is this operation considering that you could append that code to the C file generated by bindgen.

Well, one could append to the C file but than would also mean appending to the Rust file, right?
Both seems non ideal.

Another example (see below) makes me think that it's a pretty standard thing (for public API, internal APIs generally don't bother that much), but than some adds arguments (here stdout) which complicates things.

int printf(const char *restrict fmt, ...)
{
	int ret;
	va_list ap;
	va_start(ap, fmt);
	ret = vfprintf(stdout, fmt, ap);
	va_end(ap);
	return ret;
}

This makes me thinks that we could take the problem the other way around, I mean start with the va_list variant and just generate the basic stub. This would mean that we would miss some of them but this would mean that bindgen won't need to add a callback and that the function is indeed available.

(Note that currently the generated va_list functions are currently useless since they cannot be called from Rust)

So what I'm proposing is to do the following transformation:

Input:

int vfprintf(FILE *stream, const char *format, va_list ap);

Output:

int vfprintf__extern(FILE *stream, const char *format, ...)
{
    int ret;
    va_list ap;

    va_start(ap, format);
    ret = vfprintf(stream, format, ap);
    va_end(ap);
    return ret;
}
extern "C" {
    #[link_name = "vfprintf__extern"]
    pub fn vfprintf(stream: *mut FILE, format: *const ::std::os::raw::c_char, ...) -> ::std::os::raw::c_int;
}

Would this be acceptable for bindgen to do? (If yes, I could do the implementation work)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants