This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Cleanup and document <__threading_support>
ClosedPublic

Authored by EricWF on Jan 4 2017, 1:52 PM.

Details

Summary

This patch attempts to clean up the macro configuration mess in <__threading_support>, specifically the mess involving external threading variants. Additionally this patch adds design documentation for <__threading_support> and the configuration macros it uses.

The primary change in this patch is separating the idea of an "external API" provided by <__external_threading> and the idea of having an external threading library. Now _LIBCPP_HAS_THREAD_API_EXTERNAL means that libc++ should use <__external_threading> and that the header is expected to exist. Additionally the new macro _LIBCPP_HAS_THREAD_LIBRARY_EXTERNAL is now used to configure for using an "external library" with the default threading API.

Diff Detail

Event Timeline

EricWF updated this revision to Diff 83122.Jan 4 2017, 1:52 PM
EricWF retitled this revision from to [libc++] Cleanup and document <__threading_support>.
EricWF updated this object.
EricWF added reviewers: compnerd, rmaprath.
EricWF added a subscriber: cfe-commits.
compnerd accepted this revision.Jan 4 2017, 2:11 PM
compnerd edited edge metadata.

This seems much better organized I think, and easier to follow. Thanks for the docs on the various options!

This revision is now accepted and ready to land.Jan 4 2017, 2:11 PM

@compnerd Just waiting on @rmaprath before committing. This is a breaking change for his use case since this patch changes the semantics of _LIBCPP_HAS_THREAD_API_EXTERNAL and -DLIBCXX_HAS_EXTERNAL_THREAD_API.

smeenai added inline comments.
docs/DesignDocs/ThreadingSupportAPI.rst
18

s/it's/its

EricWF updated this revision to Diff 83201.Jan 5 2017, 12:56 AM
EricWF edited edge metadata.
  • Address inline comments
rmaprath edited edge metadata.Jan 5 2017, 2:08 AM

This is much better (few nits inline). And many thanks for the docs update (btw, do those docs updates land on some web URL automagically when committed?).

I think we should point out on the docs that _LIBCPP_HAS_THREAD_LIBRARY_EXTERNAL is only meant for libc++ developers / testers. The purpose of this library variant is to:

  • Demonstrate that libc++ can be built with threading support externalised. We use pthreads as the underlying implementation in this case because that's the one we can easily test on any platform. I don't think anyone wants to distribute this library variant to users; they would rather compile libc++ for pthread directly instead.
  • Write tests that applies to the LIBCXX_HAS_EXTERNAL_THREAD_API variant. For example, I hope to write a test to check that the produced libc++ dylib has no references to pthread_ symbols within it. Such a test will ensure that the core sources of libc++ remains pthread-free (all threading should be wired through the threading API).

If a library vendors really wants to distribute a variant of libc++ with the threading support externalised, they should use a custom __external_threading header to replicate the behaviour of _LIBCPP_HAS_THREAD_LIBRARY_EXTERNAL.

Hope that makes sense?

CMakeLists.txt
172

Can you add another check so that this is not used together with LIBCXX_HAS_EXTERNAL_THREAD_API ?

include/__config
894

// ignore this comment please (phab won't allow me to delete draft comments)

include/__threading_support
25

This is already enforced in the __config header (just below where it derives the thread API). I think we can remove this check.

30

Do we need the second check? I think _LIBCPP_HAS_THREAD_LIBRARY_EXTERNAL is already set through __config header (through __config_site.in) when this header gets included external_threads.cpp (?).

35

Can we add a second check here like:

#if defined(_LIBCPP_HAS_THREAD_LIBRARY_EXTERNAL) && \
    !defined(_LIBCPP_HAS_THREAD_API_PTHREAD)
#error "Don't know how to declare the thread API without pthreads".
#endif

Or we can enforce this on the cmake file itself (or both). This ties up to my comment about _LIBCPP_HAS_THREAD_LIBRARY_EXTERNAL only meant to be for libc++ developers / testers.

EricWF marked 3 inline comments as done.Jan 5 2017, 2:36 AM

(btw, do those docs updates land on some web URL automagically when committed?).

Assuming the builder isn't broken then yes, the docs should automagically update within ~3 minutes.

I think we should point out on the docs that _LIBCPP_HAS_THREAD_LIBRARY_EXTERNAL is only meant for libc++ developers / testers. The purpose of this library variant is to:

  • Demonstrate that libc++ can be built with threading support externalised. We use pthreads as the underlying implementation in this case because that's the one we can easily test on any platform. I don't think anyone wants to distribute this library variant to users; they would rather compile libc++ for pthread directly instead.
  • Write tests that applies to the LIBCXX_HAS_EXTERNAL_THREAD_API variant. For example, I hope to write a test to check that the produced libc++ dylib has no references to pthread_ symbols within it. Such a test will ensure that the core sources of libc++ remains pthread-free (all threading should be wired through the threading API).

If a library vendors really wants to distribute a variant of libc++ with the threading support externalised, they should use a custom __external_threading header to replicate the behaviour of _LIBCPP_HAS_THREAD_LIBRARY_EXTERNAL.

Hope that makes sense?

All of that makes sense. I think you could write those docs better than I could. Would you be willing to update them?

include/__threading_support
25

Good catch. Thanks!

30

I think it seems useful enough to make _LIBCPP_BUILDING_THREAD_LIBRARY_EXTERNAL work on its own to build a standalone threading library.

35

I think that's already done in the __config header if we can't find a threading API.

EricWF updated this revision to Diff 83211.Jan 5 2017, 2:37 AM
EricWF edited edge metadata.
EricWF marked an inline comment as done.

(btw, do those docs updates land on some web URL automagically when committed?).

Assuming the builder isn't broken then yes, the docs should automagically update within ~3 minutes.

I think we should point out on the docs that _LIBCPP_HAS_THREAD_LIBRARY_EXTERNAL is only meant for libc++ developers / testers. The purpose of this library variant is to:

  • Demonstrate that libc++ can be built with threading support externalised. We use pthreads as the underlying implementation in this case because that's the one we can easily test on any platform. I don't think anyone wants to distribute this library variant to users; they would rather compile libc++ for pthread directly instead.
  • Write tests that applies to the LIBCXX_HAS_EXTERNAL_THREAD_API variant. For example, I hope to write a test to check that the produced libc++ dylib has no references to pthread_ symbols within it. Such a test will ensure that the core sources of libc++ remains pthread-free (all threading should be wired through the threading API).

If a library vendors really wants to distribute a variant of libc++ with the threading support externalised, they should use a custom __external_threading header to replicate the behaviour of _LIBCPP_HAS_THREAD_LIBRARY_EXTERNAL.

Hope that makes sense?

All of that makes sense. I think you could write those docs better than I could. Would you be willing to update them?

Sure, please go ahead with the commit. I'll have to do some downstream adjustments first, then I will update the docs.

Thanks for sorting this out!

include/__threading_support
30

Makes sense.

35

Indeed, missed that.

rmaprath accepted this revision.Jan 5 2017, 4:22 AM
rmaprath edited edge metadata.
EricWF updated this revision to Diff 83399.Jan 6 2017, 12:15 PM
EricWF edited edge metadata.
  • Merge with master
EricWF closed this revision.Jan 6 2017, 12:16 PM