From owner-svn-src-all@freebsd.org Fri Feb 12 14:36:44 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 7404AAA6DF5; Fri, 12 Feb 2016 14:36:44 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id CE5B91B11; Fri, 12 Feb 2016 14:36:43 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from tom.home (kostik@localhost [127.0.0.1]) by kib.kiev.ua (8.15.2/8.15.2) with ESMTPS id u1CEaUJx036624 (version=TLSv1 cipher=DHE-RSA-CAMELLIA256-SHA bits=256 verify=NO); Fri, 12 Feb 2016 16:36:30 +0200 (EET) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.10.3 kib.kiev.ua u1CEaUJx036624 Received: (from kostik@localhost) by tom.home (8.15.2/8.15.2/Submit) id u1CEaUP8036623; Fri, 12 Feb 2016 16:36:30 +0200 (EET) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Fri, 12 Feb 2016 16:36:30 +0200 From: Konstantin Belousov To: Bruce Evans 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> References: <201602120738.u1C7cKpq093956@repo.freebsd.org> <20160212232717.P894@besplex.bde.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160212232717.P894@besplex.bde.org> User-Agent: Mutt/1.5.24 (2015-08-30) X-Spam-Status: No, score=-2.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.1 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on tom.home 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:36:44 -0000 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 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_*. 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 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. 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 (== ) 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 > > -#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); > 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.