This is an archive of the discontinued LLVM Phabricator instance.

[libcxx][rfc] Externalized threading support
AbandonedPublic

Authored by rmaprath on Apr 22 2016, 8:47 AM.

Details

Summary

This patch builds on D19412.

The motivation here is to allow toolchain vendors to build a version of libcxx with all the underlying os threading mechanics differed to runtime - sacrificing some performance for flexibility.

This API (_LIBCPP_THREAD_API_EXTERNAL) currently only works when libcxx is build as a static library. This is because the shared library builds pass -z defs linker flag, which disallows undefined symbols in the library. I'm not familiar with shared library limitations on specific platforms, but in theory, this could work on shared library builds as well.

Currently there are quite a lot of test failures because the testing infrastructure need to be updated to provide an implementation of this threading API (pthreads will do). Our plan is to hide this whole extension under some cmake variable at some point.

Before putting more effort into this, I thought of checking if the community if open to this kind of an API. Of course, we (ARM) will be committed to maintaining this API in the long run.

Diff Detail

Event Timeline

rmaprath updated this revision to Diff 54659.Apr 22 2016, 8:47 AM
rmaprath retitled this revision from to [libcxx][rfc] Externalized threading support.
rmaprath updated this object.
rmaprath added a subscriber: cfe-commits.

I forgot to mention that we will need a similar (although much smaller) porting layer for libcxxabi and libunwind, as those too currently have a pthread dependency.

bcraig added a subscriber: bcraig.Apr 26 2016, 7:03 AM

Note: My opinion doesn't count for much as far as community acceptance goes.

I'm fine with the idea. I think there is at least one significant conformance hurdle that you will need to address though. I also have some commentary on the implementation details.

include/__mutex_base
43 ↗(On Diff #54659)

This is not C++14 conformant. The default constructor for mutex() needs to be constexpr.

I'm not sure if there is a good way around this issue given the problem you are trying to solve. There have been similar constexpr issues with this constructor with regards to musl libc.

include/__os_support
23 ↗(On Diff #54659)

I would prefer that the internal types use an under-under-libcpp __libcpp prefix. I've had issues in the past with my under-under symbol names colliding with those from the underlying C library, especially for obvious names like under-under-mutex_t.

src/algorithm.cpp
65

I would rather the new branch be used across the board. I don't like littering the generic implementation files with preprocessor blocks everywhere.

Stated differently, get rid of

__libcpp_os_support::__os_mutex __rs_mut

and just use

mutex __rs_mut

everywhere instead.

src/mutex.cpp
31

My preference is for all of this to hide behind the __os_mutex_destroy call, instead of having the preprocessor block here. It looks like that has already been discussed in a prior review though ( http://reviews.llvm.org/D11781 ).

rmaprath updated this revision to Diff 55127.Apr 26 2016, 4:48 PM

Addressing review comments from @bcraig:

Adjusted according to the changes of D19412. This patch is now mostly trivial.

There is a small catch in that, this version of the API takes pointer-to-pointer type arguments for most __os_xxx functions. This is partly because we need to use opaque pointer types to hide the underlying implementation details from libcxx internals. This can be worked-around by using some template hackery (along the lines of is_pointer), but I thought to leave it as it is to avoid cluttering up the sources for the normal use case.

LGTM. You will still need to get approval from one of your original reviewers though.

rmaprath updated this revision to Diff 55503.Apr 28 2016, 3:33 PM

Updating to syc-up with D19412. Obviously, this patch needs more work in terms of setting up the testing story (using a pthreads based implementation of the threading API).

rmaprath updated this revision to Diff 55570.Apr 29 2016, 4:38 AM

Re-sync with D19412.

rmaprath updated this revision to Diff 55573.Apr 29 2016, 5:00 AM

Minor re-spin on D19412.

rmaprath updated this revision to Diff 55967.May 3 2016, 4:05 AM

Re-spun on top of D19412.