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

Eliminate the need for call_unsafe_wdf_function_binding via helper lib #51

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DrChat
Copy link
Member

@DrChat DrChat commented Dec 17, 2023

This supercedes #50 if you decide to go for this one.

Context: All Wdf* functions are simply inline stubs that look up the corresponding function from WdfFunctions and call it (with an additional WdfDriverGlobals parameter).

Normally, bindgen will not generate bindings to these functions because they are marked as inline (and thus have no implementation that Rust can link to).
However, we can create a helper object via a C compiler that implements these functions (by hackily disabling the FORCEINLINE macro 😉) and then use that as the link target for the Rust declarations.

All said and done, this allows us to avoid having to generate code via macro to call the Wdf* functions.
This makes for a much nicer end-user experience.

@wmmc88 wmmc88 self-requested a review December 19, 2023 01:05
@wmmc88
Copy link
Collaborator

wmmc88 commented Dec 19, 2023

Interesting PR. We are actually planning on using the "cc in build.rs" approach for solving some issues with several things in WDF relying on global variables being declared in headers. These variables wont generate something we can link against, unless they are included in a .c file and compiled. An example of this is WdfMinimumVersionRequired. Right now, windows-drivers-rs works around this by manually redefining some of the symbols in rust.

However, I'm wary about doing that for all the inlined WDF functions. The way it's done now with the macro, it mirrors exactly how the calling the function in C/C++ would behave. The change in this PR would add an extra layer of indirection by making a call to the compiled helper lib, which would not be possible for the compiler to optimize away. The bindgen docs specifically call out this perf issue:

Note that these functions and methods are usually marked inline for a reason: they tend to be hot. The above workaround makes them an out-of-line call, which might not provide acceptable performance.

@wmmc88
Copy link
Collaborator

wmmc88 commented Dec 19, 2023

It also seems that bindgen now has built-in support to doing something like this automatically via wrap_static_fns. According to this discussion, it seems that LTO may resolve the extra overhead I mentioned above. But extra care is also needed to make sure a change like this doesn't break cross-compilation (which I am fixing currently in #35 )

@DrChat
Copy link
Member Author

DrChat commented Dec 19, 2023

We are actually planning on using the "cc in build.rs" approach for solving some issues with several things in WDF relying on global variables being declared in headers. These variables wont generate something we can link against, unless they are included in a .c file and compiled. An example of this is WdfMinimumVersionRequired. Right now, windows-drivers-rs works around this by manually redefining some of the symbols in rust.

Ah neat - I had actually accidentally encountered this fix as well but disabled it because my assumption was that you wanted it the other way around :)

However, I'm wary about doing that for all the inlined WDF functions. The way it's done now with the macro, it mirrors exactly how the calling the function in C/C++ would behave. The change in this PR would add an extra layer of indirection by making a call to the compiled helper lib, which would not be possible for the compiler to optimize away. The bindgen docs specifically call out this perf issue:

Indeed, I'm aware of the extra layer of indirection. I had hoped that this wouldn't be a concern for you, given that:

  1. I had assumed that perf wouldn't be a huge concern at this (what I assume is an) early stage of development.
  2. I can't imagine this indirection is at all significant compared to the dynamic dispatch via WdfFunctions.
  3. As you mentioned, Rust has the LTO stage available to it that could probably mitigate this concern. I'm not sure about the wrap_static_fns feature you mentioned above - the WDF functions are not static so I don't think this will apply?

Lots of assumptions there :)
While that isn't too ideal, IMO, the tradeoff for perf here is absolutely worth it given the improved ergonomics: rust-analyzer now has full analysis available to it for WDF functions, the function calls in user code are 1:1 with actual functions, etc.

Additionally, this performance tradeoff doesn't have to be permanent.
This PR is mostly intended to be a stepping stone - ideally these wrapper functions would be fully implemented in Rust (with #[inline(always)]) and otherwise automatically generated bindgen-style.

I was going to generate these wrappers at first, but saw that it would duplicate a lot of functionality from bindgen or otherwise wouldn't play nice.
Maybe once wdkmetadata is in a better state?

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.

2 participants