This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Two more threading dependencies
AbandonedPublic

Authored by rmaprath on Jun 1 2016, 11:49 AM.

Details

Summary

Bumped into these while working on the externally threaded variant. I think it makes most sense to group these two under the main threading API. This greatly simplifies the presentation of the externally threaded library variant as well.

Diff Detail

Event Timeline

rmaprath updated this revision to Diff 59262.Jun 1 2016, 11:49 AM
rmaprath retitled this revision from to [libcxx] Two more threading dependencies.
rmaprath updated this object.
rmaprath added reviewers: EricWF, bcraig, mclow.lists.
rmaprath added a subscriber: cfe-commits.
bcraig added inline comments.Jun 1 2016, 11:56 AM
include/__threading_support
187

Why does this need to go in threading_support? Seems like this is mostly orthogonal to threading. libcxxabi seems like the better place to hold changes to terminate anyway.

rmaprath added inline comments.Jun 1 2016, 12:05 PM
include/__threading_support
187

I can clearly see the argument against it, was bit unsure of it myself.

Now, the reason I want this the most is because of the externally threaded API, where I need to do some cleanup of the storage allocated in __libcpp_thread_create(). See the change I had to do in D20328 (thread.cpp) where I introduced a __libcpp_thread_finalize() function just for the cleanup. Thought it would be much cleaner to bundle up the two together so that I can avoid an explicit #ifdef in the sources.

Is that enough of a justification? Or should I stick to the explicit #ifdef in the externally-threaded variant? I don't have a strong opinion here, either way is fine, this version is slightly more preferred.

bcraig added inline comments.Jun 2 2016, 2:22 PM
include/__threading_support
187

I guess this is fine. I just need to tell myself (and maybe it should be commented in the code?) that this isn't trying to replace std::terminate, it's trying to replace a chunk of the std::thread dtor. Having a custom hook for the std::thread dtor is reasonable, and this is a reasonable default implementation.

Note that you only get a chance to do things here when a user does naughty things and lets a joinable thread reach the std::thread dtor.

rmaprath updated this revision to Diff 59507.Jun 3 2016, 2:09 AM

Added a comment about ~std::thread() hook.

@EricWF: Gentle ping.

rmaprath abandoned this revision.Dec 30 2016, 3:41 AM

Most of this is no longer needed. I'll submit patches for the parts that are actually needed (e.g. nanosleep) later on. Thanks.