Generate extern wrappers for inlined functions by pvdrz · Pull Request #2335 · rust-lang/rust-bindgen (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation78 Commits37 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

If bindgen finds a static or static inline function and the new --wrap-static-fns option is enabled, then:

It will generate a new c source files with functions that wrap the non-external functions.
Rename the link_name of the bindings generated for non-external functions so they link against the wrappers instead.

The following options were also added:

--wrap-static-fns-suffix=<suffix>: Adds <suffix> to the name of each wrapper function (__extern is used by default).
--wrap-static-fns-path=<path>: writes the generated c code at <path>.c or <path>.cpp accordingly (/tmp/bindgen/extern is used by default).

Given that the C code serialization is experimental and only supports a very limited set of C functions, we also added a new --experimental flag and "experimental" feature and all the three new options were put behind this feature.

Fixes #1090.

If you're interested in how to use this feature see: #2405

pvdrz marked this pull request as draft

November 4, 2022 20:55

pvdrz marked this pull request as ready for review

November 8, 2022 20:08

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

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

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

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

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

Question: Is it possible this same strategy can be extended to support (some) macros?

Question: Is it possible this same strategy can be extended to support (some) macros?

what do you mean by this? Like to process function-like macros in a certain way?

we might be able to reuse some parts of this for that (or at least some things in a conceptual sense). We can continue this discussion over here: #753

If bindgen finds an inlined function and the --generate-extern-functions options is enabled, then:

It will generate two new source and header files with external functions that wrap the inlined functions.
Rerun Bindings::generate using the new header file to include these wrappers in the generated bindings.

The following additional options were added:

--extern-function-suffix=<suffix>: Adds to the name of each external wrapper function (__extern is used by default).
--extern-functions-file-name=<name>: Uses as the file name for the header and source files (extern is used by default).
--extern-function-directory=<dir>: Creates the source and header files inside (/tmp/bindgen is used by default).

The C code serialization is experimental and only supports a very limited set of C functions.

Fixes rust-lang#1090.

test(static inlined): pretty print the diff

Issue: rust-langGH-1090

test(static inlined): refactor paths once again

Because directory structure is confusing to the author of this commit

rust-langGH-1090

test(static inlined): refactor test files

Expected files should be under tests/expectations/generated
Remove extern_stub.h because we can reuse the header from generate-extern-functions.h

test(static inlined): diff test; generated files

refactor(static inlined): integration test

Issue: rust-langGH-1090

rust-langGH-1090: Integration tests

integration test: document the GH issue

integration test (macro): same order in impl as the trait for consistency

integration test: move each setup into its own function, just to save face

inline static func integration test

resolve accidental conflict

Choose a reason for hiding this comment

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

So one thing that came up while looking at this... Is the second run even necessary? Can't we just have the suffix added needed on the first run, and rely on someone providing that symbol?

Might be overlooking something tho.

After playing with c constness I was able to achieve the following. If you pass this to bindgen:

typedef int (*OPENSSL_sk_cmp_func)(const void **a, const void **b); typedef int (*sk_void_cmp_func)(const void **, const void **);

static inline int sk_void_call_cmp_func(OPENSSL_sk_cmp_func cmp_func, const void *const *a, const void *const *b) { const void *a_ptr = (const void *)*a; const void *b_ptr = (const void *)*b; return ((sk_void_cmp_func)cmp_func)(&a_ptr, &b_ptr); }

it will emit this wrapper:

int sk_void_call_cmp_func__extern(OPENSSL_sk_cmp_func cmp_func, const void *const *a, const void *const *b) asm("sk_void_call_cmp_func__extern"); int sk_void_call_cmp_func__extern(OPENSSL_sk_cmp_func cmp_func, const void *const *a, const void *const *b) { return sk_void_call_cmp_func(cmp_func, a, b); }

Thanks, looks like that does resolve the const errors. I don't want to get too optimistic, but I think we're close:

/Users/alex_gaynor/projects/boringssl/build/rust/wrapper.c:17:48: error: this function declaration is not a prototype [-Werror,-Wstrict-prototypes]
struct stack_st_void * sk_void_new_null__extern() asm("sk_void_new_null__extern");
                                               ^
                                                void
/Users/alex_gaynor/projects/boringssl/build/rust/wrapper.c🔞24: error: no previous prototype for function 'sk_void_new_null__extern' [-Werror,-Wmissing-prototypes]
struct stack_st_void * sk_void_new_null__extern() { return sk_void_new_null(); }
                       ^
/Users/alex_gaynor/projects/boringssl/build/rust/wrapper.c:17:24: note: this declaration is not a prototype; add 'void' to make it a prototype for a zero-parameter function
struct stack_st_void * sk_void_new_null__extern() asm("sk_void_new_null__extern");
                       ^
                                                void
/Users/alex_gaynor/projects/boringssl/build/rust/wrapper.c🔞48: error: this old-style function definition is not preceded by a prototype [-Werror,-Wstrict-prototypes]
struct stack_st_void * sk_void_new_null__extern() { return sk_void_new_null(); }
                                               ^

I think the fix is that for 0-argument functions, the asm() line needs to make the signature be (void) instead of ().

It builds!!!!

I have to wire everything up and make sure it works, but thank you so much for your efforts here. I'm super excited.

I think the internet is mad at us and all the server request from github workflows are timing out 😅

I'm going to leave it like this for today and re-run CI tomorrow.

alex mentioned this pull request

Feb 2, 2023

alex added a commit to alex/rust-openssl that referenced this pull request

Feb 2, 2023

Just wanted to confirm that everything is now working end-to-end.

alex added a commit to alex/rust-openssl that referenced this pull request

Feb 3, 2023

alex added a commit to alex/rust-openssl that referenced this pull request

Feb 3, 2023

Choose a reason for hiding this comment

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

From a skim it looks good, let's try this. r=me with the nits below, please squash the commits before landing as needed. Thanks for working on this, it took a while but it's great!

pvdrz deleted the generate-inline-functions branch

February 7, 2023 15:13

ojeda mentioned this pull request

Feb 7, 2023

33 tasks

alex added a commit to alex/rust-openssl that referenced this pull request

Feb 7, 2023

one thing I notice is that the headers don't get included in the generated C file. Is it intended ?
the source should probably include the h file used during the bindgen command

one thing I notice is that the headers don't get included in the generated C file. Is it intended ? the source should probably include the h file used during the bindgen command

see #2405

one thing I notice is that the headers don't get included in the generated C file. Is it intended ? the source should probably include the h file used during the bindgen command

see #2405

the issue is that as i'm building for the DS which has a pretty specific toolchain, i have to use the cc crate to build the C file
atm i have this command, which succeeds if i modify the C file to include the header
cc::Build::new().file("src/arm7_bindings.c").compiler(format!("{}/devkitARM/bin/arm-none-eabi-gcc",dkp_path)).include(format!("{}/libnds/include",dkp_path)).include(format!("{}/devkitARM/lib/gcc/arm-none-eabi/12.2.0/include",dkp_path)).compile("bindings");
but i dunno if it's even possible to force the cc command to include a header for the c file

I'm guessing that .include("the_input_headers.h") should work

I'm not that familiar with cc but from the docs it seems something like .file("input.h") should work

Meziu mentioned this pull request

May 15, 2023

Labels

next-release

This issue/PR should be solved/merged before doing a new release