Split _LIBCPP_STRING_EXTERN_TEMPLATE_LIST up into a V1 and UNSTABLE version.

[mvels]

Summary

This change splits the _LIBCPP_STRING_EXTERN_TEMPLATE_LIST up into a _LIBCPP_STRING_V1_EXTERN_TEMPLATE_LIST containing the stable ABI, and a _LIBCPP_STRING_UNSTABLE_EXTERN_TEMPLATE_LIST containing the unstable ABI.

The purpose is to explicitly define and maintain the two lists, where the unstable ABI allows for ABI breaking changes for purposes such as optimization while offering a strong guarantee that any change inside the unstable ABI does not affect the stable ABI.

As per the comment in the __string header, we do still allow etries to be added to the stable ABI list as the c++ versions and corresponding c++ std API changes.

Merge branch 'master' into unstable-abi-list

[mvels]

Inline basic_string::erase for fastpath where __n == npos

This change checks for the case where people want to erase a string to the end, i.e., __n == npos, and inlines the call if so.

This also demonstrates keeping the ABI intact for V1, but inlining the erase() method for unstable.

[mvels]

Fix external template list

[mvels]

Fix Phabricator (arc tool is not very bright)

[EricWF]

My personal preference is to not split the lists, my rational is:

• It encourages unstable to deviate from stable more. I would like us to keep the code and instantiations between the two as similar as possible. The more they differ, the more it is to maintain and the more likely it is that one contains bugs or starts to rot.
• It's useful to see the difference between stable and unstable at a glance. Having two lists makes it much harder.

For reference, here's my attempt at a similar thing:
https://reviews.llvm.org/D73807

libcxx/include/__string

76

We should add a design doc for all @mvels work on std::string and its instantiations.

See http://libcxx.llvm.org/docs/#design-documents for a list of current design docs, so you can see how libc++ does them.
They can be found in the source tree under libcxx/docs/

[EricWF]

libcxx/include/__string

76

Note: I don't want the doc to hold up these changes specifically, but we should work towards adding one.

[ldionne]

In D74870#1883765, @EricWF wrote:

My personal preference is to not split the lists, my rational is:

• It encourages unstable to deviate from stable more. I would like us to keep the code and instantiations between the two as similar as possible. The more they differ, the more it is to maintain and the more likely it is that one contains bugs or starts to rot.

I actually think that acknowledging that they are different instantiation lists makes everything clearer and easier to maintain. We don't "pretend" that they are close when they are really two different things. As we embrace changes in the unstable version of the ABI, I don't see why we'd strive to keep it close to the stable ABI. We should simply setup testers for both ABIs, since both seem to be in use.

• It's useful to see the difference between stable and unstable at a glance. Having two lists makes it much harder.

Do you really expect to need to see that difference often? If so, we could sort the lists and then diff would show you the differences easily.

My primary concern is that I find it harder to maintain a list strung together via macro expansions than a manually laid-out list like in this patch. It also allows us to have an invariant, which is that the stable list should never change (except with extreme care).

[ldionne]

I personally like this change so I'm going to approve it, but let's wait to see if we can get @EricWF 's approval too. If not, we'll have to find another way.

[mvels]

Thanks for the Feedback Eric, personally I think this version or your alternative are close, and both are based on the same (solid) principle of 'make the ABI explicit'

I do like (and prefer) Luis preference of explicitly specifying (decoupling) the V1 and UNSTABLE template list.
While somewhat more verbose, it's cleaner, and easy to validate 'unstable' changes vs 'extend new c++ API in stable'

I also think we may have to consider a future where we may want to roll unstable into a new V2 stable version (and new V3 / unstable).
Having the tables separate makes this imo a cleaner cut. (And I had an interesting discussion with Titus this morning how we should actually consider any future Vn versions as a new llvm-project rather than introducing versions, alas, that's crazy talk).

Anyway, I don't think we are strongly married into one or the other, if we don't like it for whatever reason, the changes are mostly re-arranging the explicit template macros in __string and a handful of lines in string / string.cpp, likely less than the heavy lifting you already did for making the external templates explicit (again, thanks for that!) . From out chat I understand that you are ok with this version if there is a majority preference, let's meet up when you are back next week.

I am working on two documents, one outlining the motivation, consequences and options for tweaking the unstable ABI, and a separate doc summarizing the intended string optimizations (some in detail and general principles). Hopefully I have something worth sharing tomorrow, thanks!

[EricWF]

libcxx/include/string

4341

We always introduce a named macro for each ABI break rather than gating them generically.

Please fix this in upstream.

libcxx/src/string.cpp

23

We always introduce a named macro for each ABI break rather than gating them generically.

Please fix this in upstream.

[EricWF]

In D74870#1884581, @ldionne wrote:

In D74870#1883765, @EricWF wrote:

My personal preference is to not split the lists, my rational is:

• It encourages unstable to deviate from stable more. I would like us to keep the code and instantiations between the two as similar as possible. The more they differ, the more it is to maintain and the more likely it is that one contains bugs or starts to rot.

I actually think that acknowledging that they are different instantiation lists makes everything clearer and easier to maintain. We don't "pretend" that they are close when they are really two different things. As we embrace changes in the unstable version of the ABI, I don't see why we'd strive to keep it close to the stable ABI. We should simply setup testers for both ABIs, since both seem to be in use.

Ideally we'll keep the unstable and stable ABI lists as close as possible, with the goal of moving all unstable instantiations into stable.

• It's useful to see the difference between stable and unstable at a glance. Having two lists makes it much harder.

Do you really expect to need to see that difference often? If so, we could sort the lists and then diff would show you the differences easily.

My primary concern is that I find it harder to maintain a list strung together via macro expansions than a manually laid-out list like in this patch. It also allows us to have an invariant, which is that the stable list should never change (except with extreme care).

[ldionne]

In D74870#1888514, @EricWF wrote:

In D74870#1884581, @ldionne wrote:

In D74870#1883765, @EricWF wrote:

My personal preference is to not split the lists, my rational is:

• It encourages unstable to deviate from stable more. I would like us to keep the code and instantiations between the two as similar as possible. The more they differ, the more it is to maintain and the more likely it is that one contains bugs or starts to rot.

I actually think that acknowledging that they are different instantiation lists makes everything clearer and easier to maintain. We don't "pretend" that they are close when they are really two different things. As we embrace changes in the unstable version of the ABI, I don't see why we'd strive to keep it close to the stable ABI. We should simply setup testers for both ABIs, since both seem to be in use.

Ideally we'll keep the unstable and stable ABI lists as close as possible, with the goal of moving all unstable instantiations into stable.

Are you saying that we'd want to put explicit instantiations used by the unstable ABI into the .so in the stable ABI, too? I don't understand why we'd do that?

libcxx/include/__string

 _LIBCPP_PUSH_MACROS
 #include <__undef_macros>
 
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 // The the extern template ABI lists are kept outside of <string> to improve the
 // readability of that header.
-#define _LIBCPP_STRING_EXTERN_TEMPLATE_LIST(_Func, _CharType) \
+
+// The extern template ABI lists are kept outside of <string> to improve the
+// readability of that header. We maintain 2 ABI lists:
+// - _LIBCPP_STRING_V1_EXTERN_TEMPLATE_LIST
+// - _LIBCPP_STRING_UNSTABLE_EXTERN_TEMPLATE_LIST
+// As the name implies, the ABI lists define the V1 (Stable) and unstable ABI.
+//
+// For unstable, we may explicitly remove function that are external in V1,
+// and add (new) external functions to better control inlining and compiler
+// optimization opportunities.
+//
+// For stable, the ABI list should rarely change, except for adding new
+// functions supporting new c++ version / API changes. Typically entries
+// must never be removed from the stable list.
+#define _LIBCPP_STRING_V1_EXTERN_TEMPLATE_LIST(_Func, _CharType) \
+  _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>& basic_string<_CharType>::replace(size_type, size_type, value_type const*, size_type)) \
+  _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>::size_type basic_string<_CharType>::rfind(value_type const*, size_type, size_type) const) \
+  _Func(_LIBCPP_FUNC_VIS void basic_string<_CharType>::__init(value_type const*, size_type, size_type)) \
+  _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>::basic_string(basic_string const&)) \
+  _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>& basic_string<_CharType>::replace(size_type, size_type, value_type const*)) \
+  _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>::basic_string(basic_string const&, std::allocator<_CharType> const&)) \
+  _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>::size_type basic_string<_CharType>::find_last_not_of(value_type const*, size_type, size_type) const) \
+  _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>::~basic_string()) \
+  _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>::size_type basic_string<_CharType>::find_first_not_of(value_type const*, size_type, size_type) const) \
+  _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>& basic_string<_CharType>::insert(size_type, size_type, value_type)) \
+  _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>& basic_string<_CharType>::operator=(value_type)) \
+  _Func(_LIBCPP_FUNC_VIS void basic_string<_CharType>::__init(value_type const*, size_type)) \
+  _Func(_LIBCPP_FUNC_VIS const _CharType& basic_string<_CharType>::at(size_type) const) \
+  _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>& basic_string<_CharType>::insert(size_type, value_type const*, size_type)) \
+  _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>::size_type basic_string<_CharType>::find_first_of(value_type const*, size_type, size_type) const) \
+  _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>& basic_string<_CharType>::replace(size_type, size_type, size_type, value_type)) \
+  _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>& basic_string<_CharType>::assign(value_type const*, size_type)) \
+  _Func(_LIBCPP_FUNC_VIS void basic_string<_CharType>::reserve(size_type)) \
+  _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>& basic_string<_CharType>::append(value_type const*, size_type)) \
+  _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>& basic_string<_CharType>::assign(basic_string const&, size_type, size_type)) \
+  _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>::size_type basic_string<_CharType>::copy(value_type*, size_type, size_type) const) \
+  _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>::basic_string(basic_string const&, size_type, size_type, std::allocator<_CharType> const&)) \
+  _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>::size_type basic_string<_CharType>::find(value_type, size_type) const) \
+  _Func(_LIBCPP_FUNC_VIS void basic_string<_CharType>::__init(size_type, value_type)) \
+  _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>& basic_string<_CharType>::insert(size_type, value_type const*)) \
+  _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>::size_type basic_string<_CharType>::find_last_of(value_type const*, size_type, size_type) const) \
+  _Func(_LIBCPP_FUNC_VIS void basic_string<_CharType>::__grow_by(size_type, size_type, size_type, size_type, size_type, size_type)) \
+  _Func(_LIBCPP_FUNC_VIS void basic_string<_CharType>::__grow_by_and_replace(size_type, size_type, size_type, size_type, size_type, size_type, value_type const*)) \
+  _Func(_LIBCPP_FUNC_VIS void basic_string<_CharType>::push_back(value_type)) \
+  _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>& basic_string<_CharType>::append(size_type, value_type)) \
+  _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>::size_type basic_string<_CharType>::rfind(value_type, size_type) const) \
+  _Func(_LIBCPP_FUNC_VIS const basic_string<_CharType>::size_type basic_string<_CharType>::npos) \
+  _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>& basic_string<_CharType>::assign(size_type, value_type)) \
+  _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>& basic_string<_CharType>::erase(size_type, size_type)) \
+  _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>& basic_string<_CharType>::append(basic_string const&, size_type, size_type)) \
+  _Func(_LIBCPP_FUNC_VIS int basic_string<_CharType>::compare(value_type const*) const) \
+  _Func(_LIBCPP_FUNC_VIS int basic_string<_CharType>::compare(size_type, size_type, value_type const*) const) \
+  _Func(_LIBCPP_FUNC_VIS _CharType& basic_string<_CharType>::at(size_type)) \
+  _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>& basic_string<_CharType>::assign(value_type const*)) \
+  _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>::size_type basic_string<_CharType>::find(value_type const*, size_type, size_type) const) \
+  _Func(_LIBCPP_FUNC_VIS int basic_string<_CharType>::compare(size_type, size_type, basic_string const&, size_type, size_type) const) \
+  _Func(_LIBCPP_FUNC_VIS int basic_string<_CharType>::compare(size_type, size_type, value_type const*, size_type) const) \
+  _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>& basic_string<_CharType>::operator=(basic_string const&)) \
+  _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>& basic_string<_CharType>::append(value_type const*)) \
+  _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>& basic_string<_CharType>::replace(size_type, size_type, basic_string const&, size_type, size_type)) \
+  _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>::iterator basic_string<_CharType>::insert(basic_string::const_iterator, value_type)) \
+  _Func(_LIBCPP_FUNC_VIS void basic_string<_CharType>::resize(size_type, value_type)) \
+  _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>& basic_string<_CharType>::insert(size_type, basic_string const&, size_type, size_type))
+
+#define _LIBCPP_STRING_UNSTABLE_EXTERN_TEMPLATE_LIST(_Func, _CharType) \
   _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>& basic_string<_CharType>::replace(size_type, size_type, value_type const*, size_type)) \
   _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>::size_type basic_string<_CharType>::rfind(value_type const*, size_type, size_type) const) \
   _Func(_LIBCPP_FUNC_VIS void basic_string<_CharType>::__init(value_type const*, size_type, size_type)) \
   _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>::basic_string(basic_string const&)) \
   _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>& basic_string<_CharType>::replace(size_type, size_type, value_type const*)) \
   _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>::basic_string(basic_string const&, std::allocator<_CharType> const&)) \
   _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>::size_type basic_string<_CharType>::find_last_not_of(value_type const*, size_type, size_type) const) \
   _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>::~basic_string()) \

libcxx/include/string

 basic_string<_CharT, _Traits, _Allocator>::__subscriptable(const const_iterator* __i, ptrdiff_t __n) const
 {
     const value_type* __p = _VSTD::__to_address(__i->base()) + __n;
     return this->data() <= __p && __p < this->data() + this->size();
 }
 
 #endif  // _LIBCPP_DEBUG_LEVEL >= 2
 
-_LIBCPP_STRING_EXTERN_TEMPLATE_LIST(_LIBCPP_EXTERN_TEMPLATE, char)
-_LIBCPP_STRING_EXTERN_TEMPLATE_LIST(_LIBCPP_EXTERN_TEMPLATE, wchar_t)
+#if defined(_LIBCPP_ABI_UNSTABLE) || _LIBCPP_ABI_VERSION >= 2
+_LIBCPP_STRING_UNSTABLE_EXTERN_TEMPLATE_LIST(_LIBCPP_EXTERN_TEMPLATE, char)
+_LIBCPP_STRING_UNSTABLE_EXTERN_TEMPLATE_LIST(_LIBCPP_EXTERN_TEMPLATE, wchar_t)
+#else
+_LIBCPP_STRING_V1_EXTERN_TEMPLATE_LIST(_LIBCPP_EXTERN_TEMPLATE, char)
+_LIBCPP_STRING_V1_EXTERN_TEMPLATE_LIST(_LIBCPP_EXTERN_TEMPLATE, wchar_t)
+#endif
 
 #if _LIBCPP_STD_VER > 11
 // Literal suffixes for basic_string [basic.string.literals]
 inline namespace literals
 {
   inline namespace string_literals
   {
     inline _LIBCPP_INLINE_VISIBILITY

libcxx/src/string.cpp

 #include "stdexcept"
 #include <stdio.h>
 #include "__debug"
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 template class _LIBCPP_CLASS_TEMPLATE_INSTANTIATION_VIS __basic_string_common<true>;
 
-_LIBCPP_STRING_EXTERN_TEMPLATE_LIST(_LIBCPP_EXTERN_TEMPLATE_DEFINE, char)
-_LIBCPP_STRING_EXTERN_TEMPLATE_LIST(_LIBCPP_EXTERN_TEMPLATE_DEFINE, wchar_t)
+#if defined(_LIBCPP_ABI_UNSTABLE) || _LIBCPP_ABI_VERSION >= 2
+_LIBCPP_STRING_UNSTABLE_EXTERN_TEMPLATE_LIST(_LIBCPP_EXTERN_TEMPLATE_DEFINE, char)
+_LIBCPP_STRING_UNSTABLE_EXTERN_TEMPLATE_LIST(_LIBCPP_EXTERN_TEMPLATE_DEFINE, wchar_t)
+#else
+_LIBCPP_STRING_V1_EXTERN_TEMPLATE_LIST(_LIBCPP_EXTERN_TEMPLATE_DEFINE, char)
+_LIBCPP_STRING_V1_EXTERN_TEMPLATE_LIST(_LIBCPP_EXTERN_TEMPLATE_DEFINE, wchar_t)
+#endif
 
 template
     string
     operator+<char, char_traits<char>, allocator<char> >(char const*, string const&);
 
 namespace
 {