[PATCH] D27204: [libcxxabi] Introduce an externally threaded libc++abi variant

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added a comment.

My main issue with this patch (and https://reviews.llvm.org/D27206) is that there are now two different CMake options for building two different external threading libraries. I would much prefer having libc++abi use libc++'s `__threading_support` header and `cxx_external_threads` library.



================
Comment at: CMakeLists.txt:124
+  This option may only be set to ON when LIBCXXABI_ENABLE_THREADS=ON." OFF)
+option(LIBCXX_HAS_EXTERNAL_THREAD_API
+  "Build libc++ with an externalized threading API.
----------------
It's weird that libc++abi needs to define a libc++ option? Can you elaborate on the need for this?


================
Comment at: src/CMakeLists.txt:132
+if (LIBCXXABI_HAS_EXTERNAL_THREAD_API)
+  file(GLOB LIBCXXABI_EXTERNAL_THREADING_SUPPORT_SOURCES ../test/support/external_threads.cpp)
+
----------------
Do you really need to glob a single file?


================
Comment at: test/CMakeLists.txt:41
+if (LIBCXXABI_HAS_EXTERNAL_THREAD_API)
+  list(APPEND LIBCXXABI_TEST_DEPS "cxxabi_external_threads")
+endif()
----------------
Target names shouldn't be in quotes.


================
Comment at: test/CMakeLists.txt:45
+if (LIBCXX_HAS_EXTERNAL_THREAD_API)
+  list(APPEND LIBCXXABI_TEST_DEPS "cxx_external_threads")
+endif()
----------------
For the in-tree libc++/libc++abi builds libc++abi gets configured before libc++, so I don't think libc++ target names are visible in this context.
Are you sure this works?


================
Comment at: test/libcxxabi/test/config.py:43
+        # test_exception_storage_nodynmem.pass.cpp fails under this specific configuration
+        if self.get_lit_bool('cxxabi_ext_threads', False) and self.get_lit_bool('libcxxabi_shared', False):
+            self.config.available_features.add('libcxxabi-shared-externally-threaded')
----------------
`libcxxabi_shared` should default to `True` like it does within the libc++ config.



================
Comment at: test/lit.cfg:21
 # suffixes: A list of file extensions to treat as test files.
-config.suffixes = ['.cpp', '.s']
+config.suffixes = ['.pass.cpp', '.sh.s', '.sh.cpp']
 
----------------
I'm not sure I understand the reason for this change.


https://reviews.llvm.org/D27204