[PATCH 0/7] Support for automatic checkpatch running in the kernel

[PATCH 0/7] Support for automatic checkpatch running in the kernel

[Knut Omang]

This patch series implements features to facilitate running checkpatch on the
entire kernel as part of automatic testing.

This is done by by adding a few small features to checkpatch and put these
features to use to implement support for a new Makefile environment variable
P={1,2} following the pattern of sparse and the C={1,2} variable.  The basic
functionality + docs are in patch #1-4.

It also fixes a minor issue with "checkpatch --fix-inplace" found during testing
(patch #5).

The most important checkpatch feature added is the --ignore-cfg feature, which
takes a file argument and parses that file according to this minimal language:

       # comments
       line_len <n>
       except checkpatch_type [files ...]
       pervasive checkpatch_type1 [checkpatch_type2 ...]

With "make P=2" checkpatch is called with "--file" and "--ignore_cfg
checkpatch.cfg" which causes it to look for a file named 'checkpatch.cfg' in the
same directory as the source file. If that file exists, checkpatch will be run
with an implicit --strict and with the @ignore list expanded with content from
the configuration file.  If it does not exist, make will simply silently ignore
the file.

Patches #6-7 enhances this behaviour to also scan the directories above a file
until a match for the --file parameter is found.

The idea is that the community can work to add checkpatch.cfg files to
directories, serving both as documentation and as a way for subsystem
maintainers to enforce policies and individual tastes as well as TODOs and/or
priorities, to make it easier for newcomers to contribute in this area. By
ignoring directories without such files, automation can start right away as it
is trivially possible to run errorless with P=2 for the entire kernel.

The patches includes a documentation file with some more details.

This patch set has evolved from an earlier implementation I made that was just a
wrapper script around checkpatch. That version have been used for a number of
years on a driver project I worked on where we had automatic checkin regression
testing. I extended that to also run checkpatch to avoid having to clean up
frequent unintended whitespace changes and style violations from others...

I have also tested this version on some directories I am familiar with.  The
result of that work is available in two patch sets of 10 and 11 patches, but we
agreed that it would be better to post them as separate patch sets later.

Those patch sets illustrates how I picture the "flow" from just "reining in" the
checkpatch detections to actually fixing classes of checkpatch issues one by
one, while updating the checkpatch.cfg file(s) to have 0 errors or warnings at
any commit boundary.

The combined set is available here:

   git://github.com/knuto/linux.git  branch checkpatch

Comments and suggestions appreciated!

Thanks,
Knut

Knut Omang (7):
  checkpatch: Implement new --ignore-cfg parameter
  kbuild: Add P= command line flag to run checkpatch
  checkpatch: Add a few convenience options to disable/modify features
  Documentation: Add documentation for the new P= Makefile option
  checkpatch: Improve --fix-inplace for TABSTOP
  checkpatch: Make --ignore-cfg look recursively for the file
  Documentation: Update checkpatch --ignore-cfg description

 Documentation/dev-tools/index.rst          |   1 +-
 Documentation/dev-tools/run-checkpatch.rst | 109 ++++++++++++++++++++++-
 Makefile                                   |  20 +++-
 scripts/Makefile.build                     |  13 +++-
 scripts/checkpatch.pl                      | 108 +++++++++++++++++++++-
 5 files changed, 249 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/dev-tools/run-checkpatch.rst

base-commit: bebc6082da0a9f5d47a1ea2edc099bf671058bd4
-- 
git-series 0.9.1

[PATCH 2/7] kbuild: Add P= command line flag to run checkpatch

[Knut Omang]

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
+
 # 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)
-- 
git-series 0.9.1

Re: [PATCH 0/7] Support for automatic checkpatch running in the kernel

[Kees Cook]

On Thu, Nov 16, 2017 at 9:01 AM, Knut Omang <knut.omang@oracle.com> wrote:
> The most important checkpatch feature added is the --ignore-cfg feature, which
> takes a file argument and parses that file according to this minimal language:
>
>        # comments
>        line_len <n>
>        except checkpatch_type [files ...]
>        pervasive checkpatch_type1 [checkpatch_type2 ...]
>
> With "make P=2" checkpatch is called with "--file" and "--ignore_cfg
> checkpatch.cfg" which causes it to look for a file named 'checkpatch.cfg' in the
> same directory as the source file. If that file exists, checkpatch will be run
> with an implicit --strict and with the @ignore list expanded with content from
> the configuration file.  If it does not exist, make will simply silently ignore
> the file.

Will these configurations be cascading? (For example, all of net/ uses
a different comment style, so having that recorded in a single file
would be nice.)


-- 
Kees Cook
Pixel Security

Re: [PATCH 0/7] Support for automatic checkpatch running in the kernel

[Knut Omang]

On Thu, 2017-11-16 at 14:57 -0800, Kees Cook wrote:
> On Thu, Nov 16, 2017 at 9:01 AM, Knut Omang <knut.omang@oracle.com> wrote:
> > The most important checkpatch feature added is the --ignore-cfg feature, which
> > takes a file argument and parses that file according to this minimal language:
> >
> >        # comments
> >        line_len <n>
> >        except checkpatch_type [files ...]
> >        pervasive checkpatch_type1 [checkpatch_type2 ...]
> >
> > With "make P=2" checkpatch is called with "--file" and "--ignore_cfg
> > checkpatch.cfg" which causes it to look for a file named 'checkpatch.cfg' in the
> > same directory as the source file. If that file exists, checkpatch will be run
> > with an implicit --strict and with the @ignore list expanded with content from
> > the configuration file.  If it does not exist, make will simply silently ignore
> > the file.
> 
> Will these configurations be cascading? (For example, all of net/ uses
> a different comment style, so having that recorded in a single file
> would be nice.)

Good point, the net/ use case is certainly something I have been thinking about.
I didn't want to make it too complex in the first set, so I let patch 1-4
implement the basics with one file per directory and nothing across directories.

Patch 6 and 7 in this set extends this to "fallback", which (with the net/ case in mind)
should allow a single file under net/ to cover the whole subsystem for all subtrees that
don't have their own checkpatch.cfg, but will be overridden by files in subdirectories.

So from a documentation point of view this could be done with the set as it is,
with people just copying the "pervasive" and "line_len" commands from
a "global" net/checkpatch.cfg setup down into the subtrees as they start dealing with 
exceptions.

If we want something more automatic, it can certainly be extended, 
this would just require checkpatch to parse all the upwards checkpatch.cfg files for each
source file instead just taking the first found, which is what patch 6 implements.

Also the command language in the config files is rather limited, maybe other commands
maybe with more checkpatch additions would be useful to handle such scenarios..

Let me know what you think..

Thanks,
Knut

Re: [PATCH 0/7] Support for automatic checkpatch running in the kernel

[Knut Omang]

I realize the Cc: list for the cover letter probably should have included all 
the relevant maintainers for this set, sorry about that!

For convenience I also put up a more reader friendly copy of the doc after patch 7 here:

http://heim.ifi.uio.no/~knuto/kernel/4.14/dev-tools/run-checkpatch.html

Thanks,
Knut

On Thu, 2017-11-16 at 18:01 +0100, Knut Omang wrote:
> This patch series implements features to facilitate running checkpatch on the
> entire kernel as part of automatic testing.
> 
> This is done by by adding a few small features to checkpatch and put these
> features to use to implement support for a new Makefile environment variable
> P={1,2} following the pattern of sparse and the C={1,2} variable.  The basic
> functionality + docs are in patch #1-4.
> 
> It also fixes a minor issue with "checkpatch --fix-inplace" found during testing
> (patch #5).
> 
> The most important checkpatch feature added is the --ignore-cfg feature, which
> takes a file argument and parses that file according to this minimal language:
> 
>        # comments
>        line_len <n>
>        except checkpatch_type [files ...]
>        pervasive checkpatch_type1 [checkpatch_type2 ...]
> 
> With "make P=2" checkpatch is called with "--file" and "--ignore_cfg
> checkpatch.cfg" which causes it to look for a file named 'checkpatch.cfg' in the
> same directory as the source file. If that file exists, checkpatch will be run
> with an implicit --strict and with the @ignore list expanded with content from
> the configuration file.  If it does not exist, make will simply silently ignore
> the file.
> 
> Patches #6-7 enhances this behaviour to also scan the directories above a file
> until a match for the --file parameter is found.
> 
> The idea is that the community can work to add checkpatch.cfg files to
> directories, serving both as documentation and as a way for subsystem
> maintainers to enforce policies and individual tastes as well as TODOs and/or
> priorities, to make it easier for newcomers to contribute in this area. By
> ignoring directories without such files, automation can start right away as it
> is trivially possible to run errorless with P=2 for the entire kernel.
> 
> The patches includes a documentation file with some more details.
> 
> This patch set has evolved from an earlier implementation I made that was just a
> wrapper script around checkpatch. That version have been used for a number of
> years on a driver project I worked on where we had automatic checkin regression
> testing. I extended that to also run checkpatch to avoid having to clean up
> frequent unintended whitespace changes and style violations from others...
> 
> I have also tested this version on some directories I am familiar with.  The
> result of that work is available in two patch sets of 10 and 11 patches, but we
> agreed that it would be better to post them as separate patch sets later.
> 
> Those patch sets illustrates how I picture the "flow" from just "reining in" the
> checkpatch detections to actually fixing classes of checkpatch issues one by
> one, while updating the checkpatch.cfg file(s) to have 0 errors or warnings at
> any commit boundary.
> 
> The combined set is available here:
> 
>    git://github.com/knuto/linux.git  branch checkpatch
> 
> Comments and suggestions appreciated!
> 
> Thanks,
> Knut
> 
> Knut Omang (7):
>   checkpatch: Implement new --ignore-cfg parameter
>   kbuild: Add P= command line flag to run checkpatch
>   checkpatch: Add a few convenience options to disable/modify features
>   Documentation: Add documentation for the new P= Makefile option
>   checkpatch: Improve --fix-inplace for TABSTOP
>   checkpatch: Make --ignore-cfg look recursively for the file
>   Documentation: Update checkpatch --ignore-cfg description
> 
>  Documentation/dev-tools/index.rst          |   1 +-
>  Documentation/dev-tools/run-checkpatch.rst | 109 ++++++++++++++++++++++-
>  Makefile                                   |  20 +++-
>  scripts/Makefile.build                     |  13 +++-
>  scripts/checkpatch.pl                      | 108 +++++++++++++++++++++-
>  5 files changed, 249 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/dev-tools/run-checkpatch.rst
> 
> base-commit: bebc6082da0a9f5d47a1ea2edc099bf671058bd4

Re: [PATCH 2/7] kbuild: Add P= command line flag to run checkpatch

[Masahiro Yamada]

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.



-- 
Best Regards
Masahiro Yamada

Re: [PATCH 2/7] kbuild: Add P= command line flag to run checkpatch

[Jim Davis]

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?

-- 
Jim

Re: [PATCH 2/7] kbuild: Add P= command line flag to run checkpatch

[Luc Van Oostenryck]

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

Re: [PATCH 2/7] kbuild: Add P= command line flag to run checkpatch

[Knut Omang]

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.

Re: [PATCH 2/7] kbuild: Add P= command line flag to run checkpatch

[Knut Omang]

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

Re: [PATCH 2/7] kbuild: Add P= command line flag to run checkpatch

[Luc Van Oostenryck]

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

Re: [PATCH 2/7] kbuild: Add P= command line flag to run checkpatch

[Jim Davis]

On Mon, Nov 20, 2017 at 2:22 PM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:

> Should it be possible to somehow keep the distinction between
> the flags coming from KBUILD_CFLAGS and the pure CHECKFLAGS?

Well, the practical problem seems to be that $(CHECK) is called in
scripts/Makefile.build with both $(CHECKFLAGS) and $(c_flags) as
arguments, regardless of what $(CHECK) is.  That can be hacked around
with something inelegant like

diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index bb831d49bcfd..102194f9afb9 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -98,14 +98,20 @@ __build: $(if $(KBUILD_BUILTIN),$(builtin-target)
$(lib-target) $(extra-y)) \
         $(subdir-ym) $(always)
        @:

-# Linus' kernel sanity checking tool
+# Linus' kernel sanity checking tool, or $CHECK program of choice
+ifneq ($(KBUILD_CHECKSRC),)
+  add_to_checkflags =
+  ifeq ($(CHECK),sparse)
+    add_to_checkflags = $(c_flags)
+  endif
+endif
 ifneq ($(KBUILD_CHECKSRC),0)
   ifeq ($(KBUILD_CHECKSRC),2)
     quiet_cmd_force_checksrc = CHECK   $<
-          cmd_force_checksrc = $(CHECK) $(CHECKFLAGS) $(c_flags) $< ;
+          cmd_force_checksrc = $(CHECK) $(CHECKFLAGS) $(add_to_checkflags) $< ;
   else
       quiet_cmd_checksrc     = CHECK   $<
-            cmd_checksrc     = $(CHECK) $(CHECKFLAGS) $(c_flags) $< ;
+            cmd_checksrc     = $(CHECK) $(CHECKFLAGS) $(add_to_checkflags) $< ;
   endif
 endif

and then if I run scripts/checkpatch.pl as $CHECK and pass --quiet
--file as before it works -- until checkpatch returns with a non-zero
exit code, which causes the Makefile to bail at that point.

So maybe a wrapper script, with an exit 0 fixup to keep on keeping on
in case of checkpatch warnings or errors, would be better after all.


-- 
Jim

Re: [PATCH 2/7] kbuild: Add P= command line flag to run checkpatch

[Knut Omang]

On Mon, 2017-11-20 at 17:00 -0700, Jim Davis wrote:
> On Mon, Nov 20, 2017 at 2:22 PM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> 
> > Should it be possible to somehow keep the distinction between
> > the flags coming from KBUILD_CFLAGS and the pure CHECKFLAGS?
> 
> Well, the practical problem seems to be that $(CHECK) is called in
> scripts/Makefile.build with both $(CHECKFLAGS) and $(c_flags) as
> arguments, regardless of what $(CHECK) is.  That can be hacked around
> with something inelegant like
> 
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index bb831d49bcfd..102194f9afb9 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -98,14 +98,20 @@ __build: $(if $(KBUILD_BUILTIN),$(builtin-target)
> $(lib-target) $(extra-y)) \
>          $(subdir-ym) $(always)
>         @:
> 
> -# Linus' kernel sanity checking tool
> +# Linus' kernel sanity checking tool, or $CHECK program of choice
> +ifneq ($(KBUILD_CHECKSRC),)
> +  add_to_checkflags =
> +  ifeq ($(CHECK),sparse)
> +    add_to_checkflags = $(c_flags)
> +  endif
> +endif
>  ifneq ($(KBUILD_CHECKSRC),0)
>    ifeq ($(KBUILD_CHECKSRC),2)
>      quiet_cmd_force_checksrc = CHECK   $<
> -          cmd_force_checksrc = $(CHECK) $(CHECKFLAGS) $(c_flags) $< ;
> +          cmd_force_checksrc = $(CHECK) $(CHECKFLAGS) $(add_to_checkflags) $<
> ;
>    else
>        quiet_cmd_checksrc     = CHECK   $<
> -            cmd_checksrc     = $(CHECK) $(CHECKFLAGS) $(c_flags) $< ;
> +            cmd_checksrc     = $(CHECK) $(CHECKFLAGS) $(add_to_checkflags) $<
> ;
>    endif
>  endif
> 
> and then if I run scripts/checkpatch.pl as $CHECK and pass --quiet
> --file as before it works -- until checkpatch returns with a non-zero
> exit code, which causes the Makefile to bail at that point.

yes, in an earlier version, I added a --no-errors flag to checkpatch to handle
that, but based on feedback this version promotes make -k instead. It seems
however that that only holds to the next linker step. It is useful to be able to
select whether checkpatch should cause make to stop or not. While fixing issues,
failure is a better strategy while to assess the state or generate a report, a
return 0 behavior is better.

> So maybe a wrapper script, with an exit 0 fixup to keep on keeping on
> in case of checkpatch warnings or errors, would be better after all.

I can prepare a v2 based on the wrapper script I have already.

In that version, all added functionality was implemented in the wrapper
(including the configuration file parsing) 

Would you like to keep the checkpatch changes in some form, or would you rather
see everything happening in the wrapper?

A 3rd possibility that strikes me is that maybe what's needed is a general
checker runner that can also take care of running other checks, running multiple
checks, and maybe also handling temporary or decided suppression of sparse
errors in a similar fashion as I implemented for checkpatch errors. But maybe
that can be left to a later change set..

Thanks,
Knut

Re: [PATCH 2/7] kbuild: Add P= command line flag to run checkpatch

[Jim Davis]

On Tue, Nov 21, 2017 at 1:10 AM, Knut Omang <knut.omang@oracle.com> wrote:

> Would you like to keep the checkpatch changes in some form, or would you rather
> see everything happening in the wrapper?

I don't have a strong preference one way or another, but keeping
everything in a wrapper script might be easier if only because you
wouldn't need to get signoffs from whoever maintains checkpatch too.

-- 
Jim

Re: [PATCH 2/7] kbuild: Add P= command line flag to run checkpatch

[Joe Perches]

On Tue, 2017-11-21 at 12:48 -0700, Jim Davis wrote:
> On Tue, Nov 21, 2017 at 1:10 AM, Knut Omang <knut.omang@oracle.com> wrote:
> 
> > Would you like to keep the checkpatch changes in some form, or would you rather
> > see everything happening in the wrapper?
> 
> I don't have a strong preference one way or another, but keeping
> everything in a wrapper script might be easier if only because you
> wouldn't need to get signoffs from whoever maintains checkpatch too.

Keeping everything in a wrapper script would also allow
any arbitrary checker to be run with minimal changes to
the Makefile/build subsystem.