[libc++] Remove std::basic_string's base class in ABIv2

[philnik]

Summary

Remove std::basic_string's base class in ABI version 2

[Mordante]

Interesting, I thought this looked somewhat familiar and it's indeed something that I expected to be done in D111173.

I like to hear @ldionne's opinion why it wasn't done in D111173

libcxx/include/__config

111

This looks more consistent with the other ABI macros.

libcxx/include/string

626

This should also be guarded by the ABI macro.

[• Quuxplusone]

libcxx/include/__config

111

I agree that the name shouldn't involve REMOVE; it should describe the new state of affairs, not just a diff against the obsolete state of affairs. FWLIW, I'd have said _LIBCPP_ABI_NO_BASIC_STRING_BASE_CLASS — NO_STRING_BASE just seems too short/vague. But I acknowledge I'm suffering from a little bit of "obscure things should have obscure scary names" (the same reason we've written template<class ...> in front of templates for 30 years, you know), and _LIBCPP_ABI_NO_STRING_BASE is consistent with _LIBCPP_ABI_NO_BINDER_BASES and _LIBCPP_ABI_NO_ITERATOR_BASES, both of which I myself introduced.

Also, I notice that _LIBCPP_ABI_ALTERNATE_STRING_LAYOUT already exists. I suspect we cannot combine these two ABI flags into one, because that would mess with people who are (unsupportedly) a-la-carte'ing that flag today. But I'd like to hear @ldionne's take on that idea anyway.

For reference, right now our two string-related ABI flags are _LIBCPP_ABI_ALTERNATE_STRING_LAYOUT and _LIBCPP_ABI_STRING_OPTIMIZED_EXTERNAL_INSTANTIATION. Yet another option for this one would be _LIBCPP_ABI_STRING_NO_BASE_CLASS.

TLDR: I'm reasonably happy with @Mordante's suggested edit. ;)

libcxx/include/string

1724

(1) My understanding is that the _LIBCPP_NO_EXCEPTIONS branch needs to remain. Someone who compiles with -D_LIBCPP_NO_EXCEPTIONS must not see any calls to __throw_out_of_range popping up at link time.

(2) I have the (more nebulous) impression that we call a dedicated library function __basic_string_common<true>::__throw_out_of_range() instead of __throw_out_of_range("basic_string") on purpose, because std::string is used so heavily, and we don't want the user to pay for the 13-byte string literal "basic_string" in every single one of their TUs. (This rationale does not apply in lesser-used places such as deque.) However, off the top of my head I don't know that we have ever benchmarked this or that anyone really cares.

[Mordante]

libcxx/include/string

1724

(1) No that branch can be removed. I had the same initial feeling but include/stdexcept has the branch.

(2) That may be an issue indeed. I wonder whether the linker is/can be smart enough to remove the duplicates.

[philnik]

libcxx/include/string

1724

I ran a few tests now. My general conclusion would be to put the throws directly into the header. The TU size doesn't really matter I think, because even changing the build location can have a bigger impact on the build directory size. (At least with CMake)

I used export LINK_FLAGS="-nostdlib++ -L$LLVM/default/build/lib -Wl,-rpath,$LLVM/default/build/lib -lc++"; CC=clang CXX=clang++ cmake -G Ninja -DLLVM_ENABLE_PROJECTS="llvm;clang" -DCMAKE_BUILD_TYPE=Release ../../../llvm/ -DCMAKE_CXX_FLAGS="-nostdinc++ -nostdlib++ -isystem $LLVM/default/build/include/c++/v1" -DCMAKE_EXE_LINKER_FLAGS="$LINK_FLAGS" -DCMAKE_SHARED_LINKER_FLAGS="$LINK_FLAGS" -DLLVM_USE_LLD=On -DLLVM_ENABLE_EH=On -DLLVM_ENABLE_RTTI=On (-DLLVM_ENABLE_LTO=Thin) to generate the build.
I got these results:

throw in header	ThinLTO	clang-14 binary size (bytes)
yes	no	163250384
no	no	163250400
yes	yes	182829280
no	yes	182839312

I guess that especially with ThinLTO the functions can now be inlined or eliminated.
I also timed the first tests but there was no significant difference between the two.

[philnik]

• Renamed macro to _LIBCPP_ABI_NO_STRING_BASE
• Guard __basic_string_common

[philnik]

ping

[• Quuxplusone]

All of my comments have been addressed. I think the only remaining question is "do we want to do this," which is above my pay grade. :) @ldionne ping?

[ldionne]

In D116334#3211435, @Mordante wrote:

I like to hear @ldionne's opinion why it wasn't done in D111173

Probably oversight. In D111173, I was solving a specific problem I saw internally, and I had no reason to expand the scope of the patch to removing the base class in the unstable ABI.

I am favourable to this change, but we'll have to tackle my comment (or show me I'm wrong).

Also, perhaps we should have a similar ABI flag for std::vector, which does the same thing?

libcxx/include/__config

111

Also, I notice that _LIBCPP_ABI_ALTERNATE_STRING_LAYOUT already exists. I suspect we cannot combine these two ABI flags into one, because that would mess with people who are (unsupportedly) a-la-carte'ing that flag today. But I'd like to hear @ldionne's take on that idea anyway.

Indeed, ABI flags are meant to be a-la-carte'ed. For example, when we shipped libc++ on ARM64, we enabled the new string layout (see __config):

#if defined(__APPLE__) && !defined(__i386__) && !defined(__x86_64__) &&       \
    (!defined(__arm__) || __ARM_ARCH_7K__ >= 2)
#  define _LIBCPP_ABI_ALTERNATE_STRING_LAYOUT
#endif

So individual ABI flags need to stay stable once they are introduced.

Regarding the name, I much prefer _LIBCPP_ABI_NO_BASIC_STRING_BASE_CLASS too. It more clearly explains what it does.

libcxx/include/string

1724

If you read the description in D111173, I explain that one of the ideas behind hiding the exception-throwing-business behind a function call is this:

Encapsulate the code to throw an exception (which is non-trivial) in an externally-defined function so that the important code paths that call it (e.g. vector::at) are free from that code. Basically, the intent is for the "hot" code path to contain a single conditional jump (based on checking the error condition) to an externally-defined function, which handles all the exception-throwing business.

If you look at the code generation for std::vector::at before and after your change, you should see that it's more complicated after the change, because there's code to throw the exception. Before, it was a conditional jump to the function defined in the dylib, which then contained that business. I remember looking at the codegen and coming to that conclusion.

This patch shouldn't regress that.

[philnik]

libcxx/include/string

1724

I don't know what you have looked at exactly, but I can't reproduce your problem. It seems to be that clang actually goes out of its way to not include the call in the hot path:

With llvm-objdump --disassemble-symbols=_ZNSt3__112basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEE2atEm libc++.so.1 --x86-asm-syntax=intel | c++filt I get:

libc++.so.1:	file format elf64-x86-64

Disassembly of section .text:

0000000000054ec0 <std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::at(unsigned long)>:
   54ec0: 50                           	push	rax
   54ec1: 0f b6 07                     	movzx	eax, byte ptr [rdi]
   54ec4: a8 01                        	test	al, 1
   54ec6: 74 06                        	je	0x54ece <std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::at(unsigned long)+0xe>
   54ec8: 48 8b 4f 08                  	mov	rcx, qword ptr [rdi + 8]
   54ecc: eb 06                        	jmp	0x54ed4 <std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::at(unsigned long)+0x14>
   54ece: 48 89 c1                     	mov	rcx, rax
   54ed1: 48 d1 e9                     	shr	rcx
   54ed4: 48 39 f1                     	cmp	rcx, rsi
   54ed7: 76 1c                        	jbe	0x54ef5 <std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::at(unsigned long)+0x35>
   54ed9: a8 01                        	test	al, 1
   54edb: 74 0c                        	je	0x54ee9 <std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::at(unsigned long)+0x29>
   54edd: 48 8b 7f 10                  	mov	rdi, qword ptr [rdi + 16]
   54ee1: 48 01 f7                     	add	rdi, rsi
   54ee4: 48 89 f8                     	mov	rax, rdi
   54ee7: 59                           	pop	rcx
   54ee8: c3                           	ret
   54ee9: 48 83 c7 01                  	add	rdi, 1
   54eed: 48 01 f7                     	add	rdi, rsi
   54ef0: 48 89 f8                     	mov	rax, rdi
   54ef3: 59                           	pop	rcx
   54ef4: c3                           	ret
   54ef5: e8 f6 e3 ff ff               	call	0x532f0 <std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::__throw_out_of_range() const>
   54efa: 66 0f 1f 44 00 00            	nop	word ptr [rax + rax]

Note the extra lines 54ef5 and 54efa. Clang went out of its way to even just have a jump (54ecc) in the hot path.

[philnik]

ping @ldionne

[ldionne]

Can we please change the name to _LIBCPP_ABI_NO_BASIC_STRING_BASE_CLASS?

libcxx/include/string

1724

So I looked at this again, and indeed I am unable to reproduce what I was seeing, although I do clearly remember seeing *something*, otherwise it would have been a no-brainer to just scratch all the code and just call _VSTD::__throw_foo() back in D111173. But hey, I won't block this improvement if I can't find a downside!

Godbolt does show me they are the same: https://godbolt.org/z/rjshvGxoG

[philnik]

• Rename macro

libcxx/include/__config

 #  define _LIBCPP_ABI_SPAN_POINTER_ITERATORS
 // Re-worked external template instantiations for std::string with a focus on
 // performance and fast-path inlining.
 #  define _LIBCPP_ABI_STRING_OPTIMIZED_EXTERNAL_INSTANTIATION
 // Enable clang::trivial_abi on std::unique_ptr.
 #  define _LIBCPP_ABI_ENABLE_UNIQUE_PTR_TRIVIAL_ABI
 // Enable clang::trivial_abi on std::shared_ptr and std::weak_ptr
 #  define _LIBCPP_ABI_ENABLE_SHARED_PTR_TRIVIAL_ABI
+// Remove basic_string common base
+#  define _LIBCPP_ABI_STRING_REMOVE_BASE
 #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

 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;
 };
 
 template <class _Iter>
 struct __string_is_trivial_iterator : public false_type {};
 
 template <class _Tp>
 struct __string_is_trivial_iterator<_Tp*>
     : public is_arithmetic<_Tp> {};
 

 #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_STRING_REMOVE_BASE
     : 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;
#endif
     _LIBCPP_INLINE_VISIBILITY
     bool __addr_in_range(_Tp&& __t) const {
         const volatile void *__p = _VSTD::addressof(__t);
         return data() <= __p && __p <= data() + size();
     }
 
     _LIBCPP_NORETURN _LIBCPP_HIDE_FROM_ABI
     void __throw_length_error() const {
-#ifndef _LIBCPP_NO_EXCEPTIONS
-        __basic_string_common<true>::__throw_length_error();
-#else
-        _VSTD::abort();
-#endif
+        _VSTD::__throw_length_error("basic_string");
     }
 
     _LIBCPP_NORETURN _LIBCPP_HIDE_FROM_ABI
     void __throw_out_of_range() const {
-#ifndef _LIBCPP_NO_EXCEPTIONS
-        __basic_string_common<true>::__throw_out_of_range();
-#else
-        _VSTD::abort();
-#endif
+        _VSTD::__throw_out_of_range("basic_string");
     }
 
     friend basic_string operator+<>(const basic_string&, const basic_string&);
     friend basic_string operator+<>(const value_type*, const basic_string&);
     friend basic_string operator+<>(value_type, const basic_string&);
     friend basic_string operator+<>(const basic_string&, const value_type*);
     friend basic_string operator+<>(const basic_string&, value_type);
 };

libcxx/src/string.cpp

 #include "__debug"
 
 #ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS
 #   include "cwchar"
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
+#ifndef _LIBCPP_ABI_STRING_REMOVE_BASE
 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");
 }
+#endif
 
 #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