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

Consider symbol visibility for shared library #41

Open
dkg opened this issue Jun 21, 2023 · 5 comments · Fixed by #48
Open

Consider symbol visibility for shared library #41

dkg opened this issue Jun 21, 2023 · 5 comments · Fixed by #48
Assignees
Labels
question Further information is requested

Comments

@dkg
Copy link
Contributor

dkg commented Jun 21, 2023

The current shared object produced with BUILD_SHARED_LIBS=on exposes many different C++ symbols, and appears to make no attempt to filter what is explicitly exposed.

Does every symbol need to be exposed in the shared object? I'm not enough of a C++ person to be able to tell what the library's symbol table should really look like, but i do observe that there are symbols exposed that are not part of either the sexp namespace or the ext_key_format namespace.

For example, when i try to build it i see the following symbols:

  • _ZNKSt5ctypeIcE8do_widenEc (unmangled: std::ctype<char>::do_widen(char) const)
  • _ZNSt15_Sp_counted_ptrIDnLN9__gnu_cxx12_Lock_policyE2EE10_M_disposeEv (unmangled: std::_Sp_counted_ptr<decltype(nullptr), (__gnu_cxx::_Lock_policy)2>::_M_dispose())
  • _ZZNSt8__detail18__to_chars_10_implIjEEvPcjT_E8__digits (unmangled: std::__detail::__to_chars_10_impl<unsigned int>(char*, unsigned int, unsigned int)::__digits)

(there are many more, but the list above gives a flavor of what i'm seeing)

@maxirmx
Copy link
Member

maxirmx commented Jun 22, 2023

Thank you, @dkg

It was designed as a private static library. Now it needs some reasonable public API

@dkg
Copy link
Contributor Author

dkg commented Jun 28, 2023

i don't think the symbol cleanup was as effective as intended here. the only exported symbol removed from the library between 0.8.5 and 0.8.7 was _ZSt30__lexicographical_compare_implIN9__gnu_cxx17__normal_iteratorIPKcNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEEESA_NS0_5__ops15_Iter_comp_iterIZNK14ext_key_format22extended_private_key_t7ci_lessclERKS9_SH_EUlccE_EEEbT_SK_T0_SL_T1_ (which de-mangles to bool std::__lexicographical_compare_impl<__gnu_cxx::__normal_iterator<char const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, __gnu_cxx::__normal_iterator<char const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, __gnu_cxx::__ops::_Iter_comp_iter<ext_key_format::extended_private_key_t::ci_less::operator()(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) const::{lambda(char, char)#1}> >(__gnu_cxx::__normal_iterator<char const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, __gnu_cxx::__normal_iterator<char const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, __gnu_cxx::__normal_iterator<char const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, __gnu_cxx::__normal_iterator<char const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, __gnu_cxx::__ops::_Iter_comp_iter<ext_key_format::extended_private_key_t::ci_less::operator()(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) const::{lambda(char, char)#1}>))

That is, it's good that this symbol was removed, but there are still many other symbols of dubious value to a clean API exposed (at least when building with gcc and the toolchain available in debian testing/unstable).

@ni4
Copy link

ni4 commented Jun 28, 2023

@dkg Thanks for checking this.
@maxirmx I has some nice time with symbol visibility for rnp library. Please see this snippet from the rnp/lib/CMakeLists.txt:

 # Limit symbols export only to rnp_* functions.
  if (APPLE)
    # use -export_symbols_list on Apple OSs
    target_link_options(librnp PRIVATE -Wl,-exported_symbols_list "${CMAKE_CURRENT_SOURCE_DIR}/librnp.symbols")
    set_target_properties(librnp PROPERTIES LINK_DEPENDS "${CMAKE_CURRENT_SOURCE_DIR}/librnp.symbols")
  elseif(NOT WIN32)
    target_link_options(librnp PRIVATE "-Wl,--version-script=${CMAKE_CURRENT_SOURCE_DIR}/librnp.vsc")
    set_target_properties(librnp PROPERTIES LINK_DEPENDS "${CMAKE_CURRENT_SOURCE_DIR}/librnp.vsc")
  endif()

@maxirmx
Copy link
Member

maxirmx commented Jun 30, 2023

sexpp is C++ library that uses C++ standard library heavily.

C++ standard library is a template library to the great extent. Template function (or class) is essentially code generator and
an entity that is linked(loaded) from another module. Practically it means that when a toolset needs to instatiate a template it indirectly
creates a body for function(s), compiles it and places in a current module. This approach means that multiple modules (executables and shared libraries)
may have a body for the same instantiated function. This conflict is resolved with weak symbols. When a weak defined symbol is linked with a normal
defined symbol, the normal defined symbol is used with no error. When a weak undefined symbol is linked and the symbol is not defined, the value of
the weak symbol becomes zero with no error.

If I look at the questioned symbols:
_ZNKSt5ctypeIcE8do_widenEc (unmangled: std::ctype<char>::do_widen(char) const) -- function template https://en.cppreference.com/w/cpp/locale/ctype/widen
(it is not on the export table on my Ubuntu box though)

_ZNSt15_Sp_counted_ptrIDnLN9__gnu_cxx12_Lock_policyE2EE10_M_disposeEv (unmangled: std::_Sp_counted_ptr<decltype(nullptr), (__gnu_cxx::_Lock_policy)2>::_M_dispose())
this is a template behind std::shared_ptr.
I have it marked 'W' -- the symbol is a weak symbol that has not been specifically tagged as a weak object symbol and has a default value specified.

_ZZNSt8__detail18__to_chars_10_implIjEEvPcjT_E8__digits (unmangled: std::__detail::__to_chars_10_impl<unsigned int>(char*, unsigned int, unsigned int)::__digits)
https://gcc.gnu.org/onlinedocs/gcc-13.1.0/libstdc++/api/a01663.html#a74745386dd30ed18bc932a26a766d147
I do not know what this template does, but it is also marked 'W'

Simply speaking, the when a library exports classes/functions that use C++ standard library template types as bases/parameters it also exports all member functions for instatiated templates and their underlying templates recursively as weak symbols.

It is is very different from managing export symbols for C-style libraries.

If you know how to apply a different method of linkage to C++ standart library please advise.

@ni4 ni4 reopened this Jun 30, 2023
@ronaldtse ronaldtse added the question Further information is requested label Jul 1, 2023
@dkg
Copy link
Contributor Author

dkg commented Jul 6, 2023

@maxirmx, i think you probably have a better understanding of it than i do, unfortunately. You describe some aspects of the problem in ways that make me think more deeply about it, at least ☺

That said, i don't understand how or when C++ applications are supposed to use these library interfaces. For example, consider two libraries that take the same approach, which will both have weak symbols for the templated classes. how does an application decide which one of them to use? What happens if the memory image of the object imagined by one implementation disagrees with the memory image from another implementation?

If the answer is that an application won't use either of them, and instead relies on its own instantiation, then why do the libraries need to export the functions in the first place?

Anyway, i'm happy to leave this open as a question in the hopes that someone else will come along and offer more insight. thanks for considering it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants