Page MenuHomePhabricator

Fix a layering violation in mutex - prep for fixing PR42918
AbandonedPublic

Authored by mclow.lists on Aug 7 2019, 12:13 PM.

Details

Summary

While investigating https://llvm.org/PR42918, I noticed that the recursive_timed_mutex used the primitive thread-ids directly, rather than the convenience class __thread_id.

This fixes that, and makes it so we can fix the above issue only in __thread_id, rather than enshrining the "not a thread" knowledge in each of the OS-specific bits.

I'm a bit worried about ABI here, even though the field that I'm replacing is the same size.

Diff Detail

Event Timeline

mclow.lists created this revision.Aug 7 2019, 12:13 PM
mclow.lists marked an inline comment as done.Aug 7 2019, 12:14 PM
mclow.lists added inline comments.
libcxx/include/mutex
199

If I were to commit this; I would probably move __thread_id and this_thread:: into <__threading_support> and remove this include.

Re-gen diff with more context. No other changes.

The general problem that I want to solve here is:
C++11's threading model requires a "this is no thread" thread-id. pthreads doesn't really have that.
We fake it by using the ID 0 for that. (this is complicated by the fact that the type of a pthread_id changes from system to system; fortunately, it appears always to be a scalar - int or pointer).

Bug 42918 shows a case where we pass this "not a thread" thread_id to pthreads, and in some weird cases, pthread_equal doesn't deal well with this.

We have an abstraction, __thread_id, which could manage this for us. But not everyone uses that abstraction; for example recursive_timed_mutex. This patch reworks recursive_timed_mutex so that it uses __thread_id instead of the lower-level __libcpp_thread_id. That's all. __thread_id has a single field, which is a __libcpp_thread_id, and calls __libcpp_thread_id_equal to compare those ids, so there should be no behavior change from this patch.

However, once we fix __thread_id::operator== to handle the "not a thread" thread_id case, then recursive_timed_mutex will get that corrected behavior as well.

mclow.lists marked an inline comment as done.Aug 8 2019, 8:58 AM
mclow.lists added inline comments.
libcxx/src/mutex.cpp
184

This change eliminates the "zero is special" knowledge from here, instead keeping it in __thread_id

ldionne requested changes to this revision.Aug 8 2019, 11:38 AM

Apart from my comment, this LGTM.

libcxx/include/thread
242

I would rather call this __reset() to make it clear it's not part of the public API.

This revision now requires changes to proceed.Aug 8 2019, 11:38 AM

Committed as revision 368867. I made Louis' suggested change (reset --> __reset) but somehow that didn't make it into the commit. I will fix that.

DavidSpickett added a subscriber: DavidSpickett.EditedAug 15 2019, 6:11 AM

The move of thread_id to <threading_support> has caused a build error for us when _LIBCPP_HAS_THREAD_API_EXTERNAL is true

As far as I can tell __thread_id used to only require that _LIBCPP_HAS_NO_THREADS was false, but now it checks that both defines are false. I've been able to fix it locally but in a rather ugly way initially. I thought I'd let you know in case you had an idea of how it should be fixed. (or maybe I'm missing something)

I've put up my fix for review and added you as a reviewer:
https://reviews.llvm.org/D66301

The move of thread_id to <threading_support> has caused a build error for us when _LIBCPP_HAS_THREAD_API_EXTERNAL is true

Well, crud. Sorry for the breakage.
I'll take a look at your patch.

mclow.lists abandoned this revision.Sep 12 2019, 7:04 AM

This has been landed, and the breakage has been fixed.