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

Fix and test constructor wrappers in multiple include_cpps #1099

Closed
wants to merge 1 commit into from

Conversation

bsilver8192
Copy link
Contributor

The naming convention that just used the C++ function name directly
doesn't work with multiple include_cpps in the same binary. The test
case has them in the same rust file, which could be solved other ways,
but ultimately we may link the generated C++ code from multiple Rust
files which means the names need to be globally unique.

The naming convention that just used the C++ function name directly
doesn't work with multiple include_cpps in the same binary. The test
case has them in the same rust file, which could be solved other ways,
but ultimately we may link the generated C++ code from multiple Rust
files which means the names need to be globally unique.
@adetaylor
Copy link
Collaborator

Thanks for this!

I'm aware that this is a problem with multiple mods in the same linked binary. I'm not yet sure that this is how I want to solve it.

Thoughts:

  1. Your proposed change isn't sufficient if there are several types called A in different mods.
  2. For make_string, we therefore make the name unique per mod by using a hash of the configuration on the symbol name. That would arguably be the "correct" thing to do for every symbol...
  3. ... although even that doesn't work if there are multiple identical include_cpp! macros within the same crate (though if there is, I think it's reasonable to declare that such cases aren't supported).
  4. However I'm not thrilled about mangling the symbol names for all cxx::bridge entries. That seems to add complexity for the 99% case where there's only one such mod. ("Complexity" is mostly but not entirely invisible to the user. For example added layers of function shims, more complex symbol names in stack traces.) I haven't figured out how to resolve this yet.
  5. If we really do want to mangle all the cxx::bridge names like this, then we can probably lose some other complexity. Perhaps we always assume there's both a Rust-side and C++-side wrapper function.

@bsilver8192
Copy link
Contributor Author

Yea, managing the names in all these different namespaces is tricky...

  1. Your proposed change isn't sufficient if there are several types called A in different mods.

Good point, I had it in my head that the namespace was included, but looking again it isn't.

  1. For make_string, we therefore make the name unique per mod by using a hash of the configuration on the symbol name. That would arguably be the "correct" thing to do for every symbol...
  2. ... although even that doesn't work if there are multiple identical include_cpp! macros within the same crate (though if there is, I think it's reasonable to declare that such cases aren't supported).

Could you include the filename and line number of the include_cpp! in the hash to fix that?

  1. However I'm not thrilled about mangling the symbol names for all cxx::bridge entries. That seems to add complexity for the 99% case where there's only one such mod. ("Complexity" is mostly but not entirely invisible to the user. For example added layers of function shims, more complex symbol names in stack traces.) I haven't figured out how to resolve this yet.

Are there any un-mangled names right now? I might be not using some features, but I've got some fairly complex classes being wrapped and the only functions using this codepath that I see are special member functions which need wrappers anyways.

@adetaylor
Copy link
Collaborator

Could you include the filename and line number of the include_cpp! in the hash to fix that?

Unfortunately, getting such information out of a proc_macro::Span requires nightly, as far as I understand it. In any case, I think this is an extremely niche case.

Are there any un-mangled names right now? I might be not using some features, but I've got some fairly complex classes being wrapped and the only functions using this codepath that I see are special member functions which need wrappers anyways.

Hmm, there should be. For simple functions. But, fair comment, maybe we've added so many cases requiring wrappers that nearly all names get mangled one way or another already. I will do some experiments and report back!

@adetaylor
Copy link
Collaborator

adetaylor commented May 21, 2022

Are there any un-mangled names right now? I might be not using some features, but I've got some fairly complex classes being wrapped and the only functions using this codepath that I see are special member functions which need wrappers anyways.

I gathered some figures

|| Example || Total names || Of which mangled with autocxx_wrapper ||
| chromium-fake-render-frame-host | 44 | 41 |
| s2 | 68 | 44 |
| steam-mini | 11 | 10 |

No conclusions yet! I think my next step might be to attempt to put together a PR which does indeed mangle all names, and see how much code we can discard. I did already try this in #785; see also #486 (comment), but I think I should revisit it.

@adetaylor
Copy link
Collaborator

It's possible that this specific test case is fixed by #1235 , but there are remaining problems - #1239.

@bsilver8192
Copy link
Contributor Author

Looks like my current use cases don't need this any more, so I'll close this. I'm not sure what combination of my code changing vs fixes in autocxx fixed it, but current autocxx builds my code without needing this.

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