[PATCH] D21968: [libcxx] Externally threaded libc++ variant - Take 2

[Asiri Rathnayake via cfe-commits]

@mclow.lists: Ping ?
On 24 Aug 2016 22:46, "Asiri Rathnayake via cfe-commits" <
cfe-commits at lists.llvm.org> wrote:

> rmaprath added inline comments.
>
> ================
> Comment at: include/__config:830
> @@ -829,1 +829,3 @@
> +    !defined(_LIBCPP_HAS_THREAD_API_PTHREAD) && \
> +    !defined(_LIBCPP_HAS_THREAD_API_EXTERNAL)
>  # if defined(__FreeBSD__) || \
> ----------------
> compnerd wrote:
> > I meant on the lines that you added :-).
> Will update to reflect rest of the source file.
>
> ================
> Comment at: include/__threading_support:261
> @@ -200,1 +260,3 @@
> +void* __libcpp_tl_get(__libcpp_tl_key __key);
> +void __libcpp_tl_set(__libcpp_tl_key __key, void* __p);
>
> ----------------
> compnerd wrote:
> > I think we should explicitly spell out `tls`.  The extra character won't
> hurt.
> Will fix.
>
> ================
> Comment at: lib/CMakeLists.txt:216
> @@ +215,3 @@
> +  )
> +endif()
> +
> ----------------
> compnerd wrote:
> > Have you considered a slightly alternative approach of basically
> implementing pthreads as an "external" threading model as well?  You could
> override it, or take the default implementation that is provided.  It
> avoids having a second supporting DSO for threading as we could always
> statically link against it.
> So, the particulars of this design was discussed and (generally) agreed
> upon sometime back.
>
> Currently pthreads **is** hidden behind the same threading API as the
> externally threaded variant (on this patch). To override the default
> (pthread) implementation, you just have to drop in an
> `__external_threading` header and specify the `-DLIBCXX_HAS_EXTERNAL_THREAD_API`
> cmake option.
>
> There's no need to provide a second DSO if your `__external_threading`
> header (and your  thread-system sources) are available at `libc++` build
> time.
>
> But we have a **requirement** that the library needs to be build-able in
> such a way you can provide a second DSO holding the implementation of the
> API. That is, we need to be able to build and ship a library to which
> customers can drop-in their own thread implementation (i.e. the
> thread-support DSO).
>
> Note that in one of my earlier patches, I proposed an even more de-coupled
> external threading system where even the types like `__libcpp_mutex_t` were
> opaque pointers. @mclow.lists opposed that (perhaps rightfully so, now I
> think about it) and currently these types need to be known at `libc++`
> compile time. The implementation of the API functions like
> `__libcpp_mutex_lock()` can be deferred to link time (this is what is
> provided in the second DSO).
>
> Hope that clarifies everything here?
>
>
>
>
>
>
>
> ================
> Comment at: test/support/external_threads.cpp:26
> @@ +25,3 @@
> +    if (__ec)
> +      goto fail;
> +
> ----------------
> compnerd wrote:
> > I think this would be nicer as:
> >
> >   if (int error = pthread_mutexattr_init(&attr))
> >     return error;
> I think I'm following the conventions used in the `libc++` source files
> here. Will double check tomorrow.
>
> ================
> Comment at: test/support/external_threads.cpp:52
> @@ +51,3 @@
> +fail:
> +    return __ec;
> +}
> ----------------
> compnerd wrote:
> > I don't think that the label for failure is adding anything since you
> really are handling the destruction of the mutex at all failure sites.
> Not sure what I did here. Will double check and fix.
>
> ================
> Comment at: test/support/external_threads.cpp:70
> @@ +69,3 @@
> +
> +int __libcpp_mutex_destroy(__libcpp_mutex_t* __m) {
> +    return pthread_mutex_destroy(__m);
> ----------------
> compnerd wrote:
> > Please choose a single style.
> Will fix.
>
> ================
> Comment at: test/support/external_threads.cpp:103
> @@ +102,3 @@
> +
> +bool __libcpp_thread_id_equal(__libcpp_thread_id t1, __libcpp_thread_id
> t2) {
> +    return pthread_equal(t1, t2) != 0;
> ----------------
> compnerd wrote:
> > Please choose a single style (and below).
> Will fix.
>
> ================
> Comment at: test/support/external_threads.cpp:131
> @@ +130,3 @@
> +    // before.
> +    if (*__t == 0)
> +        return -1;
> ----------------
> compnerd wrote:
> > Can you compare to `nullptr` instead?
> I can't remember exactly why, but I think there was a problem with using
> `nullptr` here. I will check and get back.
>
> ================
> Comment at: test/support/external_threads.cpp:142
> @@ +141,3 @@
> +    // before.
> +    if (*__t == 0)
> +        return -1;
> ----------------
> compnerd wrote:
> > Same.
> Need to check.
>
>
> https://reviews.llvm.org/D21968
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160904/9868795c/attachment.html>