[libcxx-commits] [PATCH] D116334: [libc++] Remove std::basic_string's base class in ABIv2

Tue Jan 11 15:22:49 PST 2022

philnik added inline comments.

================
Comment at: libcxx/include/string:1724
     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");
     }
----------------
ldionne wrote:
> philnik wrote:
> > Mordante wrote:
> > > Quuxplusone wrote:
> > > > (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.
> > > (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.
> > > 
> > 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.
> 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.
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116334