-
Notifications
You must be signed in to change notification settings - Fork 700
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
Add dynamic loading support #1846
Conversation
5c5c155
to
d71c94d
Compare
I tested the dynamic loading support with one dll file on windows and it worked like a charm! Exactly what I wanted. Question: Instead of "MyLibrary" as in your example I used "ModelLib", but "ModelLib" is marked as unknown although the program is compiled without errors. Thanks for any hint. Program extract of the automatically generated code: extern crate libloading;
pub struct ModelLib<'a> {
#[doc = ""]
createInstance: libloading::Symbol<
'a,
unsafe extern "C" fn(lengthBillet_m: f64, radiusBillet_m: f64) -> *mut ModelData,
>,
destroyInstance:
libloading::Symbol<'a, unsafe extern "C" fn(data: *mut ModelData) -> ::std::os::raw::c_int>,
calculate:
libloading::Symbol<'a, unsafe extern "C" fn(data: *mut ModelData) -> ::std::os::raw::c_int>,
enableLogging: libloading::Symbol<
'a,
unsafe extern "C" fn(
data: *mut ModelData,
path: *const ::std::os::raw::c_char,
) -> ::std::os::raw::c_int,
>,
errorLog: libloading::Symbol<
'a,
unsafe extern "C" fn(msg: *mut ::std::os::raw::c_char, maxSize: ::std::os::raw::c_int),
>,
errorLogSize: libloading::Symbol<'a, unsafe extern "C" fn() -> ::std::os::raw::c_int>,
customer: libloading::Symbol<
'a,
unsafe extern "C" fn(msg: *mut ::std::os::raw::c_char, maxSize: ::std::os::raw::c_int),
>,
date: libloading::Symbol<
'a,
unsafe extern "C" fn(msg: *mut ::std::os::raw::c_char, maxSize: ::std::os::raw::c_int),
>,
version: libloading::Symbol<
'a,
unsafe extern "C" fn(msg: *mut ::std::os::raw::c_char, maxSize: ::std::os::raw::c_int),
>,
}
impl<'a> ModelLib<'a> {
pub fn new(lib: &libloading::Library) -> ModelLib {
unsafe {
ModelLib {
createInstance: lib.get("createInstance".as_bytes()).unwrap(),
destroyInstance: lib.get("destroyInstance".as_bytes()).unwrap(),
calculate: lib.get("calculate".as_bytes()).unwrap(),
enableLogging: lib.get("enableLogging".as_bytes()).unwrap(),
errorLog: lib.get("errorLog".as_bytes()).unwrap(),
errorLogSize: lib.get("errorLogSize".as_bytes()).unwrap(),
customer: lib.get("customer".as_bytes()).unwrap(),
date: lib.get("date".as_bytes()).unwrap(),
version: lib.get("version".as_bytes()).unwrap(),
}
}
}
} |
@dergroncki, good to know that it worked for you! If the program compiles just fine, then the 'not found' error you're seeing is probably because your IDE is unable to process the On another, rather more general note -- I'm planning on spending some time today to get this PR into a production-ready state. At the moment, it's all rather ad-hoc and will occasionally generate invalid bindings for C++ headers. Happy to see that you have found it useful though. 😄 |
a277ea4
to
56c5b64
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 😃 A few comments/questions
src/lib.rs
Outdated
dynamic_loading: bool, | ||
|
||
/// The name of the dynamic library if we are generating bindings for a shared library. | ||
dynamic_library_name: Option<String>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to merge those two options in the commands and the builder? As in have a dynamic_loading
argument taking a name
value? Or something similar. That way it would be easier to use and implement maybe 😃 If the value is optional that would be even better to default to the library name in that case. Otherwise we can check if the name string is empty or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's a much better idea. I'll do this 😄
src/ir/context.rs
Outdated
// Finally, only generate dynamic bindings for functions (for now! this could | ||
// change in the future to support variables). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, I had not considered dynamically loading variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, AFAIK static constants should be dynamically-loadable too, but I haven't thought about this too much. 👍
This looks nice! I'm trying to use it with a two-crate setup, one -sys and one Rusty wrapper. Unfortunately that doesn't work right now because the struct members are private. Should they just all be declared |
Yes, you're absolutely right, they should be. This is a trivial fix, will update in just a moment. My bad! 🙂 |
Hi, Maintainer of struct SomeName<'a> {
sym1: Symbol<...>,
sym2: Symbol<...>,
}
impl SomeName<'a> {
unsafe extern "C" fn sym1(&self, ...) { /* call self.sym1 */ }
unsafe extern "C" fn sym2(&self, ...) { /* call self.sym2 */ }
} Doing it this way is somewhat safer, and more future proof as breaking changes to libloading or bindgen most likely would have no effect on the API most people would use. Another thing that needs to be triple-checked are the globals (i.e. what conventionally translates to |
@nagisa thanks for your excellent suggestions! I will update the patch accordingly. 😄 |
@Michael-F-Bryan would it be okay to adapt some of your ideas for this patch? Specifically, we're interested in doing what @nagisa has suggested above, which is very similar to your work. Just wanted to check with you! 😄 |
Many thanks for all the work that's going into getting dynamic support. I've already had to write loads of dynamic bindings manually and it's painful. @joechrisellis if you're passing the library name to So modifying the work from @Michael-F-Bryan : impl Bindings {
pub unsafe fn load_from_path<P>(
path: Option<P>,
) -> Result<Self, ::libloading::Error>
where
P: AsRef<::std::ffi::OsStr>,
{
let path = path.unwrap_or("LibraryY"); // something like this...
let library = ::libloading::Library::new(path)?;
...
}
} My dream would be that when I'm creating a So for dynamic you'd get: struct SomeName<'a> {
sym1: Symbol<...>,
sym2: Symbol<...>,
}
impl SomeName<'a> {
unsafe extern "C" fn sym1(&self, ...) { /* call self.sym1 */ }
unsafe extern "C" fn sym2(&self, ...) { /* call self.sym2 */ }
} and for static you'd get: struct SomeName;
impl SomeName<'a> {
unsafe extern "C" fn sym1(&self, ...) { /* call static bindings */ }
unsafe extern "C" fn sym2(&self, ...) { /* call static bindings */ }
} Without this, the Rust wrappers for sys crates are going to need some laborious boilerplate abstraction over the top to support both dynamic and static bindings. I'm not suggesting we modify the default bindgen behaviour, but it would be nice to support this kind of usage. |
Hi @timfish!
I am a little skeptical of overloading the However, I do like the idea of perhaps passing a path at build-time in
This is a great idea, although I do think it's probably out-of-scope for this ticket. There might also be some further challenges with doing this. I think that'd work fine for C bindings, but for C++ bindings where we generate structs for classes, we would probably have to find some workaround. Appreciate your comments, too! Glad to see that people are finding this useful. |
Yeah, for sure! The best thing that could happen is someone reuses ideas/code from my prototype and integrates them upstream 🙂 |
a48a137
to
9cb0ea4
Compare
One thing I thought about: some dynamic libraries do not implement all of the functions of their corresponding API (I saw this for the PKCS 11 Crypto API for example). |
Would it be because more parenthesis are needed 😋? As in: In any case, if making fields public is not a problem we could have both things:
I would prefer returning a |
Probably! 🤦♂️ 🤣 But yes, my point was that the methods make the interface a bit simpler to consume! |
Besides ergonomics, exposing the bindings as methods is required for safety. If the fields were public, it'd be too easy for people to extract the underlying function pointer and accidentally use it after the library is unloaded from memory. That's one of the big reasons See nagisa/rust_libloading#13 (comment) from the |
Agree that having I think also that what proposed @goto-bus-stop might be a good idea:
For each function
Then the assumption for callers would be to do something like: can_call_toto()?;
toto(); Or directly call
Maybe we could address that by having the implementation of the can_call::toto()?;
toto(); edit: hmm forgot that those are methods on a struct so the namespacing trick will probably not work 🤦 |
@hug-dev, keep in mind that That's why the wrapper's constructor (as currently formulated) would dereference a Safety is maintained by hiding function pointers behind a method call and making sure the wrapper retains ownership of the
You can't make the fields public. If they are public, it's possible to use the fact that
This approach seems to be the most ergonomic. That way you can look-before-you-leap if you know a symbol will always be present, but also perform the check if you need to. I'm guessing the To help with namespacing, what about adding a helper struct so you can write In the unlikely event that a library has a |
@Michael-F-Bryan has made some excellent points above. I also like @hug-dev's idea of implementing a Thinking about the (probably) more common case where all of the symbols in the dynamic library are guaranteed to be defined, I also think it would be useful to have an option that allows us to fail early if a |
Thanks for the explanation, your points make sense and give arguments in favor of having extra helper methods to check the I like the idea of the helper struct containing all the I like the idea of the I would agree of adding this option but not sure of what should be the default. If the default is failing late, then a |
3a6a1d5
to
51e3702
Compare
@emilio hello! I think this is ready for review now -- I think we can add the |
(sorry for the lag here, I've been a bit too busy lately :/) I'll try to get to this this week. Thanks for the patience! |
Hi I am using this (great work btw! 👍 ) and it does work however I have 2 questions/problems.
|
@DominicD thanks for the feedback! To answer your questions:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally got some time to go through this. Really sorry again for the lag.
I think I might be missing something about why you chose this approach rather than something simpler hanging off the existing code generation machinery? Off-hand it seems this patch just takes a whole separate code generation step, while it could (I think) be part of the current machinery quite easily, unless I'm missing something.
src/ir/context.rs
Outdated
.filter(|&(_, item)| { | ||
// If the user has not chosen to do dynamic loading, then we have nothing to | ||
// do. | ||
if !self.options().dynamic_loading { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this check should really go at the beginning of the function rather than being done for every item.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quite right -- will fix.
src/ir/context.rs
Outdated
// If there is a whitelist and this function is not on it, don't add it. | ||
if !self.options().whitelisted_functions.is_empty() && | ||
!self.options().whitelisted_functions.matches(&name) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect this to be accounted for via the regular whitelisting mechanism, why is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I think this can be simplified. I think I had the wrong idea of the relationship between items
, codegen_items
, and whitelisted_items
. I've changed this to iterate through self.items()
, but filter out anything whose item ID is not inside codegen_items
. It seems to do the job -- is that what you had in mind?
src/ir/context.rs
Outdated
// If there is a blacklist and this function is on it, don't add it. | ||
if !self.options().blacklisted_functions.is_empty() && | ||
self.options.blacklisted_functions.matches(&name) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same about this.
src/ir/context.rs
Outdated
|
||
// We don't want to include the root module in the list of items to generate | ||
// dynamic bindings for. | ||
if item.id() == self.root_module { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're only generating functions so this check is redundant, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, great spot.
src/lib.rs
Outdated
mut self, | ||
dynamic_library_name: T, | ||
) -> Self { | ||
self.options.dynamic_loading = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you really need to keep track of self.options.dynamic_loading
? It seems you really just need a library name, so the dynamic_loading
checks could just be self.options.dynamic_library_name.is_some()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed this.
src/codegen/mod.rs
Outdated
@@ -3948,11 +3959,45 @@ pub(crate) fn codegen( | |||
&(), | |||
); | |||
|
|||
// If the set of items to generate dynamic bindings for is nonempty... | |||
if !context.dyngen_items().is_empty() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So basically... This is just effectively generating all the functions right now, is that right?
The way I'd have approached this would be something like, maybe:
- Add an extra field to
CodegenResult
(dynamic_functions
? / whatever) - Handle this in
CodeGenerator for Function
(just something likeif ctx.options().dynamic_loading { /* generate appropriate things */ }
, and instead of pushing to the regularCodegenResult
field, push it to the dynamic functions, which you'd then handle here.
Is there any reason such an approach wouldn't work? It'd handle a lot more edge cases than this (template functions, etc, which this patch doesn't handle), and should also be much simpler / not need extra ItemSet
s.
Co-authored-by: Michael-F-Bryan <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the lag, didn't notice this was updated :(. Feel free to ping next time as needed, I get way too much github mail...
This looks basically good to me, thank you! I have some relatively minor nits, but I think with those addressed this looks good to me.
Given we need to do a breaking release anyhow for f221479, if this is updated soonish we can get it into 0.56.
Or alternatively, if you don't have the time, let me know and I can fix up the review comments myself.
The other great thing to do would be to have a documentation section in the book about this, how to use it and so on.
I don't think I want to block on that but I think it'd be great to add if any of you have the cycles.
Thanks again and sorry for the lag.
@@ -4356,6 +4403,35 @@ mod utils { | |||
args | |||
} | |||
|
|||
pub fn fnsig_argument_identifiers( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you document what this returns? The signature is not super-self-descriptive.
@@ -3948,11 +3978,28 @@ pub(crate) fn codegen( | |||
&(), | |||
); | |||
|
|||
if context.options().dynamic_library_name.is_some() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: if let Some(ref lib_name) = context.options().dynamic_library_name {
.
Then use that instead of unwrapping around.
"Check", | ||
context.options().dynamic_library_name.as_ref().unwrap(), | ||
] | ||
.join(""), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just format!("Check{}", lib_name)
?
} | ||
} | ||
|
||
pub fn add_function( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just push
in order to match the result function name?
init_fields: Vec<proc_macro2::TokenStream>, | ||
} | ||
|
||
impl Default for DynamicItems { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Derive this?
ret: proc_macro2::TokenStream, | ||
ret_ty: proc_macro2::TokenStream, | ||
) { | ||
assert_eq!(args.len(), args_identifiers.len()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note, this seems to assert on varargs parameters.
Not sure if those are not supported widely and this is expected? At least things seem to work when I remove the assert. The following snippet will trigger the assert.
int thing(int, ...);
I pushed this with my review comments addressed. Will send a follow-up for the var-args issue mentioned by @jsimmons above. |
Thanks for all your work @joechrisellis (and everyone else who contributed and helped flesh out issues!) |
Nice!! 🥳 🥳 🥳 |
#1931 addresses the varargs functions (requires a bit of an API change but I think it's fine and the result is simpler). |
Hi guys,
Following on from issue 1541, here's an implementation for libloading support in bindgen. I'd still consider this a prototype at the minute, so any comments are welcome and appreciated.
Implementation
--dynamic-loading
and--dynamic-library-name
-- which enable dynamic loading mode and specify a name for the shared library respectively. The 'name' of the shared library is what you want the struct containing the bindings to be called. For example, you could call itBZip2
-- then you'd instantiate the dynamically loaded library withlet lib = BZip2::new();
. If--dynamic-library-name
is not specified it'll just default toLib
.Example
Given the following
header.h
:And the following code inside our
build.rs
:We generate the following bindings:
Which we can then use in our code with the following:
Multiple Dynamically Loaded Libraries
If you want to use multiple dynamically loaded libraries, the correct approach would be to do this in your
build.rs
:Then do the following in your code:
(the
mod libx { ... }
is to prevent namespace collisions ifLibraryX
andLibraryY
have common includes -- e.g. if they both have#include <stdint.h>
)Comments and criticisms absolutely welcome! If this seems like a suitable approach I can update the docs in a separate PR. Cheers! 🍻