[libcxx-commits] [PATCH] D115906: [libc++] Add a bunch of missing inline and _LIBCPP_HIDE_FROM_ABI in __threading_support

Mon Jan 10 10:57:35 PST 2022

DanielMcIntosh-IBM added a comment.

In D115906#3231458 <https://reviews.llvm.org/D115906#3231458>, @ldionne wrote:

> For the Pthread case, using `_LIBCPP_THREAD_ABI_VISIBILITY` and `inline _LIBCPP_HIDE_FROM_ABI` is equivalent.

Is it not the case for `_LIBCPP_HAS_THREAD_API_C11`?

In D115906#3231458 <https://reviews.llvm.org/D115906#3231458>, @ldionne wrote:

> I find this code rather confusing and I'm going to refactor it to disentangle the different implementations.

I do agree that the current situation is rather confusing and should be refactored, or at least better documented. It took way too long for me to figure all of the above out, and I still don't know what's going on with `_LIBCPP_HAS_THREAD_API_WIN32`. This was a significant barrier to implementing your recommendations in D110349#3126010 <https://reviews.llvm.org/D110349#3126010> (which are in the process of getting reviewed downstream in addition to some additional documentation). I've been considering trying to clean things up a bit myself, but there are other pressing issues I need to work on in the short and medium term. Can you add me as a subscriber to all of your changes refactoring `__threading_support`?

In D115906#3231458 <https://reviews.llvm.org/D115906#3231458>, @ldionne wrote:

> I'm not sure how this change would break `_LIBCPP_BUILDING_THREAD_LIBRARY_EXTERNAL` though?

According to DesignDocs/ThreadingSupportAPI <https://libcxx.llvm.org/DesignDocs/ThreadingSupportAPI>, `_LIBCPP_BUILDING_THREAD_LIBRARY_EXTERNAL` "is used to build an external threading library using the `<__threading_support>`. Specifically it exposes the threading API definitions in `<__threading_support>` as non-inline definitions meant to be compiled into a library." With this change, the definitions are always inline, so that doesn't work anymore.

In more detail, my understanding of the various macros thus far is as follows (please correct me if I've misunderstood):

The API (i.e. the typedefs and forward declarations `__threading_support` provides) is determined by which (if any) of the following macros is defined: `_LIBCPP_HAS_THREAD_API_EXTERNAL`, `_LIBCPP_HAS_THREAD_API_PTHREAD`, `_LIBCPP_HAS_THREAD_API_C11` or none/generic (with `_LIBCPP_HAS_THREAD_API_EXTERNAL`, `__threading_support` provides the typedefs and forward declarations indirectly by #including `__external_threading`). `_LIBCPP_HAS_THREAD_API_WIN32` is a bit of an oddball here, and I'm not really sure I understand it yet.

Independent of which API is used, the function definitions can either be provided by `__threading_support`, or by an external library when `_LIBCPP_HAS_THREAD_LIBRARY_EXTERNAL` has been defined (Even when `_LIBCPP_HAS_THREAD_API_EXTERNAL` is the chosen API, if `__external_threading` provides definitions in addition to typedefs and forward declarations,
the function definitions can theoretically still be provided by `__threading_support` rather than an external library. However the cmake files forbid this).

With an external thread library, libc++ is detached from the underlying threading support library, allowing the user/vendor to provide one of their choosing. However, this would cause problems testing libc++ since the threading library is provided by the user/vendor, meaning we don't have one to use for testing. To work around this, we can build an external library out of the definitions that `__threading_support` would normally provide inline. This is done using `-DLIBCXX_BUILD_EXTERNAL_THREAD_LIBRARY` which builds libcxx/test/support/external_threads.cpp into a library. libcxx/test/support/external_threads.cpp does nothing more than `#define _LIBCPP_BUILDING_THREAD_LIBRARY_EXTERNAL` and `#include <__threading_support>`. As explained above, `_LIBCPP_BUILDING_THREAD_LIBRARY_EXTERNAL` causes `__threading_support` to provide the definitions as non-inline and with _LIBCPP_FUNC_VIS visibility. This way libc++ can be tested with an external threading library as long as `__threading_support` has function definitions that use a base thread library (e.g. pthread) that the platform it is getting tested on supports.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115906