Fix sigval namespace (bug 21944)

XPG4.2 defines the siginfo_t type, but not union sigval or its
contents (which were added in the 1993 edition of POSIX.1), resulting
in namespace violations for sigval, sival_int and sival_ptr for
signal.h and sys/wait.h for that standard because those headers
incorrectly expose those names in that case.

This patch fixes this problem.  The public type in this case is union
sigval, but various places in the headers use the sigval_t name for
it; direct uses of union sigval are already properly guarded or in
headers not in XPG4.2.  Now, sigval_t, although not a standard name,
does seem to be widely used outside glibc.  The approach taken by this
patch is to make installed headers use the name __sigval_t instead.
__sigval_t is then defined to either union sigval or union __sigval
(where union __sigval has __-prefixed member names as well), depending
on whether there are any namespace issues with the union sigval name
and its members.  In the case where union __sigval is used, sigval_t
is not defined at all, to avoid the problem of sigval_t having a C++
mangled name that depends on feature test macros.  sigval_t is still
defined by signal.h if __USE_MISC (reflecting the nonstandard nature
of that name).

Tested for x86_64.

2017-08-11  Joseph Myers  <joseph@codesourcery.com>

	[BZ #21944]
	* signal/bits/types/__sigval_t.h: New file.
	* signal/Makefile (headers): Add bits/types/__sigval_t.h.
	* signal/bits/types/sigval_t.h: Include <bits/types/__sigval_t.h>
	and define sigval_t using __sigval_t.
	* include/bits/types/__sigval_t.h: New file.
	* bits/types/sigevent_t.h: Include <bits/types/__sigval_t.h>
	instead of <bits/types/__sigval_t.h>.
	(struct sigevent): Use __sigval_t instead of sigval_t.
	* bits/types/siginfo_t.h: Include <bits/types/__sigval_t.h>
	instead of <bits/types/__sigval_t.h>.
	(siginfo_t): Use __sigval_t instead of sigval_t.
	* sysdeps/unix/sysv/linux/bits/types/sigevent_t.h: Include
	<bits/types/__sigval_t.h> instead of <bits/types/__sigval_t.h>.
	(struct sigevent): Use __sigval_t instead of sigval_t.
	* sysdeps/unix/sysv/linux/bits/types/siginfo_t.h: Include
	<bits/types/__sigval_t.h> instead of <bits/types/__sigval_t.h>.
	(siginfo_t): Use __sigval_t instead of sigval_t.
	* signal/signal.h [__USE_MISC]: Include <bits/types/sigval_t.h>.

Ping.  This patch 
<https://sourceware.org/ml/libc-alpha/2017-08/msg00466.html> is pending 
review.

On 11/08/2017 15:12, Joseph Myers wrote:
> XPG4.2 defines the siginfo_t type, but not union sigval or its
> contents (which were added in the 1993 edition of POSIX.1), resulting
> in namespace violations for sigval, sival_int and sival_ptr for
> signal.h and sys/wait.h for that standard because those headers
> incorrectly expose those names in that case.
> 
> This patch fixes this problem.  The public type in this case is union
> sigval, but various places in the headers use the sigval_t name for
> it; direct uses of union sigval are already properly guarded or in
> headers not in XPG4.2.  Now, sigval_t, although not a standard name,
> does seem to be widely used outside glibc.  The approach taken by this
> patch is to make installed headers use the name __sigval_t instead.
> __sigval_t is then defined to either union sigval or union __sigval
> (where union __sigval has __-prefixed member names as well), depending
> on whether there are any namespace issues with the union sigval name
> and its members.  In the case where union __sigval is used, sigval_t
> is not defined at all, to avoid the problem of sigval_t having a C++
> mangled name that depends on feature test macros.  sigval_t is still
> defined by signal.h if __USE_MISC (reflecting the nonstandard nature
> of that name).
> 
> Tested for x86_64.
> 
> 2017-08-11  Joseph Myers  <joseph@codesourcery.com>
> 
> 	[BZ #21944]
> 	* signal/bits/types/__sigval_t.h: New file.
> 	* signal/Makefile (headers): Add bits/types/__sigval_t.h.
> 	* signal/bits/types/sigval_t.h: Include <bits/types/__sigval_t.h>
> 	and define sigval_t using __sigval_t.
> 	* include/bits/types/__sigval_t.h: New file.
> 	* bits/types/sigevent_t.h: Include <bits/types/__sigval_t.h>
> 	instead of <bits/types/__sigval_t.h>.
> 	(struct sigevent): Use __sigval_t instead of sigval_t.
> 	* bits/types/siginfo_t.h: Include <bits/types/__sigval_t.h>
> 	instead of <bits/types/__sigval_t.h>.
> 	(siginfo_t): Use __sigval_t instead of sigval_t.
> 	* sysdeps/unix/sysv/linux/bits/types/sigevent_t.h: Include
> 	<bits/types/__sigval_t.h> instead of <bits/types/__sigval_t.h>.
> 	(struct sigevent): Use __sigval_t instead of sigval_t.
> 	* sysdeps/unix/sysv/linux/bits/types/siginfo_t.h: Include
> 	<bits/types/__sigval_t.h> instead of <bits/types/__sigval_t.h>.
> 	(siginfo_t): Use __sigval_t instead of sigval_t.
> 	* signal/signal.h [__USE_MISC]: Include <bits/types/sigval_t.h>.

I can't really voucher for the standard wording, but patch itself LGTM 
with just some doubt regarding new header copyright headers below.


> 
> diff --git a/bits/types/sigevent_t.h b/bits/types/sigevent_t.h
> index 7b8cb05..5611268 100644
> --- a/bits/types/sigevent_t.h
> +++ b/bits/types/sigevent_t.h
> @@ -2,15 +2,15 @@
>  #define __sigevent_t_defined 1
>  
>  #include <bits/types.h>
> -#include <bits/types/sigval_t.h>
> +#include <bits/types/__sigval_t.h>
>  
>  /* Structure to transport application-defined values with signals.  */
>  typedef struct sigevent
>    {
> -    sigval_t sigev_value;
> +    __sigval_t sigev_value;
>      int sigev_signo;
>      int sigev_notify;
> -    void (*sigev_notify_function) (sigval_t);	    /* Function to start.  */
> +    void (*sigev_notify_function) (__sigval_t);	    /* Function to start.  */
>      void *sigev_notify_attributes;		    /* Really pthread_attr_t.*/
>    } sigevent_t;
>  
> diff --git a/bits/types/siginfo_t.h b/bits/types/siginfo_t.h
> index ab6bf18..1ac2a98 100644
> --- a/bits/types/siginfo_t.h
> +++ b/bits/types/siginfo_t.h
> @@ -2,7 +2,7 @@
>  #define __siginfo_t_defined 1
>  
>  #include <bits/types.h>
> -#include <bits/types/sigval_t.h>
> +#include <bits/types/__sigval_t.h>
>  
>  typedef struct siginfo
>    {
> @@ -15,7 +15,7 @@ typedef struct siginfo
>      void *si_addr;		/* Address of faulting instruction.  */
>      int si_status;		/* Exit value or signal.  */
>      long int si_band;		/* Band event for SIGPOLL.  */
> -    sigval_t si_value;		/* Signal value.  */
> +    __sigval_t si_value;	/* Signal value.  */
>    } siginfo_t;
>  
>  #endif
> diff --git a/include/bits/types/__sigval_t.h b/include/bits/types/__sigval_t.h
> new file mode 100644
> index 0000000..62f8e48
> --- /dev/null
> +++ b/include/bits/types/__sigval_t.h
> @@ -0,0 +1 @@
> +#include <signal/bits/types/__sigval_t.h>

Shouldn't we have a copyright definition here?

> diff --git a/signal/Makefile b/signal/Makefile
> index 8c9a7d1..a6a1289 100644
> --- a/signal/Makefile
> +++ b/signal/Makefile
> @@ -30,7 +30,8 @@ headers := signal.h sys/signal.h \
>  	   bits/types/__sigset_t.h bits/types/sig_atomic_t.h \
>  	   bits/types/sigevent_t.h bits/types/siginfo_t.h \
>  	   bits/types/sigset_t.h bits/types/sigval_t.h \
> -	   bits/types/stack_t.h bits/types/struct_sigstack.h
> +	   bits/types/stack_t.h bits/types/struct_sigstack.h \
> +	   bits/types/__sigval_t.h
>  
>  routines	:= signal raise killpg \
>  		   sigaction sigprocmask kill \
> diff --git a/signal/bits/types/__sigval_t.h b/signal/bits/types/__sigval_t.h
> new file mode 100644
> index 0000000..90cba5b
> --- /dev/null
> +++ b/signal/bits/types/__sigval_t.h
> @@ -0,0 +1,23 @@
> +#ifndef ____sigval_t_defined
> +#define ____sigval_t_defined

Same as before. I noted also for signal/bits/types only struct_sigstack.h
contains the header and it is based on old kernel version. Should we omit
the copyright header for these new types definitions headers?

> +
> +/* Type for data associated with a signal.  */
> +#ifdef __USE_POSIX199309
> +union sigval
> +{
> +  int sival_int;
> +  void *sival_ptr;
> +};
> +
> +typedef union sigval __sigval_t;
> +#else
> +union __sigval
> +{
> +  int __sival_int;
> +  void *__sival_ptr;
> +};
> +
> +typedef union __sigval __sigval_t;
> +#endif
> +
> +#endif
> diff --git a/signal/bits/types/sigval_t.h b/signal/bits/types/sigval_t.h
> index 666598f..a05d7f4 100644
> --- a/signal/bits/types/sigval_t.h
> +++ b/signal/bits/types/sigval_t.h
> @@ -1,13 +1,18 @@
>  #ifndef __sigval_t_defined
>  #define __sigval_t_defined
>  
> -/* Type for data associated with a signal.  */
> -union sigval
> -{
> -  int sival_int;
> -  void *sival_ptr;
> -};
> -
> -typedef union sigval sigval_t;
> +#include <bits/types/__sigval_t.h>
> +
> +/* To avoid sigval_t (not a standard type name) having C++ name
> +   mangling depending on whether the selected standard includes union
> +   sigval, it should not be defined at all when using a standard for
> +   which the sigval name is not reserved; in that case, headers should
> +   not include <bits/types/sigval_t.h> and should use only the
> +   internal __sigval_t name.  */
> +#ifndef __USE_POSIX199309
> +# error "sigval_t defined for standard not including union sigval"
> +#endif
> +
> +typedef __sigval_t sigval_t;
>  
>  #endif
> diff --git a/signal/signal.h b/signal/signal.h
> index c8f6100..416c5a2 100644
> --- a/signal/signal.h
> +++ b/signal/signal.h
> @@ -58,6 +58,10 @@ typedef __uid_t uid_t;
>  # include <bits/siginfo-consts.h>
>  #endif
>  
> +#ifdef __USE_MISC
> +# include <bits/types/sigval_t.h>
> +#endif
> +
>  #ifdef __USE_POSIX199309
>  # include <bits/types/sigevent_t.h>
>  # include <bits/sigevent-consts.h>
> diff --git a/sysdeps/unix/sysv/linux/bits/types/sigevent_t.h b/sysdeps/unix/sysv/linux/bits/types/sigevent_t.h
> index 0d4857b..e8b28de 100644
> --- a/sysdeps/unix/sysv/linux/bits/types/sigevent_t.h
> +++ b/sysdeps/unix/sysv/linux/bits/types/sigevent_t.h
> @@ -3,7 +3,7 @@
>  
>  #include <bits/wordsize.h>
>  #include <bits/types.h>
> -#include <bits/types/sigval_t.h>
> +#include <bits/types/__sigval_t.h>
>  
>  #define __SIGEV_MAX_SIZE	64
>  #if __WORDSIZE == 64
> @@ -21,7 +21,7 @@ typedef union pthread_attr_t pthread_attr_t;
>  /* Structure to transport application-defined values with signals.  */
>  typedef struct sigevent
>    {
> -    sigval_t sigev_value;
> +    __sigval_t sigev_value;
>      int sigev_signo;
>      int sigev_notify;
>  
> @@ -35,7 +35,7 @@ typedef struct sigevent
>  
>  	struct
>  	  {
> -	    void (*_function) (sigval_t);	/* Function to start.  */
> +	    void (*_function) (__sigval_t);	/* Function to start.  */
>  	    pthread_attr_t *_attribute;		/* Thread attributes.  */
>  	  } _sigev_thread;
>        } _sigev_un;
> diff --git a/sysdeps/unix/sysv/linux/bits/types/siginfo_t.h b/sysdeps/unix/sysv/linux/bits/types/siginfo_t.h
> index bed6914..33766d1 100644
> --- a/sysdeps/unix/sysv/linux/bits/types/siginfo_t.h
> +++ b/sysdeps/unix/sysv/linux/bits/types/siginfo_t.h
> @@ -3,7 +3,7 @@
>  
>  #include <bits/wordsize.h>
>  #include <bits/types.h>
> -#include <bits/types/sigval_t.h>
> +#include <bits/types/__sigval_t.h>
>  
>  #define __SI_MAX_SIZE	128
>  #if __WORDSIZE == 64
> @@ -64,7 +64,7 @@ typedef struct
>  	  {
>  	    int si_tid;		/* Timer ID.  */
>  	    int si_overrun;	/* Overrun count.  */
> -	    sigval_t si_sigval;	/* Signal value.  */
> +	    __sigval_t si_sigval;	/* Signal value.  */
>  	  } _timer;
>  
>  	/* POSIX.1b signals.  */
> @@ -72,7 +72,7 @@ typedef struct
>  	  {
>  	    __pid_t si_pid;	/* Sending process ID.  */
>  	    __uid_t si_uid;	/* Real user ID of sending process.  */
> -	    sigval_t si_sigval;	/* Signal value.  */
> +	    __sigval_t si_sigval;	/* Signal value.  */
>  	  } _rt;
>  
>  	/* SIGCHLD.  */
>

On Wed, 16 Aug 2017, Adhemerval Zanella wrote:

> > diff --git a/include/bits/types/__sigval_t.h b/include/bits/types/__sigval_t.h
> > new file mode 100644
> > index 0000000..62f8e48
> > --- /dev/null
> > +++ b/include/bits/types/__sigval_t.h
> > @@ -0,0 +1 @@
> > +#include <signal/bits/types/__sigval_t.h>
> 
> Shouldn't we have a copyright definition here?

Definitely not for a 1-line file.  Probably for type-definition files that 
are more than ten lines long.

On 16/08/2017 17:31, Joseph Myers wrote:
> On Wed, 16 Aug 2017, Adhemerval Zanella wrote:
> 
>>> diff --git a/include/bits/types/__sigval_t.h b/include/bits/types/__sigval_t.h
>>> new file mode 100644
>>> index 0000000..62f8e48
>>> --- /dev/null
>>> +++ b/include/bits/types/__sigval_t.h
>>> @@ -0,0 +1 @@
>>> +#include <signal/bits/types/__sigval_t.h>
>>
>> Shouldn't we have a copyright definition here?
> 
> Definitely not for a 1-line file.  Probably for type-definition files that 
> are more than ten lines long.
> 

I agree that it does not make sense for this specific file, but I think
signal/bits/types/__sigval_t.h classifies for header addition.