[libcxx-commits] [PATCH] D68617: partially inline copy constructor basic_string(const basic_string&[, allocator])

Louis Dionne via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Tue Oct 15 11:35:55 PDT 2019


ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

Note: There does not appear to be anything at https://github.com/llvm/llvm-project/compare/master...martijnvels:string-optimize-copy-ctor

The ABI break wasn't immediately obvious to me at first, so let's dissect the change to make sure we're all on the same page. First, we're adding a two new symbols to the dylib via the explicit instantiation of `std::basic_string` inside the dylib:

  __ZNSt3__112basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEE11__init_longERKS5_ => std::basic_string<char, std::char_traits<char>, std::allocator<char> >::__init_long(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)
  __ZNSt3__112basic_stringIwNS_11char_traitsIwEENS_9allocatorIwEEE11__init_longERKS5_ => std::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> >::__init_long(std::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> > const&)

That's just adding symbols, which is ABI-cool. Second, there's two possibilities for existing code that has been compiled against old libc++ headers.

1. `basic_string(const basic_string& __str)` has been inlined into the code, and there's thus a call to `__init()` inside the dylib. That's OK, since we're not removing symbols from the dylib, so that code can still call `__init()` if we execute it against a new dylib.
2. `basic_string(const basic_string& __str)` has not been inlined, so there's a call to `basic_string(const basic_string& __str)` inside the dylib. That's OK too since we are not removing that symbol. The new `basic_string(const basic_string& __str)` inside the dylib will have a different implementation after that patch, but it does the same thing as it used to.

The only problem I see is building an application against the latest headers, with the intent of using it against an older dylib. Then, if `basic_string(const basic_string& __str)` is inlined into the application, that application contains a call to `__init_long()` in the dylib. However, when run against an old dylib that doesn't have `__init_long()` yet, the dynamic linker will fail to find the symbol at runtime. I don't know whether that's technically considered an ABI break, but it's definitely not something we want.

Are we on the same page?

If so, then one way this problem can be circumvented is by having the knowledge of when a certain function (here `__init_long()`) was introduced in the dylib. If the deployment target is not known to contain `__init_long()`, we'd fall back to another implementation that does not require it in the dylib (such as emitting `__init_long()` in the application). We have precedent for doing that, for example for the iostreams. From `<__config>`:

  // The stream API was dropped and re-added in the dylib shipped on macOS
  // and iOS. We can only assume the dylib to provide these definitions for
  // macosx >= 10.9 and ios >= 7.0. Otherwise, the definitions are available
  // from the headers, but not from the dylib. Explicit instantiation
  // declarations for streams exist conditionally to this; if we provide
  // an explicit instantiation declaration and we try to deploy to a dylib
  // that does not provide those symbols, we'll get a load-time error.
  #if !defined(_LIBCPP_BUILDING_LIBRARY) &&                                      \
      ((defined(__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__) &&                \
        __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ < 1090) ||                 \
       (defined(__ENVIRONMENT_IPHONE_OS_VERSION_MIN_REQUIRED__) &&               \
        __ENVIRONMENT_IPHONE_OS_VERSION_MIN_REQUIRED__ < 70000))
  #  define _LIBCPP_DO_NOT_ASSUME_STREAMS_EXPLICIT_INSTANTIATION_IN_DYLIB
  #endif

We then drop the explicit instantiation _declaration_ of iostreams from the headers when we can't guarantee that the dylib on the deployment target will contain their definition. This is not terribly pretty, however we could devise a solution similar to that in this case.

Another mechanism we have to work with this kind of issue is availability markup. We basically annotate when functions are introduced in the dylib, and the compiler will issue an error if one tries to use a function in the dylib in conjunction with a deployment target that does not contain that function. This is similar in the sense that both try to prevent forward incompatibilities from hurting, however I would recommend against this approach in the current case because this is only an optimization (`std::string` needs to stay usable when deploying to older platforms, but some performance/code size hit may be reasonable).



================
Comment at: libcxx/include/string:1847
 
-template <class _CharT, class _Traits, class _Allocator>
+template <class _CharT, class _Traits, class _Allocator
+inline
----------------
You removed a closing `>` here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68617/new/

https://reviews.llvm.org/D68617





More information about the libcxx-commits mailing list