Patchwork [RFC] Use anonymous union for siginfo_t

login
register
mail settings
Submitter Zack Weinberg
Date Aug. 11, 2017, 2:02 p.m.
Message ID <20170811140257.19664-1-zackw@panix.com>
Download mbox | patch
Permalink /patch/22081/
State New
Headers show

Comments

Zack Weinberg - Aug. 11, 2017, 2:02 p.m.
C2011 officially includes an 'anonymous union' feature, and GCC has
supported it for many years.  It makes sub-fields of a union that's a
struct field appear to be fields of the parent struct.  If we use this
in the definition of siginfo_t, we don't need to define lots of
innocuous-looking identifiers like 'si_pid' as macros expanding to
chains of field accessors.  The catch, however, is that the compiler
used to compile *programs that use glibc* - not just glibc itself -
must accept the use of this feature (in system headers) even when
running in an older conformance mode.

This patch only touches siginfo_t, but if people like the idea, we
could also do it for several other types:

netinet/in.h (in6_addr)
sys/stat.h (stat)
utmp.h (utmp)
signal.h (sigaction, sigevent_t)
ucontext.h (ucontext_t)

and maybe also - these use names in the user namespace for the fields
that would be removed:

net/if.h (ifaddr)
ifaddrs.h (ifaddrs)
netinet/in6.h (ip6_hdr) (really should be bitfields instead)
netinet/icmp6.h (many)
net/if_ppp.h (ifpppstatsreq, ifpppcstatsreq)
net/if_shaper.h (shaperconf)
a.out.h (exec) (only some versions)
sys/quota.h (dqblk?) (the dq_* macros refer to a field that doesn't exist?!)

There are still more hits in sunrpc and nis, but since hopefully that
code is going to go away, I don't propose to mess with them.  And
there may be even more that aren't caught by grepping for
'#define IDENT IDENT.IDENT'.

As a side note (and this could be split for commit if felt
appropriate), the siginfo_t field aliases 'si_int' and 'si_ptr' are
not in POSIX.  There are a few uses of these within glibc itself, and
a handful more in third-party software (not glibc, not uclibc, and not
linux) so I have preserved them, but put them under __USE_MISC and
added a deprecation warning.

This passes the glibc testsuite on x86-64-linux, which probably
*doesn't* test the case where someone is compiling a program in
an older conformance mode that uses siginfo_t
(-std=c99 -D_XOPEN_SOURCE=600, perhaps).

What do you think?

zw

	* sysdeps/unix/sysv/linux/bits/types/siginfo_t.h (siginfo_t):
	Use C2011 anonymous union and anonymous struct-in-union features
	to define this type.  Rename some public fields with their
	official names.
	(si_pid, si_uid, si_timerid, si_overrun, si_status, si_utime)
	(si_stime, si_value, si_addr, si_addr_lsb, si_lower, si_upper)
	(si_pkey, si_band, si_fd, si_call_addr, si_syscall, si_arch):
	Do not define as macros.
	(si_int, si_ptr): Define only when __USE_MISC, with deprecation
	warnings.
	* sysdeps/unix/sysv/linux/ia64/bits/siginfo-arch.h
	* sysdeps/unix/sysv/linux/sparc/bits/siginfo-arch.h
	* sysdeps/unix/sysv/linux/tile/bits/siginfo-arch.h
	(__SI_SIGFAULT_ADDL): Define all fields with their public names
	when __USE_GNU, or with impl-namespace names otherwise.
	(si_imm, si_segvflags, si_isr, si_trapno): Do not define as macros.

	* sysdeps/unix/sysv/linux/timer_routines.c (timer_helper_thread)
	Use si_value.sival_ptr instead of si_ptr.
	* sysdeps/unix/sysv/linux/tst-getpid1.c (do_test):
	Use si_value.sival_int instead of si_int.
---
 NEWS                                              | 16 ++++-
 sysdeps/unix/sysv/linux/bits/types/siginfo_t.h    | 84 +++++++++--------------
 sysdeps/unix/sysv/linux/ia64/bits/siginfo-arch.h  | 17 ++---
 sysdeps/unix/sysv/linux/sparc/bits/siginfo-arch.h |  9 +--
 sysdeps/unix/sysv/linux/tile/bits/siginfo-arch.h  |  9 +--
 sysdeps/unix/sysv/linux/timer_routines.c          |  2 +-
 sysdeps/unix/sysv/linux/tst-getpid1.c             |  5 +-
 7 files changed, 70 insertions(+), 72 deletions(-)
Joseph Myers - Aug. 11, 2017, 4:24 p.m.
On Fri, 11 Aug 2017, Zack Weinberg wrote:

> +* Some headers now make unconditional use of the C2011 'anonymous union'
> +  feature.  In order to compile a program that uses any of these headers,

This is not entirely new.  sysdeps/gnu/netinet/tcp.h has used this feature 
(inside __extension__) since the _BSD_SOURCE removal, to reconcile two 
different APIs for the same structure.  There are probably other uses.  
(sysdeps/unix/sysv/linux/alpha/bits/stat.h has one conditioned on 
__GNUC_PREREQ(3,3), where those conditions could be removed if we agree 
this feature is unconditionally required for using glibc headers.)
Paul Eggert - Aug. 11, 2017, 4:48 p.m.
On 08/11/2017 07:02 AM, Zack Weinberg wrote:
> The catch, however, is that the compiler
> used to compile*programs that use glibc*  - not just glibc itself -
> must accept the use of this feature (in system headers) even when
> running in an older conformance mode.

Perhaps you could survey other non-GCC compilers in use with glibc, as 
to whether they support this feature. Not just Clang, but also ARM, Cray 
CCE, Fujitsu/Lahey, IBM XL C/C++, Intel, PGI, Oracle Studio, and maybe 
PathScale. If recent versions of compilers in practical use on glibc 
platforms support the feature, that should be good enough.
Zack Weinberg - Aug. 11, 2017, 5:57 p.m.
On Fri, Aug 11, 2017 at 12:48 PM, Paul Eggert <eggert@cs.ucla.edu> wrote:
> On 08/11/2017 07:02 AM, Zack Weinberg wrote:
>>
>> The catch, however, is that the compiler
>> used to compile*programs that use glibc*  - not just glibc itself -
>> must accept the use of this feature (in system headers) even when
>> running in an older conformance mode.
>
>
> Perhaps you could survey other non-GCC compilers in use with glibc, as to
> whether they support this feature. Not just Clang, but also ARM, Cray CCE,
> Fujitsu/Lahey, IBM XL C/C++, Intel, PGI, Oracle Studio, and maybe PathScale.

This is a fine idea but I don't know that I can get my hands on all of
these compilers, certainly not this week or next week (family-related
travel).  Perhaps people who already have them could give it a try?
This test program should be sufficient...

#define _XOPEN_SOURCE 600
#include <signal.h>

extern void do_something(int);

void
handler(int sig, siginfo_t *info, void *ctx)
{
   do_something(info->si_value.sival_int);
}

compiled with the equivalent of -c -std=c99 and the warnings cranked
up as high as they will go.  I'll push my branch when I get home.

zw

Patch

diff --git a/NEWS b/NEWS
index 484c467569..f601ecbb37 100644
--- a/NEWS
+++ b/NEWS
@@ -15,10 +15,24 @@  Deprecated and removed features, and other changes affecting compatibility:
 
 * On GNU/Linux, the obsolete Linux constant PTRACE_SEIZE_DEVEL is no longer
   defined by <sys/ptrace.h>.
+* The nonstandard siginfo_t fields 'si_int' and 'si_ptr' are deprecated, and
+  will be removed in a future release.  Programs should instead use
+  'si_value.sival_int' and 'si_value.sival_ptr', respectively.
 
 Changes to build and runtime requirements:
 
-  [Add changes to build and runtime requirements here]
+* Some headers now make unconditional use of the C2011 'anonymous union'
+  feature.  In order to compile a program that uses any of these headers,
+  you must use a compiler that supports this feature, even when conforming
+  to an earlier language standard.  This has been true of GCC and Clang for
+  many years.  The structures and headers affected are:
+
+    - siginfo_t (signal.h, sys/wait.h)
+
+  The advantage of this change is that these headers define many fewer
+  macros with surprising expansions (for instance, the identifier 'si_pid'
+  used to be a macro expanding to '_sifields._kill.si_pid' on GNU/Linux
+  systems).
 
 Security related changes:
 
diff --git a/sysdeps/unix/sysv/linux/bits/types/siginfo_t.h b/sysdeps/unix/sysv/linux/bits/types/siginfo_t.h
index bed69148f9..dc9265b2a4 100644
--- a/sysdeps/unix/sysv/linux/bits/types/siginfo_t.h
+++ b/sysdeps/unix/sysv/linux/bits/types/siginfo_t.h
@@ -52,38 +52,32 @@  typedef struct
       {
 	int _pad[__SI_PAD_SIZE];
 
-	 /* kill().  */
+	 /* Signals sent by kill or sigqueue.  si_sigval is only valid for
+	    sigqueue.  */
 	struct
 	  {
 	    __pid_t si_pid;	/* Sending process ID.  */
 	    __uid_t si_uid;	/* Real user ID of sending process.  */
-	  } _kill;
+	    sigval_t si_value;	/* Signal value.  */
+	  };
 
-	/* POSIX.1b timers.  */
+	/* POSIX.1b timers.  'si_sigval' (above) is also valid.  */
 	struct
 	  {
-	    int si_tid;		/* Timer ID.  */
+	    int si_timerid;	/* Timer ID.  */
 	    int si_overrun;	/* Overrun count.  */
-	    sigval_t si_sigval;	/* Signal value.  */
-	  } _timer;
+	  };
 
-	/* POSIX.1b signals.  */
+	/* SIGCHLD.  The first two fields overlay the si_pid and si_uid
+	   fields above.  */
 	struct
 	  {
-	    __pid_t si_pid;	/* Sending process ID.  */
-	    __uid_t si_uid;	/* Real user ID of sending process.  */
-	    sigval_t si_sigval;	/* Signal value.  */
-	  } _rt;
-
-	/* SIGCHLD.  */
-	struct
-	  {
-	    __pid_t si_pid;	/* Which child.	 */
-	    __uid_t si_uid;	/* Real user ID of sending process.  */
+	    __pid_t __sigchld_si_pid;	/* Which child.	 */
+	    __uid_t __sigchld_si_uid;	/* Real user ID of sending process.  */
 	    int si_status;	/* Exit value or signal.  */
 	    __SI_CLOCK_T si_utime;
 	    __SI_CLOCK_T si_stime;
-	  } _sigchld;
+	  };
 
 	/* SIGILL, SIGFPE, SIGSEGV, SIGBUS.  */
 	struct
@@ -96,56 +90,42 @@  typedef struct
 		/* used when si_code=SEGV_BNDERR */
 		struct
 		  {
-		    void *_lower;
-		    void *_upper;
-		  } _addr_bnd;
+		    void *si_lower;
+		    void *si_upper;
+		  };
 		/* used when si_code=SEGV_PKUERR */
-		__uint32_t _pkey;
-	      } _bounds;
-	  } _sigfault;
+		__uint32_t si_pkey;
+	      };
+	  };
 
 	/* SIGPOLL.  */
 	struct
 	  {
 	    long int si_band;	/* Band event for SIGPOLL.  */
 	    int si_fd;
-	  } _sigpoll;
+	  };
 
 	/* SIGSYS.  */
 #if __SI_HAVE_SIGSYS
 	struct
 	  {
-	    void *_call_addr;	/* Calling user insn.  */
-	    int _syscall;	/* Triggering system call number.  */
-	    unsigned int _arch; /* AUDIT_ARCH_* of syscall.  */
-	  } _sigsys;
+	    void *si_call_addr;	  /* Calling user insn.  */
+	    int si_syscall;	  /* Triggering system call number.  */
+	    unsigned int si_arch; /* AUDIT_ARCH_* of syscall.  */
+	  };
 #endif
-      } _sifields;
+      };
   } siginfo_t __SI_ALIGNMENT;
 
 
-/* X/Open requires some more fields with fixed names.  */
-#define si_pid		_sifields._kill.si_pid
-#define si_uid		_sifields._kill.si_uid
-#define si_timerid	_sifields._timer.si_tid
-#define si_overrun	_sifields._timer.si_overrun
-#define si_status	_sifields._sigchld.si_status
-#define si_utime	_sifields._sigchld.si_utime
-#define si_stime	_sifields._sigchld.si_stime
-#define si_value	_sifields._rt.si_sigval
-#define si_int		_sifields._rt.si_sigval.sival_int
-#define si_ptr		_sifields._rt.si_sigval.sival_ptr
-#define si_addr		_sifields._sigfault.si_addr
-#define si_addr_lsb	_sifields._sigfault.si_addr_lsb
-#define si_lower	_sifields._sigfault._bounds._addr_bnd._lower
-#define si_upper	_sifields._sigfault._bounds._addr_bnd._upper
-#define si_pkey		_sifields._sigfault._bounds._pkey
-#define si_band		_sifields._sigpoll.si_band
-#define si_fd		_sifields._sigpoll.si_fd
-#if __SI_HAVE_SIGSYS
-# define si_call_addr	_sifields._sigsys._call_addr
-# define si_syscall	_sifields._sigsys._syscall
-# define si_arch	_sifields._sigsys._arch
+/* These field aliases are not in POSIX, and are preserved for
+   backward compatibility only.  They may be removed in a future
+   release.  */
+#ifdef __USE_MISC
+#define si_int		si_value.sival_int \
+  __glibc_macro_warning("si_int is deprecated, use si_value.sival_int instead")
+#define si_ptr		si_value.sival_ptr \
+  __glibc_macro_warning("si_ptr is deprecated, use si_value.sival_ptr instead")
 #endif
 
 #endif
diff --git a/sysdeps/unix/sysv/linux/ia64/bits/siginfo-arch.h b/sysdeps/unix/sysv/linux/ia64/bits/siginfo-arch.h
index 8b5647062c..a2144c3771 100644
--- a/sysdeps/unix/sysv/linux/ia64/bits/siginfo-arch.h
+++ b/sysdeps/unix/sysv/linux/ia64/bits/siginfo-arch.h
@@ -3,15 +3,16 @@ 
 
 #define __SI_HAVE_SIGSYS 0
 
-#define __SI_SIGFAULT_ADDL			\
-  int _si_imm;					\
-  unsigned int _si_flags;			\
-  unsigned long int _si_isr;
-
 #ifdef __USE_GNU
-# define si_imm		_sifields._sigfault._si_imm
-# define si_segvflags	_sifields._sigfault._si_flags
-# define si_isr		_sifields._sigfault._si_isr
+# define __SI_SIGFAULT_ADDL			\
+  int si_imm;					\
+  unsigned int si_segvflags;			\
+  unsigned long int si_isr;
+#else
+# define __SI_SIGFAULT_ADDL			\
+  int __si_imm;					\
+  unsigned int __si_segvflags;			\
+  unsigned long int __si_isr;
 #endif
 
 #endif
diff --git a/sysdeps/unix/sysv/linux/sparc/bits/siginfo-arch.h b/sysdeps/unix/sysv/linux/sparc/bits/siginfo-arch.h
index 9f79715ebe..146fa2c00a 100644
--- a/sysdeps/unix/sysv/linux/sparc/bits/siginfo-arch.h
+++ b/sysdeps/unix/sysv/linux/sparc/bits/siginfo-arch.h
@@ -4,9 +4,10 @@ 
 
 #define __SI_BAND_TYPE int
 
-#define __SI_SIGFAULT_ADDL \
-  int _si_trapno;
-
-#define si_trapno	_sifields._sigfault._si_trapno
+#ifdef __USE_GNU
+# define __SI_SIGFAULT_ADDL  int si_trapno;
+#else
+# define __SI_SIGFAULT_ADDL  int __si_trapno;
+#endif
 
 #endif
diff --git a/sysdeps/unix/sysv/linux/tile/bits/siginfo-arch.h b/sysdeps/unix/sysv/linux/tile/bits/siginfo-arch.h
index 7d0c24c84b..0421fa3585 100644
--- a/sysdeps/unix/sysv/linux/tile/bits/siginfo-arch.h
+++ b/sysdeps/unix/sysv/linux/tile/bits/siginfo-arch.h
@@ -2,9 +2,10 @@ 
 #ifndef _BITS_SIGINFO_ARCH_H
 #define _BITS_SIGINFO_ARCH_H 1
 
-#define __SI_SIGFAULT_ADDL \
-  int _si_trapno;
-
-#define si_trapno	_sifields._sigfault._si_trapno
+#ifdef __USE_GNU
+# define __SI_SIGFAULT_ADDL  int si_trapno;
+#else
+# define __SI_SIGFAULT_ADDL  int __si_trapno;
+#endif
 
 #endif
diff --git a/sysdeps/unix/sysv/linux/timer_routines.c b/sysdeps/unix/sysv/linux/timer_routines.c
index 1d81304678..696ab450c6 100644
--- a/sysdeps/unix/sysv/linux/timer_routines.c
+++ b/sysdeps/unix/sysv/linux/timer_routines.c
@@ -92,7 +92,7 @@  timer_helper_thread (void *arg)
 	{
 	  if (si.si_code == SI_TIMER)
 	    {
-	      struct timer *tk = (struct timer *) si.si_ptr;
+	      struct timer *tk = (struct timer *) si.si_value.sival_ptr;
 
 	      /* Check the timer is still used and will not go away
 		 while we are reading the values here.  */
diff --git a/sysdeps/unix/sysv/linux/tst-getpid1.c b/sysdeps/unix/sysv/linux/tst-getpid1.c
index 253ebf2e15..f24faf35e1 100644
--- a/sysdeps/unix/sysv/linux/tst-getpid1.c
+++ b/sysdeps/unix/sysv/linux/tst-getpid1.c
@@ -95,9 +95,10 @@  do_test (void)
       return 1;
     }
 
-  if (si.si_int != (int) p)
+  if (si.si_value.sival_int != (int) p)
     {
-      printf ("expected PID %d, got si_int %d\n", (int) p, si.si_int);
+      printf ("expected PID %d, got si_int %d\n",
+	      (int) p, si.si_value.sival_int);
       kill (p, SIGKILL);
       return 1;
     }