Date: Fri, 12 Feb 2016 16:36:30 +0200 From: Konstantin Belousov <kostikbel@gmail.com> To: Bruce Evans <brde@optusnet.com.au> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r295561 - in head: include sys/mips/include sys/powerpc/include sys/sparc64/include sys/sys sys/x86/include Message-ID: <20160212143630.GS91220@kib.kiev.ua> In-Reply-To: <20160212232717.P894@besplex.bde.org> References: <201602120738.u1C7cKpq093956@repo.freebsd.org> <20160212232717.P894@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160212143630.GS91220>