Re: [PATCH 04/10] Convert signal.h from __need macros to bits/types/ headers.

[Joseph Myers <joseph at codesourcery dot com>]

On Tue, 9 May 2017, Zack Weinberg wrote:

> Internal code that wants to manipulate signal masks must now include
> <bits/sigsetops.h> (which is not installed) and should be aware that

As noted, should be renamed to something not in the bits/ namespace for 
installed headers.  (signal/sigsetops.h already exists, so that shouldn't 
be the name.)

> +/* These macros needn't check for a bogus signal number;
> +   checking is done in the non-__ versions.  */
> +# define __sigismember(set, sig)		\
> +  (__extension__ ({				\
> +    __sigset_t __mask = __sigmask (sig);	\
> +    (set) & mask ? 1 : 0;			\

You're referencing plain "mask" here, when the local variable is __mask.

> @@ -112,7 +113,8 @@ rcmd_af (char **ahost, u_short rport, const char *locuser, const char *remuser,
>  		struct sockaddr_in6 sin6;
>  	} from;
>  	struct pollfd pfd[2];
> -	int32_t oldmask;
> +        sigset_t mask, omask;
> +

Indentation messed up here (was using tabs, changed to spaces).  Likewise 
elsewhere in this patch).

> diff --git a/signal/sigsetops.c b/signal/sigsetops.c
> index 0317662a14..78bd41729d 100644
> --- a/signal/sigsetops.c
> +++ b/signal/sigsetops.c
> @@ -1,11 +1,53 @@
> -/* Define the real-function versions of all inline functions
> -   defined in signal.h (or bits/sigset.h).  */
> +/* Compatibility symbols for old versions of signal.h.
> +   Copyright (C) 2016 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.

Include 2017 in copyright dates.

> +/* These were formerly defined by <signal.h> as inline functions, so
> +   they require out-of-line compatibility definitions.  */
> +#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_25)

Should be GLIBC_2_26, as the version where these were obsoleted.  (As 
there weren't any new ports in 2.25, having the wrong version here doesn't 
actually break any ABIs.)

> diff --git a/sysdeps/unix/sysv/linux/bits/types/siginfo_t.h b/sysdeps/unix/sysv/linux/bits/types/siginfo_t.h

> +	/* SIGSYS.  */
> +	struct
> +	  {
> +	    void *_call_addr;	/* Calling user insn.  */
> +	    int _syscall;	/* Triggering system call number.  */
> +	    unsigned int _arch; /* AUDIT_ARCH_* of syscall.  */
> +	  } _sigsys;

This (and the macros using it) needs to be conditional.  ia64 (only) lacks 
this part of siginfo_t.

> +/* Additional fields for _sifields._sigfault.  */
> +#define __SI_SIGFAULT_ADDL_2			\
> +  struct					\
> +     {						\
> +       void *_lower;				\
> +       void *_upper;				\
> +     } si_addr_bnd;

Remark (not needing fixing for the patch to go in, though if fixed there 
would be no need for __SI_SIGFAULT_ADDL_2): this is in fact part of 
siginfo_t in the Linux kernel for all architectures rather than just 
x86-specific (as is the more recent _pkey there), so logically should 
probably be so in glibc as well, though I don't know if it's *used* by the 
kernel on any other architectures or if it's entirely MPX-specific.

OK with the fixes noted.

-- 
Joseph S. Myers
joseph@codesourcery.com