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

[Nikolas Klauser via Phabricator via libcxx-commits]

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");
     }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116334