Page MenuHomePhabricator

[SystemZ][ZOS] Porting pthread_t related functionality within libc++ to z/OS
Needs ReviewPublic

Authored by zibi on Sep 30 2020, 10:51 AM.

Details

Reviewers
uweigand
hubert.reinterpretcast
ldionne
SeanP
xingxue
mclow.lists
Group Reviewers
Restricted Project
Summary

This patch will address the differences of definition of pthread_t on z/OS vs. Linux.
Main trick to make the code work on z/OS relies on redefining libcpp_thread_id type and
_LIBCPP_NULL_THREAD macro. This is necessary to separate initialization of
libcxx_thread_id from the one of __libcxx_thread_t;

Diff Detail

Event Timeline

zibi created this revision.Sep 30 2020, 10:51 AM
Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 30 2020, 10:51 AM
zibi requested review of this revision.Sep 30 2020, 10:51 AM
mclow.lists requested changes to this revision.Sep 30 2020, 11:04 AM
mclow.lists added a subscriber: mclow.lists.
mclow.lists added inline comments.
libcxx/include/__threading_support
495

This is what this code used to be. It's wrong, because pthread_t is not a type that we control, and there's no guarantee that it has an operator==. That's why there's a call pthread_equal

This revision now requires changes to proceed.Sep 30 2020, 11:04 AM
zibi added inline comments.Sep 30 2020, 12:31 PM
libcxx/include/__threading_support
495

This is what this code used to be. It's wrong, because pthread_t is not a type that we control, and there's no guarantee that it has an operator==. That's why there's a call pthread_equal

Thx Marshall, the problem with the original call to 'thread_equal is that it was passing wrong parameters by mixing thread_id with thread_t. We need clear separation between them and don't use them interchangeably.

uweigand added inline comments.Oct 1 2020, 2:28 AM
libcxx/include/__threading_support
495

Note that even in the current code, __libcpp_thread_id is implicitly assumed to be a numeric type, see in particular the routine __libcpp_thread_is_less immediately below.

My suggestion has been to use __libcpp_thread_t for the platform-specific opaque thread handle (e.g. pthread_t), and to use __libcpp_thread_id for a numeric ID. On platforms where pthread_t happens to be numeric itself, the types can be the same; other platforms need to provide a different definition for __libcpp_thread_id (as done by this patch for z/OS).

zibi added inline comments.Oct 1 2020, 6:39 AM
libcxx/include/__threading_support
495

Note that even in the current code, __libcpp_thread_id is implicitly assumed to be a numeric type, see in particular the routine __libcpp_thread_is_less immediately below.

Yes Ulrich, I noticed that as well.

If we want to use pthread_equal() we will have to operate on __libcpp_thread_t in both __libcpp_thread_id_less and __libcpp_thread_id_equal like we do already do in `__libcpp_thread_isnull'.
However, that will get us to a ripple effect of changing template __thread_id. Furthermore __thread_id is being used in hash<>.

Marshall, let me know if you want me to pursue this road.

libcxx/include/__threading_support
93

The macro needs extra parentheses so that sizeof _LIBCPP_NULL_THREAD is not parsed as sizeof(<function type>).

zibi updated this revision to Diff 295562.Oct 1 2020, 7:18 AM

Putting extra paren. for macro to address Hubert comment.

zibi marked an inline comment as done.Oct 1 2020, 12:00 PM
zibi added inline comments.
libcxx/include/__threading_support
495

One of the usage of __thread_id is 'recursive_timed_mutex which does retrieves the id of the thread before calling checking if it's the same as the current thread. Here is the snippet of this:

__thread_id __id = this_thread::get_id();
unique_lock<mutex> lk(__m_);
if (__id == __id_)

Therefore we are assuming that thread_id is integral type and using == is perfectly fine.
Alternatively, we could change the code to use pthread_equal(), but that would require the argument type to change and other ripple effects.

zibi added a comment.Oct 6 2020, 7:19 PM

Marshall, I really want to hear your opinion about the need to call pthread_equal().
See my last comment. The 'operator== of __thread_id is calling __libcpp_thread_id_equal passing two arguments of type __libcpp_thread_id which have integral type. Therefore, as it is now it's perfectly fine to do operator equal as apposed to call pthread_equal().

In a meantime I experimented with '__libcpp_thread_t so we can actually call pthread_equal(). I introduced __thread_t' class in place of __thread_id and all the ripple effect code associated with it. Let me know if you want to proceed this route and/or see my draft first.

zibi updated this revision to Diff 296779.Oct 7 2020, 1:20 PM

@mclow.lists Please see my proposal draft so we can call pthread_equal().

mclow.lists added inline comments.Oct 16 2020, 12:17 PM
libcxx/include/__threading_support
495

Now you're mixing types. __thread_id is a different type than __libcpp_thread_t

__thread_id has a full complement of comparison operators, and hash support as well.

mclow.lists added inline comments.Oct 16 2020, 12:31 PM
libcxx/include/mutex
283

Can't make this change. This is an ABI break if sizeof(__thread_id) != sizeof(__thread_t)

zibi added a comment.Oct 16 2020, 3:43 PM

Thank you, Marshall.

So far we have only 2 concrete solutions. Does anybody see another alternative?

  1. use built-in comparision operator instead of 'thread_equal()
  2. change '__thread_id to '`__thread_t'
libcxx/include/__threading_support
495

Mixing? Sorry, I don't see it, the call is at line 765 were we pass __libcpp_thread_t type member '__t_ as follows:

return __libcpp_thread_equal(__x.__t_, __y.__t_);

We can beef up the comparison operators for __thread_t that is not a problem. I included the minimum for the purpose of validating/approving this approach.

libcxx/include/mutex
283

Yes, you are right and I noticed that as well but do we know for fact that there is a platform where sizeof(thread_id) != sizeof(thread_t) ? I compiled the following assertions with not issues on z/OS and RHEL 7.4.

static_assert(sizeof(__th.__get_id()) == sizeof(__th), "zibi trace __thread_id != __thread_t");

As a matter of fact __thread_id is the same type as __thread_t until we discovered on z/OS that this can not be the case. If this is really no go I'm open for suggestions.

mclow.lists added inline comments.Oct 16 2020, 3:55 PM
libcxx/include/mutex
283

There are three different implementations in this file.

  • One for pthreads
  • One for C11 threads
  • One for external threading API

On two of them, __thread_id is the same type as __thread_t.
On two of them, the type __thread_t is defined outside of libc++ (to be precise, __thread_t is a typedef for a type supplied by the OS). Neither of those cases require that type to be an integral type.

We have to keep all of these working.

zibi added inline comments.Oct 19 2020, 2:22 PM
libcxx/include/__threading_support
495

Note that even in the current code, __libcpp_thread_id is implicitly assumed to be a numeric type, see in particular the routine __libcpp_thread_is_less immediately below.

My suggestion has been to use __libcpp_thread_t for the platform-specific opaque thread handle (e.g. pthread_t), and to use __libcpp_thread_id for a numeric ID. On platforms where pthread_t happens to be numeric itself, the types can be the same; other platforms need to provide a different definition for __libcpp_thread_id (as done by this patch for z/OS).

495

Note that even in the current code, __libcpp_thread_id is implicitly assumed to be a numeric type, see in particular the routine __libcpp_thread_is_less immediately below.

Marshall (@mclow.lists) going back to the beginning can we work to address Ulrich's comment first? Ideally that should be addressed independently from z/OS changes.

The 'pthread_equal() was called as intended with ptread_t prior to D19412 which introduced __libcpp_thread_id and used that type on the call to 'pthread_equal. Unfortunately this change made the assumption that __libcpp_pthread_t is same as __libcpp_pthread_id which is not the case on z/OS and prompted this discussion and the need for another patch.

In regards to API break pointed by @mclow.lists __thread_id had only one member of type pthread_t which only after D19412 became __libcpp_pthread_id. Same is true for __id_ member of recursive_timed_mutex. To me the API was broken at that time and my proposal will revert that. However, I'm not sure if that is the route we want to take.

Up to now only _LIBCPP_HAS_THREAD_API_EXTERNAL implementation had different underlying types as follows:

typedef long __libcpp_thread_id;
typedef void* __libcpp_thread_t;

This implementation did not run into the issue we discussing here since it didn't need and did not provide __libcpp_thread_id_equal.
Let me ping the original authors @rmaprath, @espositofulvio for input how to proceed.

zibi added a comment.Thu, Nov 5, 6:27 AM

ping, any feedback?

zibi updated this revision to Diff 305012.Thu, Nov 12, 7:18 PM
zibi marked 5 inline comments as done.

Resubmit the patch with small delta to make use of member function thread_t::get_id() and validate CI.