svn commit: r349459 - head/sys/sys

[Bruce Evans]

On Thu, 27 Jun 2019, Andriy Gapon wrote:

> On 27/06/2019 20:37, Bruce Evans wrote:
>> On Thu, 27 Jun 2019, Andriy Gapon wrote:
>>
>>> On 27/06/2019 18:47, Bruce Evans wrote:
>>>> On Thu, 27 Jun 2019, Andriy Gapon wrote:
>>>>
>>>>> Log:
>>>>> Â upgrade the warning printf-s in bus accessors to KASSERT-s
>>>>>
>>>>> Â After this change sys/bus.h includes sys/systm.h.
>>>>
>>>> This is further namespace pollution.  sys/systm.h is a prerequiste for
>>>> all kernel headers except sys/param.h and the headers that that includes,
>>>> since any kernel header (except sys/param.hm etc.) may have inlines which
>>>> use features in systm.h, e.g., KASSERT() or an inline function.
>>>
>>> what do you think about amending style(9) to require that if sys/systm.h is to
>>> be included, then it must be included before any other header except for
>>> sys/param.h (and possibly sys/cdefs.h) ?
>>
>> It is not a style matter.
>
> I know... but style(9) documents sys/param.h for similar reasons.
>
>> sys/systm.h is just a prerequisite for almost
>> all kernel code.  Perhaps this can be enforced using #ifdef magic even
>> for headers that don't currently use it.  If it is to be included nested,
>> then sys/param.h is the place to include it, but I don't like that since
>> it is even less related to parameters than sys/param.h, and this would be
>> a regression from minor depollution of sys/param.h.
>>
>> sys/systm.h is also kernel-only, and as you found, including it nested
>> tends to break applications, so other headers that have the bug of
>> including it tend to have _KERNEL ifdefs to avoid including it in
>> applications.  This is so fundamental that it doesn't have a "No
>> user-serviceable parts inside" ifdef.
>
> I think that there is a trivial fix to my commit: moving the sys/systm.h include
> to the _KERNEL section of sys/bus.h.  That's where its definitions are actually
> used.
> But I agree that there should be a better approach.

I did a quick check of how many .c files are missing includes of sys/systm.h.
In an old version of FreeBSD, my normal kernel has about 1100 object files,
and about 89 of these broke when a KASSERT() was added to sys/bus.h without
adding the pollution.  That is many more than I expected.

Many of these errors are in automatically generated bus if.c files.  E.g.,
the include list in acpi_if.c is:

#include <sys/param.h>
#include <sys/queue.h>
#include <sys/kernel.h>
#include <sys/kobj.h>
#include <sys/bus.h>
#include <sys/types.h>
#include <contrib/dev/acpica/include/acpi.h>
#include "acpi_if.h"

Here sys/bus.h and sys/types.h and the contrib include are in the .m file,
and the others are automatically generated.  Including sys/types.h is
nonsense since sys/param.h already included it and sys/bus.h already used
typedefs in it.  The generation also misorders sys/queue.h and mangles
the formatting of the contrib include (in the .m file, it is separated
by spaces).

Here the .m file should know sys/bus.h's prerequisites and add them all
except sys/param.h and sys/systm.h, or maybe the include of sys/bus.h
should be auto-generated too (my test kernel has 25 foo_if.c files and
21 of these include sys/bus.c).

Most of the generated if.c files include sys/systm.h by nested pollution
anyway.  Just not early enough to actually work.  E.g., in acpi_if.c,
the above 8 includes expand to 45 lines, 144 words and 2679 chars in
.depend.acpi_if.o according to wc.  The word count of 144 is
approximately the number of includes.  sys/systm.h is first included
deeply nested in contrib/..../acpi.h.

11 of the 25 generated foo_if.c end up never including sys/systm.h.

Of the remaining 64 files that fail to compile, all except 7 including
1 maintained by me (cy_pci.c) *blush* end up including sys/systm.h.
The other 57 apparently include it either misordered in the .c file
or via nested pollution.

After removing all nested includes of sys/systm.h in <sys> headers, only
another 50 object files fail to build.  most of the extras are for
libkern functions (20 for str*() alone).  libkern was polluted.h relatively
recently as part of unimproving the implementation of bcd conversion APIs
from macros to inlines with KASSERT()s.  The KASSERT()s were a bad fix for
missing sanity checks of bcd data in callers.

Bruce