This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v8 1/8] nptl: Add C11 threads thrd_* functions
- From: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>
- To: Florian Weimer <fweimer at redhat dot com>, libc-alpha at sourceware dot org
- Date: Thu, 12 Jul 2018 14:52:32 -0300
- Subject: Re: [PATCH v8 1/8] nptl: Add C11 threads thrd_* functions
- References: <1517591084-11347-1-git-send-email-adhemerval.zanella@linaro.org> <1517591084-11347-2-git-send-email-adhemerval.zanella@linaro.org> <c6e9640b-182a-71ac-2417-838fdeeb2465@redhat.com>
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.