This is an archive of the discontinued LLVM Phabricator instance.

Refactored pthread usage in libcxx
Needs ReviewPublic

Authored by espositofulvio on Aug 5 2015, 2:38 PM.

Details

Summary

This patch is a refactoring of pthread usage in libcxx to support/pthread in order to ease porting to Windows.

Diff Detail

Event Timeline

espositofulvio retitled this revision from to Refactored pthread usage in libcxx.
espositofulvio updated this object.
espositofulvio added a reviewer: theraven.
danalbert set the repository for this revision to rL LLVM.Aug 5 2015, 4:24 PM
danalbert added a subscriber: cfe-commits.
jroelofs added inline comments.
include/__mutex_base
19

I think it might make sense to create a file: <support/mutex.h> where the contents are just:

#ifndef _WIN32
#include <support/pthread/mutex.hpp>
#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 <support/pthread/mutex.hpp> is never included directly, but rather only via <support/mutex.h>.

Same should go for thread.hpp, too.

asl added a subscriber: asl.Aug 6 2015, 12:57 AM
theraven added inline comments.Aug 6 2015, 6:37 AM
include/__mutex_base
246

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

include/mutex
182

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

269

I didn't realise this idiocy made it into C++11, I thought it was confined to C11. Locking a mutex using anything other than time intervals on the monotonic time source is a great way of producing non-working programs. We should probably warn if anyone actually calls this...

espositofulvio added inline comments.Aug 6 2015, 7:21 AM
include/__mutex_base
19

A very good idea indeed. I'll make the change and update the patch.

246

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.

include/mutex
182

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?

theraven added inline comments.Aug 6 2015, 8:23 AM
include/__mutex_base
246

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.

include/mutex
182

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.

espositofulvio added inline comments.Aug 6 2015, 4:04 PM
include/__mutex_base
246

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.

theraven added inline comments.Aug 7 2015, 1:38 AM
include/__mutex_base
246

That's going to need some more refactoring then. Breaking the public ABI of libc++ would be a show-stopper.

espositofulvio added inline comments.Aug 7 2015, 2:42 AM
include/__mutex_base
246

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?

better to just update this one... you've got our attention here review-wise.

espositofulvio updated this object.
espositofulvio removed rL LLVM as the repository for this revision.

To avoid breaking public api and ABI, the os abstraction layer is made up of typedefs for pthread types + plain functions that are called from the std:: stuff.
The recursive mutex layer is identical for pthread to the mutex layer because pthread internally select locking implementation depending on the mutex attributes, but we need both because on Windows the recursive stuff must be implemented with a counter.

espositofulvio set the repository for this revision to rL LLVM.Aug 8 2015, 12:28 PM
theraven edited edge metadata.Aug 8 2015, 12:52 PM

This version looks much better (well, the previous one is cleaner, but this one looks like it shouldn't break the ABI - did you test this? It looks like it should work, but I've not actually tried it).

include/__config
742 ↗(On Diff #31593)

#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...)

espositofulvio marked an inline comment as done.Aug 8 2015, 1:12 PM

This version looks much better (well, the previous one is cleaner, but this one looks like it shouldn't break the ABI - did you test this? It looks like it should work, but I've not actually tried it).

I tried it on Linux, using the example from http://en.cppreference.com/w/cpp/thread/condition_variable, but splitting the main and the worker function in two different translation units and compiling one with the libc++ coming from ArchLinux and the other with my branch, and then linking both with the new lib. I don't have freebsd installed though, I can set up another virtual machine but I never used FreeBsd so I don't know how easy is to install and set up.

espositofulvio added inline comments.Aug 8 2015, 1:23 PM
include/__config
742 ↗(On Diff #31593)

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?

ed added a subscriber: ed.Aug 10 2015, 8:11 AM
ed added inline comments.
include/__config
742 ↗(On Diff #31593)

CloudABI has pthreads, but it does not have unix defined. Both FreeBSD and CloudABI also support C11 <threads.h>.

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 <unistd.h>
#if _POSIX_THREADS > 0
...
#endif

See: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/unistd.h.html

ed added a comment.Aug 10 2015, 8:58 AM

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.

It would be nice to use rwlocks, but that's an ABI-breaking change. We'd need to bump the .so version and other annoyances. Definitely something I'd be in favour of seeing hidden behind some #ifdefs (and enabled for FreeBSD 11 and probably for the libc++ in packages), but it's beyond the scope of this set of changes.

espositofulvio added inline comments.Aug 10 2015, 1:31 PM
include/__config
742 ↗(On Diff #31593)

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.

742 ↗(On Diff #31593)

It looks like unix is not defined on Linux, but it has __unix. I think it's safer to stick to the list.

espositofulvio edited edge metadata.

Added CloudABI to the list of platform using pthread

espositofulvio marked 5 inline comments as done.Aug 10 2015, 1:40 PM
jroelofs added inline comments.Aug 10 2015, 1:43 PM
include/__config
742 ↗(On Diff #31716)

@espositofulvio: @ed meant this:

#ifndef _WIN32
#  include <unistd.h>
#  if _POSIX_THREADS > 0
...
#  endif
#endif

Which is the correct way to test for this.

jroelofs added inline comments.Aug 10 2015, 1:48 PM
include/__config
742 ↗(On Diff #31716)

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.

espositofulvio added inline comments.Aug 10 2015, 3:16 PM
include/__config
742 ↗(On Diff #31716)

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?

jroelofs added inline comments.Aug 10 2015, 3:29 PM
include/__config
742 ↗(On Diff #31716)

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.

espositofulvio added inline comments.Aug 10 2015, 4:06 PM
include/__config
742 ↗(On Diff #31716)

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

theraven added inline comments.Aug 11 2015, 12:58 AM
include/__config
742 ↗(On Diff #31716)

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.

espositofulvio added inline comments.Aug 11 2015, 2:24 AM
include/__config
742 ↗(On Diff #31716)

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

jroelofs added inline comments.
include/__config
742 ↗(On Diff #31716)

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.

mclow.lists added a subscriber: mclow.lists.

I agree with @jroelofs that we don't want to #include <unistd.h> in <__config>

jroelofs added inline comments.Aug 11 2015, 3:13 PM
include/__config
742 ↗(On Diff #31716)

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.

espositofulvio added inline comments.Aug 11 2015, 3:28 PM
include/__config
742 ↗(On Diff #31716)

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?

espositofulvio removed rL LLVM as the repository for this revision.

Thread library selection is done at configure time by CMake now.

espositofulvio set the repository for this revision to rL LLVM.Aug 11 2015, 3:42 PM
jroelofs added inline comments.Aug 11 2015, 3:58 PM
include/__config
744 ↗(On Diff #31874)

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.

jroelofs added inline comments.Aug 11 2015, 4:27 PM
include/__config
744 ↗(On Diff #31874)

See: D11963 and D11964

mclow.lists edited edge metadata.Aug 11 2015, 9:30 PM

How does this change interact with D11963 ?

How does this change interact with 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.

joerg added a subscriber: joerg.Aug 12 2015, 3:09 AM

This feels a bit like a regression. Before, libc++ works fine by just pointing into the include directory.

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.

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.

include/mutex
534

This looks like an ABI break to me.

EricWF edited edge metadata.Aug 18 2015, 11:43 PM

Added more inline comments.

include/__mutex_base
36

Why was the cast insignificant?

275

Why was the cast insignificant?

include/thread
412

I'm 99% this is an ABI break. Don't inline this function and make the change within the definition

include/type_traits
222 ↗(On Diff #31874)

This change seems to have snuck in.

src/algorithm.cpp
51

I think this prevents __rs_mut from being initialized during constant initialization. (http://en.cppreference.com/w/cpp/language/constant_initialization)

src/memory.cpp
130

I have no idea what is going on here. Do you understand what this code was trying to do?

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

include/type_traits
222 ↗(On Diff #31874)

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.

src/algorithm.cpp
51

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.

src/memory.cpp
130

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.

espositofulvio added inline comments.Aug 19 2015, 2:39 AM
include/__mutex_base
36

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.

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

Sorry for giving conflicting feedback. I'm happy with multiple headers for now. We can always address it later.

src/algorithm.cpp
51

The top of the documentation it shows the syntax that is required to get constant initialization. The initialization must have the form T obj = <initalizer>. I would be fine relying on constexpr though.

espositofulvio updated this revision to Diff 34104.EditedSep 5 2015, 5:12 AM
espositofulvio updated this object.
espositofulvio edited edge metadata.
  • Addressed possible ABI breaks
  • Reverted to not using a __config_file as @jroelofs has two separate patches for that

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

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.

include/support/pthread/condition_variable.h
54 ↗(On Diff #34104)

This is an internal function. It shouldn't be declared in these headers.

include/support/pthread/mutex.h
27 ↗(On Diff #34104)

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.

59 ↗(On Diff #34104)

This is an internal function and shouldn't be declared in the headers.

62 ↗(On Diff #34104)

Shouldn't these functions take __os_recursive_mutex* types?

85 ↗(On Diff #34104)

This is an internal function and shouldn't be declared in the headers.

include/support/pthread/thread.h
60 ↗(On Diff #34104)

res needs to be an int, not a bool.

67 ↗(On Diff #34104)

This function seems superfluous and introduces a dependency on IO streams. Get rid of it.

114 ↗(On Diff #34104)

This is an internal function and shouldn't be declared in the headers.

include/thread
127–128

This now call's pthread_equal, across a library boundary, where it didn't before. That seems like a pessimization.

127–128

Same as above.

127–128

As mentioned elsewhere just do this operation inline. No need for an extra function.

254–255

Typo _Gp should be _Fp.

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

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.

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

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

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

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

@espositofulvio: Thanks for the patch! :)

Committed as r268734.

@espositofulvio: Thanks for the patch! :)

Committed as r268734.

Glad to see you land the patch! Great work :)

EricWF resigned from this revision.Mar 27 2017, 10:03 PM

This revision is no longer relevant after. The __threading_support changes have applied changes similar to this.