[PATCH] D27575: [libcxxabi] Introduce an externally threaded libc++abi variant (take-2)
Eric Fiselier via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Dec 23 19:54:35 PST 2016
EricWF accepted this revision.
EricWF added a comment.
This revision is now accepted and ready to land.
LGTM. I'll re-review the mutexor changes post-commit.
================
Comment at: CMakeLists.txt:121
option(LIBCXXABI_HAS_PTHREAD_API "Ignore auto-detection and force use of pthread API" OFF)
+option(LIBCXXABI_HAS_EXTERNAL_THREAD_API
+ "Build libc++abi with an externalized threading API.
----------------
rmaprath wrote:
> EricWF wrote:
> > Maybe use a dependent option that sets it to the value of `LIBCXX_ENABLE_THREADS` when it is defined?
> Are you referring to `CMAKE_DEPENDENT_OPTION` macro? I don't see this macro used anywhere else in llvm, but I suppose that's not a concern?
>
> Although, it would have to be something like:
>
> ```
> CMAKE_DEPENDENT_OPTION(LIBCXXABI_HAS_EXTERNAL_THREAD_API "Externally Threaded libcxxabi" OFF "LIBCXX_ENABLE_THREADS" OFF)
> ```
>
> Because, `LIBCXX_ENABLE_THREADS=ON` should not mean `LIBCXXABI_HAS_EXTERNAL_THREAD_API=ON`.
>
Ah OK. This looks good as is then.
Although it would be really nice if `LIBCXX_HAS_EXTERNAL_THREAD_API=ON` also enabled this option unless otherwise specified, but I don't see an easy way to do that.
================
Comment at: src/fallback_malloc.cpp:37
class mutexor {
public:
----------------
rmaprath wrote:
> EricWF wrote:
> > Can't we replace this class with `std::mutex` directly?
> Again, I should've included more context to the patch :(
>
> The `mutexor` here is supposed to degrade to a nop when threading is disabled. I think we cannot use `std::mutex` without threading support.
>
> Will update the patch with more context.
We cannot use `std::mutex` without threading support but I would still rather use it.
I would `typedef std::lock_guard<std::mutex> mutexor` when threading is enabled and otherwise I would just `#ifdef` all usages away.
Also please add `_LIBCPP_SAFE_STATIC` to the heap_mutex declaration.
https://reviews.llvm.org/D27575
More information about the cfe-commits
mailing list