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.

Sign up for GitHub

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

Jump to bottom

proto_caster's as shared library have broken symbol visibility #160

Open
StefanBruens opened this issue Jun 12, 2024 · 6 comments
Open

proto_caster's as shared library have broken symbol visibility #160

StefanBruens opened this issue Jun 12, 2024 · 6 comments

Comments

@StefanBruens
Copy link
Contributor

Both native and wrapped proto_casters use several functions from proto_cast_util.cc. Unfortunately, most of these functions are not actually usable, as the pybind11 default hidden visibility is propagated to these functions.

This can be verified by analyzing the generated object file:

> readelf -a -CW ./build/CMakeFiles/pybind11_native_proto_caster.dir/pybind11_protobuf/proto_cast_util.cc.o  | grep -E 'FUNC .*GLOBAL'   592: 0000000000001630    41 FUNC    GLOBAL HIDDEN   160 pybind11_protobuf::PyBytesAsStringView(pybind11::bytes)
   593: 0000000000001660     3 FUNC    GLOBAL HIDDEN   160 pybind11_protobuf::PyProtoGetCppMessagePointer(pybind11::handle)
   594: 0000000000001670   172 FUNC    GLOBAL HIDDEN   160 pybind11_protobuf::PyProtoDescriptorFullName[abi:cxx11](pybind11::handle)
   595: 0000000000001720   158 FUNC    GLOBAL HIDDEN   160 pybind11_protobuf::PyProtoHasMatchingFullName(pybind11::handle, google::protobuf::Descriptor const*)
   748: 0000000000001ed0  1090 FUNC    GLOBAL HIDDEN   160 pybind11_protobuf::PyProtoSerializePartialToString(pybind11::handle, bool)
   829: 00000000000029e0   457 FUNC    GLOBAL HIDDEN   160 pybind11_protobuf::CProtoCopyToPyProto(google::protobuf::Message*, pybind11::handle)
   839: 0000000000003790  1089 FUNC    GLOBAL HIDDEN   160 pybind11_protobuf::AllocateCProtoFromPythonSymbolDatabase(pybind11::handle, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)
   845: 0000000000003be0   104 FUNC    GLOBAL DEFAULT  160 pybind11_protobuf::InitializePybindProtoCastUtil()
   846: 0000000000003c50   274 FUNC    GLOBAL DEFAULT  160 pybind11_protobuf::ImportProtoDescriptorModule(google::protobuf::Descriptor const*)
   847: 00000000000041b0   201 FUNC    GLOBAL HIDDEN   160 pybind11_protobuf::GenericPyProtoCast(google::protobuf::Message*, pybind11::return_value_policy, pybind11::handle, bool)
   848: 0000000000004280     8 FUNC    GLOBAL HIDDEN   160 pybind11_protobuf::GenericProtoCast(google::protobuf::Message*, pybind11::return_value_policy, pybind11::handle, bool)

All method using a parameter from the pybind11:: namespace becomes "hidden", and when linking the object file into the shared library the HIDDEN symbols are omitted from the symbol table.

See https://pybind11.readthedocs.io/en/stable/faq.html#someclass-declared-with-greater-visibility-than-the-type-of-its-field-someclass-member-wattributes

google-or-tools e.g. patches the pybind11_protobuf build to create a static library (which is not affected by symbol visibility) to work around this problem:
https://github.com/google/or-tools/blob/2384738a951561bf905f3435651310841c309b4c/patches/pybind11_protobuf.patch#L32
@rwgk
Copy link
Contributor

rwgk commented Jun 12, 2024

Tagging @srmainwaring who contributed the cmake support.

Could/should we adopt the or-tools patch here?

@StefanBruens
Copy link
Contributor Author

It all depends on the architecture you want to have.

Most of the code could just be moved to header-only. Having the implementation in a static library does not provide any benefit, but has several drawbacks:

  • the compiler can no longer inline cheap functions
  • the compiler has less possibilities to analyze the code and check for errors
  • the compiler can't do interprocedural optimizations

The big outlier is the GlobalState singleton. That does not fit "header-only". The question is, is this singleton actually required to be a singleton, or is this just an optimization to cache the protocol definitions. I am tempted to vote for the latter to be the case here - otherwise several python modules using pybind11_protobuf (possibly different versions) would not actually work.

Note, having the implementation in a static library get rids of the the symbol visibility problem, but does not solve the ABI problem the limited visibility is meant to solve. The static library may be compiled with one pybind11 version, but the module using it may use a different pybind11 version.

@rwgk
Copy link
Contributor

rwgk commented Jun 13, 2024

Most of the code could just be moved to header-only.

I think that's very unlikely to happen. (Just wanted to let you know; very long background omitted.)

StefanBruens added a commit to StefanBruens/pybind11_protobuf that referenced this issue Jun 14, 2024
@StefanBruens
[CMake] Make the pybind11_*_proto_caster libraries STATIC
a5112c3 
As pybind11 does not guarantee stable ABI for its types (e.g.
pybind11::handle), these types have "hidden" visibility at least on
Linux.

This visibility is propagated to any method which has any of the pybind11
types in its parameters, which is the case for many of the public methods
in proto_cast_util.{h,cc}. Trying to link to e.g. a SHARED
pybind11_native_proto_caster library will create linker errors to to
undefined symbols for any non-trivial case.

The other approach would be to move everything which uses a pybind11
type to the headers, and only leave pybind11 agnostic methods in the
shared library. While this is trivially possible for e.g.
PyBytesAsStringView, for the majority of the methods (e.g. anything
depending on the GlobalState object) a significant refactoring would
be required.

See pybind#160.
StefanBruens added a commit to StefanBruens/pybind11_protobuf that referenced this issue Jun 14, 2024
@StefanBruens
[CMake] Make the pybind11_*_proto_caster libraries STATIC
885c511 
As pybind11 does not guarantee stable ABI for its types (e.g.
pybind11::handle), these types have "hidden" visibility at least on
Linux.

This visibility is propagated to any method which has any of the pybind11
types in its parameters, which is the case for many of the public methods
in proto_cast_util.{h,cc}. Trying to link to e.g. a SHARED
pybind11_native_proto_caster library will create linker errors to to
undefined symbols for any non-trivial case.

The other approach would be to move everything which uses a pybind11
type to the headers, and only leave pybind11 agnostic methods in the
shared library. While this is trivially possible for e.g.
PyBytesAsStringView, for the majority of the methods (e.g. anything
depending on the GlobalState object) a significant refactoring would
be required.

See pybind#160.
StefanBruens added a commit to StefanBruens/pybind11_protobuf that referenced this issue Jun 14, 2024
@StefanBruens
[CMake] Make the pybind11_*_proto_caster libraries STATIC
f732396 
As pybind11 does not guarantee stable ABI for its types (e.g.
pybind11::handle), these types have "hidden" visibility at least on
Linux.

This visibility is propagated to any method which has any of the pybind11
types in its parameters, which is the case for many of the public methods
in proto_cast_util.{h,cc}. Trying to link to e.g. a SHARED
pybind11_native_proto_caster library will create linker errors to to
undefined symbols for any non-trivial case.

The other approach would be to move everything which uses a pybind11
type to the headers, and only leave pybind11 agnostic methods in the
shared library. While this is trivially possible for e.g.
PyBytesAsStringView, for the majority of the methods (e.g. anything
depending on the GlobalState object) a significant refactoring would
be required.

See pybind#160.
StefanBruens added a commit to StefanBruens/pybind11_protobuf that referenced this issue Jun 15, 2024
@StefanBruens
[CMake] Make the pybind11_*_proto_caster libraries STATIC
5bb8f38 
As pybind11 does not guarantee stable ABI for its types (e.g.
pybind11::handle), these types have "hidden" visibility at least on
Linux.

This visibility is propagated to any method which has any of the pybind11
types in its parameters, which is the case for many of the public methods
in proto_cast_util.{h,cc}. Trying to link to e.g. a SHARED
pybind11_native_proto_caster library will create linker errors to to
undefined symbols for any non-trivial case.

The other approach would be to move everything which uses a pybind11
type to the headers, and only leave pybind11 agnostic methods in the
shared library. While this is trivially possible for e.g.
PyBytesAsStringView, for the majority of the methods (e.g. anything
depending on the GlobalState object) a significant refactoring would
be required.

See pybind#160.
StefanBruens added a commit to StefanBruens/pybind11_protobuf that referenced this issue Jun 15, 2024
@StefanBruens
[CMake] Make the pybind11_*_proto_caster libraries STATIC
508257b 
As pybind11 does not guarantee stable ABI for its types (e.g.
pybind11::handle), these types have "hidden" visibility at least on
Linux.

This visibility is propagated to any method which has any of the pybind11
types in its parameters, which is the case for many of the public methods
in proto_cast_util.{h,cc}. Trying to link to e.g. a SHARED
pybind11_native_proto_caster library will create linker errors to to
undefined symbols for any non-trivial case.

The other approach would be to move everything which uses a pybind11
type to the headers, and only leave pybind11 agnostic methods in the
shared library. While this is trivially possible for e.g.
PyBytesAsStringView, for the majority of the methods (e.g. anything
depending on the GlobalState object) a significant refactoring would
be required.

See pybind#160.
@StefanBruens
Copy link
Contributor Author

Most of the code could just be moved to header-only.

I think that's very unlikely to happen. (Just wanted to let you know; very long background omitted.)

Care to elaborate? As far as I can see, the static library only has downsides.

@rwgk
Copy link
Contributor

rwgk commented Jun 26, 2024

Nothing nice, I'm afraid, tbh: currently unclear ownership and governance, "best effort" only (fancy for: no actual staffing). — Hopefully some things will improve in the next few months.

@StefanBruens
Copy link
Contributor Author

Nothing nice, I'm afraid, tbh: currently unclear ownership and governance, "best effort" only (fancy for: no actual staffing). — Hopefully some things will improve in the next few months.

Ah, so not a policy decision, but "just" lack of time ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@rwgk @StefanBruens