[libc++] Add a bunch of missing inline and _LIBCPP_HIDE_FROM_ABI in __threading_support

[ldionne]

Summary

The inline keyword is required on those functions because they are defined
in the headers, so we need them to be inline to avoid ODR violations.
While we're at it, slap _LIBCPP_HIDE_FROM_ABI on them because they are
implementation details and we don't want them to be part of our ABI under
any circumstances.

[DanielMcIntosh-IBM]

Should this have used _LIBCPP_THREAD_ABI_VISIBILITY instead? Won't this break _LIBCPP_BUILDING_THREAD_LIBRARY_EXTERNAL? Also, why is this needed when the forward declarations already specify that the functions are inline?

[ldionne]

In D115906#3225588, @DanielMcIntosh-IBM wrote:

Should this have used _LIBCPP_THREAD_ABI_VISIBILITY instead? Won't this break _LIBCPP_BUILDING_THREAD_LIBRARY_EXTERNAL? Also, why is this needed when the forward declarations already specify that the functions are inline?

For the Pthread case, using _LIBCPP_THREAD_ABI_VISIBILITY and inline _LIBCPP_HIDE_FROM_ABI is equivalent. However your'e right, this doesn't look necessary to me after all since they are all forward declared above with the correct visibility anyways. I find this code rather confusing and I'm going to refactor it to disentangle the different implementations.

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

[DanielMcIntosh-IBM]

In 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, @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 everything below 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 (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, @ldionne wrote:

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

According to 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 and hidden from the ABI, 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 indirectly 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.

If the definitions in __threading_support are always inline and hidden from the ABI, we cannot build libcxx/test/support/external_threads.cpp into a library that is usable as an external threading library.

[DanielMcIntosh-IBM]

Ping, since as per my previous comment, I'm pretty sure this change will have broken _LIBCPP_BUILDING_THREAD_LIBRARY_EXTERNAL.

[miyuki]

In D115906#3308435, @DanielMcIntosh-IBM wrote:

Ping, since as per my previous comment, I'm pretty sure this change will have broken _LIBCPP_BUILDING_THREAD_LIBRARY_EXTERNAL.

Yes, indeed it has.

[ldionne]

In D115906#3310651, @miyuki wrote:

In D115906#3308435, @DanielMcIntosh-IBM wrote:

Ping, since as per my previous comment, I'm pretty sure this change will have broken _LIBCPP_BUILDING_THREAD_LIBRARY_EXTERNAL.

Yes, indeed it has.

Sorry, this went under my radar. I just spun off a patch to revert this. If it passes, I'll cherry-pick onto LLVM 14.

libcxx/include/__threading_support

 };
 
 #if (!defined(_LIBCPP_HAS_THREAD_LIBRARY_EXTERNAL) || \
      defined(_LIBCPP_BUILDING_THREAD_LIBRARY_EXTERNAL))
 
 
 namespace __thread_detail {
 
-inline __libcpp_timespec_t __convert_to_timespec(const chrono::nanoseconds& __ns)
+_LIBCPP_HIDE_FROM_ABI inline
+__libcpp_timespec_t __convert_to_timespec(const chrono::nanoseconds& __ns)
 {
   using namespace chrono;
   seconds __s = duration_cast<seconds>(__ns);
   __libcpp_timespec_t __ts;
   typedef decltype(__ts.tv_sec) __ts_sec;
   const __ts_sec __ts_sec_max = numeric_limits<__ts_sec>::max();
 
   if (__s.count() < __ts_sec_max)
__libcpp_timespec_t __convert_to_timespec(const chrono::nanoseconds& __ns)
 
   return __ts;
 }
 
 } // namespace __thread_detail
 
 #if defined(_LIBCPP_HAS_THREAD_API_PTHREAD)
 
+_LIBCPP_HIDE_FROM_ABI inline
 int __libcpp_recursive_mutex_init(__libcpp_recursive_mutex_t *__m)
 {
   pthread_mutexattr_t attr;
   int __ec = pthread_mutexattr_init(&attr);
   if (__ec)
     return __ec;
   __ec = pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE);
   if (__ec) {
     pthread_mutexattr_destroy(&attr);
     return __ec;
   }
   __ec = pthread_mutex_init(__m, &attr);
   if (__ec) {
     pthread_mutexattr_destroy(&attr);
     return __ec;
   }
   __ec = pthread_mutexattr_destroy(&attr);
   if (__ec) {
     pthread_mutex_destroy(__m);
     return __ec;
   }
   return 0;
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 int __libcpp_recursive_mutex_lock(__libcpp_recursive_mutex_t *__m)
 {
   return pthread_mutex_lock(__m);
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 bool __libcpp_recursive_mutex_trylock(__libcpp_recursive_mutex_t *__m)
 {
   return pthread_mutex_trylock(__m) == 0;
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 int __libcpp_recursive_mutex_unlock(__libcpp_recursive_mutex_t *__m)
 {
   return pthread_mutex_unlock(__m);
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 int __libcpp_recursive_mutex_destroy(__libcpp_recursive_mutex_t *__m)
 {
   return pthread_mutex_destroy(__m);
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 int __libcpp_mutex_lock(__libcpp_mutex_t *__m)
 {
   return pthread_mutex_lock(__m);
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 bool __libcpp_mutex_trylock(__libcpp_mutex_t *__m)
 {
   return pthread_mutex_trylock(__m) == 0;
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 int __libcpp_mutex_unlock(__libcpp_mutex_t *__m)
 {
   return pthread_mutex_unlock(__m);
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 int __libcpp_mutex_destroy(__libcpp_mutex_t *__m)
 {
   return pthread_mutex_destroy(__m);
 }
 
 // Condition Variable
+_LIBCPP_HIDE_FROM_ABI inline
 int __libcpp_condvar_signal(__libcpp_condvar_t *__cv)
 {
   return pthread_cond_signal(__cv);
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 int __libcpp_condvar_broadcast(__libcpp_condvar_t *__cv)
 {
   return pthread_cond_broadcast(__cv);
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 int __libcpp_condvar_wait(__libcpp_condvar_t *__cv, __libcpp_mutex_t *__m)
 {
   return pthread_cond_wait(__cv, __m);
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 int __libcpp_condvar_timedwait(__libcpp_condvar_t *__cv, __libcpp_mutex_t *__m,
                                __libcpp_timespec_t *__ts)
 {
   return pthread_cond_timedwait(__cv, __m, __ts);
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 int __libcpp_condvar_destroy(__libcpp_condvar_t *__cv)
 {
   return pthread_cond_destroy(__cv);
 }
 
 // Execute once
+_LIBCPP_HIDE_FROM_ABI inline
 int __libcpp_execute_once(__libcpp_exec_once_flag *flag,
                           void (*init_routine)()) {
   return pthread_once(flag, init_routine);
 }
 
 // Thread id
 // Returns non-zero if the thread ids are equal, otherwise 0
+_LIBCPP_HIDE_FROM_ABI inline
 bool __libcpp_thread_id_equal(__libcpp_thread_id t1, __libcpp_thread_id t2)
 {
   return t1 == t2;
 }
 
 // Returns non-zero if t1 < t2, otherwise 0
+_LIBCPP_HIDE_FROM_ABI inline
 bool __libcpp_thread_id_less(__libcpp_thread_id t1, __libcpp_thread_id t2)
 {
   return t1 < t2;
 }
 
 // Thread
+_LIBCPP_HIDE_FROM_ABI inline
 bool __libcpp_thread_isnull(const __libcpp_thread_t *__t) {
   return __libcpp_thread_get_id(__t) == 0;
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 int __libcpp_thread_create(__libcpp_thread_t *__t, void *(*__func)(void *),
                            void *__arg)
 {
   return pthread_create(__t, nullptr, __func, __arg);
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 __libcpp_thread_id __libcpp_thread_get_current_id()
 {
   const __libcpp_thread_t thread = pthread_self();
   return __libcpp_thread_get_id(&thread);
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 __libcpp_thread_id __libcpp_thread_get_id(const __libcpp_thread_t *__t)
 {
 #if defined(__MVS__)
   return __t->__;
 #else
   return *__t;
 #endif
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 int __libcpp_thread_join(__libcpp_thread_t *__t)
 {
   return pthread_join(*__t, nullptr);
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 int __libcpp_thread_detach(__libcpp_thread_t *__t)
 {
   return pthread_detach(*__t);
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 void __libcpp_thread_yield()
 {
   sched_yield();
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 void __libcpp_thread_sleep_for(const chrono::nanoseconds& __ns)
 {
    __libcpp_timespec_t __ts = __thread_detail::__convert_to_timespec(__ns);
    while (nanosleep(&__ts, &__ts) == -1 && errno == EINTR);
 }
 
 // Thread local storage
+_LIBCPP_HIDE_FROM_ABI inline
 int __libcpp_tls_create(__libcpp_tls_key *__key, void (*__at_exit)(void *))
 {
   return pthread_key_create(__key, __at_exit);
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 void *__libcpp_tls_get(__libcpp_tls_key __key)
 {
   return pthread_getspecific(__key);
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 int __libcpp_tls_set(__libcpp_tls_key __key, void *__p)
 {
     return pthread_setspecific(__key, __p);
 }
 
 #elif defined(_LIBCPP_HAS_THREAD_API_C11)
 
+_LIBCPP_HIDE_FROM_ABI inline
 int __libcpp_recursive_mutex_init(__libcpp_recursive_mutex_t *__m)
 {
   return mtx_init(__m, mtx_plain | mtx_recursive) == thrd_success ? 0 : EINVAL;
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 int __libcpp_recursive_mutex_lock(__libcpp_recursive_mutex_t *__m)
 {
   return mtx_lock(__m) == thrd_success ? 0 : EINVAL;
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 bool __libcpp_recursive_mutex_trylock(__libcpp_recursive_mutex_t *__m)
 {
   return mtx_trylock(__m) == thrd_success;
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 int __libcpp_recursive_mutex_unlock(__libcpp_recursive_mutex_t *__m)
 {
   return mtx_unlock(__m) == thrd_success ? 0 : EINVAL;
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 int __libcpp_recursive_mutex_destroy(__libcpp_recursive_mutex_t *__m)
 {
   mtx_destroy(__m);
   return 0;
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 int __libcpp_mutex_lock(__libcpp_mutex_t *__m)
 {
   return mtx_lock(__m) == thrd_success ? 0 : EINVAL;
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 bool __libcpp_mutex_trylock(__libcpp_mutex_t *__m)
 {
   return mtx_trylock(__m) == thrd_success;
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 int __libcpp_mutex_unlock(__libcpp_mutex_t *__m)
 {
   return mtx_unlock(__m) == thrd_success ? 0 : EINVAL;
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 int __libcpp_mutex_destroy(__libcpp_mutex_t *__m)
 {
   mtx_destroy(__m);
   return 0;
 }
 
 // Condition Variable
+_LIBCPP_HIDE_FROM_ABI inline
 int __libcpp_condvar_signal(__libcpp_condvar_t *__cv)
 {
   return cnd_signal(__cv) == thrd_success ? 0 : EINVAL;
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 int __libcpp_condvar_broadcast(__libcpp_condvar_t *__cv)
 {
   return cnd_broadcast(__cv) == thrd_success ? 0 : EINVAL;
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 int __libcpp_condvar_wait(__libcpp_condvar_t *__cv, __libcpp_mutex_t *__m)
 {
   return cnd_wait(__cv, __m) == thrd_success ? 0 : EINVAL;
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 int __libcpp_condvar_timedwait(__libcpp_condvar_t *__cv, __libcpp_mutex_t *__m,
                                timespec *__ts)
 {
   int __ec = cnd_timedwait(__cv, __m, __ts);
   return __ec == thrd_timedout ? ETIMEDOUT : __ec;
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 int __libcpp_condvar_destroy(__libcpp_condvar_t *__cv)
 {
   cnd_destroy(__cv);
   return 0;
 }
 
 // Execute once
+_LIBCPP_HIDE_FROM_ABI inline
 int __libcpp_execute_once(__libcpp_exec_once_flag *flag,
                           void (*init_routine)(void)) {
   ::call_once(flag, init_routine);
   return 0;
 }
 
 // Thread id
 // Returns non-zero if the thread ids are equal, otherwise 0
+_LIBCPP_HIDE_FROM_ABI inline
 bool __libcpp_thread_id_equal(__libcpp_thread_id t1, __libcpp_thread_id t2)
 {
   return thrd_equal(t1, t2) != 0;
 }
 
 // Returns non-zero if t1 < t2, otherwise 0
+_LIBCPP_HIDE_FROM_ABI inline
 bool __libcpp_thread_id_less(__libcpp_thread_id t1, __libcpp_thread_id t2)
 {
   return t1 < t2;
 }
 
 // Thread
+_LIBCPP_HIDE_FROM_ABI inline
 bool __libcpp_thread_isnull(const __libcpp_thread_t *__t) {
   return __libcpp_thread_get_id(__t) == 0;
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 int __libcpp_thread_create(__libcpp_thread_t *__t, void *(*__func)(void *),
                            void *__arg)
 {
   int __ec = thrd_create(__t, reinterpret_cast<thrd_start_t>(__func), __arg);
   return __ec == thrd_nomem ? ENOMEM : __ec;
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 __libcpp_thread_id __libcpp_thread_get_current_id()
 {
   return thrd_current();
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 __libcpp_thread_id __libcpp_thread_get_id(const __libcpp_thread_t *__t)
 {
   return *__t;
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 int __libcpp_thread_join(__libcpp_thread_t *__t)
 {
   return thrd_join(*__t, nullptr) == thrd_success ? 0 : EINVAL;
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 int __libcpp_thread_detach(__libcpp_thread_t *__t)
 {
   return thrd_detach(*__t) == thrd_success ? 0 : EINVAL;
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 void __libcpp_thread_yield()
 {
   thrd_yield();
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 void __libcpp_thread_sleep_for(const chrono::nanoseconds& __ns)
 {
    __libcpp_timespec_t __ts = __thread_detail::__convert_to_timespec(__ns);
   thrd_sleep(&__ts, nullptr);
 }
 
 // Thread local storage
+_LIBCPP_HIDE_FROM_ABI inline
 int __libcpp_tls_create(__libcpp_tls_key *__key, void (*__at_exit)(void *))
 {
   return tss_create(__key, __at_exit) == thrd_success ? 0 : EINVAL;
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 void *__libcpp_tls_get(__libcpp_tls_key __key)
 {
   return tss_get(__key);
 }
 
+_LIBCPP_HIDE_FROM_ABI inline
 int __libcpp_tls_set(__libcpp_tls_key __key, void *__p)
 {
   return tss_set(__key, __p) == thrd_success ? 0 : EINVAL;
 }
 
 #endif