This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Apply _LIBCPP_EXCLUDE_FROM_EXPLICIT_INSTANTIATION only in classes that we have instantiated externally
AbandonedPublic

Authored by philnik on May 18 2023, 11:30 AM.

Details

Summary

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.

Diff Detail

Event Timeline

philnik created this revision.May 18 2023, 11:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 18 2023, 11:30 AM
philnik updated this revision to Diff 523572.May 18 2023, 3:03 PM

Try to fix CI

philnik updated this revision to Diff 523805.May 19 2023, 8:54 AM

Try to fix CI

philnik updated this revision to Diff 523827.May 19 2023, 9:40 AM

Try to fix CI

philnik updated this revision to Diff 523884.May 19 2023, 12:01 PM

Try to fix CI

philnik updated this revision to Diff 523960.May 19 2023, 4:16 PM

Try to fix CI

philnik updated this revision to Diff 524114.May 21 2023, 10:52 AM

Try to fix CI

ldionne added inline comments.
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.

philnik updated this revision to Diff 524383.May 22 2023, 10:28 AM
philnik marked 3 inline comments as done.

Address comments

philnik published this revision for review.May 22 2023, 10:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 22 2023, 10:28 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne added inline comments.May 22 2023, 6:02 PM
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 ↗(On Diff #524383)

Can you document how you checked that there were no additional (hidden) symbols generated inside the dylib in the commit message?

philnik edited the summary of this revision. (Show Details)May 23 2023, 8:51 AM
philnik updated this revision to Diff 524744.May 23 2023, 8:52 AM
philnik marked 2 inline comments as done.

Address comments

philnik edited the summary of this revision. (Show Details)May 23 2023, 8:52 AM
philnik added a subscriber: Mordante.

@Mordante This patch might fix some of the issues you have with std::format + GCC.

ldionne accepted this revision.May 23 2023, 9:49 AM

Nice, thanks for fixing this long standing issue! @Mordante That should indeed unblock many of your format tests that timed out on GCC.

This revision is now accepted and ready to land.May 23 2023, 9:49 AM

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>>))

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.

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.

Thanks for looking into this! Why would such a use case be unsupported?

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.

Thanks for looking into this! Why would such a use case be unsupported?

http://eel.is/c++draft/requirements#namespace.std-5

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.

Thanks for looking into this! Why would such a use case be unsupported?

http://eel.is/c++draft/requirements#namespace.std-5

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.

EricWF reopened this revision.May 24 2023, 6:12 AM
EricWF added a subscriber: EricWF.

@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.

This revision is now accepted and ready to land.May 24 2023, 6:12 AM
EricWF requested changes to this revision.May 24 2023, 6:13 AM
This revision now requires changes to proceed.May 24 2023, 6:13 AM

@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.

  • 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 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.

  • 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 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.

philnik abandoned this revision.Jun 9 2023, 10:03 AM