svn commit: r295561 - in head: include sys/mips/include sys/powerpc/include sys/sparc64/include sys/sys sys/x86/include

Fri Feb 12 14:09:06 UTC 2016

On Fri, 12 Feb 2016, Konstantin Belousov wrote:

> Log:
>  POSIX states that #include <signal.h> shall make both mcontext_t and
>  ucontext_t available.  Our code even has XXX comment about this.

Only newer versions of POSIX have this bug.  In the 2001 version, the
bug was limited to the XSI section.  <ucontext.h> was limited to XSI
too.  Now, <ucontext.h> specified to be obsolescent, but it is further
gone that that -- web searches find only old versions and its functions
seem to be only specified without prototyps in the "Removed" section.

The POSIX bug also reserves uc_* in <signal.h>.  This is needed for
exposing ucontext_t.  POSIX has the bug that ucontext_t must be a
struct to work, but is declared as a typedef.  Structs cannot be very
opaque since they expose struct members...

>  Add a bit of compliance by moving struct __ucontext definition into
>  sys/_ucontext.h and including it into signal.h and sys/ucontext.h.
>
>  Several machine/ucontext.h headers were changed to use namespace-safe
>  types (like uint64_t->__uint64_t) to not depend on sys/types.h.
>  struct __stack_t from sys/signal.h is made always visible in private
>  namespace to satisfy sys/_ucontext.h requirements.
>
>  Apparently mips _types.h pollutes global namespace with f_register_t
>  type definition.  This commit does not try to fix the issue.

However, mcontext_t is opaque, and mc_* is not reserved even in
<ucontext.h>.  In FreeBSD, mcontext_t is implemented as a struct so the
names of its struct members now pollute even <signal.h>.  It is a
reasonable fix to abuse the reserved uc_ prefix in mcontext_t.
mcontext_t is just the MD part of ucontext_t.  Prefixes like uc_mc_
would express its nesting but are too long.

> Modified: head/include/signal.h
> ==============================================================================
> --- head/include/signal.h	Fri Feb 12 07:27:24 2016	(r295560)
> +++ head/include/signal.h	Fri Feb 12 07:38:19 2016	(r295561)
> @@ -36,6 +36,8 @@
> #include <sys/cdefs.h>
> #include <sys/_types.h>
> #include <sys/signal.h>
> +#include <machine/ucontext.h>
> +#include <sys/_ucontext.h>

This pollutes even the non-POSIX case with POSIX and FreeBSD names.

The correct ifdefs for this are messy.  No names should be added for
non-POSIX and old versions of POSIX.  2001 POSIX needs an XSI ifdef.  Later
versions of POSIX don't need an ifdef.  Normally it is better to hide
the ifdefs in the included headers, but _ucontext.h should always
provide ucontext_t and uc_*.

> Modified: head/sys/mips/include/ucontext.h
> ==============================================================================
> --- head/sys/mips/include/ucontext.h	Fri Feb 12 07:27:24 2016	(r295560)
> +++ head/sys/mips/include/ucontext.h	Fri Feb 12 07:38:19 2016	(r295561)
> @@ -50,13 +50,13 @@ typedef struct	__mcontext {
> 	 * struct sigcontext and ucontext_t at the same time.
> 	 */
> 	int		mc_onstack;	/* sigstack state to restore */
> -	register_t	mc_pc;		/* pc at time of signal */
> -	register_t	mc_regs[32];	/* processor regs 0 to 31 */
> -	register_t	sr;		/* status register */
> -	register_t	mullo, mulhi;	/* mullo and mulhi registers... */
> +	__register_t	mc_pc;		/* pc at time of signal */
> +	__register_t	mc_regs[32];	/* processor regs 0 to 31 */
> +	__register_t	sr;		/* status register */
> +	__register_t	mullo, mulhi;	/* mullo and mulhi registers... */
> 	int		mc_fpused;	/* fp has been used */
> 	f_register_t	mc_fpregs[33];	/* fp regs 0 to 31 and csr */
> -	register_t	mc_fpc_eir;	/* fp exception instruction reg */
> +	__register_t	mc_fpc_eir;	/* fp exception instruction reg */
> 	void		*mc_tls;	/* pointer to TLS area */
> 	int		__spare__[8];	/* XXX reserved */
> } mcontext_t;
>

These mc_* names always polluted <ucontext.h> but no one cared since it
was an XSI extension.

sr, mullo and mulhi have worse names.  These names don't even use the
same style as the others.  They now pollute <signal.h> of course.

__spare__ is bogusly named and has a banal comment.  The names of spares
should be in the normal (reserved) namespace for the struct.  No namespace
is reserved here, bug mc_spare would be no worse than the other mc_'s.

i386 was missing all of these bugs except the mc_* pollution.

> Copied and modified: head/sys/sys/_ucontext.h (from r295560, head/sys/sys/ucontext.h)

sys/ucontext.h (== <POSIX <ucontext.h>) remains polluted.  I point out its
old bugs here since most of it is all quoted.

> ==============================================================================
> --- head/sys/sys/ucontext.h	Fri Feb 12 07:27:24 2016	(r295560, copy source)
> +++ head/sys/sys/_ucontext.h	Fri Feb 12 07:38:19 2016	(r295561)
> @@ -28,11 +28,8 @@
>  * $FreeBSD$
>  */
>
> -#ifndef _SYS_UCONTEXT_H_
> -#define	_SYS_UCONTEXT_H_
> -
> -#include <sys/signal.h>
> -#include <machine/ucontext.h>
> +#ifndef _SYS__UCONTEXT_H_
> +#define	_SYS__UCONTEXT_H_
>
> typedef struct __ucontext {
> 	/*
> @@ -43,64 +40,13 @@ typedef struct __ucontext {
> 	 * support them both at the same time.
> 	 * note: the union is not defined, though.
> 	 */
> -	sigset_t	uc_sigmask;
> +	__sigset_t	uc_sigmask;
> 	mcontext_t	uc_mcontext;
>
> 	struct __ucontext *uc_link;
> -	stack_t		uc_stack;
> +	struct __stack_t uc_stack;
> 	int		uc_flags;
> -#define	UCF_SWAPPED	0x00000001	/* Used by swapcontext(3). */

UCF is in the application namespace.  _UC doesn't seem to be used.
mcontext.h is carfeful to use _MC.

> 	int		__spare__[4];

Bogus name.  uc_spare is in the reserved namespace.

> } ucontext_t;

> [... names under _KERNEL not a problem]

> -#ifndef _KERNEL
> -
> -__BEGIN_DECLS
> -
> -int	getcontext(ucontext_t *) __returns_twice;
> -ucontext_t *getcontextx(void);

getcontextx isn't in old versions of POSIX.

> -int	setcontext(const ucontext_t *);
> -void	makecontext(ucontext_t *, void (*)(void), int, ...);
> -int	signalcontext(ucontext_t *, int, __sighandler_t *);

signalcontext isn't in old versions of POSIX.

> -int	swapcontext(ucontext_t *, const ucontext_t *);

None of these functions are in the current version of POSIX.  They
are all pollution except for XSI between about 2001 and 2008.

> -
> -#if __BSD_VISIBLE
> -int __getcontextx_size(void);
> -int __fillcontextx(char *ctx) __returns_twice;
> -int __fillcontextx2(char *ctx);

ctx is in the application namespace.

> Modified: head/sys/sys/signal.h
> ==============================================================================
> --- head/sys/sys/signal.h	Fri Feb 12 07:27:24 2016	(r295560)
> +++ head/sys/sys/signal.h	Fri Feb 12 07:38:19 2016	(r295561)
> @@ -354,18 +354,10 @@ typedef	void __siginfohandler_t(int, str
> #endif
>
> #if __XSI_VISIBLE
> -/*
> - * Structure used in sigaltstack call.
> - */
> #if __BSD_VISIBLE
> -typedef	struct sigaltstack {
> -#else
> -typedef	struct {
> +#define	__stack_t sigaltstack
> #endif
> -	void	*ss_sp;			/* signal stack base */
> -	__size_t ss_size;		/* signal stack length */
> -	int	ss_flags;		/* SS_DISABLE and/or SS_ONSTACK */
> -} stack_t;
> +typedef	struct __stack_t stack_t;

This seems to have been broken even in the 2001 version.  stack_t is
declared unconditionally even there.  ss_* isn't reserved but the 3 ss_*
names are specified.

This should be underer a 2001+ POSIX ifdef, not XSI.

>
> #define	SS_ONSTACK	0x0001	/* take signal on alternate stack */
> #define	SS_DISABLE	0x0004	/* disable taking signals on alternate stack */
> @@ -373,6 +365,17 @@ typedef	struct {
> #define	SIGSTKSZ	(MINSIGSTKSZ + 32768)	/* recommended stack size */
> #endif

The XSI ifdef seems to be correct for sigalttack, SS_* and *SIGSTKSZ.

>
> +/*
> + * Structure used in sigaltstack call.  Its definition is always
> + * needed for __ucontext.  If __BSD_VISIBLE is defined, the structure
> + * tag is actually sigaltstack.
> + */
> +struct __stack_t {
> +	void	*ss_sp;			/* signal stack base */
> +	__size_t ss_size;		/* signal stack length */
> +	int	ss_flags;		/* SS_DISABLE and/or SS_ONSTACK */
> +};

This is now broken since it is outside of all ifdefs.  Its ss_* names are
in the application namespace for the non-POSIX case and POSIX before about
2001.

> Modified: head/sys/x86/include/ucontext.h
> ==============================================================================
> --- head/sys/x86/include/ucontext.h	Fri Feb 12 07:27:24 2016	(r295560)
> +++ head/sys/x86/include/ucontext.h	Fri Feb 12 07:38:19 2016	(r295561)
> @@ -162,4 +162,9 @@ typedef struct __mcontext {
> } mcontext_t;
> #endif /* __amd64__ */
>
> +#ifdef __LINT__
> +typedef struct __mcontext {
> +} mcontext_t;
> +#endif /* __LINT__ */
> +
> #endif /* !_X86_UCONTEXT_H_ */

Aren't you going to remove lint? :-)

This looks just broken at first.  It gives a a second declaration of
the struct and if it is the only declaration then lint should still warn 
even more than compilers since an empty struct declaration is invalid in
C (compilers need -pedantic to resemble C compilers, but lint should be
strict by default).  Then I rememebered that one of the lint bugs is that
it uses -Wundef so __amd64__ and __i386__ are not defined so this is the
only declaration for lint.

I don't like the combined headers for x86, and this file is an especially
good bad example.  It consists of an __amd64__ ifdef and a __i386__ ifdef
with nothing shared outside except the copyright notice and the
idempotency ifdef.  This gives the lint bug.

Bruce