Date: Tue, 21 Feb 2017 13:29:06 -0800 From: Mark Johnston <markj@freebsd.org> To: Bruce Evans <brde@optusnet.com.au> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r313850 - in head/sys/cddl/dev/dtrace: amd64 i386 Message-ID: <20170221212906.GA46447@wkstn-mjohnston.west.isilon.com> In-Reply-To: <20170217160016.D1386@besplex.bde.org> References: <201702170327.v1H3RKK4078742@repo.freebsd.org> <20170217160016.D1386@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Feb 17, 2017 at 05:05:54PM +1100, Bruce Evans wrote: > On Fri, 17 Feb 2017, Mark Johnston wrote: > > > Log: > > Directly include needed headers rather than relying on pollution. > > > > We get machine/cpu.h via kmem.h -> proc.h -> _vm_domain.h -> seq.h. > > machine/cpu.h is not added here. Do you mean machine/cpufnc.h? > > > Modified: head/sys/cddl/dev/dtrace/amd64/dtrace_subr.c > > ============================================================================== > > --- head/sys/cddl/dev/dtrace/amd64/dtrace_subr.c Fri Feb 17 00:50:00 2017 (r313849) > > +++ head/sys/cddl/dev/dtrace/amd64/dtrace_subr.c Fri Feb 17 03:27:20 2017 (r313850) > > @@ -41,7 +41,9 @@ > > #include <sys/dtrace_impl.h> > > #include <sys/dtrace_bsd.h> > > #include <machine/clock.h> > > +#include <machine/cpufunc.h> > > This was correct. <machine/cpufunc.h> is standard pollution in <sys/systm.h>, > and it is a style bug to include it directly, and a bug to not include > <sys/systm.h> after <sys/param.h> and before all other headers in kernel > .c files. > > It is pollution to include <machine/cpufunc.h> in any header except > <sys/systm.h>. kmem.h has grosser pollution (param, proc, malloc, vmem, > vm/uma, vm/vm, vm/vm_extern, but not systm which is a prerequisite for > most of the other headers that kmem.h includes). Most of the other headers > also have gross pollution, ending with seq.h which includes systm.h and > its standard pollution. > > seq.h also includes machine/cpu.h. Apparently kmem.h does depend on this, > and this commit doesn't fix it. This commit was intended to address an issue with r313841, which added a "KASSERT((read_rflags() & PSL_I) == 0, ...);" to dtrace_subr.c. That change fails to compile when ported to a kernel that predates seq.h; it's only because we include kmem.h and thus seq.h that r313841 compiles on HEAD - kmem.h itself doesn't depend on cpufunc.h as far as I know. > > > #include <machine/frame.h> > > +#include <machine/psl.h> > > This include is not mentioned in the log message. > > machine/psl.h is stamdard pollution in machine/cpu.h (on at least amd64 and > i386), so would not be needed here if machine/cpu.h had been spelled > correctly. Sorry, my commit log message wasn't very good. psl.h is needed for the definition of PSL_I. I take it then that the diff below is the correct fix, since sys/systm.h isn't sufficient. diff --git a/sys/cddl/dev/dtrace/amd64/dtrace_subr.c b/sys/cddl/dev/dtrace/amd64/dtrace_subr.c index 717f4557a64d..e180f4d0a57c 100644 --- a/sys/cddl/dev/dtrace/amd64/dtrace_subr.c +++ b/sys/cddl/dev/dtrace/amd64/dtrace_subr.c @@ -41,9 +41,8 @@ #include <sys/dtrace_impl.h> #include <sys/dtrace_bsd.h> #include <machine/clock.h> -#include <machine/cpufunc.h> +#include <machine/cpu.h> #include <machine/frame.h> -#include <machine/psl.h> #include <vm/pmap.h> extern void dtrace_getnanotime(struct timespec *tsp); diff --git a/sys/cddl/dev/dtrace/i386/dtrace_subr.c b/sys/cddl/dev/dtrace/i386/dtrace_subr.c index 3801c1ba772c..c266f27bbd52 100644 --- a/sys/cddl/dev/dtrace/i386/dtrace_subr.c +++ b/sys/cddl/dev/dtrace/i386/dtrace_subr.c @@ -42,9 +42,8 @@ #include <sys/dtrace_impl.h> #include <sys/dtrace_bsd.h> #include <machine/clock.h> -#include <machine/cpufunc.h> +#include <machine/cpu.h> #include <machine/frame.h> -#include <machine/psl.h> #include <vm/pmap.h> extern uintptr_t kernelbase;
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170221212906.GA46447>