[prev in list] [next in list] [prev in thread] [next in thread] 

List:       cfe-commits
Subject:    Re: [PATCH] D11781: Refactored pthread usage in libcxx
From:       David Chisnall via cfe-commits <cfe-commits () lists ! llvm ! org>
Date:       2015-08-06 15:23:47
Message-ID: 17a7eaa48625c7d611ba091d0fc8ffcb () localhost ! localdomain
[Download RAW message or body]

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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[prev in list] [next in list] [prev in thread] [next in thread]