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

Fri Feb 12 14:36:44 UTC 2016

On Sat, Feb 13, 2016 at 01:08:55AM +1100, Bruce Evans wrote:
> 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_*.
Our non-POSIX namespace is strictly superset of the POSIX space.
I would not hide {m,u}context_t from the default visibility, and this
would defeat the goal of the change.

> 
> > 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.
The members names for the structures are private per the structure
namespace.  The names of the members cannot pollute signal.h.

> 
> > 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.

I am not sure what do you mean by 'quoted' (enough quotes ?). I
carefully left everything not related to the ucontext_t definition out
of sys/_ucontext.h. This way, there is no pollution of signal.h from
the symbols you list below as contaminating. Since ucontext.h is not in
POSIX, there is no compliance issues.

> 
> > ==============================================================================
> > --- 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);
> 
Nothing of the listed functions is in any POSIX header.

> 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.
It only consumes the __stack_t tag which is both in the implementation name
space (__ prefix) and in the POSIX reserved namespace (_t suffix).

> 
> > 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__ */
The x86 ucontext.h is very accurate in not conflicting with the application
namespace, all constants are defined in implementation-reserved namespace
(have _M prefix).  I did not verified other arches, I only noted that MIPS
has f_register_t bug.

> >
> > +#ifdef __LINT__
> > +typedef struct __mcontext {
> > +} mcontext_t;
> > +#endif /* __LINT__ */
> > +
> > #endif /* !_X86_UCONTEXT_H_ */
> 
> Aren't you going to remove lint? :-)
Yes, I am.  But I did not wanted to commit this in a way which would make
the lint removal a dependency of the commit.  Also, I want this change
to be mergeable to stable/10 (but I did not decided if I really want to
do the merge).

> 
> 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.
Yes, exactly the -undef thing is what triggered my understanding that
lint cannot be usefully used.

> 
> 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.

And I do like them.  More, I consider -m32 feature that was completed by
the merging, an essential feature of the modern OS on x86_64.