In message <20180323150709.h...@besplex.bde.org>, Bruce Evans writes: > 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.
Agreed on alphabetic sorting. > > 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. That's not the case here. sys/types.h is not included in this file but point taken. > > .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. The reason is amd64 doesn't use critical_enter() and critical_exit() because counter_enter() and counter_exit() are NOPs. The reason they are NOPs in amd64 and not in i386 is not all i386 processors support cmpxchg8b. It is only then that the critical_*() functions are called. > > Bruce I can create a phabricator revision to clean this instance up and move sys/systm.h just after sys/param.h. I'm just about to head out of town so I'll create it after I get back, after April 4. Thank you for your input Bruce. -- Cheers, Cy Schubert <cy.schub...@cschubert.com> FreeBSD UNIX: <c...@freebsd.org> Web: http://www.FreeBSD.org The need of the many outweighs the greed of the few. _______________________________________________ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"