Page MenuHomePhabricator

[libcxx] Externally threaded libc++ variant - Take 2
ClosedPublic

Authored by rmaprath on Jul 4 2016, 5:27 AM.

Details

Summary

This is the replacement for D20328.

Rather than using opaque pointer types to delegate the internal representations of mutex / condvar / thread types, in this patch we expect the libc++ vendor to explicitly define the implementation types upfront (at library compile time). The API is still left open for platform vendors to implement, it's only the types that need to be agreed upon.

Diff Detail

Repository
rL LLVM

Event Timeline

rmaprath updated this revision to Diff 62670.Jul 4 2016, 5:27 AM
rmaprath retitled this revision from to [libcxx] Externally threaded libc++ variant - Take 2.
rmaprath updated this object.
rmaprath added reviewers: mclow.lists, EricWF, bcraig.
rmaprath added a subscriber: cfe-commits.
bcraig added inline comments.Jul 5 2016, 9:55 AM
CMakeLists.txt
131 ↗(On Diff #62670)

I would prefer that you not change this option name, as that will break my builds. It's easy enough to fix on my side, but unless there's a really compelling reason, I'd rather not deal with the breakage.

include/__external_threading
26 ↗(On Diff #62670)

So users of external pthreading (or their compiler driver) would need to provide a pile of -D options on the command line? Or is it expected that users will have their own __external_threading header that comes earlier in the include path? Please document your expectation.

include/__threading_support
33 ↗(On Diff #62670)

More context would be useful here. "-U99999" perhaps (if using git).

rmaprath updated this revision to Diff 62844.EditedJul 6 2016, 4:20 AM

Addressed review comments from @bcraig:

  • Got rid of some of the option name cleanups (done to make them more consistent, but not relevant to the patch)
  • Arranged it so that libc++ vendors can drop in __external_threading header to override the default setup
    • By default, __threading_support declares the function prototypes and relies on pthread types - this makes it possible to build+test an externally-threaded libc++ variant with vanilla sources. The implementation of the thread api (in test/support/external_threads.cpp) is separate from the library, the two are glued together by __threading_support.
rmaprath marked 2 inline comments as done.Jul 6 2016, 4:23 AM
rmaprath updated this revision to Diff 62847.Jul 6 2016, 4:41 AM

Improve comment. NFC.

bcraig edited edge metadata.Jul 6 2016, 6:36 AM

LGTM. As usual, you'll want to get one of the code owner's sign off first though.

rmaprath updated this revision to Diff 63670.Jul 12 2016, 5:34 AM
rmaprath edited edge metadata.

Minor cleanup + Ping.

compnerd added inline comments.
CMakeLists.txt
139 ↗(On Diff #68011)

Can you use cmake_dependent_option here instead please?

include/__config
830 ↗(On Diff #68011)

clang-format?

rmaprath added inline comments.Aug 19 2016, 4:49 AM
CMakeLists.txt
139 ↗(On Diff #68011)

I have two problems with this:

  • Currently we don't use the CMakeDepedentOption module anywhere else in the llvm project
  • This option silently forces the cmake switch in question to the FORCE (last) argument when the dependency in question is not satisfied. Our current approach on the other hand is to issue an error to the user indicating the incompatible options (see hunk below).

I'm slightly leaning towards keeping the current approach. @EricWF: WDYT?

include/__config
830 ↗(On Diff #68011)

Running clang-format -style=llvm on this file shows that it's going to do a lot of changes to it. Do we want that? Perhaps it should be a separate patch?

Not sure if this is what you meant.

@compnerd: I plan to look at D18482 soon (going to need it when this lands). Got moved to a new machine, still setting up the environment :)

@mclow.lists: Ping?

mclow.lists added inline comments.Aug 22 2016, 8:07 AM
include/__external_threading
26 ↗(On Diff #62670)

My expectation is that system vendors who have a different threading implementation will ship a customized version of the libc++ headers and dylib.

The headers will include a replacement include/__external_threading, and a modified include/__config that contains additional #defines

What this patch does is to provide a customization point to produce such libraries.

rmaprath added inline comments.Aug 22 2016, 9:08 AM
include/__external_threading
26 ↗(On Diff #62670)

This is correct.

With the latest revision, I (following @bcraig's comments) have made this a bit more simpler; system vendors can simply drop in the header include/__external_threading to override the default threading implementation. When this header is provided (and _LIBCPP_HAS_THREAD_API_EXTERNAL is defined - which is automatically wired into __config header through __config_site.in at build time), the built library will end up using the new threading implementation.

Comments from @mclow.lists on the latest revision (received offline):

  • Need instructions on how to build and test the patch...
  • Need to be able to build+test on Mac (looks like there is a small problem in the patch w.r.t pthreads that makes the build fail on Mac)
  • Need to work on shared library builds as well

I will address these comments soon (as soon as I get familiar with how Macs work...)

rmaprath updated this revision to Diff 69010.Aug 23 2016, 9:55 AM

Addressed above comments from @mclow.lists.

Getting this to work on shared library builds was a bit tricky, had to use -undefined dynamic_lookup linker option on OSX and strip off the default -Wl,-z,defs linker option on linux.

Now this should build+test on OSX with the usual cmake options.

compnerd added inline comments.Aug 23 2016, 9:41 PM
CMakeLists.txt
372 ↗(On Diff #69010)

There is a similar change in swift to deal with this pollution :-(.

include/__config
830 ↗(On Diff #69010)

I meant on the lines that you added :-).

include/__threading_support
261 ↗(On Diff #69010)

I think we should explicitly spell out tls. The extra character won't hurt.

lib/CMakeLists.txt
216 ↗(On Diff #69010)

Have you considered a slightly alternative approach of basically implementing pthreads as an "external" threading model as well? You could override it, or take the default implementation that is provided. It avoids having a second supporting DSO for threading as we could always statically link against it.

test/support/external_threads.cpp
26 ↗(On Diff #69010)

I think this would be nicer as:

if (int error = pthread_mutexattr_init(&attr))
  return error;
52 ↗(On Diff #69010)

I don't think that the label for failure is adding anything since you really are handling the destruction of the mutex at all failure sites.

70 ↗(On Diff #69010)

Please choose a single style.

103 ↗(On Diff #69010)

Please choose a single style (and below).

131 ↗(On Diff #69010)

Can you compare to nullptr instead?

142 ↗(On Diff #69010)

Same.

rmaprath added inline comments.Aug 24 2016, 1:46 PM
include/__config
830 ↗(On Diff #69010)

Will update to reflect rest of the source file.

include/__threading_support
261 ↗(On Diff #69010)

Will fix.

lib/CMakeLists.txt
216 ↗(On Diff #69010)

So, the particulars of this design was discussed and (generally) agreed upon sometime back.

Currently pthreads is hidden behind the same threading API as the externally threaded variant (on this patch). To override the default (pthread) implementation, you just have to drop in an __external_threading header and specify the -DLIBCXX_HAS_EXTERNAL_THREAD_API cmake option.

There's no need to provide a second DSO if your __external_threading header (and your thread-system sources) are available at libc++ build time.

But we have a requirement that the library needs to be build-able in such a way you can provide a second DSO holding the implementation of the API. That is, we need to be able to build and ship a library to which customers can drop-in their own thread implementation (i.e. the thread-support DSO).

Note that in one of my earlier patches, I proposed an even more de-coupled external threading system where even the types like __libcpp_mutex_t were opaque pointers. @mclow.lists opposed that (perhaps rightfully so, now I think about it) and currently these types need to be known at libc++ compile time. The implementation of the API functions like __libcpp_mutex_lock() can be deferred to link time (this is what is provided in the second DSO).

Hope that clarifies everything here?

test/support/external_threads.cpp
26 ↗(On Diff #69010)

I think I'm following the conventions used in the libc++ source files here. Will double check tomorrow.

52 ↗(On Diff #69010)

Not sure what I did here. Will double check and fix.

70 ↗(On Diff #69010)

Will fix.

103 ↗(On Diff #69010)

Will fix.

131 ↗(On Diff #69010)

I can't remember exactly why, but I think there was a problem with using nullptr here. I will check and get back.

142 ↗(On Diff #69010)

Need to check.

EricWF edited edge metadata.Sep 6 2016, 4:19 AM

This looks great. Two comments:

  1. The declarations should be used in both the inline and external pthread implementation. They also need visibility declarations.
  2. Why can't we use the inline implementation to implement external_threads.cpp?

I took a stab at it here.

After that this LGTM.

lib/CMakeLists.txt
211 ↗(On Diff #69010)

When this is building as a shared library it needs to do the -nodefaultlibs dance to prevent linking to libstdc++.so or other unwanted system libraries.

mclow.lists edited edge metadata.Sep 6 2016, 10:09 PM

The patch (again) doesn't apply cleanly (test/CMakeLists.txt), so I applied it manually.
After that, it builds w/o errors, and passes all the tests.

After addressing @EricWF's concerns, I think this is ready to land.

This looks great. Two comments:

  1. The declarations should be used in both the inline and external pthread implementation. They also need visibility declarations.
  2. Why can't we use the inline implementation to implement external_threads.cpp?

    I took a stab at it here.

Yup, that looks better.

I will incorporate this change and also address the remaining comments from @compnerd before I commit.

Thanks all!

/ Asiri

rmaprath updated this revision to Diff 70840.Sep 9 2016, 9:25 AM
rmaprath edited edge metadata.

Final patch incorporating all the changes from @EricWF and @compnerd.

Will commit tomorrow (@mclow.lists gave approval earlier)

/ Asiri

This revision was automatically updated to reflect the committed changes.