| Message ID | 716fa938a4ab0ad66490b72e2ed750cd6583728f.1510840787.git-series.knut.omang@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2017-11-17 2:01 GMT+09:00 Knut Omang <knut.omang@oracle.com>: > Add interpretation of a new environment variable P={1,2} in spirit of the > C= option, but executing checkpatch instead of sparse. > > Signed-off-by: Knut Omang <knut.omang@oracle.com> > Reviewed-by: Håkon Bugge <haakon.bugge@oracle.com> > Acked-by: Åsmund Østvold <asmund.ostvold@oracle.com> > --- > Makefile | 20 +++++++++++++++++++- > scripts/Makefile.build | 13 +++++++++++++ > 2 files changed, 32 insertions(+), 1 deletion(-) > > diff --git a/Makefile b/Makefile > index ccd9818..eb4bca9 100644 > --- a/Makefile > +++ b/Makefile > @@ -176,6 +176,20 @@ ifndef KBUILD_CHECKSRC > KBUILD_CHECKSRC = 0 > endif > > +# Run scripts/checkpatch.pl with --ignore-cfg checkpatch.cfg > +# > +# Use 'make P=1' to enable checking of only re-compiled files. > +# Use 'make P=2' to enable checking of *all* source files, regardless > +# > +# See the file "Documentation/dev-tools/run-checkpatch.rst" for more details, > +# > +ifeq ("$(origin P)", "command line") > + KBUILD_CHECKPATCH = $(P) > +endif > +ifndef KBUILD_CHECKPATCH > + KBUILD_CHECKPATCH = 0 > +endif I am unhappy about adding a new interface for each checker. The default of CHECK is "sparse", but users can override it to use another checker. As Decumentation/dev-tools/coccinelle.rst says, if you want to use coccinelle as a checker, make C=1 CHECK="scripts/coccicheck" Recently, I saw a patch to use scripts/kernel-doc as a checker. https://patchwork.kernel.org/patch/10030521/ If I accept your patch, we would end up with KBUILD_CHECKPATCH, KBUILD_CHECKCOCCI KBUILD_CHECKDOC, ... This is ugly.
On Mon, Nov 20, 2017 at 9:18 AM, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > > I am unhappy about adding a new interface > for each checker. > > The default of CHECK is "sparse", but > users can override it to use another checker. > > > > As Decumentation/dev-tools/coccinelle.rst says, > if you want to use coccinelle as a checker, > > make C=1 CHECK="scripts/coccicheck" > I'd be nice if people could just specify CHECK and CHECKFLAGS to run their favorite checker, but currently CHECKFLAGS seems hardwired for running sparse. So something liike make C=1 CHECK="scripts/checkpatch.pl" CHECKFLAGS="--quiet --file" fails when checkpatch is passed lots of arguments like -D__linux__ -Dlinux -D__STDC__ . A little shell wrapper to grab the last argument in that long list is a workaround, but perhaps CHECKFLAGS should be made less sparse-specific?
On Mon, Nov 20, 2017 at 12:48:35PM -0700, Jim Davis wrote: > > I'd be nice if people could just specify CHECK and CHECKFLAGS to run > their favorite checker, but currently CHECKFLAGS seems hardwired for > running sparse. So something liike > > make C=1 CHECK="scripts/checkpatch.pl" CHECKFLAGS="--quiet --file" > > fails when checkpatch is passed lots of arguments like -D__linux__ > -Dlinux -D__STDC__ . A little shell wrapper to grab the last argument > in that long list is a workaround, but perhaps CHECKFLAGS should be > made less sparse-specific? It should be noted though that CHECKFLAGS contains very very few sparse specific things. It's mainly flags for the compiler coming from KBUILD_CFLAGS (which of course, sparse needs to do its job properly). -- Luc Van Oostenryck -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2017-11-21 at 01:18 +0900, Masahiro Yamada wrote: > 2017-11-17 2:01 GMT+09:00 Knut Omang <knut.omang@oracle.com>: > > Add interpretation of a new environment variable P={1,2} in spirit of the > > C= option, but executing checkpatch instead of sparse. > > > > Signed-off-by: Knut Omang <knut.omang@oracle.com> > > Reviewed-by: Håkon Bugge <haakon.bugge@oracle.com> > > Acked-by: Åsmund Østvold <asmund.ostvold@oracle.com> > > --- > > Makefile | 20 +++++++++++++++++++- > > scripts/Makefile.build | 13 +++++++++++++ > > 2 files changed, 32 insertions(+), 1 deletion(-) > > > > diff --git a/Makefile b/Makefile > > index ccd9818..eb4bca9 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -176,6 +176,20 @@ ifndef KBUILD_CHECKSRC > > KBUILD_CHECKSRC = 0 > > endif > > > > +# Run scripts/checkpatch.pl with --ignore-cfg checkpatch.cfg > > +# > > +# Use 'make P=1' to enable checking of only re-compiled files. > > +# Use 'make P=2' to enable checking of *all* source files, regardless > > +# > > +# See the file "Documentation/dev-tools/run-checkpatch.rst" for more > > details, > > +# > > +ifeq ("$(origin P)", "command line") > > + KBUILD_CHECKPATCH = $(P) > > +endif > > +ifndef KBUILD_CHECKPATCH > > + KBUILD_CHECKPATCH = 0 > > +endif > > > I am unhappy about adding a new interface > for each checker. I see your point. I wanted to extend C= to handle checkpatch as well but see no obvious good non-intrusive solution. > The default of CHECK is "sparse", but > users can override it to use another checker. > > > > As Decumentation/dev-tools/coccinelle.rst says, > if you want to use coccinelle as a checker, > > make C=1 CHECK="scripts/coccicheck" That works well with coccinelle since both sparse and coccinelle rely on getting the same command line options as what's passed to the compiler, while checkpatch is quite different: make C=2 CHECK="\$(srctree)/scripts/checkpatch.pl" fails, complaining about every single compiler flag. Thanks, Knut > Recently, I saw a patch to use scripts/kernel-doc as a checker. > https://patchwork.kernel.org/patch/10030521/ > > > > If I accept your patch, > we would end up with > > KBUILD_CHECKPATCH, > KBUILD_CHECKCOCCI > KBUILD_CHECKDOC, > ... > > This is ugly. -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2017-11-20 at 21:08 +0100, Luc Van Oostenryck wrote: > On Mon, Nov 20, 2017 at 12:48:35PM -0700, Jim Davis wrote: > > > > I'd be nice if people could just specify CHECK and CHECKFLAGS to run > > their favorite checker, but currently CHECKFLAGS seems hardwired for > > running sparse. So something liike > > > > make C=1 CHECK="scripts/checkpatch.pl" CHECKFLAGS="--quiet --file" > > > > fails when checkpatch is passed lots of arguments like -D__linux__ > > -Dlinux -D__STDC__ . A little shell wrapper to grab the last argument > > in that long list is a workaround, but perhaps CHECKFLAGS should be > > made less sparse-specific? > > It should be noted though that CHECKFLAGS contains very very few > sparse specific things. It's mainly flags for the compiler > coming from KBUILD_CFLAGS (which of course, sparse needs to > do its job properly). Yes, and we would want some arguments passed to checkpatch by default as well. A wrapper script (which by the way was what I started this with..) could of course solve this and other issues such as the ability to run multiple checkers, but I am not convinced that that would be less ugly? Thanks, Knut > > -- Luc Van Oostenryck -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 20, 2017 at 10:10:12PM +0100, Knut Omang wrote: > On Mon, 2017-11-20 at 21:08 +0100, Luc Van Oostenryck wrote: > > > > It should be noted though that CHECKFLAGS contains very very few > > sparse specific things. It's mainly flags for the compiler > > coming from KBUILD_CFLAGS (which of course, sparse needs to > > do its job properly). > > Yes, and we would want some arguments passed to checkpatch by default as well. > > A wrapper script (which by the way was what I started this with..) > could of course solve this and other issues such as the ability > to run multiple checkers, but I am not convinced that that would be > less ugly? A wrapper script is something else that need to be maintained but of course, it's very flexible. I don't have a strong opinion on this and prefer to let speak the people who maintain kbuild. Should it be possible to somehow keep the distinction between the flags coming from KBUILD_CFLAGS and the pure CHECKFLAGS? -- Luc Van Oostenryck -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Makefile b/Makefile index ccd9818..eb4bca9 100644 --- a/Makefile +++ b/Makefile @@ -176,6 +176,20 @@ ifndef KBUILD_CHECKSRC KBUILD_CHECKSRC = 0 endif +# Run scripts/checkpatch.pl with --ignore-cfg checkpatch.cfg +# +# Use 'make P=1' to enable checking of only re-compiled files. +# Use 'make P=2' to enable checking of *all* source files, regardless +# +# See the file "Documentation/dev-tools/run-checkpatch.rst" for more details, +# +ifeq ("$(origin P)", "command line") + KBUILD_CHECKPATCH = $(P) +endif +ifndef KBUILD_CHECKPATCH + KBUILD_CHECKPATCH = 0 +endif + # Use make M=dir to specify directory of external module to build # Old syntax make ... SUBDIRS=$PWD is still supported # Setting the environment variable KBUILD_EXTMOD take precedence @@ -340,7 +354,7 @@ ifeq ($(MAKECMDGOALS),) endif export KBUILD_MODULES KBUILD_BUILTIN -export KBUILD_CHECKSRC KBUILD_SRC KBUILD_EXTMOD +export KBUILD_CHECKSRC KBUILD_CHECKPATCH KBUILD_SRC KBUILD_EXTMOD # We need some generic definitions (do not try to remake the file). scripts/Kbuild.include: ; @@ -363,9 +377,12 @@ DEPMOD = /sbin/depmod PERL = perl PYTHON = python CHECK = sparse +CHECKP = scripts/checkpatch.pl CHECKFLAGS := -D__linux__ -Dlinux -D__STDC__ -Dunix -D__unix__ \ -Wbitwise -Wno-return-void $(CF) +CHECKPFLAGS := --quiet --show-types --emacs \ + --ignore-cfg checkpatch.cfg --file $(PF) NOSTDINC_FLAGS = CFLAGS_MODULE = AFLAGS_MODULE = @@ -419,6 +436,7 @@ export ARCH SRCARCH CONFIG_SHELL HOSTCC HOSTCFLAGS CROSS_COMPILE AS LD CC export CPP AR NM STRIP OBJCOPY OBJDUMP HOSTLDFLAGS HOST_LOADLIBES export MAKE AWK GENKSYMS INSTALLKERNEL PERL PYTHON UTS_MACHINE export HOSTCXX HOSTCXXFLAGS LDFLAGS_MODULE CHECK CHECKFLAGS +export CHECKP CHECKPFLAGS export KBUILD_CPPFLAGS NOSTDINC_FLAGS LINUXINCLUDE OBJCOPYFLAGS LDFLAGS export KBUILD_CFLAGS CFLAGS_KERNEL CFLAGS_MODULE CFLAGS_GCOV CFLAGS_KCOV CFLAGS_KASAN CFLAGS_UBSAN diff --git a/scripts/Makefile.build b/scripts/Makefile.build index bb831d4..cfc540a 100644 --- a/scripts/Makefile.build +++ b/scripts/Makefile.build @@ -109,6 +109,17 @@ ifneq ($(KBUILD_CHECKSRC),0) endif endif +# Run per-directory/per-file specific checkpatch testing: +ifneq ($(KBUILD_CHECKPATCH),0) + ifeq ($(KBUILD_CHECKPATCH),2) + quiet_cmd_force_checkpatch = CHECKP $< + cmd_force_checkpatch = $(srctree)/$(CHECKP) $(POPTS) $< $(CHECKPFLAGS) ; + else + quiet_cmd_checkpatch = CHECKP $< + cmd_checkpatch = $(srctree)/$(CHECKP) $(POPTS) $< $(CHECKPFLAGS) ; + endif +endif + # Do section mismatch analysis for each module/built-in.o ifdef CONFIG_DEBUG_SECTION_MISMATCH cmd_secanalysis = ; scripts/mod/modpost $@ @@ -290,6 +301,7 @@ objtool_dep = $(objtool_obj) \ define rule_cc_o_c $(call echo-cmd,checksrc) $(cmd_checksrc) \ + $(call echo-cmd,checkpatch) $(cmd_checkpatch) \ $(call cmd_and_fixdep,cc_o_c) \ $(cmd_modversions_c) \ $(call echo-cmd,objtool) $(cmd_objtool) \ @@ -312,6 +324,7 @@ endif # Built-in and composite module parts $(obj)/%.o: $(src)/%.c $(recordmcount_source) $(objtool_dep) FORCE $(call cmd,force_checksrc) + $(call cmd,force_checkpatch) $(call if_changed_rule,cc_o_c) # Single-part modules are special since we need to mark them in $(MODVERDIR)
Add interpretation of a new environment variable P={1,2} in spirit of the C= option, but executing checkpatch instead of sparse. Signed-off-by: Knut Omang <knut.omang@oracle.com> Reviewed-by: Håkon Bugge <haakon.bugge@oracle.com> Acked-by: Åsmund Østvold <asmund.ostvold@oracle.com> --- Makefile | 20 +++++++++++++++++++- scripts/Makefile.build | 13 +++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-)