Re: [PATCH v8 1/8] nptl: Add C11 threads thrd_* functions

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 12/07/2018 13:46, Florian Weimer wrote:
> On 02/02/2018 06:04 PM, Adhemerval Zanella wrote:
> 
>> diff --git a/include/stdc-predef.h b/include/stdc-predef.h
>> index c569759..c2ab78a 100644
>> --- a/include/stdc-predef.h
>> +++ b/include/stdc-predef.h
>> @@ -57,7 +57,4 @@
>>      - 3 additional Zanabazar Square characters */
>>   #define __STDC_ISO_10646__        201706L
>>   -/* We do not support C11 <threads.h>.  */
>> -#define __STDC_NO_THREADS__        1
> 
> Should we do this only if we know that the compiler has _Thread_local support (based on a GCC and __cplusplus version check)?

It seems reasonable, since its a installed header.  Do we need to 
check for __cplusplus too? Shouldn't __GNUC_PREREQ (4.9) be suffice?

> 
>> diff --git a/nptl/descr.h b/nptl/descr.h
>> index 64ba29e..f00e2c0 100644
>> --- a/nptl/descr.h
>> +++ b/nptl/descr.h
>> @@ -371,6 +371,8 @@ struct pthread
>>        to the function.  */
>>     void *(*start_routine) (void *);
>>     void *arg;
>> +  /* Indicates whether is a C11 thread created by thrd_creat.  */
>> +  bool c11;
>>       /* Debug state.  */
>>     td_eventbuf_t eventbuf;
> 
> Can you move the new member towards the end of the struct?  I'm worried about the ABI implications.

Right, I will do it.

> 
>> diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
>> index caaf07c..74e773a 100644
>> --- a/nptl/pthread_create.c
>> +++ b/nptl/pthread_create.c
>> @@ -460,7 +460,19 @@ START_THREAD_DEFN
>>         LIBC_PROBE (pthread_start, 3, (pthread_t) pd, pd->start_routine, pd->arg);
>>           /* Run the code the user provided.  */
>> -      THREAD_SETMEM (pd, result, pd->start_routine (pd->arg));
>> +      void *ret;
>> +      if (pd->c11)
>> +    {
>> +      /* The function pointer of the c11 thread start is cast to an incorrect
>> +         type on __pthread_create_2_1 call, however it is casted back to correct
>> +         one so the call behavior is well-defined (it is assumed that pointers
>> +         to void are able to represent all values of int.  */
>> +      int (*start)(void*) = (int (*) (void*)) pd->start_routine;
>> +      ret = (void*) (intptr_t) start (pd->arg);
> 
> (I think this required on m68k, where void * and int are returned in different registers.)
> 
>> +int
>> +thrd_join (thrd_t thr, int *res)
>> +{
>> +  void *pthread_res;
>> +  int err_code = __pthread_join (thr, &pthread_res);
>> +  if (res)
>> +   *res = (int)((uintptr_t) pthread_res);
>> +
>> +  return thrd_err_map (err_code);
>> +}
> 
> Slight inconsistency with intptr_t above.

Indeed, it seems there is no need to cast.

> 
>> diff --git a/sysdeps/nptl/threads.h b/sysdeps/nptl/threads.h
>> new file mode 100644
>> index 0000000..6adcac4
>> --- /dev/null
>> +++ b/sysdeps/nptl/threads.h
> 
> Should this be nptl/threads.h, not sysdeps/nptl/threads.h?

Right, I will move it.