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

Wrap va_list function as variadic function #2502

Merged
merged 6 commits into from
Jun 5, 2023

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented Apr 12, 2023

Opening a draft before doing more to get opinion on it.

This PR add a way to wrap va_list function as variadic function. See below for an example.

Input:

#include <stdarg.h>

static inline int vfoo(int flags, va_list va) {
    // ...
    return flags;
}

Output:

int vfoo__extern(int flags, ...) {
int ret;
va_list ap;

va_start(ap, flags);
ret = vfoo(flags, ap);
va_end(ap);
return ret;
}
extern "C" {
    #[link_name = "vfoo__extern"]
    pub fn vfoo(flags: ::std::os::raw::c_int, ...) -> ::std::os::raw::c_int;
}

Related to #2498

cc @pvdrz

@Urgau Urgau marked this pull request as draft April 12, 2023 15:12
@Urgau Urgau force-pushed the wrapped-fn-va_list branch from a5f3535 to fb2f0b9 Compare April 12, 2023 15:13
@bors-servo
Copy link

☔ The latest upstream changes (presumably 4f9fcbf) made this pull request unmergeable. Please resolve the merge conflicts.

@Urgau
Copy link
Member Author

Urgau commented Apr 14, 2023

@pvdrz Would you be able to give me pre-review?

@pvdrz
Copy link
Contributor

pvdrz commented Apr 17, 2023

I think it would be best if this is not done for every single function that takes valist as a last argument but instead let it be configurable using ParseCallbacks:

trait ParseCallbacks {
    ...
    fn wrap_as_variadic_fn(&mut self, name: &str) -> Option<String> {
        None
    }
}

That would mean that the user can decide which functions should be wrapped and would be their responsibility to check the signature instead of us.

For example, if callbacks.wrap_as_variadic_fn("vfoo") returns Some("foo"), we emit the following code:

int foo(int flags, ...) {
int ret;
va_list ap;

va_start(ap, flags);
ret = vfoo(flags, ap);
va_end(ap);
return ret;
}
extern "C" {
    pub fn foo(flags: ::std::os::raw::c_int, ...) -> ::std::os::raw::c_int;
}

What do you think?

@Urgau
Copy link
Member Author

Urgau commented Apr 17, 2023

I think it would be best if this is not done for every single function that takes valist as a last argument but instead let it be configurable using ParseCallbacks:

[...]

What do you think?

Sure, this seems like a good idea. I will do the necessary work and make the PR ready for review.

Btw, my logic works with any function that has 1 (and only 1) va_list no matter his position. (I went trough great length to ensure that)

@Urgau Urgau force-pushed the wrapped-fn-va_list branch 3 times, most recently from 97a4292 to c9f17d7 Compare April 18, 2023 13:57
@Urgau Urgau marked this pull request as ready for review April 18, 2023 14:00
@Urgau Urgau changed the title Draft: Wrapped va_list functions into variadic one Wrap va_list function as variadic function Apr 18, 2023
@Urgau
Copy link
Member Author

Urgau commented Apr 18, 2023

@pvdrz As discussed I've added ParseCallbacks::wrap_as_variadic_fn. I also did some cleanup and improvement to the added code. Should be ready to review.

Btw, I ended up only changing the name of the Rust function because changing the other one felt unnecessary complicated and also it's the only one that matters (the other one are implementation details).

@Urgau
Copy link
Member Author

Urgau commented Apr 25, 2023

@pvdrz Let me know if there is something I can do to help with the review ;-)

@pvdrz
Copy link
Contributor

pvdrz commented Apr 25, 2023

@pvdrz Let me know if there is something I can do to help with the review ;-)

Hey :) I have this on my todo list but I only will be able to review it later this week. Thanks for the patience

@Urgau Urgau force-pushed the wrapped-fn-va_list branch 2 times, most recently from 439bc76 to fcae20e Compare May 2, 2023 10:08
@Urgau
Copy link
Member Author

Urgau commented May 9, 2023

@pvdrz The PR is ready for review.

(btw, do you want pings from me?)

Copy link
Contributor

@pvdrz pvdrz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. The only two extra things I'd ask you to do are:

  • Check how wrap-static-fns is tested in bindgen-integration and update the tests to include this new feature.
  • Add the changes to the changelog.

@Urgau Urgau force-pushed the wrapped-fn-va_list branch from 1a24bf3 to 38de00e Compare May 25, 2023 08:26
@Urgau
Copy link
Member Author

Urgau commented May 25, 2023

@pvdrz I've updated as requested the PR (bindgen-integration + changelog).

@pvdrz
Copy link
Contributor

pvdrz commented Jun 3, 2023

Hey! the test suite broke due to an unrelated issue that has already been fixed in the main branch, could you rebase your PR against it so we can be sure the tests are passing?

@Urgau Urgau force-pushed the wrapped-fn-va_list branch 2 times, most recently from 3501fe9 to 8e8ebdc Compare June 3, 2023 08:56
@Urgau
Copy link
Member Author

Urgau commented Jun 3, 2023

I've rebased, the tests suite now passes (had to add an exception for stdarg.h which I don't think is an issue, it's avalaible since C89 and even a little before) ;-)

@bors-servo
Copy link

☔ The latest upstream changes (presumably 4faa366) made this pull request unmergeable. Please resolve the merge conflicts.

@Urgau Urgau force-pushed the wrapped-fn-va_list branch from 8e8ebdc to 40ddf17 Compare June 5, 2023 20:08
@pvdrz pvdrz merged commit bfc5b7f into rust-lang:main Jun 5, 2023
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 this pull request may close these issues.

3 participants