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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
