Repository: rust-lang/rust-bindgen
Pull request #3101 (2025-01-28 04:53:14)

Use link_name for dynamic library loading
# Problems
Dynamic library codegen generates code trying to load symbols using the name exposed in bindings instead of the overriden link name or mangled name.
[Note that libloading::Library::get does not mangle the name for you](https://docs.rs/libloading/latest/libloading/struct.Library.html#method.get)

1. When generating bindings for a dynamically loaded library (behaviour available by calling `dynamic_library_name` on the builder), the symbol names used in the generated `__library.get("XXX")` calls aren't the mangled link name as one might expect, but instead the bindings name. This behaviour by chance works with C-style exports, but is incorrect with mangled symbols (e.g. C++ exports) or when a user overrides the name of an item.
2. In addition, the link_name attribute is sometimes erroneously applied to methods in the generated struct, causing this warning:
    ```
    attribute should be applied to a foreign function or static
    this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
     ```

### Example
If my function in C is defined like this:
```C
COOLLIBRARY_API float CoolLib_coolFunction(void);
```
I might have something like this implemented on my Processor to lop off the prefix, since it feels redundant when the item is already namespaced by being a member of the generated dylib type.
```rust
    fn generated_name_override(
        &self,
        item_info: bindgen::callbacks::ItemInfo<'_>,
    ) -> Option<String> {
        if let ItemKind::Function = item_info.kind {
            let name = item_info.name.to_string();
            if name.starts_with("CoolLib_") {
                return Some(name[8..].to_string());
            }
        }
        None
    }
```

The line in the generated from_library will then look for the wrong symbol "coolFunction"
```rust
let coolFunction = __library.get(b"coolFunction");
```
when it should be this:
```rust
let coolFunction = __library.get(b"CoolLib_coolFunction");
```

When troubleshooting I messed around with `generated_link_name_override`, and noticed that it doesn't affect that generated code at all, and also reveals the second problem mentioned above about the link_name attribute -- the attribute is added if the symbol name would differ from the canonical one -- in this case because it's overriden.

Note that `generated_link_name_override` is, confusingly, passed the normal name (possibly overriden by the processor's `generated_name_override`), rather than the link name. So in my case even with the PR's changes I had to add the following to my Processor impl to re-add the prefix for to the symbol so that it can be found:
```rust
    fn generated_link_name_override(
        &self,
        item_info: bindgen::callbacks::ItemInfo<'_>,
    ) -> Option<String> {
        format!("CoolLib_{}", item_info.name.to_string()).into()
    }
```
I decided not to touch this last oddity in the PR in case I'm overlooking something, but would be happy to tack it on with go-ahead from a maintainer.

# Changes in the PR
- Fix problem 1: The symbol name used in the generated `__library.get` call will use the link_name for a function or var; i.e., the same as what is generated for link_name attributes. In order of precedence, this is:
    1. the overriden link name if it exists, or
    2. the mangled name if it exists, or
    3. the parsed name.
- Some dynamic_loading_xx expectations tests were also victim to problem 1, so they've been adjusted. Some integration tests for dynamic loading would be handy here.
- Fix problem 2: Methods in the generated library type's impl to call the inner dynamically-loaded functions will no longer be erroneously given the link_name attribute. 

I have tested and confirmed that the changes generate fully working bindings on my own windows and linux environments with a test dynamic library and binary that loads it.

Molot2032: @emilio 
@pvdrz
Requesting review 🙂
Molot2032: @emilio
Fixed merge conflicts with commit c704b14, which touched some of the same function signatures.

Patch:
diff --git a/Cargo.toml b/Cargo.toml
index 6978f49e..b0a5bbb0 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -58,7 +58,6 @@ cast_possible_wrap = "allow"
 cast_precision_loss = "allow"
 cast_sign_loss = "allow"
 default_trait_access = "allow"
-explicit_into_iter_loop = "allow"
 ignored_unit_patterns = "allow"
 implicit_hasher = "allow"
 items_after_statements = "allow"
@@ -79,7 +78,6 @@ used_underscore_binding = "allow"
 wildcard_imports = "allow"
 
 # TODO
-borrow_as_ptr = "allow"
 trivially_copy_pass_by_ref = "allow"
 unused_self = "allow"
 
diff --git a/bindgen/clang.rs b/bindgen/clang.rs
index 2440bc0a..9afc6e93 100644
--- a/bindgen/clang.rs
+++ b/bindgen/clang.rs
@@ -494,7 +494,7 @@ impl Cursor {
     where
         Visitor: FnMut(Cursor) -> CXChildVisitResult,
     {
-        let data = &mut visitor as *mut Visitor;
+        let data = ptr::addr_of_mut!(visitor);
         unsafe {
             clang_visitChildren(self.x, visit_children::<Visitor>, data.cast());
         }
diff --git a/bindgen/codegen/dyngen.rs b/bindgen/codegen/dyngen.rs
index 410cc0d6..76f38057 100644
--- a/bindgen/codegen/dyngen.rs
+++ b/bindgen/codegen/dyngen.rs
@@ -131,6 +131,7 @@ impl DynamicItems {
     pub(crate) fn push_func(
         &mut self,
         ident: &Ident,
+        symbol: &str,
         abi: ClangAbi,
         is_variadic: bool,
         is_required: bool,
@@ -181,11 +182,12 @@ impl DynamicItems {
         }
 
         // N.B: Unwrap the signature upon construction if it is required to be resolved.
-        let ident_str = codegen::helpers::ast_ty::cstr_expr(ident.to_string());
+        let symbol_cstr =
+            codegen::helpers::ast_ty::cstr_expr(symbol.to_string());
         let library_get = if ctx.options().wrap_unsafe_ops {
-            quote!(unsafe { __library.get(#ident_str) })
+            quote!(unsafe { __library.get(#symbol_cstr) })
         } else {
-            quote!(__library.get(#ident_str))
+            quote!(__library.get(#symbol_cstr))
         };
 
         self.constructor_inits.push(if is_required {
@@ -206,6 +208,7 @@ impl DynamicItems {
     pub fn push_var(
         &mut self,
         ident: &Ident,
+        symbol: &str,
         ty: &TokenStream,
         is_required: bool,
         wrap_unsafe_ops: bool,
@@ -231,12 +234,13 @@ impl DynamicItems {
             }
         });
 
-        let ident_str = codegen::helpers::ast_ty::cstr_expr(ident.to_string());
+        let symbol_cstr =
+            codegen::helpers::ast_ty::cstr_expr(symbol.to_string());
 
         let library_get = if wrap_unsafe_ops {
-            quote!(unsafe { __library.get::<*mut #ty>(#ident_str) })
+            quote!(unsafe { __library.get::<*mut #ty>(#symbol_cstr) })
         } else {
-            quote!(__library.get::<*mut #ty>(#ident_str))
+            quote!(__library.get::<*mut #ty>(#symbol_cstr))
         };
 
         let qmark = if is_required { quote!(?) } else { quote!() };
diff --git a/bindgen/codegen/mod.rs b/bindgen/codegen/mod.rs
index ccf63253..f58a2341 100644
--- a/bindgen/codegen/mod.rs
+++ b/bindgen/codegen/mod.rs
@@ -779,20 +779,20 @@ impl CodeGenerator for Var {
                 }
             }
         } else {
-            // If necessary, apply a `#[link_name]` attribute
-            if let Some(link_name) = self.link_name() {
-                attrs.push(attributes::link_name::<false>(link_name));
-            } else {
+            let symbol: &str = self.link_name().unwrap_or_else(|| {
                 let link_name =
                     self.mangled_name().unwrap_or_else(|| self.name());
-                if !utils::names_will_be_identical_after_mangling(
+                if utils::names_will_be_identical_after_mangling(
                     &canonical_name,
                     link_name,
                     None,
                 ) {
+                    canonical_name.as_str()
+                } else {
                     attrs.push(attributes::link_name::<false>(link_name));
+                    link_name
                 }
-            }
+            });
 
             let maybe_mut = if self.is_const() {
                 quote! {}
@@ -816,6 +816,7 @@ impl CodeGenerator for Var {
             if ctx.options().dynamic_library_name.is_some() {
                 result.dynamic_items().push_var(
                     &canonical_ident,
+                    symbol,
                     &self
                         .ty()
                         .to_rust_ty_or_opaque(ctx, &())
@@ -4639,21 +4640,19 @@ impl CodeGenerator for Function {
             write!(&mut canonical_name, "{times_seen}").unwrap();
         }
 
-        let mut has_link_name_attr = false;
-        if let Some(link_name) = self.link_name() {
-            attributes.push(attributes::link_name::<false>(link_name));
-            has_link_name_attr = true;
-        } else {
-            let link_name = mangled_name.unwrap_or(name);
-            if !is_dynamic_function &&
-                !utils::names_will_be_identical_after_mangling(
-                    &canonical_name,
-                    link_name,
-                    Some(abi),
-                )
-            {
+        let link_name_attr = self.link_name().or_else(|| {
+            let mangled_name = mangled_name.unwrap_or(name);
+            (!utils::names_will_be_identical_after_mangling(
+                &canonical_name,
+                mangled_name,
+                Some(abi),
+            ))
+            .then(|| mangled_name)
+        });
+
+        if let Some(link_name) = link_name_attr {
+            if !is_dynamic_function {
                 attributes.push(attributes::link_name::<false>(link_name));
-                has_link_name_attr = true;
             }
         }
 
@@ -4665,8 +4664,9 @@ impl CodeGenerator for Function {
                 quote! { #[link(wasm_import_module = #name)] }
             });
 
-        let should_wrap =
-            is_internal && ctx.options().wrap_static_fns && !has_link_name_attr;
+        let should_wrap = is_internal &&
+            ctx.options().wrap_static_fns &&
+            link_name_attr.is_none();
 
         if should_wrap {
             let name = canonical_name.clone() + ctx.wrap_static_fns_suffix();
@@ -4732,11 +4732,14 @@ impl CodeGenerator for Function {
 
         // If we're doing dynamic binding generation, add to the dynamic items.
         if is_dynamic_function {
+            let ident_str = ident.to_string();
+            let symbol = link_name_attr.unwrap_or(&ident_str);
             let args_identifiers =
                 utils::fnsig_argument_identifiers(ctx, signature);
             let ret_ty = utils::fnsig_return_ty(ctx, signature);
             result.dynamic_items().push_func(
                 &ident,
+                symbol,
                 abi,
                 signature.is_variadic(),
                 ctx.options().dynamic_link_require_all,
diff --git a/bindgen/lib.rs b/bindgen/lib.rs
index 94c6246e..0a8f29d1 100644
--- a/bindgen/lib.rs
+++ b/bindgen/lib.rs
@@ -835,7 +835,7 @@ impl Bindings {
             };
 
             if let Some(search_paths) = search_paths {
-                for path in search_paths.into_iter() {
+                for path in search_paths {
                     if let Ok(path) = path.into_os_string().into_string() {
                         options.clang_args.push("-isystem".into());
                         options.clang_args.push(path.into_boxed_str());


Test patch:
diff --git a/bindgen-tests/tests/expectations/tests/dynamic_loading_template.rs b/bindgen-tests/tests/expectations/tests/dynamic_loading_template.rs
index 65f079be..1f63a789 100644
--- a/bindgen-tests/tests/expectations/tests/dynamic_loading_template.rs
+++ b/bindgen-tests/tests/expectations/tests/dynamic_loading_template.rs
@@ -20,8 +20,8 @@ impl TestLib {
         L: Into<::libloading::Library>,
     {
         let __library = library.into();
-        let foo = __library.get(b"foo\0").map(|sym| *sym);
-        let foo1 = __library.get(b"foo1\0").map(|sym| *sym);
+        let foo = __library.get(b"_Z3fooIiET_S0_\0").map(|sym| *sym);
+        let foo1 = __library.get(b"_Z3fooIfET_S0_\0").map(|sym| *sym);
         Ok(TestLib { __library, foo, foo1 })
     }
     pub unsafe fn foo(&self, x: ::std::os::raw::c_int) -> ::std::os::raw::c_int {
diff --git a/bindgen-tests/tests/expectations/tests/dynamic_loading_with_allowlist.rs b/bindgen-tests/tests/expectations/tests/dynamic_loading_with_allowlist.rs
index e65176b8..34ebe8d9 100644
--- a/bindgen-tests/tests/expectations/tests/dynamic_loading_with_allowlist.rs
+++ b/bindgen-tests/tests/expectations/tests/dynamic_loading_with_allowlist.rs
@@ -27,9 +27,9 @@ impl TestLib {
         L: Into<::libloading::Library>,
     {
         let __library = library.into();
-        let foo = __library.get(b"foo\0").map(|sym| *sym);
-        let baz = __library.get(b"baz\0").map(|sym| *sym);
-        let bazz = __library.get(b"bazz\0").map(|sym| *sym);
+        let foo = __library.get(b"_Z3fooPv\0").map(|sym| *sym);
+        let baz = __library.get(b"_Z3bazPv\0").map(|sym| *sym);
+        let bazz = __library.get(b"_Z4bazziz\0").map(|sym| *sym);
         Ok(TestLib {
             __library,
             foo,
diff --git a/bindgen-tests/tests/expectations/tests/dynamic_loading_with_blocklist.rs b/bindgen-tests/tests/expectations/tests/dynamic_loading_with_blocklist.rs
index b214d0ef..8c86674f 100644
--- a/bindgen-tests/tests/expectations/tests/dynamic_loading_with_blocklist.rs
+++ b/bindgen-tests/tests/expectations/tests/dynamic_loading_with_blocklist.rs
@@ -62,8 +62,8 @@ impl TestLib {
         L: Into<::libloading::Library>,
     {
         let __library = library.into();
-        let foo = __library.get(b"foo\0").map(|sym| *sym);
-        let bar = __library.get(b"bar\0").map(|sym| *sym);
+        let foo = __library.get(b"_Z3fooPv\0").map(|sym| *sym);
+        let bar = __library.get(b"_Z3barPv\0").map(|sym| *sym);
         Ok(TestLib { __library, foo, bar })
     }
     pub unsafe fn foo(&self, x: *mut ::std::os::raw::c_void) -> ::std::os::raw::c_int {
diff --git a/bindgen-tests/tests/expectations/tests/dynamic_loading_with_class.rs b/bindgen-tests/tests/expectations/tests/dynamic_loading_with_class.rs
index 6395b0c6..65ff2b2f 100644
--- a/bindgen-tests/tests/expectations/tests/dynamic_loading_with_class.rs
+++ b/bindgen-tests/tests/expectations/tests/dynamic_loading_with_class.rs
@@ -59,8 +59,8 @@ impl TestLib {
         L: Into<::libloading::Library>,
     {
         let __library = library.into();
-        let foo = __library.get(b"foo\0").map(|sym| *sym);
-        let bar = __library.get(b"bar\0").map(|sym| *sym);
+        let foo = __library.get(b"_Z3fooPv\0").map(|sym| *sym);
+        let bar = __library.get(b"_Z3barv\0").map(|sym| *sym);
         Ok(TestLib { __library, foo, bar })
     }
     pub unsafe fn foo(&self, x: *mut ::std::os::raw::c_void) -> ::std::os::raw::c_int {