diff mbox

Deprecate union wait and remove support from wait functions [BZ #19613]

Message ID 56BE07C8.40205@redhat.com
State New
Headers show

Commit Message

Florian Weimer Feb. 12, 2016, 4:26 p.m. UTC 
The overloading approach in the macros was incompatible with integer
expressions of a type different from int.  Applications using union wait
and these macros will have to migrate to the POSIX-specified int
status type.

This commit also removes the transparent union overloads from the
process wait functions.  As a result, application code which uses
union wait will receive compile-time warnings about pointer
compatibility.

This does not have ABI impact.  The functionality was deprecated in BSD
in the early 1990s.  I plan to remove union wait completely in glibc 2.25.

Florian

Comments

Florian Weimer March 7, 2016, 8:38 p.m. UTC | #1 
On 02/12/2016 05:26 PM, Florian Weimer wrote:
> The overloading approach in the macros was incompatible with integer
> expressions of a type different from int.  Applications using union wait
> and these macros will have to migrate to the POSIX-specified int
> status type.
> 
> This commit also removes the transparent union overloads from the
> process wait functions.  As a result, application code which uses
> union wait will receive compile-time warnings about pointer
> compatibility.
> 
> This does not have ABI impact.  The functionality was deprecated in BSD
> in the early 1990s.  I plan to remove union wait completely in glibc 2.25.

Ping?

I'd like to have some buy-in for this API change.

Thanks,
Florian
Paul Eggert March 7, 2016, 10:44 p.m. UTC | #2 
On 03/07/2016 12:38 PM, Florian Weimer wrote:
> I'd like to have some buy-in for this API change.

I have read the patch and it looks good to me. Thanks for persevering on 
this.
Roland McGrath March 7, 2016, 10:55 p.m. UTC | #3 
I don't see a rationale for keeping union wait at all if it can be longer
be used with the wait* functions.  If we need staged deprecation, then it
should be a staged deprecation of all the features (with the possible
exception of W* macros accepting 'union wait' argument values, which was
always a GNU extension over the 4.3BSD API that specified 'union wait').
Personally I'm OK with just removing it entirely in one cycle.

> 	[BZ #19613]
> 	Deprecate union wait and remove union wait * support from W*
> 	macros in <sys/wait.h>.

The W* macros support arguments of type 'union wait', not 'union wait *'.

> 	* sunrpc/key_call.c (key_call_keyenvoy): Use int instead of union
> 	wait.

This one change could and should go in separately first.

> diff --git a/sunrpc/key_call.c b/sunrpc/key_call.c
> index 7ecf6fb..7880ab9 100644
> --- a/sunrpc/key_call.c
> +++ b/sunrpc/key_call.c
> @@ -304,7 +304,7 @@ key_call_keyenvoy (u_long proc, xdrproc_t xdr_arg, char *arg,
>    FILE *fargs;
>    FILE *frslt;
>    sigset_t oldmask, mask;
> -  union wait status;
> +  int status;
>    int pid;
>    int success;
>    uid_t ruid;

This is insufficient.  You need to adjust the use of STATUS as well.  This
function is under #ifndef SO_PASSCRED so you'd probably have to do a Hurd
build to see actually get built.  If you hand-hacked out the #ifndef for
purposes of compile testing on a Linux configuration, then that would be
good enough for the change to go in.


Thanks,
Roland
diff mbox

Patch

2016-02-12  Florian Weimer  <fweimer@redhat.com>
 
	[BZ #19613]
	Deprecate union wait and remove union wait * support from W*
	macros in <sys/wait.h>.
	* bits/waitstatus.h (union wait): Mark as deprecated.
	* include/sys/wait.h (__wait, __wait3, __wait4): Use int * for the
	stat_loc argument.
	* posix/sys/wait.h (__WAIT_INT, __WAIT_STATUS)
	(__WAIT_STATUS_DEFN): Remove.
	(WEXITSTATUS, WTERMSIG, WSTOPSIG, WIFEXITED, WIFSIGNALED)
	(WIFSTOPPED, WIFCONTINUED, WCOREDUMP): Remove __WAIT_INT.
	(wait, wait3, wait4): Use int * for the stat_loc argument.
	* posix/wait.c (__wait): Likewise.
	* posix/wait3.c (__wait3): Likewise.
	* posix/wait4.c (__wait4): Likewise.
	* stdlib/stdlib.h (__WAIT_INT, __WAIT_STATUS)
	(__WAIT_STATUS_DEFN): Remove.
	(WEXITSTATUS, WTERMSIG, WSTOPSIG, WIFEXITED, WIFSIGNALED)
	(WIFSTOPPED, WIFCONTINUED): Remove __WAIT_INT.
	* sunrpc/key_call.c (key_call_keyenvoy): Use int instead of union
	wait.
	* sysdeps/mach/hurd/wait4.c (__wait4): Use int * for the stat_loc
	argument.
	* sysdeps/posix/wait.c (__libc_wait): Likewise.
	* sysdeps/posix/wait3.c (__wait3): Likewise.
	* sysdeps/unix/bsd/wait.c (__libc_wait): Likewise.
	* sysdeps/unix/bsd/wait3.c (__wait3): Likewise.
	* sysdeps/unix/bsd/waitpid.c (__waitpid): Remove cast.
	* sysdeps/unix/sysv/linux/wait.c (__libc_wait): Use int * for the
	stat_loc argument.
	* manual/process.texi (BSD Wait Functions): Remove union wait.

diff --git a/NEWS b/NEWS
index 93c09be..d58d732 100644
--- a/NEWS
+++ b/NEWS
@@ -73,6 +73,11 @@  Version 2.23
   C Library is GCC 4.7.  Older GCC versions, and non-GNU compilers, can
   still be used to compile programs using the GNU C Library.
 
+* The type union wait is deprecated.  It was obsoleted by 4.3+BSD in the
+  early 1990s.  Support for union wait *arguments in the W* status decoding
+  macros define <sys/wait.h> has been removed.  Application code should use
+  the int type instead of union wait.
+
 Security related changes:
 
 * The nan, nanf and nanl functions no longer have unbounded stack usage
diff --git a/bits/waitstatus.h b/bits/waitstatus.h
index 38c33bc..d6af076 100644
--- a/bits/waitstatus.h
+++ b/bits/waitstatus.h
@@ -63,7 +63,7 @@ 
 
 # include <endian.h>
 
-union wait
+union __attribute_deprecated__ wait
   {
     int w_status;
     struct
diff --git a/include/sys/wait.h b/include/sys/wait.h
index 9a38e61..5ac9cd6 100644
--- a/include/sys/wait.h
+++ b/include/sys/wait.h
@@ -9,10 +9,10 @@  libc_hidden_proto (__waitpid)
 extern int __waitid (idtype_t idtype, id_t id, siginfo_t *infop, int options);
 
 extern __pid_t __libc_wait (int *__stat_loc);
-extern __pid_t __wait (__WAIT_STATUS __stat_loc);
-extern __pid_t __wait3 (__WAIT_STATUS __stat_loc,
+extern __pid_t __wait (int *__stat_loc);
+extern __pid_t __wait3 (int *__stat_loc,
 			int __options, struct rusage * __usage);
-extern __pid_t __wait4 (__pid_t __pid, __WAIT_STATUS __stat_loc,
+extern __pid_t __wait4 (__pid_t __pid, int *__stat_loc,
 			int __options, struct rusage *__usage)
 			attribute_hidden;
 #endif
diff --git a/manual/process.texi b/manual/process.texi
index aa59040..25bdb8e 100644
--- a/manual/process.texi
+++ b/manual/process.texi
@@ -763,53 +763,17 @@  signal number of the signal that caused the child process to stop.
 
 
 @node BSD Wait Functions
-@section BSD Process Wait Functions
+@section BSD Process Wait Function
 
-@Theglibc{} also provides these related facilities for compatibility
-with BSD Unix.  BSD uses the @code{union wait} data type to represent
-status values rather than an @code{int}.  The two representations are
-actually interchangeable; they describe the same bit patterns.  @Theglibc{}
-defines macros such as @code{WEXITSTATUS} so that they will
-work on either kind of object, and the @code{wait} function is defined
-to accept either type of pointer as its @var{status-ptr} argument.
-
-These functions are declared in @file{sys/wait.h}.
+@Theglibc{} also provides the @code{wait3} function for compatibility
+with BSD.  This function is declared in @file{sys/wait.h}.  It is the
+predecessor to @code{wait4}, which is more flexible.  @code{wait3} is
+now obsolete.
 @pindex sys/wait.h
 
 @comment sys/wait.h
 @comment BSD
-@deftp {Data Type} {union wait}
-This data type represents program termination status values.  It has
-the following members:
-
-@table @code
-@item int w_termsig
-The value of this member is the same as that of the
-@code{WTERMSIG} macro.
-
-@item int w_coredump
-The value of this member is the same as that of the
-@code{WCOREDUMP} macro.
-
-@item int w_retcode
-The value of this member is the same as that of the
-@code{WEXITSTATUS} macro.
-
-@item int w_stopsig
-The value of this member is the same as that of the
-@code{WSTOPSIG} macro.
-@end table
-
-Instead of accessing these members directly, you should use the
-equivalent macros.
-@end deftp
-
-The @code{wait3} function is the predecessor to @code{wait4}, which is
-more flexible.  @code{wait3} is now obsolete.
-
-@comment sys/wait.h
-@comment BSD
-@deftypefun pid_t wait3 (union wait *@var{status-ptr}, int @var{options}, struct rusage *@var{usage})
+@deftypefun pid_t wait3 (int *@var{status-ptr}, int @var{options}, struct rusage *@var{usage})
 @safety{@prelim{}@mtsafe{}@assafe{}@acsafe{}}
 If @var{usage} is a null pointer, @code{wait3} is equivalent to
 @code{waitpid (-1, @var{status-ptr}, @var{options})}.
diff --git a/posix/sys/wait.h b/posix/sys/wait.h
index a673b41..acda43a 100644
--- a/posix/sys/wait.h
+++ b/posix/sys/wait.h
@@ -34,62 +34,23 @@  __BEGIN_DECLS
    bits to `waitpid', `wait3', and `wait4'.  */
 # include <bits/waitflags.h>
 
-# ifdef	__USE_MISC
-
-/* Lots of hair to allow traditional BSD use of `union wait'
-   as well as POSIX.1 use of `int' for the status word.  */
-
-#  if defined __GNUC__ && !defined __cplusplus
-#   define __WAIT_INT(status) \
-  (__extension__ (((union { __typeof(status) __in; int __i; }) \
-		   { .__in = (status) }).__i))
-#  else
-#   define __WAIT_INT(status)	(*(const int *) &(status))
-#  endif
-
-/* This is the type of the argument to `wait'.  The funky union
-   causes redeclarations with either `int *' or `union wait *' to be
-   allowed without complaint.  __WAIT_STATUS_DEFN is the type used in
-   the actual function definitions.  */
-
-#  if !defined __GNUC__ || __GNUC__ < 2 || defined __cplusplus
-#   define __WAIT_STATUS	void *
-#   define __WAIT_STATUS_DEFN	void *
-#  else
-/* This works in GCC 2.6.1 and later.  */
-typedef union
-  {
-    union wait *__uptr;
-    int *__iptr;
-  } __WAIT_STATUS __attribute__ ((__transparent_union__));
-#   define __WAIT_STATUS_DEFN	int *
-#  endif
-
-# else /* Don't use misc.  */
-
-#  define __WAIT_INT(status)	(status)
-#  define __WAIT_STATUS		int *
-#  define __WAIT_STATUS_DEFN	int *
-
-# endif /* Use misc.  */
-
 /* This will define all the `__W*' macros.  */
 # include <bits/waitstatus.h>
 
-# define WEXITSTATUS(status)	__WEXITSTATUS (__WAIT_INT (status))
-# define WTERMSIG(status)	__WTERMSIG (__WAIT_INT (status))
-# define WSTOPSIG(status)	__WSTOPSIG (__WAIT_INT (status))
-# define WIFEXITED(status)	__WIFEXITED (__WAIT_INT (status))
-# define WIFSIGNALED(status)	__WIFSIGNALED (__WAIT_INT (status))
-# define WIFSTOPPED(status)	__WIFSTOPPED (__WAIT_INT (status))
+# define WEXITSTATUS(status)	__WEXITSTATUS (status)
+# define WTERMSIG(status)	__WTERMSIG (status)
+# define WSTOPSIG(status)	__WSTOPSIG (status)
+# define WIFEXITED(status)	__WIFEXITED (status)
+# define WIFSIGNALED(status)	__WIFSIGNALED (status)
+# define WIFSTOPPED(status)	__WIFSTOPPED (status)
 # ifdef __WIFCONTINUED
-#  define WIFCONTINUED(status)	__WIFCONTINUED (__WAIT_INT (status))
+#  define WIFCONTINUED(status)	__WIFCONTINUED (status)
 # endif
 #endif	/* <stdlib.h> not included.  */
 
 #ifdef	__USE_MISC
 # define WCOREFLAG		__WCOREFLAG
-# define WCOREDUMP(status)	__WCOREDUMP (__WAIT_INT (status))
+# define WCOREDUMP(status)	__WCOREDUMP (status)
 # define W_EXITCODE(ret, sig)	__W_EXITCODE (ret, sig)
 # define W_STOPCODE(sig)	__W_STOPCODE (sig)
 #endif
@@ -110,7 +71,7 @@  typedef enum
 
    This function is a cancellation point and therefore not marked with
    __THROW.  */
-extern __pid_t wait (__WAIT_STATUS __stat_loc);
+extern __pid_t wait (int *__stat_loc);
 
 #ifdef	__USE_MISC
 /* Special values for the PID argument to `waitpid' and `wait4'.  */
@@ -170,13 +131,13 @@  struct rusage;
    nil, store information about the child's resource usage there.  If the
    WUNTRACED bit is set in OPTIONS, return status for stopped children;
    otherwise don't.  */
-extern __pid_t wait3 (__WAIT_STATUS __stat_loc, int __options,
+extern __pid_t wait3 (int *__stat_loc, int __options,
 		      struct rusage * __usage) __THROWNL;
 #endif
 
 #ifdef __USE_MISC
 /* PID is like waitpid.  Other args are like wait3.  */
-extern __pid_t wait4 (__pid_t __pid, __WAIT_STATUS __stat_loc, int __options,
+extern __pid_t wait4 (__pid_t __pid, int *__stat_loc, int __options,
 		      struct rusage *__usage) __THROWNL;
 #endif /* Use misc.  */
 
diff --git a/posix/wait.c b/posix/wait.c
index 3abf30e..c49375b 100644
--- a/posix/wait.c
+++ b/posix/wait.c
@@ -21,7 +21,7 @@ 
 /* Wait for a child to die.  When one does, put its status in *STAT_LOC
    and return its process ID.  For errors, return (pid_t) -1.  */
 __pid_t
-__wait (__WAIT_STATUS_DEFN stat_loc)
+__wait (int *stat_loc)
 {
   __set_errno (ENOSYS);
   return -1;
diff --git a/posix/wait3.c b/posix/wait3.c
index 356839a..e8c2930 100644
--- a/posix/wait3.c
+++ b/posix/wait3.c
@@ -25,7 +25,7 @@ 
    there.  If the WUNTRACED bit is set in OPTIONS, return status for stopped
    children; otherwise don't.  */
 pid_t
-__wait3 (__WAIT_STATUS_DEFN stat_loc, int options, struct rusage *usage)
+__wait3 (int *stat_loc, int options, struct rusage *usage)
 {
   if ((options & ~(WNOHANG|WUNTRACED)) != 0)
     {
diff --git a/posix/wait4.c b/posix/wait4.c
index e5b0376..4137617 100644
--- a/posix/wait4.c
+++ b/posix/wait4.c
@@ -20,8 +20,7 @@ 
 #include <errno.h>
 
 pid_t
-__wait4 (__pid_t pid, __WAIT_STATUS stat_loc, int options,
-	 struct rusage *usage)
+__wait4 (__pid_t pid, int *stat_loc, int options, struct rusage *usage)
 {
   __set_errno (ENOSYS);
   return (pid_t) -1;
diff --git a/stdlib/stdlib.h b/stdlib/stdlib.h
index cc77708..d0c78fa 100644
--- a/stdlib/stdlib.h
+++ b/stdlib/stdlib.h
@@ -41,54 +41,15 @@  __BEGIN_DECLS
 # include <bits/waitflags.h>
 # include <bits/waitstatus.h>
 
-# ifdef __USE_MISC
-
-/* Lots of hair to allow traditional BSD use of `union wait'
-   as well as POSIX.1 use of `int' for the status word.  */
-
-#  if defined __GNUC__ && !defined __cplusplus
-#   define __WAIT_INT(status) \
-  (__extension__ (((union { __typeof(status) __in; int __i; }) \
-		   { .__in = (status) }).__i))
-#  else
-#   define __WAIT_INT(status)	(*(int *) &(status))
-#  endif
-
-/* This is the type of the argument to `wait'.  The funky union
-   causes redeclarations with either `int *' or `union wait *' to be
-   allowed without complaint.  __WAIT_STATUS_DEFN is the type used in
-   the actual function definitions.  */
-
-#  if !defined __GNUC__ || __GNUC__ < 2 || defined __cplusplus
-#   define __WAIT_STATUS	void *
-#   define __WAIT_STATUS_DEFN	void *
-#  else
-/* This works in GCC 2.6.1 and later.  */
-typedef union
-  {
-    union wait *__uptr;
-    int *__iptr;
-  } __WAIT_STATUS __attribute__ ((__transparent_union__));
-#   define __WAIT_STATUS_DEFN	int *
-#  endif
-
-# else /* Don't use misc.  */
-
-#  define __WAIT_INT(status)	(status)
-#  define __WAIT_STATUS		int *
-#  define __WAIT_STATUS_DEFN	int *
-
-# endif /* Use misc.  */
-
 /* Define the macros <sys/wait.h> also would define this way.  */
-# define WEXITSTATUS(status)	__WEXITSTATUS (__WAIT_INT (status))
-# define WTERMSIG(status)	__WTERMSIG (__WAIT_INT (status))
-# define WSTOPSIG(status)	__WSTOPSIG (__WAIT_INT (status))
-# define WIFEXITED(status)	__WIFEXITED (__WAIT_INT (status))
-# define WIFSIGNALED(status)	__WIFSIGNALED (__WAIT_INT (status))
-# define WIFSTOPPED(status)	__WIFSTOPPED (__WAIT_INT (status))
+# define WEXITSTATUS(status)	__WEXITSTATUS (status)
+# define WTERMSIG(status)	__WTERMSIG (status)
+# define WSTOPSIG(status)	__WSTOPSIG (status)
+# define WIFEXITED(status)	__WIFEXITED (status)
+# define WIFSIGNALED(status)	__WIFSIGNALED (status)
+# define WIFSTOPPED(status)	__WIFSTOPPED (status)
 # ifdef __WIFCONTINUED
-#  define WIFCONTINUED(status)	__WIFCONTINUED (__WAIT_INT (status))
+#  define WIFCONTINUED(status)	__WIFCONTINUED (status)
 # endif
 #endif	/* X/Open or XPG7 and <sys/wait.h> not included.  */
 
diff --git a/sunrpc/key_call.c b/sunrpc/key_call.c
index 7ecf6fb..7880ab9 100644
--- a/sunrpc/key_call.c
+++ b/sunrpc/key_call.c
@@ -304,7 +304,7 @@  key_call_keyenvoy (u_long proc, xdrproc_t xdr_arg, char *arg,
   FILE *fargs;
   FILE *frslt;
   sigset_t oldmask, mask;
-  union wait status;
+  int status;
   int pid;
   int success;
   uid_t ruid;
diff --git a/sysdeps/mach/hurd/wait4.c b/sysdeps/mach/hurd/wait4.c
index 3bc9fa8..f392a98 100644
--- a/sysdeps/mach/hurd/wait4.c
+++ b/sysdeps/mach/hurd/wait4.c
@@ -22,8 +22,7 @@ 
 #include <hurd/port.h>
 
 pid_t
-__wait4 (pid_t pid, __WAIT_STATUS_DEFN stat_loc, int options,
-	 struct rusage *usage)
+__wait4 (pid_t pid, int *stat_loc, int options, struct rusage *usage)
 {
   pid_t dead;
   error_t err;
diff --git a/sysdeps/posix/wait.c b/sysdeps/posix/wait.c
index 7f1d71a..210ece8 100644
--- a/sysdeps/posix/wait.c
+++ b/sysdeps/posix/wait.c
@@ -21,7 +21,7 @@ 
 /* Wait for a child to die.  When one does, put its status in *STAT_LOC
    and return its process ID.  For errors, return (pid_t) -1.  */
 __pid_t
-__libc_wait (__WAIT_STATUS_DEFN stat_loc)
+__libc_wait (int *stat_loc)
 {
   return __waitpid (WAIT_ANY, (int *) stat_loc, 0);
 }
diff --git a/sysdeps/posix/wait3.c b/sysdeps/posix/wait3.c
index 2e76892..cf43d97 100644
--- a/sysdeps/posix/wait3.c
+++ b/sysdeps/posix/wait3.c
@@ -26,7 +26,7 @@ 
    there.  If the WUNTRACED bit is set in OPTIONS, return status for stopped
    children; otherwise don't.  */
 pid_t
-__wait3 (__WAIT_STATUS stat_loc, int options, struct rusage *usage)
+__wait3 (int *stat_loc, int options, struct rusage *usage)
 {
   if (usage != NULL)
     {
diff --git a/sysdeps/unix/bsd/wait.c b/sysdeps/unix/bsd/wait.c
index 31de60e..a9e29f2 100644
--- a/sysdeps/unix/bsd/wait.c
+++ b/sysdeps/unix/bsd/wait.c
@@ -23,7 +23,7 @@ 
 /* Wait for a child to die.  When one does, put its status in *STAT_LOC
    and return its process ID.  For errors, return (pid_t) -1.  */
 __pid_t
-__libc_wait (__WAIT_STATUS_DEFN stat_loc)
+__libc_wait (int *stat_loc)
 {
   return __wait4 (WAIT_ANY, stat_loc, 0, (struct rusage *) NULL);
 }
diff --git a/sysdeps/unix/bsd/wait3.c b/sysdeps/unix/bsd/wait3.c
index 3f95ac7..ee33a65 100644
--- a/sysdeps/unix/bsd/wait3.c
+++ b/sysdeps/unix/bsd/wait3.c
@@ -25,7 +25,7 @@ 
    there.  If the WUNTRACED bit is set in OPTIONS, return status for stopped
    children; otherwise don't.  */
 pid_t
-__wait3 (__WAIT_STATUS stat_loc, int options, struct rusage *usage)
+__wait3 (int *stat_loc, int options, struct rusage *usage)
 {
   return __wait4 (WAIT_ANY, stat_loc, options, usage);
 }
diff --git a/sysdeps/unix/bsd/waitpid.c b/sysdeps/unix/bsd/waitpid.c
index 083c686..cfe5614 100644
--- a/sysdeps/unix/bsd/waitpid.c
+++ b/sysdeps/unix/bsd/waitpid.c
@@ -35,7 +35,7 @@ 
 pid_t
 __waitpid (pid_t pid, int *stat_loc, int options)
 {
-  return __wait4 (pid, (union wait *) stat_loc, options, NULL);
+  return __wait4 (pid, stat_loc, options, NULL);
 }
 
 libc_hidden_def (__waitpid)
diff --git a/sysdeps/unix/sysv/linux/wait.c b/sysdeps/unix/sysv/linux/wait.c
index 71f9044..9b7c9c9 100644
--- a/sysdeps/unix/sysv/linux/wait.c
+++ b/sysdeps/unix/sysv/linux/wait.c
@@ -24,7 +24,7 @@ 
 /* Wait for a child to die.  When one does, put its status in *STAT_LOC
    and return its process ID.  For errors, return (pid_t) -1.  */
 pid_t
-__libc_wait (__WAIT_STATUS_DEFN stat_loc)
+__libc_wait (int *stat_loc)
 {
   pid_t result = SYSCALL_CANCEL (wait4, WAIT_ANY, stat_loc, 0,
 				 (struct rusage *) NULL);