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

[Asiri Rathnayake via Phabricator via cfe-commits]

rmaprath added a comment.

In https://reviews.llvm.org/D27204#613177, @EricWF wrote:

> In https://reviews.llvm.org/D27204#613172, @rmaprath wrote:
>
> > In https://reviews.llvm.org/D27204#613122, @EricWF wrote:
> >
> > > 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.
> >
> >
> > I would like to do that too!
> >
> > But I always viewed `libcxx` and `libcxxabi` as two independent libraries that should be usable on their own. In this case, binding `libcxxabi` to `__threading_support` from `libcxx` sounded wrong, because `__threading_support` is something internal to `libcxx` (e.g. all the functions there are prefixed with `__libcpp_` - there may be other `libcpp`-isms arising from `__config` in there too).
>
>
> Ironically I've always viewed `libcxxabi` as fully dependent on `libcxx` (and personally I would like to see the repos merged; Although using libc++ w/o libc++abi would still be supported).
>
>     
>
> > Is that not a concern? I could give it a shot and try to merge the two APIs into one if not.
>
> It's not a concern. Libc++abi already depends on libc++ internals to build (See the `#include "__refstring" in `stdlib_stdexcept.cpp`).


Thanks for the clarification. I'll re-spin the patch.

Responses to inline comments follow, but some of those would go away with the re-spin I imagine.



================
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.
----------------
EricWF wrote:
> It's weird that libc++abi needs to define a libc++ option? Can you elaborate on the need for this?
The `libcxxabi` test suite builds `libcxx` as part of running the test suite, I thought a separate option would be appropriate here to allow users to select which configuration they want to build `libcxx` for those tests. 

Given the new knowledge I have, I think the right thing to do here is configure `libcxx` to also use external threads when `libcxxabi` is configured to use external threads, rather than providing a separate option here. Will fix.


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


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


================
Comment at: test/CMakeLists.txt:45
+if (LIBCXX_HAS_EXTERNAL_THREAD_API)
+  list(APPEND LIBCXXABI_TEST_DEPS "cxx_external_threads")
+endif()
----------------
EricWF wrote:
> 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?
This worked fine for me. All my builds are in-tree as well.

I also checked if the order in which tests are run (whether `libcxx` runs before `libcxxabi` and vice-versa) makes no difference.

I will double check still.


================
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')
----------------
EricWF wrote:
> `libcxxabi_shared` should default to `True` like it does within the libc++ config.
> 
Not sure if I follow, I'm trying to detect this particular combination of configs so that I can xfail a particular test.


================
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']
 
----------------
EricWF wrote:
> I'm not sure I understand the reason for this change.
I should've put a comment. The test suite keeps trying to run `test/support/external_threading.cpp` as a test case without this. Perhaps there is a better way to handle that situation?


https://reviews.llvm.org/D27204