From owner-svn-src-head@freebsd.org Fri Nov 10 04:47:54 2017 Return-Path: Delivered-To: svn-src-head@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 12A5AE64EA0; Fri, 10 Nov 2017 04:47:54 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail108.syd.optusnet.com.au (mail108.syd.optusnet.com.au [211.29.132.59]) by mx1.freebsd.org (Postfix) with ESMTP id A57DE65860; Fri, 10 Nov 2017 04:47:52 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.104] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail108.syd.optusnet.com.au (Postfix) with ESMTPS id C2CA31A0A52; Fri, 10 Nov 2017 15:47:44 +1100 (AEDT) Date: Fri, 10 Nov 2017 15:47:44 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Conrad Meyer cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r325625 - head/sys/sys In-Reply-To: <201711100200.vAA20epD011382@repo.freebsd.org> Message-ID: <20171110144148.C1160@besplex.bde.org> References: <201711100200.vAA20epD011382@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.2 cv=bc8baKHB c=1 sm=1 tr=0 a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=kj9zAlcOel0A:10 a=HBzeS8a9Jooz5TqGV3gA:9 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 10 Nov 2017 04:47:54 -0000 On Fri, 10 Nov 2017, Conrad Meyer wrote: > Log: > systm.h: Include cdefs.h first > > Ever since r143063, machine/atomic.h requires cdefs.h. So, include it > first. Weak support: style(9) tells us to include cdefs.h first. This just moves a style bug. The existing include of sys/cdefs.h was bogus. machine/atomic.h uses lots of typedefed types, and is not so polluted that it includes sys/types.h, so it has sys/param.h as a prerequisite (not sys/types.h since that would be chummy with the implementation -- perhaps sys/types.h is enough for machine/atomic.h, but sys/systm.h includes the kitchen sink (but not the kitchen sys/param.h), so it is unclear if sys/types.h us enough for it. sys/param.h (and even sys/types.h) includes sys/cdefs.h, and it is a style bug to not depend on this. Rather, it is a style bug to not include both sys/param.h and sys/systm.h in .c files in the kernel, in that order and before any other includes, or to not depend on the standard pollution of them including the kitchen and its sink. In particular, it is a style bug to include machine/atomic.h directly -- this is part of the sink. sys/systm.h defines macros like KASSERT() which may be used in any other header, so all kernel .c files have sys/systm.h as a potential prerequisite, and the implementation of this is to require explicit includes of sys/systm.h and not depend on .h files including it as nonstandard pollution. Some .h files do have this pollution. Some are so bad that they even include sys/param.h too. r143063 added lots of ifdefs using feature test macros defined in cdefs.h, and also some ifdefs that sys/cdefs.h is included before it is used. This was a good idea at the time, but was badly implemented and is worse now. The test for sys/cdefs.h being included before machine/atomic.h is now only done for amd64, i386, mips and powerpc. The feature tests are rarely done outside of i386. Even amd64 mostly doesn't bother with them. On i386, they are mostly for gnu asm (especially in i386/include/atomic.h) and for gcc compiler features that are not in Intel cc. Gnu asm must now be considered standard, so tests for it are vacuously true. Support for Intel cc has rotted. clang strictly needs many more feature tests, but it is still too close to gcc so results of old tests are vacuously the same as for gcc, and there are no new tests. > Argument against: since code that includes systm.h still compiles, > compilation units that include systm.h must already include cdefs.h. So, an Yes, they must include it and much more -- see above. Just using __FBSDID() requires including sys/cdefs.h before, so the requirement of including sys/cdefs.h is automatically satisfied in .c files that don't have the style bug of not using __FBSDID() near the beginning of the file before other includes. But that is not enough for sys/systm.h to be self- sufficient. > argument could be made that the cdefs.h include could just be removed > entirely. That is maybe a bigger change and not one I am interested in > bikeshedding. It is just the correct change... > Modified: head/sys/sys/systm.h > ============================================================================== > --- head/sys/sys/systm.h Fri Nov 10 01:17:26 2017 (r325624) > +++ head/sys/sys/systm.h Fri Nov 10 02:00:40 2017 (r325625) > @@ -38,10 +38,10 @@ > #ifndef _SYS_SYSTM_H_ > #define _SYS_SYSTM_H_ > > +#include > #include > #include > #include > -#include > #include > #include /* for people using printf mainly */ The old include was a style bug in r120610. sys/queue.h is the last place that needs an include of sys/cdefs.h before it, since it has always been documented to be self-sufficient. In 4.4BSD-Lite2, it doesn't include sys/cdefs.h. I don't know if it should have or if FreeBSD grew a dependency on it. The dependency was fixed in r99594 by including sys/cdefs.h in sys/queue.h. r120610 added a use of __predict_false() in KASSERT() in sys/systm.h, and added the above old include in a nonsense place. Adding it before all other includes wouldn't be nonsense. It is just a style bug. My version of style(9) says: Kernel include files (i.e., sys/*.h) come first; normally, or will be needed before any others. includes . Do not include both. Many headers, including , include , and it is OK to depend on that. This doesn't say that it is a style bug to not depend on the other headers including sys/cdefs.h. I actually don't like including sys/cdefs at all. Even or especially for __FBSDID() -- this alone makes most FreeBSD sources unportable almost as well as using __P(()) used to.. __FBSDID() is easy to emulate, but not so if more or subtler macros in it are used. Kernel sources are inherently unportable, but using sys/cdefs.h in the implementation is essential and it is a style bug for the implementation to not understand itself and include sys/cdefs.h in every file. Only headers designed to be self-sufficient should include it. Bruce