Fix C11 conformance issues

  Remove conformace assumption of NPTL implementation for ISO C threads
and revert wrong libcrypt addition on linknamespace-libs-XPG4.

The i686-gnu target now shows two new conformance failures:

FAIL: conform/ISO11/threads.h/conform
FAIL: conform/ISO11/threads.h/linknamespace

It is expected due missing HTL ISO C threads support and both conformance
.out files indicates the reason ("#error "HTL does not implement ISO C
threads").

Checked on i686-linux-gnu and i686-gnu.

	* include/threads.h: Move to ...
	* sysdeps/nptl/threads.h: ... here.
	* sysdeps/htl/threads.h: New file.
	* conform/Makefile (linknamespace-libs-ISO11): Use
	static-thread-library instead of linking libpthread.
	(linknamespace-libs-XPG4): Revert wrong libcrypt.a addition.
---
 ChangeLog                           | 9 +++++++++
 conform/Makefile                    | 5 ++---
 sysdeps/htl/threads.h               | 1 +
 {include => sysdeps/nptl}/threads.h | 0
 4 files changed, 12 insertions(+), 3 deletions(-)
 create mode 100644 sysdeps/htl/threads.h
 rename {include => sysdeps/nptl}/threads.h (100%)
  

Adhemerval Zanella, le mer. 25 juil. 2018 10:43:25 -0300, a ecrit:
> It is expected due missing HTL ISO C threads support and both conformance
> .out files indicates the reason ("#error "HTL does not implement ISO C
> threads").

I thought the ISO C threads were just a layer on top of the POSIX
threads, can't it be used on top of HTL too?

I'm not saying we should aim for this in 2.28, just wondering.

Samuel
  

On 07/25/2018 09:43 AM, Adhemerval Zanella wrote:
> Remove conformace assumption of NPTL implementation for ISO C threads
> and revert wrong libcrypt addition on linknamespace-libs-XPG4.
> 
> The i686-gnu target now shows two new conformance failures:
> 
> FAIL: conform/ISO11/threads.h/conform
> FAIL: conform/ISO11/threads.h/linknamespace
> 
> It is expected due missing HTL ISO C threads support and both conformance
> .out files indicates the reason ("#error "HTL does not implement ISO C
> threads").
> 
> Checked on i686-linux-gnu and i686-gnu.
> 
> 	* include/threads.h: Move to ...
> 	* sysdeps/nptl/threads.h: ... here.
> 	* sysdeps/htl/threads.h: New file.
> 	* conform/Makefile (linknamespace-libs-ISO11): Use
> 	static-thread-library instead of linking libpthread.
> 	(linknamespace-libs-XPG4): Revert wrong libcrypt.a addition.

OK for 2.28.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> ---
>  ChangeLog                           | 9 +++++++++
>  conform/Makefile                    | 5 ++---
>  sysdeps/htl/threads.h               | 1 +
>  {include => sysdeps/nptl}/threads.h | 0
>  4 files changed, 12 insertions(+), 3 deletions(-)
>  create mode 100644 sysdeps/htl/threads.h
>  rename {include => sysdeps/nptl}/threads.h (100%)
> 
> diff --git a/conform/Makefile b/conform/Makefile
> index a0ab70e..d430931 100644
> --- a/conform/Makefile
> +++ b/conform/Makefile
> @@ -197,9 +197,8 @@ linknamespace-libs-xsi = $(linknamespace-libs-posix)
>  linknamespace-libs-ISO = $(linknamespace-libs-isoc)
>  linknamespace-libs-ISO99 = $(linknamespace-libs-isoc)
>  linknamespace-libs-ISO11 = $(linknamespace-libs-isoc) \
> -			   $(common-objpfx)nptl/libpthread.a
> -linknamespace-libs-XPG4 = $(linknamespace-libs-isoc) \
> -			  $(common-objpfx)crypt/libcrypt.a
> +			   $(static-thread-library)

OK, uses static-thread-library.

> +linknamespace-libs-XPG4 = $(linknamespace-libs-isoc)

OK, fixes libcrypt issue.

>  linknamespace-libs-XPG42 = $(linknamespace-libs-XPG4)
>  linknamespace-libs-POSIX = $(linknamespace-libs-thr)
>  linknamespace-libs-UNIX98 = $(linknamespace-libs-xsi)
> diff --git a/sysdeps/htl/threads.h b/sysdeps/htl/threads.h
> new file mode 100644
> index 0000000..3c04fbc
> --- /dev/null
> +++ b/sysdeps/htl/threads.h
> @@ -0,0 +1 @@
> +#error "HTL does not implement ISO C threads"

OK.

> diff --git a/include/threads.h b/sysdeps/nptl/threads.h
> similarity index 100%
> rename from include/threads.h
> rename to sysdeps/nptl/threads.h
>
  

On 25/07/2018 10:51, Samuel Thibault wrote:
> Adhemerval Zanella, le mer. 25 juil. 2018 10:43:25 -0300, a ecrit:
>> It is expected due missing HTL ISO C threads support and both conformance
>> .out files indicates the reason ("#error "HTL does not implement ISO C
>> threads").
> 
> I thought the ISO C threads were just a layer on top of the POSIX
> threads, can't it be used on top of HTL too?
> 
> I'm not saying we should aim for this in 2.28, just wondering.
> 
> Samuel
> 

Currently implementation is based on POSIX internally, although it
does not require to be (Florian has suggested we might use a more
streamline implementation in the future).

However you will need to correctly tie the HTL required types with
ISO C threads without adding namespace or linkspace pollution. For
NPTL we refactored the required information on pthreadtypes-arch.h,
so to make threads.h generic HTL will need to provide a similar 
implementation (it also defines any internal structure alignment
requirement, but afaiu HTL does not have any arch-specific constraint
so far).

I am not familiar with HTL internals, but ISO C required some NPTL
internal adjustments:

  - the function pointer of the ISO C thread thrd_start start
    function has a different signature of pthread_create
    (int (*thrd_start_t) (void*) for ISO C and
    void *(*start_routine) (void *) for POSIX).

  - thrd_sleep call the syscall directly to avoid to handle the 
    expected C11 states.  It would require to add new nanosleep
    wrapper with expected behaviour.

  - thrd_yield also call the syscall directly, but adding a
    wrapper would be easier since there is no need to handle
    the return code.

However I think it quite feasible for 2.29 to add ISO C threads 
Hurd/HTL support.
  

On Wed, 25 Jul 2018, Adhemerval Zanella wrote:

> Checked on i686-linux-gnu and i686-gnu.
> 
> 	* include/threads.h: Move to ...
> 	* sysdeps/nptl/threads.h: ... here.
> 	* sysdeps/htl/threads.h: New file.

To confirm: nptl/threads.h, not the wrapper you're moving, still gets 
correctly installed as threads.h after this patch, while it remains the 
case that no threads.h is installed for Hurd?

I think you need two separate versions of stdc-predef.h as well, so that 
Hurd gets a version that defines __STDC_NO_THREADS__.  Then once there is 
a C11 threads implementation for HTL, we can move back to just having a 
single version of stdc-predef.h.  (There should also be a single version 
of threads.h in that case, and we should aim to move towards a single 
pthread.h shared between NPTL and HTL as well.)
  

On Wed, 25 Jul 2018, Adhemerval Zanella wrote:

> I am not familiar with HTL internals, but ISO C required some NPTL
> internal adjustments:

- The C11 threads functions call __pthread_* internally, so all those 
__pthread_* names needed to exist and, where they called POSIX functions 
internally, they needed to be made to call __* names for those functions 
where that wasn't previously necessary for POSIX namespace reasons.
  

On Wed, 25 Jul 2018, Joseph Myers wrote:

> On Wed, 25 Jul 2018, Adhemerval Zanella wrote:
> 
> > Checked on i686-linux-gnu and i686-gnu.
> > 
> > 	* include/threads.h: Move to ...
> > 	* sysdeps/nptl/threads.h: ... here.
> > 	* sysdeps/htl/threads.h: New file.
> 
> To confirm: nptl/threads.h, not the wrapper you're moving, still gets 
> correctly installed as threads.h after this patch, while it remains the 
> case that no threads.h is installed for Hurd?

(The normal convention for a wrapper not in include would be for it to be 
in sysdeps/nptl/include/ rather than directly in sysdeps/nptl/.)
  

On Wed, 25 Jul 2018, Joseph Myers wrote:

> On Wed, 25 Jul 2018, Adhemerval Zanella wrote:
> 
> > Checked on i686-linux-gnu and i686-gnu.
> > 
> > 	* include/threads.h: Move to ...
> > 	* sysdeps/nptl/threads.h: ... here.
> > 	* sysdeps/htl/threads.h: New file.
> 
> To confirm: nptl/threads.h, not the wrapper you're moving, still gets 
> correctly installed as threads.h after this patch, while it remains the 
> case that no threads.h is installed for Hurd?

I tested a build as of commit 08ac6bed146c0546d63163ac0d42c9a35880412d.  
This patch has resulted in the wrong threads.h being installed, i.e. 
usr/include/threads.h is now the wrapper that just does #include 
<nptl/threads.h>, which obviously won't work for an installed header.

I think you need to put the real threads.h header in sysdeps/nptl/, not 
the wrapper (with a view to the Hurd work moving both the header, and the 
C11 threads implementation and tests, to sysdeps/pthread/ or some such 
shared location).
  

On 07/25/2018 03:43 PM, Adhemerval Zanella wrote:
>   linknamespace-libs-ISO11 = $(linknamespace-libs-isoc) \
> -			   $(common-objpfx)nptl/libpthread.a
> -linknamespace-libs-XPG4 = $(linknamespace-libs-isoc) \
> -			  $(common-objpfx)crypt/libcrypt.a
> +			   $(static-thread-library)

I've just realized this:

Linking with the static thread library obscures linknamespace issues in 
libc.a for symbols which are also defined in libpthread.a.  So this 
change is a bit questionable.

Thanks,
Florian
  

On Wed, 25 Jul 2018, Florian Weimer wrote:

> On 07/25/2018 03:43 PM, Adhemerval Zanella wrote:
> >   linknamespace-libs-ISO11 = $(linknamespace-libs-isoc) \
> > -			   $(common-objpfx)nptl/libpthread.a
> > -linknamespace-libs-XPG4 = $(linknamespace-libs-isoc) \
> > -			  $(common-objpfx)crypt/libcrypt.a
> > +			   $(static-thread-library)
> 
> I've just realized this:
> 
> Linking with the static thread library obscures linknamespace issues in libc.a
> for symbols which are also defined in libpthread.a.  So this change is a bit
> questionable.

The linknamespace code looks at every definition of a strong undefined 
symbol being resolved, not just the definition in a particular library.
  

On 07/25/2018 07:18 PM, Joseph Myers wrote:
> On Wed, 25 Jul 2018, Florian Weimer wrote:
> 
>> On 07/25/2018 03:43 PM, Adhemerval Zanella wrote:
>>>    linknamespace-libs-ISO11 = $(linknamespace-libs-isoc) \
>>> -			   $(common-objpfx)nptl/libpthread.a
>>> -linknamespace-libs-XPG4 = $(linknamespace-libs-isoc) \
>>> -			  $(common-objpfx)crypt/libcrypt.a
>>> +			   $(static-thread-library)
>>
>> I've just realized this:
>>
>> Linking with the static thread library obscures linknamespace issues in libc.a
>> for symbols which are also defined in libpthread.a.  So this change is a bit
>> questionable.
> 
> The linknamespace code looks at every definition of a strong undefined
> symbol being resolved, not just the definition in a particular library.

So there is not a problem after all?

What seems to be a problem is that libpthread.a must now be C11-clean 
for its C11 symbols (which is currently not the case on Hurd).  But 
fixing that is probably not a bad idea anyway.

Florian
  

On Wed, 25 Jul 2018, Florian Weimer wrote:

> On 07/25/2018 07:18 PM, Joseph Myers wrote:
> > On Wed, 25 Jul 2018, Florian Weimer wrote:
> > 
> > > On 07/25/2018 03:43 PM, Adhemerval Zanella wrote:
> > > >    linknamespace-libs-ISO11 = $(linknamespace-libs-isoc) \
> > > > -			   $(common-objpfx)nptl/libpthread.a
> > > > -linknamespace-libs-XPG4 = $(linknamespace-libs-isoc) \
> > > > -			  $(common-objpfx)crypt/libcrypt.a
> > > > +			   $(static-thread-library)
> > > 
> > > I've just realized this:
> > > 
> > > Linking with the static thread library obscures linknamespace issues in
> > > libc.a
> > > for symbols which are also defined in libpthread.a.  So this change is a
> > > bit
> > > questionable.
> > 
> > The linknamespace code looks at every definition of a strong undefined
> > symbol being resolved, not just the definition in a particular library.
> 
> So there is not a problem after all?

I don't think there's any problem with that part of the patch.

> What seems to be a problem is that libpthread.a must now be C11-clean for its
> C11 symbols (which is currently not the case on Hurd).  But fixing that is
> probably not a bad idea anyway.

Yes, if libpthread is used to provide some C11 symbols, it needs to be 
C11-clean for all the C11 symbols it provides (including those also 
present in libc).
  

On 25/07/2018 12:42, Joseph Myers wrote:
> On Wed, 25 Jul 2018, Adhemerval Zanella wrote:
> 
>> I am not familiar with HTL internals, but ISO C required some NPTL
>> internal adjustments:
> 
> - The C11 threads functions call __pthread_* internally, so all those 
> __pthread_* names needed to exist and, where they called POSIX functions 
> internally, they needed to be made to call __* names for those functions 
> where that wasn't previously necessary for POSIX namespace reasons.
> 

So summarize and assuming the fix for the stdc-predef.h for HURD [1], 
ISO C threads support for HTL would require:

  - Add the required __pthread_*  alias called from C11 threads and
    also fix any internal possible linkspace name issue by calling
    libpthread __* names.

  - Correctly tie the HTL required types with ISO C threads without 
    adding namespace or linkspace pollution by adding a platform
    pthreadtypes-arch.h implementation (not sure if it would require
    arch-specific ones since HURD since to support only i686).

  - Check if HTL requires some handling of the function pointer 
    mismatch between ISO C thread thrd_start start and pthread_create.

  - Add a wrapper to nanosleep to return syscall return code directly
    to use on thrd_sleep.

  - Call __sched_yield in thrd_yield.

  - Remove hurd specific stdc-predef.h and htl threads.h and make
    threads.h the generic one.

[1] https://sourceware.org/ml/libc-alpha/2018-07/msg00835.html
  

On Wed, 25 Jul 2018, Adhemerval Zanella wrote:

> So summarize and assuming the fix for the stdc-predef.h for HURD [1], 
> ISO C threads support for HTL would require:
> 
>   - Add the required __pthread_*  alias called from C11 threads and
>     also fix any internal possible linkspace name issue by calling
>     libpthread __* names.
> 
>   - Correctly tie the HTL required types with ISO C threads without 
>     adding namespace or linkspace pollution by adding a platform
>     pthreadtypes-arch.h implementation (not sure if it would require
>     arch-specific ones since HURD since to support only i686).
> 
>   - Check if HTL requires some handling of the function pointer 
>     mismatch between ISO C thread thrd_start start and pthread_create.
> 
>   - Add a wrapper to nanosleep to return syscall return code directly
>     to use on thrd_sleep.
> 
>   - Call __sched_yield in thrd_yield.
> 
>   - Remove hurd specific stdc-predef.h and htl threads.h and make
>     threads.h the generic one.

- Move all the relevant implementation files and tests and associated 
makefile logic out of nptl/ into a shared directory such as 
sysdeps/pthread/ so that they get built for Hurd as well.
  

On 25/07/2018 17:20, Joseph Myers wrote:
> On Wed, 25 Jul 2018, Adhemerval Zanella wrote:
> 
>> So summarize and assuming the fix for the stdc-predef.h for HURD [1], 
>> ISO C threads support for HTL would require:
>>
>>   - Add the required __pthread_*  alias called from C11 threads and
>>     also fix any internal possible linkspace name issue by calling
>>     libpthread __* names.
>>
>>   - Correctly tie the HTL required types with ISO C threads without 
>>     adding namespace or linkspace pollution by adding a platform
>>     pthreadtypes-arch.h implementation (not sure if it would require
>>     arch-specific ones since HURD since to support only i686).
>>
>>   - Check if HTL requires some handling of the function pointer 
>>     mismatch between ISO C thread thrd_start start and pthread_create.
>>
>>   - Add a wrapper to nanosleep to return syscall return code directly
>>     to use on thrd_sleep.
>>
>>   - Call __sched_yield in thrd_yield.
>>
>>   - Remove hurd specific stdc-predef.h and htl threads.h and make
>>     threads.h the generic one.
> 
> - Move all the relevant implementation files and tests and associated 
> makefile logic out of nptl/ into a shared directory such as 
> sysdeps/pthread/ so that they get built for Hurd as well.
> 

Right, I will try to spend some cycles for 2.29.