[libcxx] Fix compile for BUILD_EXTERNAL_THREAD_LIBRARY

[Nicu]

Summary

Fix compilation with -DLIBCXX_BUILD_EXTERNAL_THREAD_LIBRARY when using clang. Now linking target 'cxx_external_threads' with 'cxx-headers'. Fix mismatching visibility for libcpp_timed_backoff_policy function in file <__threading_support>.

[miyuki]

libcxx/include/__threading_support

277

Please remove stray changes (2 added/removed blank lines)

[Nicu]

Removed previously added blank line.
Added previously removed blank line.

[ldionne]

libcxx/src/CMakeLists.txt

344

I believe it would be better to use target_link_libraries(cxx_external_threads PRIVATE cxx-headers). This will setup the right flags for using the current libc++ headers.

[Nicu]

Updated CMakeList.txt to link against 'cxx-headers' rather than just including 'libcxx/include'.
Updated commit's summary to reflect this change.

[ldionne]

Did you run the tests before committing this? It looks like it broke several builders, e.g. http://lab.llvm.org:8011/builders/libcxx-libcxxabi-x86_64-linux-debian-noexceptions/builds/2370

[miyuki]

Sorry about that. I've reverted the change and will investigate.

[ldionne]

In D86598#2242322, @miyuki wrote:

Sorry about that. I've reverted the change and will investigate.

Thanks. I think the fix is just to remove the inline before _LIBCPP_THREAD_ABI_VISIBILITY, since _LIBCPP_THREAD_ABI_VISIBILITY already contains inline when not in the external threading library mode.

[Nicu]

In D86598#2242340, @ldionne wrote:

In D86598#2242322, @miyuki wrote:

Sorry about that. I've reverted the change and will investigate.

Thanks. I think the fix is just to remove the inline before _LIBCPP_THREAD_ABI_VISIBILITY, since _LIBCPP_THREAD_ABI_VISIBILITY already contains inline when not in the external threading library mode.

My bad, I did run tests but using the BUILD_EXTERNAL_THREAD_LIBRARY mode only, and forgot about the normal config.
As for the fix, if I remove that inline it breaks the external threading mode, but in order to fix that do you think is safe to change line 43 in __threading_support to #define _LIBCPP_THREAD_ABI_VISIBILITY inline _LIBCPP_FUNC_VIS. Adding the inline there?

[ldionne]

In D86598#2242413, @Nicu wrote:

As for the fix, if I remove that inline it breaks the external threading mode, [...]

Actually, looking at it again, the fix is to always use _LIBCPP_INLINE_VISIBILITY. Please rebase your change on top of 21a1a263a6d9c0c44ef8eb0744786e2aa5d59e53

[miyuki]

Thanks!

[ldionne]

This seems to have been reverted in

commit a19fd1aab519ccec18654f76a01b0345880c5200
Author: Mikhail Maltsev <[e-mail]>
Date:   Thu Aug 27 16:47:18 2020 +0100

    Revert "[libcxx] Fix compile for BUILD_EXTERNAL_THREAD_LIBRARY"

    This reverts commit 3b71f91558ff8b569199547efe800cb501c3cf94.

    The commit is breaking some build bots.

Has it been committed again with a fix? I don't see it -- what's the status?

[Nicu]

In D86598#2248202, @ldionne wrote:

This seems to have been reverted in

commit a19fd1aab519ccec18654f76a01b0345880c5200
Author: Mikhail Maltsev <[e-mail]>
Date:   Thu Aug 27 16:47:18 2020 +0100

    Revert "[libcxx] Fix compile for BUILD_EXTERNAL_THREAD_LIBRARY"

    This reverts commit 3b71f91558ff8b569199547efe800cb501c3cf94.

    The commit is breaking some build bots.

Has it been committed again with a fix? I don't see it -- what's the status?

Hi Louis, this is the revision that fixes the compilation with EXTERNAL_LIBRARY (https://reviews.llvm.org/D86773), without changes to the visibility in the operator. As you fixed that already.

[miyuki]

I've just committed D86773

[ldionne]

Ah, I was confused, thanks for clarifying!

libcxx/include/__threading_support

 int __libcpp_tls_create(__libcpp_tls_key* __key,
                         void(_LIBCPP_TLS_DESTRUCTOR_CC* __at_exit)(void*));
 
 _LIBCPP_THREAD_ABI_VISIBILITY
 void *__libcpp_tls_get(__libcpp_tls_key __key);
 
 _LIBCPP_THREAD_ABI_VISIBILITY
 int __libcpp_tls_set(__libcpp_tls_key __key, void *__p);
 
 #endif // !defined(_LIBCPP_HAS_THREAD_API_EXTERNAL)
 
 struct __libcpp_timed_backoff_policy {
   _LIBCPP_THREAD_ABI_VISIBILITY
   bool operator()(chrono::nanoseconds __elapsed) const;
 };
 
-inline _LIBCPP_INLINE_VISIBILITY
+inline _LIBCPP_THREAD_ABI_VISIBILITY
 bool __libcpp_timed_backoff_policy::operator()(chrono::nanoseconds __elapsed) const
 {
     if(__elapsed > chrono::milliseconds(128))
         __libcpp_thread_sleep_for(chrono::milliseconds(8));
     else if(__elapsed > chrono::microseconds(64))
         __libcpp_thread_sleep_for(__elapsed / 2);
     else if(__elapsed > chrono::microseconds(4))
       __libcpp_thread_yield();

libcxx/src/CMakeLists.txt

PROPERTIES
       COMPILE_FLAGS "${LIBCXX_COMPILE_FLAGS}"
       OUTPUT_NAME   "c++experimental"
   )
   cxx_add_common_build_flags(cxx_experimental)
 endif()
 
 
 if (LIBCXX_BUILD_EXTERNAL_THREAD_LIBRARY)
-  file(GLOB LIBCXX_EXTERNAL_THREADING_SUPPORT_SOURCES ../test/support/external_threads.cpp)
+  set(LIBCXX_EXTERNAL_THREADING_SUPPORT_SOURCES
+      "${CMAKE_CURRENT_SOURCE_DIR}/../test/support/external_threads.cpp")
 
   if (LIBCXX_ENABLE_SHARED)
     add_library(cxx_external_threads SHARED ${LIBCXX_EXTERNAL_THREADING_SUPPORT_SOURCES})
   else()
     add_library(cxx_external_threads STATIC ${LIBCXX_EXTERNAL_THREADING_SUPPORT_SOURCES})
   endif()
 
   set_target_properties(cxx_external_threads
     PROPERTIES
       LINK_FLAGS    "${LIBCXX_LINK_FLAGS}"
       COMPILE_FLAGS "${LIBCXX_COMPILE_FLAGS}"
       OUTPUT_NAME   "c++external_threads"
   )
+
+  target_link_libraries(cxx_external_threads PRIVATE cxx-headers)
 endif()
 
 if (LIBCXX_INSTALL_LIBRARY)
   if (LIBCXX_INSTALL_SHARED_LIBRARY)
     install(TARGETS cxx_shared
       ARCHIVE DESTINATION ${LIBCXX_INSTALL_PREFIX}${LIBCXX_INSTALL_LIBRARY_DIR} COMPONENT cxx
       LIBRARY DESTINATION ${LIBCXX_INSTALL_PREFIX}${LIBCXX_INSTALL_LIBRARY_DIR} COMPONENT cxx
       RUNTIME DESTINATION ${LIBCXX_INSTALL_PREFIX}bin COMPONENT cxx)
   endif()