This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Refactor how basic_string and vector hoist exception-throwing functions
ClosedPublic

Authored by ldionne on Oct 5 2021, 11:44 AM.

Details

Summary

In basic_string and vector, we've been encapsulating all exception
throwing code paths in helper functions of a base class, which are defined
in the compiled library. For example, vector_base_common defines two
methods,
throw_length_error() and __throw_out_of_range(), and the class
is externally instantiated in the library. This was done a long time ago,
but after investigating, I believe the goal of the current design was to:

  1. 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.
  1. Avoid defining this exception-throwing function once per instantiation of the class template. In other words, we want a single copy of __throw_length_error even if we have vector<int>, vector<char>, etc.
  1. Encapsulate the passing of the container-specific string (i.e. "vector" and "basic_string") to the underlying exception-throwing function so that object files don't contain those duplicated string literals. For example, we'd like to have a single "vector" string literal for passing to std::__throw_length_error in the library, instead of having one per translation unit.

However, the way this is achieved right now has two problems:

  • Using a base class and exporting it is really weird - I've been confused about this ever since I first saw it. It's just a really unusual way of achieving the above goals. Also, it's made even worse by the fact that the definitions of throw_length_error and throw_out_of_range appear in the headers despite always being intended to be defined in the compiled library (via the extern template instantiation).
  • We end up exporting those functions as weak symbols, which isn't great for load times. Instead, it would be better to export those as strong symbols from the library.

This patch fixes those issues while retaining ABI compatibility (e.g. we
still export the exact same symbols as before). Note that we need to
keep the base classes as-is to avoid breaking the ABI of someone who
might inherit from std::basic_string or std::vector.

Diff Detail

Event Timeline

ldionne requested review of this revision.Oct 5 2021, 11:44 AM
ldionne created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptOct 5 2021, 11:44 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne accepted this revision.Oct 5 2021, 2:26 PM
This revision is now accepted and ready to land.Oct 5 2021, 2:26 PM
ldionne updated this revision to Diff 377356.Oct 5 2021, 2:34 PM
ldionne edited the summary of this revision. (Show Details)

Don't touch the base classes since that's an ABI break.

ldionne updated this revision to Diff 377358.Oct 5 2021, 2:35 PM

Add comments about ABI compat of base classes

Mordante added inline comments.
libcxx/include/string
677

Do we want to remove this is ABI v2?