Page MenuHomePhabricator

[libcxx] Do not declare the thread api when __external_threading is present
ClosedPublic

Authored by rmaprath on Oct 11 2016, 5:26 AM.

Details

Summary

This fixes a small omission where even when __external_threading is provided, we attempt to declare a pthread based threading API. Instead, we should leave out everything for the __external_threading header to take care of.

The __threading_support header provides a proof-of-concept externally threaded libc++ variant when _LIBCPP_HAS_THREAD_API_EXTERNAL is defined. But if the __external_threading header is present, we should exclude all of that POC stuff.

Diff Detail

Repository
rL LLVM

Event Timeline

rmaprath updated this revision to Diff 74234.Oct 11 2016, 5:26 AM
rmaprath retitled this revision from to [libcxx] Do not declare the thread api when __external_threading is present.
rmaprath updated this object.
rmaprath added a reviewer: EricWF.
rmaprath added a subscriber: cfe-commits.
EricWF added inline comments.Oct 11 2016, 2:35 PM
include/__threading_support
25 ↗(On Diff #74234)

If _LIBCPP_HAS_THREAD_API_EXTERNAL is defined can't we just assume that `<__external_threading> is present?

35 ↗(On Diff #74234)

Instead of using a new macro couldn't this just be _LIBCPP_HAS_THREAD_API_EXTERNAL?

Added some comments to @EricWF's feedback. Will check back tomorrow (falling asleep...)

/ Asiri

include/__threading_support
25 ↗(On Diff #74234)

So, _LIBCPP_HAS_THREAD_API_EXTERNAL is set from the cmake option for building the externally threaded variant. For the proof-of-concept implementation (that we use for running the test suite), we embed a pthread-based external API within __threading_support itself and provide its implementation in test/support/external_threads.cpp (which is built into a separate library when running the test suite).

That means, in the default externally-threaded variant (POC), we don't have a __external_threading header; the idea is to allow library vendors to drop-in a __external_threading header without conflicting with upstream sources, and it will be picked up automatically by __threading_support header. Moreover, this way we avoid the cost of shipping an additional header which is not necessary for the most common use case (direct pthread dependency).

This is why we need this contraption to detect if the externally threaded library is built with a __external_threading header (i.e. for production) or not (the default - vanilla upstream setup, the POC).

Hope that makes sense? Perhaps I should explain all this in a comment as well?

35 ↗(On Diff #74234)

I think my comment above covers this point as well.

EricWF accepted this revision.Oct 13 2016, 12:21 PM
EricWF edited edge metadata.
This revision is now accepted and ready to land.Oct 13 2016, 12:21 PM
This revision was automatically updated to reflect the committed changes.