[libc++] Remove the std::string base class

[philnik]

Summary

Removing the base class of std::basic_string is not an ABI break, so we can remove any references to it from the header.

[Mordante]

This change breaks ABI stability in the CI, so it needs more work.

libcxx/include/string

686

The comment contradicts the commit message. But the base indeed seems to be empty.

libcxx/src/string.cpp

24–38

Maybe add a comment here that the struct isn't declared in any header anymore and is only provided here to export the symbols for ABI stability. (It's kind of obvious when you think about it, but can't hurt to explain it for future readers.)

[ldionne]

libcxx/src/string.cpp

41–43

I suggest this instead:

template <bool>
struct __basic_string_common {
    _LIBCPP_NORETURN _LIBCPP_EXPORTED_FROM_ABI void __throw_length_error() const;
    _LIBCPP_NORETURN _LIBCPP_EXPORTED_FROM_ABI void __throw_out_of_range() const;
};

void __basic_string_common<true>::__throw_length_error() const {
    _VSTD::__throw_length_error("basic_string");
}

void __basic_string_common<true>::__throw_out_of_range() const {
    _VSTD::__throw_out_of_range("basic_string");
}

Also, I suggest we rename _LIBCPP_ABI_NO_BASIC_STRING_BASE_CLASS to _LIBCPP_ABI_DONT_EXPORT_BASIC_STRING_COMMON or something along those lines. Basically, the ABI breaking change here would only be that we're not exporting the functions anymore from the dylib.

[philnik]

• Address comments

[Mordante]

LGTM!

[ldionne]

Oooooh, I'm trembling to approve this. If my reasoning on vector_base_common isn't right, this is going to break the whole world (basic_string is like the most used type in the library). But yeah, we've thought it out and it seems legit, so let's go.

[philnik]

In D118733#3295008, @ldionne wrote:

Oooooh, I'm trembling to approve this. If my reasoning on vector_base_common isn't right, this is going to break the whole world (basic_string is like the most used type in the library). But yeah, we've thought it out and it seems legit, so let's go.

What could go wrong? The worst would be that we break thousands of people's software silently :D
Do you cherry-pick the commit onto the release branch?

[dim]

FWIW, this breaks compilation of Firefox:

In file included from Unified_cpp_accessible_base1.cpp:20:
In file included from /wrkdirs/share/dim/ports/www/firefox/work/firefox-97.0/accessible/base/TextLeafRange.cpp:11:
In file included from /wrkdirs/share/dim/ports/www/firefox/work/.build/dist/include/mozilla/a11y/DocAccessibleParent.h:11:
In file included from /wrkdirs/share/dim/ports/www/firefox/work/.build/ipc/ipdl/_ipdlheaders/mozilla/a11y/PDocAccessibleParent.h:9:
In file included from /wrkdirs/share/dim/ports/www/firefox/work/.build/ipc/ipdl/_ipdlheaders/mozilla/a11y/PDocAccessible.h:24:
In file included from /wrkdirs/share/dim/ports/www/firefox/work/.build/dist/include/mozilla/a11y/IPCTypes.h:16:
In file included from /wrkdirs/share/dim/ports/www/firefox/work/.build/dist/include/mozilla/GfxMessageUtils.h:30:
In file included from /wrkdirs/share/dim/ports/www/firefox/work/.build/dist/include/mozilla/ipc/ProtocolUtils.h:27:
In file included from /wrkdirs/share/dim/ports/www/firefox/work/.build/dist/include/mozilla/ipc/MessageChannel.h:23:
In file included from /wrkdirs/share/dim/ports/www/firefox/work/.build/dist/stl_wrappers/stack:64:
In file included from /wrkdirs/share/dim/ports/www/firefox/work/.build/dist/system_wrappers/stack:3:
In file included from /usr/include/c++/v1/stack:105:
In file included from /wrkdirs/share/dim/ports/www/firefox/work/.build/dist/stl_wrappers/deque:64:
In file included from /wrkdirs/share/dim/ports/www/firefox/work/.build/dist/system_wrappers/deque:3:
/usr/include/c++/v1/deque:1854:9: error: call to '__throw_out_of_range' is ambiguous
        _VSTD::__throw_out_of_range("deque");
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/c++/v1/__config:824:15: note: expanded from macro '_VSTD'
#define _VSTD std
              ^
/wrkdirs/share/dim/ports/www/firefox/work/.build/dist/include/mozilla/throw_gcc.h:97:59: note: candidate function
MOZ_THROW_NORETURN MOZ_THROW_EXPORT MOZ_THROW_INLINE void __throw_out_of_range(
                                                          ^
/usr/include/c++/v1/stdexcept:264:6: note: candidate function
void __throw_out_of_range(const char*__msg)
     ^

That said, they're doing very naughty things in e.g. https://hg.mozilla.org/mozilla-central/file/tip/memory/mozalloc/throw_gcc.h#l47 :

namespace std {

// NB: user code is not supposed to touch the std:: namespace.  We're
// doing this after careful review because we want to define our own
// exception throwing semantics.  Don't try this at home!

MOZ_THROW_NORETURN MOZ_THROW_EXPORT MOZ_THROW_INLINE void __throw_bad_exception(
    void) {
  mozalloc_abort("fatal: STL threw bad_exception");
}

[...]

MOZ_THROW_NORETURN MOZ_THROW_EXPORT MOZ_THROW_INLINE void __throw_out_of_range(
    const char* msg) {
  mozalloc_abort(msg);
}

[ldionne]

In D118733#3307739, @dim wrote:

FWIW, this breaks compilation of Firefox:
[...]

Thanks for the heads up. I just sent a courtesy email to a Firefox contributor to make them aware of this problem and CC'd you on the thread. Long story short, they are violating the Standard and there's nothing we can do about it -- we can only be nice and give them a heads up, which I did.

libcxx/include/__config

 // std::random_device holds some state when it uses an implementation that gets
 // entropy from a file (see _LIBCPP_USING_DEV_RANDOM). When switching from this
 // implementation to another one on a platform that has already shipped
 // std::random_device, one needs to retain the same object layout to remain ABI
 // compatible. This switch removes these workarounds for platforms that don't care
 // about ABI compatibility.
 #  define _LIBCPP_ABI_NO_RANDOM_DEVICE_COMPATIBILITY_LAYOUT
 // Remove basic_string common base
-#  define _LIBCPP_ABI_NO_BASIC_STRING_BASE_CLASS
+#  define _LIBCPP_ABI_DO_NOT_EXPORT_BASIC_STRING_COMMON
 #elif _LIBCPP_ABI_VERSION == 1
 #  if !defined(_LIBCPP_OBJECT_FORMAT_COFF)
 // Enable compiling copies of now inline methods into the dylib to support
 // applications compiled against older libraries. This is unnecessary with
 // COFF dllexport semantics, since dllexport forces a non-inline definition
 // of inline functions to be emitted anyway. Our own non-inline copy would
 // conflict with the dllexport-emitted copy, so we disable it.
 #    define _LIBCPP_DEPRECATED_ABI_LEGACY_LIBRARY_DEFINITIONS_FOR_INLINE_FUNCTIONS

libcxx/include/string

 operator+(const basic_string<_CharT, _Traits, _Allocator>& __x, const _CharT* __y);
 
 template<class _CharT, class _Traits, class _Allocator>
 basic_string<_CharT, _Traits, _Allocator>
 operator+(const basic_string<_CharT, _Traits, _Allocator>& __x, _CharT __y);
 
 _LIBCPP_EXTERN_TEMPLATE(_LIBCPP_FUNC_VIS string operator+<char, char_traits<char>, allocator<char> >(char const*, string const&))
 
-#ifndef _LIBCPP_ABI_NO_BASIC_STRING_BASE_CLASS
-template <bool>
-struct __basic_string_common;
-
-template <>
-struct __basic_string_common<true> {
-    // Both are defined in string.cpp
-    _LIBCPP_NORETURN _LIBCPP_EXPORTED_FROM_ABI void __throw_length_error() const;
-    _LIBCPP_NORETURN _LIBCPP_EXPORTED_FROM_ABI void __throw_out_of_range() const;
-};
-#endif
-
 template <class _Iter>
 struct __string_is_trivial_iterator : public false_type {};
 
 template <class _Tp>
 struct __string_is_trivial_iterator<_Tp*>
     : public is_arithmetic<_Tp> {};
 
 template <class _Iter>

 #ifndef _LIBCPP_HAS_NO_CHAR8_T
     _LIBCPP_PREFERRED_NAME(u8string)
 #endif
 #ifndef _LIBCPP_HAS_NO_UNICODE_CHARS
     _LIBCPP_PREFERRED_NAME(u16string)
     _LIBCPP_PREFERRED_NAME(u32string)
 #endif
     basic_string
-#ifndef _LIBCPP_ABI_NO_BASIC_STRING_BASE_CLASS
-    : private __basic_string_common<true> // This base class is historical, but it needs to remain for ABI compatibility
-#endif
 {
 public:
     typedef basic_string                                 __self;
     typedef basic_string_view<_CharT, _Traits>           __self_view;
     typedef _Traits                                      traits_type;
     typedef _CharT                                       value_type;
     typedef _Allocator                                   allocator_type;
     typedef allocator_traits<allocator_type>             __alloc_traits;

libcxx/src/string.cpp

 #include "__debug"
 
 #ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS
 #   include "cwchar"
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
-#ifndef _LIBCPP_ABI_NO_BASIC_STRING_BASE_CLASS
+#ifndef _LIBCPP_ABI_DO_NOT_EXPORT_BASIC_STRING_COMMON
+
+template <bool>
+struct __basic_string_common;
+
+// The struct isn't declared anymore in the headers. It's only here for ABI compatibility.
+template <>
+struct __basic_string_common<true> {
+    _LIBCPP_NORETURN _LIBCPP_EXPORTED_FROM_ABI void __throw_length_error() const;
+    _LIBCPP_NORETURN _LIBCPP_EXPORTED_FROM_ABI void __throw_out_of_range() const;
+};
+
 void __basic_string_common<true>::__throw_length_error() const {
-    _VSTD::__throw_length_error("basic_string");
+    std::__throw_length_error("basic_string");
 }
-
 void __basic_string_common<true>::__throw_out_of_range() const {
-    _VSTD::__throw_out_of_range("basic_string");
+    std::__throw_out_of_range("basic_string");
 }
-#endif
+
+#endif // _LIBCPP_ABI_DO_NOT_EXPORT_BASIC_STRING_COMMON
 
 #define _LIBCPP_EXTERN_TEMPLATE_DEFINE(...) template __VA_ARGS__;
 #ifdef _LIBCPP_ABI_STRING_OPTIMIZED_EXTERNAL_INSTANTIATION
     _LIBCPP_STRING_UNSTABLE_EXTERN_TEMPLATE_LIST(_LIBCPP_EXTERN_TEMPLATE_DEFINE, char)
 #   ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS
         _LIBCPP_STRING_UNSTABLE_EXTERN_TEMPLATE_LIST(_LIBCPP_EXTERN_TEMPLATE_DEFINE, wchar_t)
 #   endif
 #else