From patchwork Fri Aug 11 14:02:57 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Zack Weinberg X-Patchwork-Id: 22081 Received: (qmail 26383 invoked by alias); 11 Aug 2017 14:03:02 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 25268 invoked by uid 89); 11 Aug 2017 14:03:01 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.6 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RP_MATCHES_RCVD, SPF_PASS autolearn=ham version=3.3.2 spammy=nis, band, si_code, sigchld X-HELO: mailbackend.panix.com From: Zack Weinberg To: libc-alpha@sourceware.org Cc: joseph@codesourcery.com Subject: [RFC PATCH] Use anonymous union for siginfo_t Date: Fri, 11 Aug 2017 10:02:57 -0400 Message-Id: <20170811140257.19664-1-zackw@panix.com> MIME-Version: 1.0 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(-) 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 . +* 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; }