[libcxx] Do not declare the thread api when __external_threading is present

[rmaprath]

Summary

This fixes a small omission where even when __external_threading is provided, we attempt to declare a pthread based threading API. Instead, we should leave out everything for the __external_threading header to take care of.

The __threading_support header provides a proof-of-concept externally threaded libc++ variant when _LIBCPP_HAS_THREAD_API_EXTERNAL is defined. But if the __external_threading header is present, we should exclude all of that POC stuff.

[EricWF]

include/__threading_support

25

If _LIBCPP_HAS_THREAD_API_EXTERNAL is defined can't we just assume that `<__external_threading> is present?

35

Instead of using a new macro couldn't this just be _LIBCPP_HAS_THREAD_API_EXTERNAL?

[rmaprath]

Added some comments to @EricWF's feedback. Will check back tomorrow (falling asleep...)

/ Asiri

include/__threading_support

25

So, _LIBCPP_HAS_THREAD_API_EXTERNAL is set from the cmake option for building the externally threaded variant. For the proof-of-concept implementation (that we use for running the test suite), we embed a pthread-based external API within __threading_support itself and provide its implementation in test/support/external_threads.cpp (which is built into a separate library when running the test suite).

That means, in the default externally-threaded variant (POC), we don't have a __external_threading header; the idea is to allow library vendors to drop-in a __external_threading header without conflicting with upstream sources, and it will be picked up automatically by __threading_support header. Moreover, this way we avoid the cost of shipping an additional header which is not necessary for the most common use case (direct pthread dependency).

This is why we need this contraption to detect if the externally threaded library is built with a __external_threading header (i.e. for production) or not (the default - vanilla upstream setup, the POC).

Hope that makes sense? Perhaps I should explain all this in a comment as well?

35

I think my comment above covers this point as well.

[rmaprath]

@EricWF: Gentle ping.

include/__threading_support

 #pragma GCC system_header
 #endif
 
 #ifndef _LIBCPP_HAS_NO_THREADS
 
 // These checks are carefully arranged so as not to trigger a gcc pre-processor
 // defect which causes it to fail to parse the __has_include check below, the
 // redundancy is intentional.
 #if defined(_LIBCPP_HAS_THREAD_API_EXTERNAL)
 #if !defined(__clang__) && (_GNUC_VER < 500)
 #include <__external_threading>
-#define _LIBCPP_EXTERNAL_THREADING
+#define _LIBCPP_HAS_EXTERNAL_THREADING_HEADER
 #elif !defined(__has_include) || __has_include(<__external_threading>)
 #include <__external_threading>
-#define _LIBCPP_EXTERNAL_THREADING
+#define _LIBCPP_HAS_EXTERNAL_THREADING_HEADER
 #endif
 #endif
 
-#if !defined(_LIBCPP_EXTERNAL_THREADING)
+#if !defined(_LIBCPP_HAS_EXTERNAL_THREADING_HEADER)
 #include <pthread.h>
 #include <sched.h>
-#endif
 
 #if defined(_LIBCPP_HAS_THREAD_API_EXTERNAL)
 #define _LIBCPP_THREAD_ABI_VISIBILITY _LIBCPP_FUNC_VIS
 #else
 #define _LIBCPP_THREAD_ABI_VISIBILITY inline _LIBCPP_INLINE_VISIBILITY
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD

 {
     pthread_setspecific(__key, __p);
 }
 
 #endif // _LIBCPP_HAS_THREAD_API_PTHREAD || _LIBCPP_BUILDING_EXTERNAL_THREADS
 
 _LIBCPP_END_NAMESPACE_STD
 
+#endif // !_LIBCPP_HAS_EXTERNAL_THREADING_HEADER
+
 #endif // _LIBCPP_HAS_NO_THREADS
 
 #endif // _LIBCPP_THREADING_SUPPORT