Re: [PATCH] D11781: Refactored pthread usage in libcxx
espositofulvio added a comment. In http://reviews.llvm.org/D11781#423520, @rmaprath wrote: > @espositofulvio: Thanks for the patch! :) > > Committed as r268734. Glad to see you land the patch! Great work :) Repository: rL LLVM http://reviews.llvm.org/D11781 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11781: Refactored pthread usage in libcxx
rmaprath added a comment. @espositofulvio: Thanks for the patch! :) Committed as r268734. Repository: rL LLVM http://reviews.llvm.org/D11781 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11781: Refactored pthread usage in libcxx
rmaprath added a comment.
In http://reviews.llvm.org/D11781#403349, @rmaprath wrote:
> In http://reviews.llvm.org/D11781#403343, @theraven wrote:
>
> > In http://reviews.llvm.org/D11781#403335, @rmaprath wrote:
> >
> > > For us (ARM), a threads porting layer is important on several RTOSes
> > > (where a full-blown pthreads implementations is not available). I will
> > > see if I can publish one of those porting layer implementations, but
> > > perhaps a windows porting layer is more interesting to the community, in
> > > which case I'll look into hacking one up (unless of course, if
> > > @espositofulvio has already got one).
> >
> >
> > I'd be equally interested (and willing to review) an mbed implementation.
>
>
> Thanks!
>
> If you meant the mbed OS [1], I think it is still single-threaded (although
> that might change in the near future [2]). Most of our customers (of
> armclang) have proprietary RTOSes, but we might be able upstream a sample
> implementation (I'm thinking Keil RTX [3], but need to double check a few
> things), if it seems generally useful to the community.
>
> Cheers,
>
> / Asiri
>
> [1] https://www.mbed.com/
> [2] https://www.mbed.com/en/development/software/mbed-os/ ("...we intend to
> re-introduce it in 2016")
> [3] http://www.keil.com/rl-arm/kernel.asp
I stand corrected, mbed already has an RTOS layer:
https://developer.mbed.org/handbook/RTOS. I'm not very familiar with mbed
(obviously). I will do some digging to see how difficult it would be to port
libcxx to mbed (we already build libcxx for cortex-m bare-metal systems, so
this shouldn't be that hard).
After reading through the past threads, providing a windows thread porting
layer sounds like a useless thing, given that getting libcxx to build on
windows itself is a rather large piece of work.
Cheers,
- Asiri
Repository:
rL LLVM
http://reviews.llvm.org/D11781
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11781: Refactored pthread usage in libcxx
rmaprath added a comment. In http://reviews.llvm.org/D11781#403378, @espositofulvio wrote: > In http://reviews.llvm.org/D11781#400968, @rmaprath wrote: > > > Hi, could I know the status of this? I'd like to push this forward. > > > > @espositofulvio: Are you working on this? (just checking since this has > > gone stale for a while). @EricWF: I can create a separate diff for further > > review if this one is too cluttered. > > > Unfortunately I don't have much spare time at the moment, and that being > something I was doing to learn about libc++ and not something I needed, it > got sidetracked. So go ahead with your work. > Best regards, > Fulvio I think you've done all the difficult bits already :) I'll polish it up and address the remaining comments. Thanks!! / Asiri Repository: rL LLVM http://reviews.llvm.org/D11781 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11781: Refactored pthread usage in libcxx
espositofulvio added a comment. In http://reviews.llvm.org/D11781#400968, @rmaprath wrote: > Hi, could I know the status of this? I'd like to push this forward. > > @espositofulvio: Are you working on this? (just checking since this has gone > stale for a while). @EricWF: I can create a separate diff for further review > if this one is too cluttered. Unfortunately I don't have much spare time at the moment, and that being something I was doing to learn about libc++ and not something I needed, it got sidetracked. So go ahead with your work. Best regards, Fulvio Repository: rL LLVM http://reviews.llvm.org/D11781 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11781: Refactored pthread usage in libcxx
rmaprath added a comment.
In http://reviews.llvm.org/D11781#403343, @theraven wrote:
> In http://reviews.llvm.org/D11781#403335, @rmaprath wrote:
>
> > For us (ARM), a threads porting layer is important on several RTOSes (where
> > a full-blown pthreads implementations is not available). I will see if I
> > can publish one of those porting layer implementations, but perhaps a
> > windows porting layer is more interesting to the community, in which case
> > I'll look into hacking one up (unless of course, if @espositofulvio has
> > already got one).
>
>
> I'd be equally interested (and willing to review) an mbed implementation.
Thanks!
If you meant the mbed OS [1], I think it is still single-threaded (although
that might change in the near future [2]). Most of our customers (of armclang)
have proprietary RTOSes, but we might be able upstream a sample implementation
(I'm thinking Keil RTX [3], but need to double check a few things), if it seems
generally useful to the community.
Cheers,
/ Asiri
[1] https://www.mbed.com/
[2] https://www.mbed.com/en/development/software/mbed-os/ ("...we intend to
re-introduce it in 2016")
[3] http://www.keil.com/rl-arm/kernel.asp
Repository:
rL LLVM
http://reviews.llvm.org/D11781
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11781: Refactored pthread usage in libcxx
theraven added a comment. In http://reviews.llvm.org/D11781#403335, @rmaprath wrote: > For us (ARM), a threads porting layer is important on several RTOSes (where a > full-blown pthreads implementations is not available). I will see if I can > publish one of those porting layer implementations, but perhaps a windows > porting layer is more interesting to the community, in which case I'll look > into hacking one up (unless of course, if @espositofulvio has already got > one). I'd be equally interested (and willing to review) an mbed implementation. Repository: rL LLVM http://reviews.llvm.org/D11781 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11781: Refactored pthread usage in libcxx
rmaprath added a comment. Hi Eric, Thanks for the comments! I'll wait a bit for @espositofulvio in case if he wants to push this, otherwise I'll create a new diff with the suggested changes. For us (ARM), a threads porting layer is important on several RTOSes (where a full-blown pthreads implementations is not available). I will see if I can publish one of those porting layer implementations, but perhaps a windows porting layer is more interesting to the community, in which case I'll look into hacking one up (unless of course, if @espositofulvio has already got one). Cheers, / Asiri Repository: rL LLVM http://reviews.llvm.org/D11781 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11781: Refactored pthread usage in libcxx
EricWF added a comment.
I would like to see this patch without the `support/pthread/*.cpp` files. There
are a couple of reasons for this.
1. The symbols in those files are private to the dylib, but they are declared
in the headers and given external visibility.
2. Those symbols frequently use std::chrono types in their interface. This
creates a layering violating between the main STL and the support headers.
3. Moving the implementations of those functions now creates a bunch of noise
in an already noisy patch. Keep the implementations in place for now.
Those changes can be re-added in a follow up patch.
Please combine all everything in `support` into a single file in the top level
directory called `__thread_support` (Or similar, the name really doesn't matter
but it should start with two underscores). Libc++ keeps the number of private
headers is kept to a minimum and only introduces a new one when it needs to
break a cycle or factor out code used in two places.
Overall this is looking good. Have you implemented a support layer for windows
at all? If so I would be curious to see it, I would like to get a better idea
of the end goal.
Comment at: include/support/pthread/condition_variable.h:54
@@ +53,3 @@
+_LIBCPP_FUNC_VIS
+void __os_cond_var_timed_wait(__os_cond_var* __cv,
+ __os_mutex* __m,
This is an internal function. It shouldn't be declared in these headers.
Comment at: include/support/pthread/mutex.h:27
@@ +26,3 @@
+typedef pthread_mutex_t __os_mutex;
+_LIBCPP_CONSTEXPR pthread_mutex_t __os_mutex_init = PTHREAD_MUTEX_INITIALIZER;
+
I think with would work for C++11 onward but unfortunately the mutex internals
need to support C++03. For this reason we have to use a macro to ensure the
initializer is constexpr.
Comment at: include/support/pthread/mutex.h:59
@@ +58,3 @@
+_LIBCPP_FUNC_VIS
+void __os_recursive_mutex_init(__os_mutex* __m);
+
This is an internal function and shouldn't be declared in the headers.
Comment at: include/support/pthread/mutex.h:62
@@ +61,3 @@
+inline _LIBCPP_INLINE_VISIBILITY
+int __os_recursive_mutex_lock(__os_mutex* __m)
+{
Shouldn't these functions take `__os_recursive_mutex*` types?
Comment at: include/support/pthread/mutex.h:85
@@ +84,3 @@
+
+_LIBCPP_FUNC_VIS void __os_call_once(volatile unsigned long&, void*,
void(*)(void*));
+
This is an internal function and shouldn't be declared in the headers.
Comment at: include/support/pthread/thread.h:60
@@ +59,3 @@
+{
+bool res = pthread_equal(t1, t2);
+return res != 0 ? 0 : (t1 < t2 ? -1 : 1);
`res` needs to be an int, not a bool.
Comment at: include/support/pthread/thread.h:67
@@ +66,3 @@
+basic_ostream<_CharT, _Traits>&
+__os_write_to_ostream(basic_ostream<_CharT, _Traits>& __os, __os_thread_id
__id)
+{
This function seems superfluous and introduces a dependency on IO streams. Get
rid of it.
Comment at: include/support/pthread/thread.h:114
@@ +113,3 @@
+
+_LIBCPP_FUNC_VIS void __os_sleep_for(const chrono::nanoseconds& ns);
+
This is an internal function and shouldn't be declared in the headers.
Comment at: include/thread:238
@@ -236,3 +237,3 @@
bool operator==(__thread_id __x, __thread_id __y) _NOEXCEPT
-{return __x.__id_ == __y.__id_;}
+{return __libcpp_support::__os_thread_id_compare(__x.__id_, __y.__id_)
== 0;}
friend _LIBCPP_INLINE_VISIBILITY
This now call's `pthread_equal`, across a library boundary, where it didn't
before. That seems like a pessimization.
Comment at: include/thread:244
@@ -242,3 +243,3 @@
bool operator< (__thread_id __x, __thread_id __y) _NOEXCEPT
-{return __x.__id_ < __y.__id_;}
+{return __libcpp_support::__os_thread_id_compare(__x.__id_,
__y.__id_) < 0;}
friend _LIBCPP_INLINE_VISIBILITY
Same as above.
Comment at: include/thread:260
@@ -258,3 +259,3 @@
operator<<(basic_ostream<_CharT, _Traits>& __os, __thread_id __id)
-{return __os << __id.__id_;}
+{return __libcpp_support::__os_write_to_ostream(__os, __id.__id_);}
As mentioned elsewhere just do this operation inline. No need for an extra
function.
Comment at: include/thread:393
@@ -390,3 +392,3 @@
std::unique_ptr<_Fp> __p(new _Fp(__f));
-int __ec = pthread_create(&__t_, 0, &__thread_proxy<_Fp>, __p.get());
+int __ec = __libcpp_support::__os_create_thread(&__t_,
&__thread_proxy<_Gp>, __p.get());
if (__ec == 0)
Typo `_Gp` should be `_Fp`.
Repository:
rL LLVM
http://reviews.llvm.org/D11781
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
Re: [PATCH] D11781: Refactored pthread usage in libcxx
rmaprath added a subscriber: rmaprath. rmaprath added a comment. Hi, could I know the status of this? I'd like to push this forward. @espositofulvio: Are you working on this? (just checking since this has gone stale for a while). @EricWF: I can create a separate diff for further review if this one is too cluttered. Cheers, / Asiri Repository: rL LLVM http://reviews.llvm.org/D11781 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11781: Refactored pthread usage in libcxx
espositofulvio updated the summary for this revision.
espositofulvio updated this revision to Diff 34104.
espositofulvio added a comment.
- Addressed possible ABI breaks
- Reverted to not using a __config_file as @jroelofs has two separate patch for
that
Repository:
rL LLVM
http://reviews.llvm.org/D11781
Files:
.gitignore
CMakeLists.txt
include/__config
include/__mutex_base
include/mutex
include/support/condition_variable.h
include/support/mutex.h
include/support/pthread/condition_variable.h
include/support/pthread/mutex.h
include/support/pthread/thread.h
include/support/thread.h
include/thread
lib/CMakeLists.txt
src/algorithm.cpp
src/condition_variable.cpp
src/memory.cpp
src/mutex.cpp
src/support/pthread/condition_variable.cpp
src/support/pthread/mutex.cpp
src/support/pthread/thread.cpp
src/thread.cpp
Index: src/thread.cpp
===
--- src/thread.cpp
+++ src/thread.cpp
@@ -32,6 +32,8 @@
_LIBCPP_BEGIN_NAMESPACE_STD
+using namespace __libcpp_support;
+
thread::~thread()
{
if (__t_ != 0)
@@ -41,7 +43,7 @@
void
thread::join()
{
-int ec = pthread_join(__t_, 0);
+int ec = __os_thread_join(__t_);
#ifndef _LIBCPP_NO_EXCEPTIONS
if (ec)
throw system_error(error_code(ec, system_category()), "thread::join failed");
@@ -57,7 +59,7 @@
int ec = EINVAL;
if (__t_ != 0)
{
-ec = pthread_detach(__t_);
+ec = __os_thread_detach(__t_);
if (ec == 0)
__t_ = 0;
}
@@ -104,34 +106,12 @@
namespace this_thread
{
-void
-sleep_for(const chrono::nanoseconds& ns)
+void sleep_for(const chrono::nanoseconds& ns)
{
-using namespace chrono;
-if (ns > nanoseconds::zero())
-{
-seconds s = duration_cast(ns);
-timespec ts;
-typedef decltype(ts.tv_sec) ts_sec;
-_LIBCPP_CONSTEXPR ts_sec ts_sec_max = numeric_limits::max();
-if (s.count() < ts_sec_max)
-{
-ts.tv_sec = static_cast(s.count());
-ts.tv_nsec = static_cast((ns-s).count());
-}
-else
-{
-ts.tv_sec = ts_sec_max;
-ts.tv_nsec = giga::num - 1;
-}
-
-while (nanosleep(, ) == -1 && errno == EINTR)
-;
-}
+__libcpp_support::__os_sleep_for(ns);
}
-} // this_thread
-
+}
__thread_specific_ptr<__thread_struct>&
__thread_local_data()
{
Index: src/support/pthread/thread.cpp
===
--- /dev/null
+++ src/support/pthread/thread.cpp
@@ -0,0 +1,54 @@
+// -*- C++ -*-
+//===-- support/pthread/thread.cpp ===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "__config"
+
+#include
+#include
+#define _LIBCPP_INCLUDE_THREAD_API
+#include
+#undef _LIBCPP_INCLUDE_THREAD_API
+
+_LIBCPP_BEGIN_NAMESPACE_STD
+#ifndef _LIBCPP_HAS_NO_THREADS
+
+namespace __libcpp_support
+{
+
+void
+__os_sleep_for(const chrono::nanoseconds& ns)
+{
+using namespace chrono;
+if (ns > nanoseconds::zero())
+{
+seconds s = duration_cast(ns);
+timespec ts;
+typedef decltype(ts.tv_sec) ts_sec;
+_LIBCPP_CONSTEXPR ts_sec ts_sec_max = numeric_limits::max();
+if (s.count() < ts_sec_max)
+{
+ts.tv_sec = static_cast(s.count());
+ts.tv_nsec = static_cast((ns-s).count());
+}
+else
+{
+ts.tv_sec = ts_sec_max;
+ts.tv_nsec = giga::num - 1;
+}
+
+while (nanosleep(, ) == -1 && errno == EINTR)
+;
+}
+}
+
+} //namespace __libcpp_support
+
+#endif // _LIBCPP_HAS_NO_THREADS
+_LIBCPP_END_NAMESPACE_STD
Index: src/support/pthread/mutex.cpp
===
--- /dev/null
+++ src/support/pthread/mutex.cpp
@@ -0,0 +1,131 @@
+// -*- C++ -*-
+//===-- support/pthread/thread.cpp ===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "__config"
+
+#include
+#include
+#include "../../include/atomic_support.h"
+
+#define _LIBCPP_INCLUDE_THREAD_API
+#include
+#undef _LIBCPP_INCLUDE_THREAD_API
+
+_LIBCPP_BEGIN_NAMESPACE_STD
+#ifndef _LIBCPP_HAS_NO_THREADS
+
+namespace __libcpp_support
+{
+
+void __os_recursive_mutex_init(__os_mutex* __m)
+{
+pthread_mutexattr_t attr;
+int ec = pthread_mutexattr_init();
+if
Re: [PATCH] D11781: Refactored pthread usage in libcxx
EricWF added a comment.
Added more inline comments.
Comment at: include/__mutex_base:36
@@ -35,3 +37,3 @@
#else
- mutex() _NOEXCEPT {__m_ = (pthread_mutex_t)PTHREAD_MUTEX_INITIALIZER;}
#endif
Why was the cast insignificant?
Comment at: include/__mutex_base:275
@@ -274,3 +277,3 @@
#else
-condition_variable() {__cv_ = (pthread_cond_t)PTHREAD_COND_INITIALIZER;}
#endif
Why was the cast insignificant?
Comment at: include/thread:412
@@ -411,3 +413,6 @@
-_LIBCPP_FUNC_VIS void sleep_for(const chrono::nanoseconds ns);
I'm 99% this is an ABI break. Don't inline this function and make the change
within the definition
Comment at: include/type_traits:222
@@ -221,3 +221,3 @@
-template class _Tp
-struct __identity { typedef _Tp type; };
+template class T
+struct __identity { typedef T type; };
This change seems to have snuck in.
Comment at: src/algorithm.cpp:51
@@ -50,3 +50,3 @@
#ifndef _LIBCPP_HAS_NO_THREADS
-static pthread_mutex_t __rs_mut = PTHREAD_MUTEX_INITIALIZER;
+static mutex __rs_mut;
#endif
I think this prevents __rs_mut from being initialized during constant
initialization.
(http://en.cppreference.com/w/cpp/language/constant_initialization)
Comment at: src/memory.cpp:130
@@ -129,3 +129,1 @@
static const std::size_t __sp_mut_count = 16;
-static pthread_mutex_t mut_back_imp[__sp_mut_count] =
-{
I have no idea what is going on here. Do you understand what this code was
trying to do?
Repository:
rL LLVM
http://reviews.llvm.org/D11781
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11781: Refactored pthread usage in libcxx
EricWF added a reviewer: EricWF. EricWF added a comment. This patch has a long way to go but it has also come a long way. Here are a couple of problems I see with it. 1. There are still plenty of ABI breaks. I'll try and point them all out. 2. This patch adds a lot of headers. libc++ has historically tried to keep the number of headers to a minimum for the reason that filesystem operations are expensive and its cheaper to include a few big headers as opposed to many small ones. 3. There where some subtle static initialization changes. However they may have been fixed. I'll do a more through review soon. Comment at: include/mutex:534 @@ -533,3 +516,3 @@ -_LIBCPP_FUNC_VIS void __call_once(volatile unsigned long, void*, void(*)(void*)); This looks like an ABI break to me. Repository: rL LLVM http://reviews.llvm.org/D11781 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11781: Refactored pthread usage in libcxx
espositofulvio added inline comments.
Comment at: include/__config:742
@@ +741,3 @@
+#ifndef _LIBCPP_HAS_NO_THREADS
+# if defined(__FreeBSD__) || \
+defined(__NetBSD__) || \
jroelofs wrote:
jroelofs wrote:
espositofulvio wrote:
theraven wrote:
espositofulvio wrote:
jroelofs wrote:
espositofulvio wrote:
jroelofs wrote:
jroelofs wrote:
@espositofulvio: @ed meant this:
```
#ifndef _WIN32
# include unistd.h
# if _POSIX_THREADS 0
...
# endif
#endif
```
Which //is// the correct way to test for this.
That being said, there have been discussions before about
whether or not we should #include unistd.h in __config,
with the conclusion being that we shouldn't.
It would be better if this were a CMake configure-time check
that sets _LIBCPP_THREAD_API, rather than these build-time
guards.
Tried adding that as configure time checks, but then libcxxabi
fails to compile because of the guard in __config to check that
_LIBCPP_THREAD_API has beed defined when _LIBCPP_HAS_NO_THREADS
is not.
As a side note: Is Windows the only OS which hasn't got unistd.h?
Tried adding that as configure time checks...
Can you put the patch for that up on gist.github.com, or a
pastebin?... I'll take a look.
As a side note: Is Windows the only OS which hasn't got unistd.h?
For the platforms libcxx currently builds on, yes.
Can you put the patch for that up on gist.github.com, or a
pastebin?... I'll take a look.
It's here https://gist.github.com/espositofulvio/eac2fb08acf2e430c516
I'm sorry to have triggered this bikeshed. Given that, of the
currently supported platforms, Windows is the only one where we want to
use threads but don't want to use PTHREADS, I think it's fine to have
this check be !WIN32. The important thing is having the check *in one
place* so that the person who wants to add the *second* non-pthread
threading implementation doesn't have a load of different places to
look: they can just change it in `__config` and then chase all of the
compile failures.
Given that it's something I suggested in a comment on the first revision,
I was happy to try it out. It was also a quick change. Unfortunately it
brakes libcxxabi build (at least the way I've done it), so I'm happy to
make the change if @jroelofs finds another way
I meant something like this:
https://gist.github.com/jroelofs/279cb2639ad910b953d2
... which doesn't quite work yet, but should give you the idea. I don't
know how to get CMake to treat `${CMAKE_BINARY_DIR}/include/c++/v1` as the
includes directory instead of `${CMAKE_CURRENT_SOURCE_DIR}/../include`.
Maybe @ericwf can help with that?
Basically the idea is that we have cmake create a __config_site header
which __config includes. This new header would then have the definitions
for these kinds of configure-time decisions.
I tinkered around with this a bit more, and got it to work. I've updated the
gist to reflect that.
To simplify things for @espositofulvio, I'm going to split out the
__config_site part of this change into its own review.
I was working on your idea and make it to compile as well, I'm just re-running
the test suite to be 100% sure. Looking at your diff it seems there's a
difference in the path.
${CMAKE_BINARY_DIR}/include/c++/v1/__config_site
the above doesn't work for me, I've used:
${CMAKE_CURRENT_SOURCE_DIR}/include/__config_site
Should I update my review?
Repository:
rL LLVM
http://reviews.llvm.org/D11781
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11781: Refactored pthread usage in libcxx
jroelofs added inline comments. Comment at: include/__config:744 @@ +743,3 @@ +#ifndef _LIBCPP_HAS_NO_THREADS +# ifndef _LIBCPP_THREAD_API +# error No thread API The reason to use `CMAKE_BINARY_DIR` over `CMAKE_CURRENT_SOURCE_DIR` as the location for this build product is so that the source directory doesn't get polluted by builds. Unfortunately that means that both libcxx and libcxxabi need to be pointed at the headers in the build directory, and not the ones from the source directory. I do have a pair of patches for this, which I'll post shortly. Repository: rL LLVM http://reviews.llvm.org/D11781 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11781: Refactored pthread usage in libcxx
theraven added inline comments. Comment at: include/__config:742 @@ +741,3 @@ +#ifndef _LIBCPP_HAS_NO_THREADS +# if defined(__FreeBSD__) || \ +defined(__NetBSD__) || \ espositofulvio wrote: jroelofs wrote: espositofulvio wrote: jroelofs wrote: jroelofs wrote: @espositofulvio: @ed meant this: ``` #ifndef _WIN32 # include unistd.h # if _POSIX_THREADS 0 ... # endif #endif ``` Which //is// the correct way to test for this. That being said, there have been discussions before about whether or not we should #include unistd.h in __config, with the conclusion being that we shouldn't. It would be better if this were a CMake configure-time check that sets _LIBCPP_THREAD_API, rather than these build-time guards. Tried adding that as configure time checks, but then libcxxabi fails to compile because of the guard in __config to check that _LIBCPP_THREAD_API has beed defined when _LIBCPP_HAS_NO_THREADS is not. As a side note: Is Windows the only OS which hasn't got unistd.h? Tried adding that as configure time checks... Can you put the patch for that up on gist.github.com, or a pastebin?... I'll take a look. As a side note: Is Windows the only OS which hasn't got unistd.h? For the platforms libcxx currently builds on, yes. Can you put the patch for that up on gist.github.com, or a pastebin?... I'll take a look. It's here https://gist.github.com/espositofulvio/eac2fb08acf2e430c516 I'm sorry to have triggered this bikeshed. Given that, of the currently supported platforms, Windows is the only one where we want to use threads but don't want to use PTHREADS, I think it's fine to have this check be !WIN32. The important thing is having the check *in one place* so that the person who wants to add the *second* non-pthread threading implementation doesn't have a load of different places to look: they can just change it in `__config` and then chase all of the compile failures. Repository: rL LLVM http://reviews.llvm.org/D11781 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11781: Refactored pthread usage in libcxx
espositofulvio added inline comments. Comment at: include/__config:742 @@ +741,3 @@ +#ifndef _LIBCPP_HAS_NO_THREADS +# if defined(__FreeBSD__) || \ +defined(__NetBSD__) || \ theraven wrote: espositofulvio wrote: jroelofs wrote: espositofulvio wrote: jroelofs wrote: jroelofs wrote: @espositofulvio: @ed meant this: ``` #ifndef _WIN32 # include unistd.h # if _POSIX_THREADS 0 ... # endif #endif ``` Which //is// the correct way to test for this. That being said, there have been discussions before about whether or not we should #include unistd.h in __config, with the conclusion being that we shouldn't. It would be better if this were a CMake configure-time check that sets _LIBCPP_THREAD_API, rather than these build-time guards. Tried adding that as configure time checks, but then libcxxabi fails to compile because of the guard in __config to check that _LIBCPP_THREAD_API has beed defined when _LIBCPP_HAS_NO_THREADS is not. As a side note: Is Windows the only OS which hasn't got unistd.h? Tried adding that as configure time checks... Can you put the patch for that up on gist.github.com, or a pastebin?... I'll take a look. As a side note: Is Windows the only OS which hasn't got unistd.h? For the platforms libcxx currently builds on, yes. Can you put the patch for that up on gist.github.com, or a pastebin?... I'll take a look. It's here https://gist.github.com/espositofulvio/eac2fb08acf2e430c516 I'm sorry to have triggered this bikeshed. Given that, of the currently supported platforms, Windows is the only one where we want to use threads but don't want to use PTHREADS, I think it's fine to have this check be !WIN32. The important thing is having the check *in one place* so that the person who wants to add the *second* non-pthread threading implementation doesn't have a load of different places to look: they can just change it in `__config` and then chase all of the compile failures. Given that it's something I suggested in a comment on the first revision, I was happy to try it out. It was also a quick change. Unfortunately it brakes libcxxabi build (at least the way I've done it), so I'm happy to make the change if @jroelofs finds another way Repository: rL LLVM http://reviews.llvm.org/D11781 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11781: Refactored pthread usage in libcxx
mclow.lists added a subscriber: mclow.lists. mclow.lists added a reviewer: mclow.lists. mclow.lists added a comment. I agree with @jroelofs that we don't want to `#include unistd.h` in `__config` Repository: rL LLVM http://reviews.llvm.org/D11781 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11781: Refactored pthread usage in libcxx
jroelofs added inline comments.
Comment at: include/__config:742
@@ +741,3 @@
+#ifndef _LIBCPP_HAS_NO_THREADS
+# if defined(__FreeBSD__) || \
+defined(__NetBSD__) || \
jroelofs wrote:
espositofulvio wrote:
theraven wrote:
espositofulvio wrote:
jroelofs wrote:
espositofulvio wrote:
jroelofs wrote:
jroelofs wrote:
@espositofulvio: @ed meant this:
```
#ifndef _WIN32
# include unistd.h
# if _POSIX_THREADS 0
...
# endif
#endif
```
Which //is// the correct way to test for this.
That being said, there have been discussions before about whether
or not we should #include unistd.h in __config, with the
conclusion being that we shouldn't.
It would be better if this were a CMake configure-time check that
sets _LIBCPP_THREAD_API, rather than these build-time guards.
Tried adding that as configure time checks, but then libcxxabi
fails to compile because of the guard in __config to check that
_LIBCPP_THREAD_API has beed defined when _LIBCPP_HAS_NO_THREADS is
not.
As a side note: Is Windows the only OS which hasn't got unistd.h?
Tried adding that as configure time checks...
Can you put the patch for that up on gist.github.com, or a
pastebin?... I'll take a look.
As a side note: Is Windows the only OS which hasn't got unistd.h?
For the platforms libcxx currently builds on, yes.
Can you put the patch for that up on gist.github.com, or a
pastebin?... I'll take a look.
It's here https://gist.github.com/espositofulvio/eac2fb08acf2e430c516
I'm sorry to have triggered this bikeshed. Given that, of the currently
supported platforms, Windows is the only one where we want to use threads
but don't want to use PTHREADS, I think it's fine to have this check be
!WIN32. The important thing is having the check *in one place* so that
the person who wants to add the *second* non-pthread threading
implementation doesn't have a load of different places to look: they can
just change it in `__config` and then chase all of the compile failures.
Given that it's something I suggested in a comment on the first revision, I
was happy to try it out. It was also a quick change. Unfortunately it
brakes libcxxabi build (at least the way I've done it), so I'm happy to
make the change if @jroelofs finds another way
I meant something like this:
https://gist.github.com/jroelofs/279cb2639ad910b953d2
... which doesn't quite work yet, but should give you the idea. I don't know
how to get CMake to treat `${CMAKE_BINARY_DIR}/include/c++/v1` as the
includes directory instead of `${CMAKE_CURRENT_SOURCE_DIR}/../include`. Maybe
@ericwf can help with that?
Basically the idea is that we have cmake create a __config_site header which
__config includes. This new header would then have the definitions for these
kinds of configure-time decisions.
I tinkered around with this a bit more, and got it to work. I've updated the
gist to reflect that.
To simplify things for @espositofulvio, I'm going to split out the
__config_site part of this change into its own review.
Repository:
rL LLVM
http://reviews.llvm.org/D11781
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11781: Refactored pthread usage in libcxx
espositofulvio updated this revision to Diff 31716.
espositofulvio added a comment.
Added __CloudABI__ to the list of platform using pthread
Repository:
rL LLVM
http://reviews.llvm.org/D11781
Files:
include/__config
include/__mutex_base
include/mutex
include/support/condition_variable.h
include/support/mutex.h
include/support/pthread/condition_variable.h
include/support/pthread/mutex.h
include/support/pthread/thread.h
include/support/thread.h
include/thread
include/type_traits
lib/CMakeLists.txt
src/algorithm.cpp
src/condition_variable.cpp
src/memory.cpp
src/mutex.cpp
src/support/pthread/condition_variable.cpp
src/support/pthread/mutex.cpp
src/support/pthread/thread.cpp
src/thread.cpp
Index: src/thread.cpp
===
--- src/thread.cpp
+++ src/thread.cpp
@@ -32,6 +32,8 @@
_LIBCPP_BEGIN_NAMESPACE_STD
+using namespace __libcpp_support;
+
thread::~thread()
{
if (__t_ != 0)
@@ -41,7 +43,7 @@
void
thread::join()
{
-int ec = pthread_join(__t_, 0);
+int ec = __os_thread_join(__t_);
#ifndef _LIBCPP_NO_EXCEPTIONS
if (ec)
throw system_error(error_code(ec, system_category()), thread::join failed);
@@ -57,7 +59,7 @@
int ec = EINVAL;
if (__t_ != 0)
{
-ec = pthread_detach(__t_);
+ec = __os_thread_detach(__t_);
if (ec == 0)
__t_ = 0;
}
@@ -101,37 +103,6 @@
#endif // defined(CTL_HW) defined(HW_NCPU)
}
-namespace this_thread
-{
-
-void
-sleep_for(const chrono::nanoseconds ns)
-{
-using namespace chrono;
-if (ns nanoseconds::zero())
-{
-seconds s = duration_castseconds(ns);
-timespec ts;
-typedef decltype(ts.tv_sec) ts_sec;
-_LIBCPP_CONSTEXPR ts_sec ts_sec_max = numeric_limitsts_sec::max();
-if (s.count() ts_sec_max)
-{
-ts.tv_sec = static_castts_sec(s.count());
-ts.tv_nsec = static_castdecltype(ts.tv_nsec)((ns-s).count());
-}
-else
-{
-ts.tv_sec = ts_sec_max;
-ts.tv_nsec = giga::num - 1;
-}
-
-while (nanosleep(ts, ts) == -1 errno == EINTR)
-;
-}
-}
-
-} // this_thread
-
__thread_specific_ptr__thread_struct
__thread_local_data()
{
Index: src/support/pthread/thread.cpp
===
--- /dev/null
+++ src/support/pthread/thread.cpp
@@ -0,0 +1,54 @@
+// -*- C++ -*-
+//===-- support/pthread/thread.cpp ===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include __config
+
+#include ostream
+#include chrono
+#define _LIBCPP_INCLUDE_THREAD_API
+#include support/pthread/thread.h
+#undef _LIBCPP_INCLUDE_THREAD_API
+
+_LIBCPP_BEGIN_NAMESPACE_STD
+#ifndef _LIBCPP_HAS_NO_THREADS
+
+namespace __libcpp_support
+{
+
+void
+__os_sleep_for(const chrono::nanoseconds ns)
+{
+using namespace chrono;
+if (ns nanoseconds::zero())
+{
+seconds s = duration_castseconds(ns);
+timespec ts;
+typedef decltype(ts.tv_sec) ts_sec;
+_LIBCPP_CONSTEXPR ts_sec ts_sec_max = numeric_limitsts_sec::max();
+if (s.count() ts_sec_max)
+{
+ts.tv_sec = static_castts_sec(s.count());
+ts.tv_nsec = static_castdecltype(ts.tv_nsec)((ns-s).count());
+}
+else
+{
+ts.tv_sec = ts_sec_max;
+ts.tv_nsec = giga::num - 1;
+}
+
+while (nanosleep(ts, ts) == -1 errno == EINTR)
+;
+}
+}
+
+} //namespace __libcpp_support
+
+#endif // _LIBCPP_HAS_NO_THREADS
+_LIBCPP_END_NAMESPACE_STD
Index: src/support/pthread/mutex.cpp
===
--- /dev/null
+++ src/support/pthread/mutex.cpp
@@ -0,0 +1,131 @@
+// -*- C++ -*-
+//===-- support/pthread/thread.cpp ===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include __config
+
+#include chrono
+#include system_error
+#include ../atomic_support.h
+
+#define _LIBCPP_INCLUDE_THREAD_API
+#include support/pthread/mutex.h
+#undef _LIBCPP_INCLUDE_THREAD_API
+
+_LIBCPP_BEGIN_NAMESPACE_STD
+#ifndef _LIBCPP_HAS_NO_THREADS
+
+namespace __libcpp_support
+{
+
+void __os_recursive_mutex_init(__os_mutex* __m)
+{
+pthread_mutexattr_t attr;
+int ec = pthread_mutexattr_init(attr);
+if (ec)
+goto fail;
+ec = pthread_mutexattr_settype(attr,
Re: [PATCH] D11781: Refactored pthread usage in libcxx
jroelofs added inline comments. Comment at: include/__config:742 @@ +741,3 @@ +#ifndef _LIBCPP_HAS_NO_THREADS +# if defined(__FreeBSD__) || \ +defined(__NetBSD__) || \ @espositofulvio: @ed meant this: ``` #ifndef _WIN32 # include unistd.h # if _POSIX_THREADS 0 ... # endif #endif ``` Which //is// the correct way to test for this. Repository: rL LLVM http://reviews.llvm.org/D11781 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11781: Refactored pthread usage in libcxx
ed added a comment. A general note I have regarding this change: Now that we're introducing separate implementations for mutexes and condition variables, could we also consider letting `shared_mutex` and friends simply use `pthread_rwlock_*()`? We currently have it implemented as a wrapper on top of mutexes and condition variables, but the downside of this approach is that it potentially has more overhead, but also works around priority inheritance if supported by the implementation. Repository: rL LLVM http://reviews.llvm.org/D11781 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11781: Refactored pthread usage in libcxx
espositofulvio marked 5 inline comments as done. espositofulvio added a comment. Repository: rL LLVM http://reviews.llvm.org/D11781 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11781: Refactored pthread usage in libcxx
espositofulvio added inline comments. Comment at: include/__config:742 @@ +741,3 @@ +#ifndef _LIBCPP_HAS_NO_THREADS +# if defined(__FreeBSD__) || \ +defined(__NetBSD__) || \ jroelofs wrote: jroelofs wrote: @espositofulvio: @ed meant this: ``` #ifndef _WIN32 # include unistd.h # if _POSIX_THREADS 0 ... # endif #endif ``` Which //is// the correct way to test for this. That being said, there have been discussions before about whether or not we should #include unistd.h in __config, with the conclusion being that we shouldn't. It would be better if this were a CMake configure-time check that sets _LIBCPP_THREAD_API, rather than these build-time guards. Tried adding that as configure time checks, but then libcxxabi fails to compile because of the guard in __config to check that _LIBCPP_THREAD_API has beed defined when _LIBCPP_HAS_NO_THREADS is not. As a side note: Is Windows the only OS which hasn't got unistd.h? Repository: rL LLVM http://reviews.llvm.org/D11781 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11781: Refactored pthread usage in libcxx
jroelofs added inline comments. Comment at: include/__config:742 @@ +741,3 @@ +#ifndef _LIBCPP_HAS_NO_THREADS +# if defined(__FreeBSD__) || \ +defined(__NetBSD__) || \ espositofulvio wrote: jroelofs wrote: jroelofs wrote: @espositofulvio: @ed meant this: ``` #ifndef _WIN32 # include unistd.h # if _POSIX_THREADS 0 ... # endif #endif ``` Which //is// the correct way to test for this. That being said, there have been discussions before about whether or not we should #include unistd.h in __config, with the conclusion being that we shouldn't. It would be better if this were a CMake configure-time check that sets _LIBCPP_THREAD_API, rather than these build-time guards. Tried adding that as configure time checks, but then libcxxabi fails to compile because of the guard in __config to check that _LIBCPP_THREAD_API has beed defined when _LIBCPP_HAS_NO_THREADS is not. As a side note: Is Windows the only OS which hasn't got unistd.h? Tried adding that as configure time checks... Can you put the patch for that up on gist.github.com, or a pastebin?... I'll take a look. As a side note: Is Windows the only OS which hasn't got unistd.h? For the platforms libcxx currently builds on, yes. Repository: rL LLVM http://reviews.llvm.org/D11781 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11781: Refactored pthread usage in libcxx
espositofulvio added inline comments. Comment at: include/__config:742 @@ +741,3 @@ +#ifndef _LIBCPP_HAS_NO_THREADS +# if defined(__FreeBSD__) || \ +defined(__NetBSD__) || \ jroelofs wrote: espositofulvio wrote: jroelofs wrote: jroelofs wrote: @espositofulvio: @ed meant this: ``` #ifndef _WIN32 # include unistd.h # if _POSIX_THREADS 0 ... # endif #endif ``` Which //is// the correct way to test for this. That being said, there have been discussions before about whether or not we should #include unistd.h in __config, with the conclusion being that we shouldn't. It would be better if this were a CMake configure-time check that sets _LIBCPP_THREAD_API, rather than these build-time guards. Tried adding that as configure time checks, but then libcxxabi fails to compile because of the guard in __config to check that _LIBCPP_THREAD_API has beed defined when _LIBCPP_HAS_NO_THREADS is not. As a side note: Is Windows the only OS which hasn't got unistd.h? Tried adding that as configure time checks... Can you put the patch for that up on gist.github.com, or a pastebin?... I'll take a look. As a side note: Is Windows the only OS which hasn't got unistd.h? For the platforms libcxx currently builds on, yes. Can you put the patch for that up on gist.github.com, or a pastebin?... I'll take a look. It's here https://gist.github.com/espositofulvio/eac2fb08acf2e430c516 Repository: rL LLVM http://reviews.llvm.org/D11781 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11781: Refactored pthread usage in libcxx
jroelofs added inline comments. Comment at: include/__config:742 @@ +741,3 @@ +#ifndef _LIBCPP_HAS_NO_THREADS +# if defined(__FreeBSD__) || \ +defined(__NetBSD__) || \ jroelofs wrote: @espositofulvio: @ed meant this: ``` #ifndef _WIN32 # include unistd.h # if _POSIX_THREADS 0 ... # endif #endif ``` Which //is// the correct way to test for this. That being said, there have been discussions before about whether or not we should #include unistd.h in __config, with the conclusion being that we shouldn't. It would be better if this were a CMake configure-time check that sets _LIBCPP_THREAD_API, rather than these build-time guards. Repository: rL LLVM http://reviews.llvm.org/D11781 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11781: Refactored pthread usage in libcxx
theraven added inline comments.
Comment at: include/__mutex_base:246
@@ -266,3 +245,3 @@
-class _LIBCPP_TYPE_VIS condition_variable
+class _LIBCPP_TYPE_VIS condition_variable : private
__libcxx_support::condition_variable
{
espositofulvio wrote:
theraven wrote:
espositofulvio wrote:
theraven wrote:
Does this change the ABI for a mutex on *NIX platforms? We can't
change the class layout for existing platforms (though we can indirect
things via typedefs).
My hunch is that it doesn't, std::condition_variable has no data member
now and the first base class (__libcxx_support::condition_variable)
contains only pthread_cond_t which will be effectively laid out at the
starting address of the object, as it was previously for
std::condition_variable,. I will check this out later in the evening
though.
I *think* that it's correct, but it's worth compiling a program that has
one compilation unit using the old header and another using the new one and
check that it's able to interoperate correctly.
Also check whether this depends on the implementation of the condition
variable. On FreeBSD, the pthread types are all pointers (currently -
we're going to have some painful ABI breakage at some point when we move
them to being something that can live in shared memory). In glibc, they're
structures. I don't think that you'll end up with different padding in the
ABI from wrapping them in a class, but it's worth checking.
it's worth compiling a program that has one compilation unit using the old
header and another using the new one and check that it's able to
interoperate correctly
Unfortunately this isn't going to work, some simbols are now defined in
__libcxx_support and not in std (they are just made available through using),
so an object file that includes the old header won't link against the new
library (unreferenced symbols). The alternative is to create inline
forwarding methods.
That's going to need some more refactoring then. Breaking the public ABI of
libc++ would be a show-stopper.
Repository:
rL LLVM
http://reviews.llvm.org/D11781
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
