[libcxx-commits] [PATCH] D88599: [SystemZ][ZOS] Porting pthread_t related functionality within libc++ to z/OS

[Zbigniew Sarbinowski via Phabricator via libcxx-commits]

zibi added inline comments.


================
Comment at: libcxx/include/__threading_support:495
 {
-  return pthread_equal(t1, t2) != 0;
+  return t1 == t2;
 }
----------------
zibi wrote:
> mclow.lists wrote:
> > zibi wrote:
> > > zibi wrote:
> > > > uweigand wrote:
> > > > > zibi wrote:
> > > > > > mclow.lists wrote:
> > > > > > > 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 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.
> > > > > 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).
> > > > > 
> > > > > 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.
> > > 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.
> > > 
> > 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.
> 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.
> 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).
> 




================
Comment at: libcxx/include/__threading_support:495
 {
-  return pthread_equal(t1, t2) != 0;
+  return t1 == t2;
 }
----------------
zibi wrote:
> zibi wrote:
> > mclow.lists wrote:
> > > zibi wrote:
> > > > zibi wrote:
> > > > > uweigand wrote:
> > > > > > zibi wrote:
> > > > > > > mclow.lists wrote:
> > > > > > > > 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 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.
> > > > > > 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).
> > > > > > 
> > > > > > 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.
> > > > 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.
> > > > 
> > > 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.
> > 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.
> > 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).
> > 
> 
> 
> 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.



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88599/new/

https://reviews.llvm.org/D88599