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__) || defined(__linux__) || defined(__APPLE__) +# define _LIBCPP_THREAD_API _LIBCPP_PTHREAD ed wrote: > espositofulvio wrote: > > theraven wrote: > > > #ifdef unix will catch most of these (for some reason, not OS X, even > > > though it's the only one that actually is certified as UNIX...) > > I didn't know that and I'm not sure I've included all the supported > > platforms (in include/__config there are definitions for __ sun __ and __ > > CloudABI __ which I know very little about). Is there somewhere a list of > > supported platforms? > CloudABI has pthreads, but it does not have `unix` defined. Both FreeBSD and > CloudABI also support C11 ``. > > If you're going to stick to the code you have right now, be sure to add > `defined(__CloudABI__)` to that list of operating systems. > > The POSIX way of testing whether pthreads is available is: > > #include > #if _POSIX_THREADS > 0 > ... > #endif > > See: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/unistd.h.html Unfortunately I think the POSIX way would fail on any system that hasn't unistd.h (Windows), so I think I'll stick with the list and add the missing ones. Comment at: include/__config:742 @@ +741,3 @@ +#ifndef _LIBCPP_HAS_NO_THREADS +# if defined(__FreeBSD__) || defined(__NetBSD__) || defined(__linux__) || defined(__APPLE__) +# define _LIBCPP_THREAD_API _LIBCPP_PTHREAD espositofulvio wrote: > ed wrote: > > espositofulvio wrote: > > > theraven wrote: > > > > #ifdef unix will catch most of these (for some reason, not OS X, even > > > > though it's the only one that actually is certified as UNIX...) > > > I didn't know that and I'm not sure I've included all the supported > > > platforms (in include/__config there are definitions for __ sun __ and __ > > > CloudABI __ which I know very little about). Is there somewhere a list of > > > supported platforms? > > CloudABI has pthreads, but it does not have `unix` defined. Both FreeBSD > > and CloudABI also support C11 ``. > > > > If you're going to stick to the code you have right now, be sure to add > > `defined(__CloudABI__)` to that list of operating systems. > > > > The POSIX way of testing whether pthreads is available is: > > > > #include > > #if _POSIX_THREADS > 0 > > ... > > #endif > > > > See: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/unistd.h.html > Unfortunately I think the POSIX way would fail on any system that hasn't > unistd.h (Windows), so I think I'll stick with the list and add the missing > ones. It looks like unix is not defined on Linux, but it has __unix. I think it's safer to stick to the list. 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_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(&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
+#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(&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
+#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(&attr);
+if (ec)
+goto fail;
+ec = pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE);
+if (ec)
+{
+pthread_mutexattr_destroy(&attr);
+goto fail;
+}
+ec = pthread_mute
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 > > # 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 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
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 > > > > # 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 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
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 > > > > > > # 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 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
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
> > > > > > > > > # 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 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
espositofulvio removed rL LLVM as the repository for this revision.
espositofulvio updated this revision to Diff 31874.
espositofulvio added a comment.
Thread library selection is done at configure time by CMake now.
http://reviews.llvm.org/D11781
Files:
.gitignore
CMakeLists.txt
include/CMakeLists.txt
include/__config
include/__config_site.in
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_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(&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
+#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(&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
+#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(&attr);
+if (ec)
+goto fail;
+ec = pthread_mutexattr_settype(&attr, PTH
Re: [PATCH] D11781: Refactored pthread usage in libcxx
espositofulvio added a comment. In http://reviews.llvm.org/D11781#222452, @mclow.lists wrote: > How does this change interact with http://reviews.llvm.org/D11963 ? The difference between this and @jroelofs's one is that this copy the __config_site in the source directory which makes everything compile fine without having to point libcxx and libcxxabi to the build directory. Jonathan thinks it's better not to pollute the source dir so he made those patches. 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] D11963: Create a __config_site file to capture configuration decisions.
espositofulvio added inline comments. Comment at: include/__config:19 @@ -18,1 +18,3 @@ +#include <__config_site> + #ifdef __GNUC__ mclow.lists wrote: > I'm reluctant to do this; because every include file slows down compilation - > for every program that we compile. > > However, this may be the right thing to do. I'm with Jonathan here, having config params dealt with this way it's easier and make things more manageable while the price of a slowdown, I think, shouldn't be substantial. http://reviews.llvm.org/D11963 ___ 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#222572, @joerg wrote: > This feels a bit like a regression. Before, libc++ works fine by just > pointing into the include directory. With my change it still is fine pointing at the include directory. But as I said, Jonathan feels it's not right to have the configure generated include file in the source directory hence he's proposing a separate patch to address that. 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#227446, @EricWF wrote:
> 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.
>
> 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.
I know but it was a request from reviewers so that platform specific decision
were localized. I can obviously merge all of the support/mutex.h,
support/thread.h and support/condition_variable.h in a single support/thread.h
if that's the case.
Comment at: include/type_traits:222
@@ -221,3 +221,3 @@
-template
-struct __identity { typedef _Tp type; };
+template
+struct __identity { typedef T type; };
EricWF wrote:
> This change seems to have snuck in.
Yes, and actually I'm not sure how it did as it's something I haven't touched.
Unfortunately I don't have time before later next week to address all the
issues.
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
EricWF wrote:
> I think this prevents __rs_mut from being initialized during constant
> initialization.
> (http://en.cppreference.com/w/cpp/language/constant_initialization)
I think this falls in the second case:
Static or thread-local object of class type that is initialized by a
constructor call, if the constructor is constexpr and all constructor arguments
(including implicit conversions) are constant expressions, and if the
initializers in the constructor's initializer list and the brace-or-equal
initializers of the class memebers only contain constant expressions.
but I was actually relying on the fact that we have constexpr, which now I
understand could not be the case.
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] =
-{
EricWF wrote:
> I have no idea what is going on here. Do you understand what this code was
> trying to do?
My understanding is that this is trying to init the array during constant
initialization and later relying on the internal layout of std::mutex to be
equal to a pthread_mutex_t to cast it and use it as a std::mutex.
from cppreference:
Static or thread-local object (not necessarily of class type), that is not
initialized by a constructor call, if the object is value-initialized or if
every expression in its initializer is a constant expression.
Again, I think my change should be ok as long as constexpr is available, but I
could be wrong.
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/__mutex_base:36
@@ -35,3 +37,3 @@
#else
- mutex() _NOEXCEPT {__m_ = (pthread_mutex_t)PTHREAD_MUTEX_INITIALIZER;}
#endif
EricWF wrote:
> Why was the cast insignificant?
The cast was significant, but it's not needed anymore.
PTHREAD_MUTEX_INITIALIZER can be only used in initialization expression, not
regular assignment and that's why there was the cast. But now I'm assigning an
already-initialized mutex:
_LIBCPP_CONSTEXPR pthread_mutex_t __os_mutex_init = PTHREAD_MUTEX_INITIALIZER;
thus the cast is not needed anymore.
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(&ts, &ts) == -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(&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
+#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(&attr);
+if (ec)
+goto fail;
+e
Re: [PATCH] D11781: Refactored pthread usage in libcxx
espositofulvio added inline comments.
Comment at: include/__mutex_base:19
@@ +18,3 @@
+#ifndef _WIN32
+#include
+#endif
jroelofs wrote:
> I think it might make sense to create a file: `` where the
> contents are just:
>
> ```
> #ifndef _WIN32
> #include
> #endif
> ```
>
> And include that file here so that the platform decision of "which mutex
> implementation should we use" doesn't have these #ifndef guards spread
> everywhere.
>
> Maybe you could also add some defines & guards to enforce the idea that
> `` is never included directly, but rather only via
> ``.
>
> Same should go for thread.hpp, too.
A very good idea indeed. I'll make the change and update the patch.
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
{
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.
Comment at: include/mutex:182
@@ -181,2 +181,3 @@
#endif
-#include
+#ifndef _WIN32
+#include
theraven wrote:
> As above, there should probably be in a cross-platform support file that
> includes these. In particular, not-win32 is not the correct condition for
> uses-pthreads. We should probably have something in __config that determines
> the thread library to use. It would be quite nice to have a C11 thread back
> end, for example (though most implementations currently wrap pthreads).
In this case having a series of #ifdef __FreeBSD__ (or _WIN32, etc.) doesn't
quite cut it as we want to be able to select C11 or pthread on most of them and
on Windows one day it might even be C11, pthread or the win32 api. I guess the
alternative is to provide a cmake variable that default to something different
on each platform?
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/__mutex_base:246
@@ -266,3 +245,3 @@
-class _LIBCPP_TYPE_VIS condition_variable
+class _LIBCPP_TYPE_VIS condition_variable : private
__libcxx_support::condition_variable
{
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.
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/__mutex_base:246
@@ -266,3 +245,3 @@
-class _LIBCPP_TYPE_VIS condition_variable
+class _LIBCPP_TYPE_VIS condition_variable : private
__libcxx_support::condition_variable
{
theraven wrote:
> 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.
Yes, that's what I thought. In this case it's better to create a new diff and
abandon this one or just update it?
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__) || defined(__linux__) || defined(__APPLE__) +# define _LIBCPP_THREAD_API _LIBCPP_PTHREAD theraven wrote: > #ifdef unix will catch most of these (for some reason, not OS X, even though > it's the only one that actually is certified as UNIX...) I didn't know that and I'm not sure I've included all the supported platforms (in include/__config there are definitions for __ sun __ and __ CloudABI __ which I know very little about). Is there somewhere a list of supported platforms? 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#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
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
