svn commit: r331369 - head/sys/vm

Bruce Evans brde at optusnet.com.au
Fri Mar 23 04:38:14 UTC 2018


On Thu, 22 Mar 2018, Jeff Roberson wrote:

> On Thu, 22 Mar 2018, Cy Schubert wrote:
>
>> It broke i386 too.
>
> I just did
> TARGET_ARCH=i386 make buildworld
> TARGET_ARCH=i386 make buildkernel
>
> This worked for me?
>> 
>> Index: sys/vm/vm_reserv.c
>> ===================================================================
>> --- sys/vm/vm_reserv.c	(revision 331399)
>> +++ sys/vm/vm_reserv.c	(working copy)
>> @@ -45,8 +45,6 @@
>> 
>> #include <sys/param.h>
>> #include <sys/kernel.h>
>> -#include <sys/counter.h>
>> -#include <sys/ktr.h>
>> #include <sys/lock.h>
>> #include <sys/malloc.h>
>> #include <sys/mutex.h>
>> @@ -55,6 +53,8 @@
>> #include <sys/sbuf.h>
>> #include <sys/sysctl.h>
>> #include <sys/systm.h>
>> +#include <sys/counter.h>
>> +#include <sys/ktr.h>
>> #include <sys/vmmeter.h>
>> #include <sys/smp.h>
>> 
>> This is because sys/i386/include/machine.h uses critical_enter() and
>> critical_exit() which are defined in sys/systm.h.

Wrong fix.  I see you committed this.  Now there are more bugs to fix.

<sys/systm.h> is a prerequisite for all kernel headers except
<sys/param.h>, since it defines and declares things like KASSERT() and
critical_enter() which might be used in other headers (except
sys/param.h and its standard pollution).  Sometimes sys/systm.h is
included as undocumented namespace pollution in headers that are
accidentally included before the (other) ones that use KASSERT(), etc.
The headers that have this bug have it to work around bugs in .c files
like the one above.  It is more usual to have this bug by not including
sys/systm.h at all than to have it by including it in a wrong order.
Sorting it alphabetically almost always gives a wrong order.  It must
be included after sys/param.h and that must be included first.

It is a related bug to include only sys/types.h and not sys/param.h.
This requires chumminess with the current implementation and all
future implementations.  sys/param.h provides certain undocumented
but standard namespace pollution which might vary with the implementation,
as necessary to satisfy some of the pollution requirements of all current
and future implementations of other headers.  (The pollution should be
monotonically decreasing but it was only that for a few years about 20
years ago when I worked on fixing it.)  .c files that include sys/types.h
instead of sys/param.h have do some subset of the includes in sys/param.h.
Since nothing is documented and the subset might depend on the arch and
user options, it is hard to know the minimal subset.

.c files that include sys/types.h tend to have lots of other #include
bugs like not including sys/systm.h.  Again it is hard to know the
minimal replacement for sys/systm.h and its undocumented but standard
pollution.  It is a style bug to include both sys/types.h and sys/param.h.
style(9) even explicitly forbids including both.  It is a larger style
bug to include the standard pollution in sys/systm.h direction.  This
includes especially <machine/atomic.h> and <machine/cpufunc.h>.  These
should be considered as being implemented in sys/systm.h, with the
<machine> headers for them only and implementation detail.  Similarly
for <sys/libkern.h>.

>> It built nicely on my amd64's though.

amd64 apparently has more namespace pollution which breaks detection
of the bug.  But I couldn't find where it is.  sys/systm.h isn't included
nested in any amd64 or x86 headers.  Apparently some amd64 option gives
it.

Bruce


More information about the svn-src-head mailing list