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

We do not uniqueify cxx::bridge names except for functions #486

Open
adetaylor opened this issue May 14, 2021 · 6 comments
Open

We do not uniqueify cxx::bridge names except for functions #486

adetaylor opened this issue May 14, 2021 · 6 comments
Labels
bug Something isn't working

Comments

@adetaylor
Copy link
Collaborator

This error encountered on an internal codebase:

"thread '<unnamed>' panicked at 'Unable to generate header and C++ code: Syn(Error("the name Key is defined multiple times"))', /src/main.rs:189:18"

Working on reducing now.

@adetaylor adetaylor added the bug Something isn't working label May 14, 2021
@adetaylor
Copy link
Collaborator Author

Minimized test case:

namespace a {
namespace spanner {
class Key;
}
} // namespace a
namespace spanner {
class Key {
public:
  bool b(a::spanner::Key &);
};
} // namespace spanner

@adetaylor
Copy link
Collaborator Author

This problem is understood. It's because, for functions, we're using a bridge_name_tracker to ensure that entries in the (flat) cxx::bridge namespace are unique. We're not doing that for typedefs (which is what goes wrong here) not for structs/enums.

@adetaylor
Copy link
Collaborator Author

I'm now thinking that we should "simply" mangle all names in the cxx::bridge mod to include namespaces. To avoid conflicts in the cxx::bridge output Rust code, we'd need to avoid using #[rust_name]. We can use RustRenameStrategy::RenameInOutputMod to get them back to the desired name.

It would be ideal if we could come up with a name mangling scheme which, for simple names, happens to match user expectations. This would significantly help with debugging when people were looking through complex call stacks across languages.

e.g.

  • A (in root namespace) => A
  • foo in bar namespace => bar_foo

but then we would need something different for a type literally called bar_foo or a struct foo nested inside a struct bar.

So the alternative is indeed to have a bridge_name_tracker as originally planned, which allocates a new name for each thing, and that new name is stored with the thing in the API list. This remains a bit fiddly, so if we can make a deterministic mangling scheme good enough, maybe we should.

adetaylor added a commit that referenced this issue Feb 10, 2022
Previously we generated slightly simpler output by using
`#[rust_name]` attributes on the cxx::bridge mod. With this change,
we never do that and instead always perform any such renames
with a `use` statement in the output set of mods.

This simplifies our code, while making output more verbose;
it is a prerequisite for a subsequent step towards solving #486.
adetaylor added a commit that referenced this issue Feb 10, 2022
This is part one of multiple commits which will solve #486 by
mangling all names in the cxx::bridge in a predictable way.
adetaylor added a commit that referenced this issue Mar 14, 2022
Previously we sometimes panicked if we found APIs with identical names.
We know we have issues like #486 where we don't always eliminate identically
named items, and we know bindgen also has similar bugs. With this change we
do not panic but instead ignore such items.
@VirxEC
Copy link
Contributor

VirxEC commented Feb 1, 2023

Just wanted to say that I'm encountering this issue with multiple definitions of cxxbridge1$new_autocxx_autocxx_wrapper in gen3.cxx and gen1.cxx as well as cxxbridge1$GetState_autocxx_wrapper in gen10.cxx and gen7.cxx.

Is it possible for the names to be mangled based on the gen # in the file name? When I cargo clean and rebuild the names of the gen files are the same, so it seems like it would be a predictable way to mangle the names. Could just append _gen3 and _gen1 to the end maybe?

@adetaylor
Copy link
Collaborator Author

That feels to me like a slightly different problem than the one here - the current issue is about multiple conflicting names within a single include_cpp! whereas it sounds like you have a conflict between multiple include_cpp! macro invocations. Please could you raise a new issue to avoid muddling this one?

@VirxEC
Copy link
Contributor

VirxEC commented Feb 1, 2023

Sure haha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants