[RFC] Use anonymous union for siginfo_t
Commit Message
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(-)
Comments
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.)
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.
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
@@ -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:
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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. */
@@ -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;
}