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

Make embind work with -fvisibility=hidden #22095

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

msorvig
Copy link
Contributor

@msorvig msorvig commented Jun 14, 2024

Export emscripten::val and emscripten::memory_view in order to prevent embind errors:

BindingError: _emval_take_value has unknown type N10emscripten11memory_viewIhEE

Embind generates a numerical type id from the address of the std::type_info object resulting from evaluating a typeid expressiion (e.g. 'void *id = &typeid(T)').

However, C++ does not guarantee that this address is identical for all evaluations of the typeid expression. In practice it is when using static linking, but not when using dynamic linking when the libraries are
built with the '-fvisibility=hidden' compiler option.

The non-identical id's then cause embind to decide that types have not been registered when used from a library, since they have been registered with a different id by the main wasm module.

Exporting the types in question makes typeid addresses identical again, and fixes/works around the issue.

@msorvig
Copy link
Contributor Author

msorvig commented Jun 14, 2024

This looks to be sufficient for Qt's use case. More types can be exported if needed.

There's a bit more to it: ideally embind should work with non-exported ("hidden") types as well, and it looks like C++ API like std::type_info::hash_code() and std::type_index may not work correctly for non-exported types.

Looking at at system/lib/libcxx/include/typeinfo, there are two options from implementing typeinfo:

  1. _LIBCPP_TYPEINFO_COMPARISON_IMPLEMENTATION = 1: (default): "This implementation of type_info assumes a unique copy of the RTTI for a given type inside a program"
  2. _LIBCPP_TYPEINFO_COMPARISON_IMPLEMENTATION = 2: "This implementation of type_info does not assume there is always a unique copy of the RTTI for a given type inside a program"

Emscripten currently uses the default option 1, but does not generate unique RTTI for non-exported types in libraries/side modules.

@brendandahl
Copy link
Collaborator

@sbc100 Do you think there's some way we could properly have unique rtti for the program?

@msorvig
Copy link
Contributor Author

msorvig commented Jun 17, 2024

Note previous discussion on #16711

Export emscripten::val and emscripten::memory_view
in order to prevent embind errors:

  BindingError: _emval_take_value has unknown type N10emscripten11memory_viewIhEE

Embind generates a numerical type id from the address
of the std::type_info object resulting from evaluating
a typeid expressiion (e.g. 'void *id = &typeid(T)').

However, C++ does not guarantee that this address is
identical for all evaluations of the typeid expression.
In practice it is when using static linking, but not
when using dynamic linking when the libraries are
built with the '-fvisibility=hidden' compiler option.

The non-identical id's then cause embind to decide
that types have not been registered when used from a
library, since they have been registered with a different
id by the main wasm module.

Exporting the types in question makes typeid addresses
identical again, and fixes/works around the issue.
@sbc100
Copy link
Collaborator

sbc100 commented Jun 17, 2024

@sbc100 Do you think there's some way we could properly have unique rtti for the program?

I think the way to achieve that is make sure the types are marked as defaultand not hidden visibility.

This fix seems correct to me for the core embind types. They always want to be shared I think.

We also have -DEMSCRIPTEN_HAS_UNBOUND_TYPE_NAMES=0 which looks like it tells embind not to use RTTI info at all... although I don't really understand the naming of that macro.

self.set_setting('MAIN_MODULE', 1)

@requires_dylink
def test_embind_dylink_visibility_hidden(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks similar to what test_dylink_rtti is doing in test_core.py, but with some embind specifics. As such, perhaps it belongs alongside that test?

Rather than duplicating the three helper functions from test_core.py why not just put this test in test_core.py itself?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also might make more sense alongside test_embind_no_rtti

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, tst_core could make sense as well (I had it there first). Is it primarily a dylink or embind test? test_embind_no_rtti sounds related, I'll check what that does.

(Note I have vacation time is coming up, so I'll get back to this in a couple of week's time.)

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.

None yet

3 participants