Date: Fri, 10 Nov 2017 15:47:44 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Conrad Meyer <cem@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r325625 - head/sys/sys Message-ID: <20171110144148.C1160@besplex.bde.org> In-Reply-To: <201711100200.vAA20epD011382@repo.freebsd.org> References: <201711100200.vAA20epD011382@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <sys/cdefs.h>
> #include <machine/atomic.h>
> #include <machine/cpufunc.h>
> #include <sys/callout.h>
> -#include <sys/cdefs.h>
> #include <sys/queue.h>
> #include <sys/stdint.h> /* 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, <sys/types.h>
or <sys/param.h> will be needed before any others. <sys/param.h>
includes <sys/types.h>. Do not include both. Many headers, including
<sys/types.h>, include <sys/cdefs.h>, 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20171110144148.C1160>