Page MenuHomePhabricator

[libcxx] Fix testing of the externally-threaded library build after r290850
ClosedPublic

Authored by rmaprath on Jan 3 2017, 3:32 AM.

Details

Reviewers
compnerd
EricWF
Summary

Before r290850, building libcxx with -DLIBCXX_HAS_EXTERNAL_THREAD_API=ON had two uses:

  • Allow platform vendors to plug-in an __external_threading header which should take care of the entire threading infrastructure of libcxx
  • Allow testing of an externally-threaded library build; where the thread API is declared using pthread data structures, and the implementation of this API is provided as a separate library (test/support/external_threads.cpp) and linked-in when running the test suite.

r290850 breaks the second use case (pthread data structures are no longer available). This patch re-stores the ability to build+test an externally-threaded library variant on a pthread based system.

This wasn't caught on any of the official bots because we don't have any builds of the externally-threaded library build at the moment (apart from my downstream one).

Diff Detail

Event Timeline

rmaprath updated this revision to Diff 82858.Jan 3 2017, 3:32 AM
rmaprath retitled this revision from to [libcxx] Fix testing of the externally-threaded library build after r290850.
rmaprath updated this object.
rmaprath added reviewers: compnerd, EricWF.
rmaprath added a subscriber: cfe-commits.

I will commit this now to get the testing working again. Thought of putting up the patch for review in case if there is a better way to do this.

@compnerd: I wondered, for windows threading support, why not do something similar to __external_threading (and how it is handled in __threading_support) and offload the entire implementation to a separate windows-only header?

compnerd edited edge metadata.Jan 3 2017, 11:24 AM

I dont think that making Win32 threading an external one makes much sense unless we also do the same for pthreads. Its just as valid a threading model as pthreads, so why give preferential treatment to that? (Sure, there are more buildbots using it, but thats not a strong technical justification).

include/__threading_support
30–50

Can you break this down into this which IMO is far more readable:

#if defined(_LIBCPP_HAS_THREAD_API_EXTERNAL)
#if __libcpp_has_include(<external_threading>)
#include <__external_threading>
#else
// Why pthread?
#define _LIBCPP_HAS_THREAD_API_EXTERNAL_PTHREAD
#endif
#endif

#if defined(_LIBCPP_HAS_THREAD_API_PTHREAD) ||
    defined(_LIBCPP_HAS_THREAD_API_EXTERNAL_PTHREAD)
#include <pthread.h>
#include <sched.h>
#endif

I dont think that making Win32 threading an external one makes much sense unless we also do the same for pthreads. Its just as valid a threading model as pthreads, so why give preferential treatment to that? (Sure, there are more buildbots using it, but thats not a strong technical justification).

If the Win32 threading is support is going to be upstreamed, yes, it makes sense to give it the same treatment as pthreads (keep it inside __threading_support). libcxx has been tied to pthreads for a while, would be good to see something else supported as well. This will also allow us to iron out pthread-isms from the sources into the threading API. I have a couple of patches in that area (which I discovered while implementing the libcxx threading API with a different thread implementation), will put them up for review soon.

include/__threading_support
30–50

The reason I kept it in the current form is, when __external_threading is present, everything else in this header needs to be silenced. This is enforced by the current form because everything else in this header lives inside the #else side of the __external_threading inclusion.

Sure, the header will be a bit more readable if we arrange it the way you suggested, but then it's easy to add something to the header in a way that does not get removed when __external_threading is present.

EricWF accepted this revision.Jan 4 2017, 2:56 PM
EricWF edited edge metadata.

Accepting and Closing. This was committed in r290878.

This revision is now accepted and ready to land.Jan 4 2017, 2:56 PM
EricWF closed this revision.Jan 4 2017, 2:56 PM