diff mbox

2.26 release blockers?

Message ID fcd9fac8-f689-4f38-a53c-c2d09dd5230f@linaro.org
State Dropped
Headers

Commit Message

Adhemerval Zanella Netto July 5, 2017, 1:48 p.m. UTC 
  On 05/07/2017 07:34, Joseph Myers wrote:
> On Wed, 5 Jul 2017, Siddhesh Poyarekar wrote:
> 
>>> there's also the possibility of interactions with the thread types 
>>> headers, if there's anything in the architecture-specific headers that 
>>> works for pthreads but not for C11 threads.
>>
>> From a quick look at the changes, I couldn't find any changes in the
>> arch-specific sysdeps directories other than the abilist changes, i.e.
>> there are no architecture-specific headers.  Bugs in C11 should thus be
> 
> The thread types headers refactoring, to make them usable for both C11 
> threads and pthreads, went in some time ago.
> 
> All that architecture-specific header content will get used in C11 threads 
> and it's far from obvious that any issues that appear with C11 threads 
> would also appear with pthreads.

It was not clear to me if you still consider C11 threads patches a
disruptive for arch-testing that can't not get validated in current
cross-compiling testing.  

I think the only snippet that differs significantly from pthread is 
related to thread creation [1] where since different signatures between 
POSIX and C11 I had to add an explicit cast and recast:

iff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
index 7a970ff..3efb76d 100644

Besides that, all other implementation uses pthread code directly and
with a validation of pthread primitives it should not show any
issues as you pointed out.


[1] https://sourceware.org/ml/libc-alpha/2017-06/msg01418.html

Comments

Joseph Myers July 5, 2017, 1:51 p.m. UTC | #1 
On Wed, 5 Jul 2017, Adhemerval Zanella wrote:

> > The thread types headers refactoring, to make them usable for both C11 
> > threads and pthreads, went in some time ago.
> > 
> > All that architecture-specific header content will get used in C11 threads 
> > and it's far from obvious that any issues that appear with C11 threads 
> > would also appear with pthreads.
> 
> It was not clear to me if you still consider C11 threads patches a
> disruptive for arch-testing that can't not get validated in current
> cross-compiling testing.  

I think the ABI baselines are adequately validated by cross-compilation 
testing, but there's a risk of architecture-specific problems that only 
show up in C11 threads execution testing being missed.
Adhemerval Zanella Netto July 5, 2017, 6:19 p.m. UTC | #2 
On 05/07/2017 10:51, Joseph Myers wrote:
> On Wed, 5 Jul 2017, Adhemerval Zanella wrote:
> 
>>> The thread types headers refactoring, to make them usable for both C11 
>>> threads and pthreads, went in some time ago.
>>>
>>> All that architecture-specific header content will get used in C11 threads 
>>> and it's far from obvious that any issues that appear with C11 threads 
>>> would also appear with pthreads.
>>
>> It was not clear to me if you still consider C11 threads patches a
>> disruptive for arch-testing that can't not get validated in current
>> cross-compiling testing.  
> 
> I think the ABI baselines are adequately validated by cross-compilation 
> testing, but there's a risk of architecture-specific problems that only 
> show up in C11 threads execution testing being missed.
> 
I do see your rationale, but we are already pushing changes without much
testing in master that are much more disruptive IMHO (maybe not for
architecture cases, but on performance for instance) such as single thread
FILE optimization and thread local cache.  

But I think we can postpone C11 threads for 2.27, although for the specific
architecture where you already posted some testing results (arm hard-float,
powerpc soft-float, and mips), I think it would be feasible to run the C11
threads tests on emulated qemu system to validate them.
Carlos O'Donell July 5, 2017, 8:16 p.m. UTC | #3 
On 07/05/2017 02:19 PM, Adhemerval Zanella wrote:
> 
> 
> On 05/07/2017 10:51, Joseph Myers wrote:
>> On Wed, 5 Jul 2017, Adhemerval Zanella wrote:
>>
>>>> The thread types headers refactoring, to make them usable for both C11 
>>>> threads and pthreads, went in some time ago.
>>>>
>>>> All that architecture-specific header content will get used in C11 threads 
>>>> and it's far from obvious that any issues that appear with C11 threads 
>>>> would also appear with pthreads.
>>>
>>> It was not clear to me if you still consider C11 threads patches a
>>> disruptive for arch-testing that can't not get validated in current
>>> cross-compiling testing.  
>>
>> I think the ABI baselines are adequately validated by cross-compilation 
>> testing, but there's a risk of architecture-specific problems that only 
>> show up in C11 threads execution testing being missed.
>>
> I do see your rationale, but we are already pushing changes without much
> testing in master that are much more disruptive IMHO (maybe not for
> architecture cases, but on performance for instance) such as single thread
> FILE optimization and thread local cache.  

Each issue needs to be discussed independently.

Each issue has their own risks and rewards.

> But I think we can postpone C11 threads for 2.27, although for the specific
> architecture where you already posted some testing results (arm hard-float,
> powerpc soft-float, and mips), I think it would be feasible to run the C11
> threads tests on emulated qemu system to validate them.
 
Can we commit to checking in C11 as soon as 2.27 opens?

What else do we need to do to get there?
Adhemerval Zanella Netto July 5, 2017, 9:09 p.m. UTC | #4 
On 05/07/2017 17:16, Carlos O'Donell wrote:
> On 07/05/2017 02:19 PM, Adhemerval Zanella wrote:
>>
>>
>> On 05/07/2017 10:51, Joseph Myers wrote:
>>> On Wed, 5 Jul 2017, Adhemerval Zanella wrote:
>>>
>>>>> The thread types headers refactoring, to make them usable for both C11 
>>>>> threads and pthreads, went in some time ago.
>>>>>
>>>>> All that architecture-specific header content will get used in C11 threads 
>>>>> and it's far from obvious that any issues that appear with C11 threads 
>>>>> would also appear with pthreads.
>>>>
>>>> It was not clear to me if you still consider C11 threads patches a
>>>> disruptive for arch-testing that can't not get validated in current
>>>> cross-compiling testing.  
>>>
>>> I think the ABI baselines are adequately validated by cross-compilation 
>>> testing, but there's a risk of architecture-specific problems that only 
>>> show up in C11 threads execution testing being missed.
>>>
>> I do see your rationale, but we are already pushing changes without much
>> testing in master that are much more disruptive IMHO (maybe not for
>> architecture cases, but on performance for instance) such as single thread
>> FILE optimization and thread local cache.  
> 
> Each issue needs to be discussed independently.
> 
> Each issue has their own risks and rewards.

I understand that and I give you that both examples I gave had a more 
thoughtfully review before inclusion. And I also understand C11 threads
seems to not be an urgent upcoming new feature for 2.26 (given relative
lack of interest and review).

> 
>> But I think we can postpone C11 threads for 2.27, although for the specific
>> architecture where you already posted some testing results (arm hard-float,
>> powerpc soft-float, and mips), I think it would be feasible to run the C11
>> threads tests on emulated qemu system to validate them.
>  
> Can we commit to checking in C11 as soon as 2.27 opens?
> 
> What else do we need to do to get there?

Mostly some reviews of the patch themselves. Mostly are straightforward wrapper
and code examples, maybe expect the thrd_* one that changes some internal
pthread code for c11 call format.  The documentation got some review, but I 
am not sure if it would be better to glue it to threads.texi or let in its
own file.
diff mbox

Patch

--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -461,7 +461,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);
+	}
+      else
+	ret = pd->start_routine (pd->arg);
+      THREAD_SETMEM (pd, result, ret);
     }
 
   /* Call destructors for the thread_local TLS variables.  */