To make sure all member functions that require it are marked _LIBCPP_EXCLUDE_FROM_EXPLICIT_INSTANTIATION I compared the output of objdump --syms lib/libc++.1.0.dylib before and after, ignoring addresses.
Details
- Reviewers
ldionne EricWF - Group Reviewers
Restricted Project - Commits
- rGb3c9150062dc: [libc++] Apply _LIBCPP_EXCLUDE_FROM_EXPLICIT_INSTANTIATION only in classes that…
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
libcxx/include/__config | ||
---|---|---|
621–622 | This needs to be updated. | |
648–656 | Let's add a comment explaining that _LIBCPP_HIDE_FROM_ABI_AFTER_V1 also prevents newly compiled programs from relying on symbols that we know are removed from the ABI in a future version. | |
libcxx/include/__locale | ||
207 | Could you please make sure that this patch doesn't change the set of symbols instantiated in the dylib (including hidden symbols)? If the dylib contains a new symbol X after this change, it would indicate that you forgot to add _LIBCPP_EXCLUDE_FROM_EXPLICIT_INSTANTIATION to symbol X in this patch. We should be careful to try to catch this here cause this is going to be a linker error potentially far downstream otherwise, in case we have missing coverage for symbol X in our test suite. |
libcxx/include/__config | ||
---|---|---|
621–622 | I would actually move it above to document _LIBCPP_EXCLUDE_FROM_EXPLICIT_INSTANTIATION and say something like: // _LIBCPP_EXCLUDE_FROM_EXPLICIT_INSTANTIATION is used on methods of explicitly instantiated // classes that we don't want to make part of the ABI to opt those methods out of the explicit instantiation // and avoid exporting them from the dylib or relying on their definition being in the dylib. | |
libcxx/utils/libcxx/test/params.py | ||
35 | Can you document how you checked that there were no additional (hidden) symbols generated inside the dylib in the commit message? |
Nice, thanks for fixing this long standing issue! @Mordante That should indeed unblock many of your format tests that timed out on GCC.
This change is causing some link failures on an internal project of the form below. The code is very old. What is the proper fix?
ERROR: /google/src/cloud/saugustine/llvm_integrate/google3/webutil/io/BUILD:301:11: Linking webutil/io/libhtmlpage.so failed: (Exit 1) link_dynamic_library.sh failed: error executing CppLink command (from target //webutil/io:htmlpage) tools/cpp/link_dynamic_library.sh yes tools/cpp/build_interface_so blaze-out/k8-opt/bin/webutil/io/libhtmlpage.so blaze-out/k8-opt/bin/webutil/io/libhtmlpage.ifso ... (remaining 38 arguments skipped). [forge_remote_host=ixqb4] ld: error: undefined hidden symbol: __gnu_cxx::hash_map<std::__u::basic_string<char, std::__u::char_traits<char>, std::__u::allocator<char>>, std::__u::basic_string<char, std::__u::char_traits<char>, std::__u::allocator<char>>, __gnu_cxx::hash<std::__u::basic_string<char, std::__u::char_traits<char>, std::__u::allocator<char>>>, std::__u::equal_to<std::__u::basic_string<char, std::__u::char_traits<char>, std::__u::allocator<char>>>, std::__u::allocator<std::__u::pair<std::__u::basic_string<char, std::__u::char_traits<char>, std::__u::allocator<char>> const, std::__u::basic_string<char, std::__u::char_traits<char>, std::__u::allocator<char>>>>>::hash_map(unsigned long, __gnu_cxx::hash<std::__u::basic_string<char, std::__u::char_traits<char>, std::__u::allocator<char>>> const&, std::__u::equal_to<std::__u::basic_string<char, std::__u::char_traits<char>, std::__u::allocator<char>>> const&) >>> referenced by htmlpage.cc >>> blaze-out/k8-opt/bin/webutil/io/_objs/htmlpage/htmlpage.pic.o:(webutil::HTMLPage::HTMLPage(std::__u::basic_string_view<char, std::__u::char_traits<char>>))
Do you possibly have an explicit instantiation of __gnu_cxx::hash_map<std::string>? That would break things with this patch, and is most likely not supported.
Sure, but imagine we copy __gnu_cxx::hash_map and rename it to gnu_cxx::hash_map. It would break in the same way, but now it is 100% user-written code.
@philnik Please revert this change. It breaks a bunch of downstream users. We can discuss how or if to reapply it again later, but for the moment, please revert the change while we figure it out.
We appreciate your help unblocking downstream.
@philnik and I just discussed this issue in depth.
Fundamentally, the problem is that the library needs the ability to control what is contained in an explicit instantiation of a class by users. The Standard guarantees that users can explicitly instantiate stdlib classes if they provide at least one user-defined type in the instantiation. However, when a member function "escapes" into a user dylib by means of an explicit instantiation, ABI stability requires us to keep that member function stable. Since we can't commit to ABI stability for all implementation detail member functions, we need a way of excluding some member functions from the user's explicit instantiations. This is what the exclude_from_explicit_instantiation attribute achieves, and we are trying to approximate its effect on GCC by using always_inline.
We discussed for over an hour how to not require exclude_from_explicit_instantiation, which would then also get rid of the need for always_inline on GCC. However, the conclusion is that it's a fundamental and important tool for us to control what we let escape into users' dylibs via explicit instantiations. So we can't get rid of that attribute. As a result, I think the conclusion here is that we either
- need GCC to support this attribute
- or we drop the guarantee that users can do explicit instantiations on GCC (or if they do, then we don't guarantee ABI stability for them)
This is a mess. @jwakely Do you think there's any chance we could get GCC to add this attribute? The attribute is pretty simple, it basically excludes the marked member functions from any explicit instantiation or explicit instantiation declaration.
This seems acceptable to me.
This is a mess. @jwakely Do you think there's any chance we could get GCC to add this attribute? The attribute is pretty simple, it basically excludes the marked member functions from any explicit instantiation or explicit instantiation declaration.
Dunno, file a bugzilla and find out :-)
Why is this only a problem for explicit instantiations? If users have implicit instantiations of members of std::vector, isn't there also an ABI stability requirement there? If two translation units each have an implicit instantiation, but with different ABI semantics, the linker could pick either of the definitions. That could be incompatible with one of the TUs.
All the functions have abi_tags which get bumped with every release, so if two TUs get compiled against different libc++ versions, the functions have different symbols names.
This needs to be updated.