From owner-svn-src-all@freebsd.org Fri Feb 12 14:09:06 2016 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id B6025AA6066; Fri, 12 Feb 2016 14:09:06 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail104.syd.optusnet.com.au (mail104.syd.optusnet.com.au [211.29.132.246]) by mx1.freebsd.org (Postfix) with ESMTP id 61FAD65B; Fri, 12 Feb 2016 14:09:06 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from c110-21-41-193.carlnfd1.nsw.optusnet.com.au (c110-21-41-193.carlnfd1.nsw.optusnet.com.au [110.21.41.193]) by mail104.syd.optusnet.com.au (Postfix) with ESMTPS id 31F2C427A38; Sat, 13 Feb 2016 01:08:57 +1100 (AEDT) Date: Sat, 13 Feb 2016 01:08:55 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Konstantin Belousov 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 In-Reply-To: <201602120738.u1C7cKpq093956@repo.freebsd.org> Message-ID: <20160212232717.P894@besplex.bde.org> References: <201602120738.u1C7cKpq093956@repo.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=PfoC/XVd c=1 sm=1 tr=0 a=73JWPhLeruqQCjN69UNZtQ==:117 a=L9H7d07YOLsA:10 a=9cW_t1CCXrUA:10 a=s5jvgZ67dGcA:10 a=kj9zAlcOel0A:10 a=kVPtpndFqo2daTyJ7XwA:9 a=KYDUXh1Pg-0WReiS:21 a=cs04WIf58lYJx0QY:21 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 12 Feb 2016 14:09:06 -0000 On Fri, 12 Feb 2016, Konstantin Belousov wrote: > Log: > POSIX states that #include 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. was limited to XSI too. Now, 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 . 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 . In FreeBSD, mcontext_t is implemented as a struct so the names of its struct members now pollute even . 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 > #include > #include > +#include > +#include 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 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 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 (== ) 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 > -#include > +#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