In message <201803230615.w2n6ftmj040...@slippy.cwsent.com>, Cy Schubert writes: > 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.
Hi Bruce, Can you please give this a once over? diff --git a/sys/vm/vm_reserv.c b/sys/vm/vm_reserv.c index d8869e3bdbe..6d31d79da39 100644 --- a/sys/vm/vm_reserv.c +++ b/sys/vm/vm_reserv.c @@ -44,6 +44,7 @@ __FBSDID("$FreeBSD$"); #include "opt_vm.h" #include <sys/param.h> +#include <sys/systm.h> #include <sys/kernel.h> #include <sys/lock.h> #include <sys/malloc.h> @@ -52,7 +53,6 @@ __FBSDID("$FreeBSD$"); #include <sys/rwlock.h> #include <sys/sbuf.h> #include <sys/sysctl.h> -#include <sys/systm.h> #include <sys/counter.h> #include <sys/ktr.h> #include <sys/vmmeter.h> -- 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. _______________________________________________ email@example.com mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"