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

Add checks on no_mangle & test attributes, on pub visibility #60

Closed
wants to merge 4 commits into from

Conversation

snaulX
Copy link

@snaulX snaulX commented Oct 18, 2023

Applies as implementation for #58.
Now csbindgen::Builder has an option rust_disable_mangle that by default equals true. It means that bindings for functions without attribute #[no_mangle] will not be generated. Also I add check on attribute #[test] (function will not be passed if it has this attribute) and on pub visibility (function will not be passed if it has other visibility).

@MolotovCherry
Copy link

MolotovCherry commented Oct 30, 2023

Can you add export_name to this? This is another valid way to export a symbol

I also want to note that pub visibility does not affect whether the symbol is exported or not. (You can see this yourself by checking the exported symbols on the dll). It only affects the visibility to other Rust code. If it's exported, it will be exported, regardless of visibility.

So the correct behavior should be whether it exports or not, ignoring the visibility. To the best of my current knowledge, both no_mangle and export_name are the only ones that currently export symbols

@snaulX
Copy link
Author

snaulX commented Oct 30, 2023

Apply your fixes and suggestions: remove check on pub and add check on export_name (I hope it works because this testing environment is strange and I didn't test it normally).

@hadashiA
Copy link
Contributor

hadashiA commented Jan 5, 2024

Thanks a lot. As I see it, this PR has three additional conditions to look for the function's target.

  1. Exclude anything with #[test]
  2. Include #[no_mangle] or #[export_name = "foo"]
  3. Adding rust_disable_mangle option. (If true, ignore if no_mangle is not attached.)

I have confimed this. Unfortunately, it is difficult to merge this PR for the reasons given below.

  • There seems to be a problem with implementations of 2 and 3. Perhaps this patch cannot to handle the foreign type (bindgen rust to rust). I.e., extern { ... } functions in the block would be no_mangle without an explicit attribute, but that fact is ignored.
  • For 1, I do not see much strong reason why it is necessary.
    Don't you normally not put the extern modifier on #[test]? The extern modifier is for ffi.
  • For 3, the usefulness of the rust_disable_mangle option is not known. My understanding is that csbindgen cannot resolve the method name if it is mangled.

However, it would be useful to be able to output symbol names without mangling via the export_name attribute.
Thanks for sharing that with us. We will consider supporting export_name separately.

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