[PATCH] D11781: Refactored pthread usage in libcxx

[David Chisnall via cfe-commits]

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:
> > 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.

================
Comment at: include/mutex:182
@@ -181,2 +181,3 @@
 #endif
-#include <sched.h>
+#ifndef _WIN32
+#include <support/pthread/mutex.hpp>
----------------
espositofulvio wrote:
> 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?
> 
We'll probably end up with a set of #ifdef FreeBSD (or whatever) things, but making sure that they're all in a single file will help.  If you're doing bring-up of a new platform, just having to set #define __LIBCXX_THREAD_API __LIBCXX_PTHREAD, or __LIBCXX_C11_THREADS, (or Haiku threads, or whatever) in one place makes life a bit easier.  One of the annoyances with trying to port asan was that the original developers used #ifdef __APPLE__ to mean 'is not Linux' and 'is Darwin' in various places, so we needed to look at every single change and determine whether they were shared between multiple non-GNU platforms or whether they were specific to OS X / iOS.  


Repository:
  rL LLVM

http://reviews.llvm.org/D11781