Message-ID: <1513439553.31439.1.camel@oracle.com>
Date:   Sat, 16 Dec 2017 16:52:33 +0100
From:   Knut Omang <knut.omang@...cle.com>
To:     Julia Lawall <julia.lawall@...6.fr>
Cc:     linux-kernel@...r.kernel.org,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Shuah Khan <shuah@...nel.org>,
        Nicolas Palix <nicolas.palix@...g.fr>,
        Masahiro Yamada <yamada.masahiro@...ionext.com>,
        John Haxby <john.haxby@...cle.com>, linux-doc@...r.kernel.org,
        Jonathan Corbet <corbet@....net>,
        Gilles Muller <Gilles.Muller@...6.fr>,
        Michal Marek <michal.lkml@...kovi.net>,
        Mickaël Salaün <mic@...ikod.net>,
        "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
        Håkon Bugge <haakon.bugge@...cle.com>,
        Åsmund Østvold <asmund.ostvold@...cle.com>,
        Matthew Wilcox <willy@...radead.org>,
        "Levin, Alexander (Sasha Levin)" <alexander.levin@...izon.com>,
        cocci@...teme.lip6.fr, linux-kbuild@...r.kernel.org
Subject: Re: [PATCH v2 2/5] Documentation: Add doc for runchecks, a checker
 runner

On Sat, 2017-12-16 at 16:08 +0100, Julia Lawall wrote:
> > diff --git a/Documentation/dev-tools/runchecks.rst b/Documentation/dev-
> tools/runchecks.rst
> > new file mode 100644
> > index 0000000..b25b3de
> > --- /dev/null
> > +++ b/Documentation/dev-tools/runchecks.rst
> > @@ -0,0 +1,215 @@
> > +.. Copyright 2017 Knut Omang <knut.omang@...cle.com>
> > +
> > +Makefile support for running checkers
> > +=====================================
> > +
> > +Tools like sparse, coccinelle and scripts/checkpatch.pl is able to detect a
> 
> is -> are
> 
> > +lot of syntactic and semantic issues with the code, and is also constantly
> 
> is -> are
> 
> > +evolving and detecting more. In an ideal world, all source files should
> > +adhere to whatever rules imposed by checkpatch.pl and sparse etc. with all
> > +bells and whistles enabled, in a way that these checkers can be run as a reflex
> > +by developers (and by bots) from the top level Makefile for every changing
> > +source file. In the real world however there's a number of challenges:
> > +
> > +* Sometimes there are valid reasons for accepting violations of a checker
> > +  rule, even if that rule is a sensible one in the general case.
> > +* Some subsystems have different restrictions and requirements.
> > +  (Ideally, the number of subsystems with differing restrictions and
> > +  requirements will diminish over time.)
> > +* Similarly, the kernel contains a lot of code that predates the tools, or at
> > +  least some of the newer rules, and we would like these tools to evolve without
> > +  requiring the need to fix all issues detected with it in the same commit.
> > +  We also want to accommodate new tools, so that each new tool does not
> > +  have to reinvent it's own mechanism for running checks.
> 
> it's -> its
> 
> > +* On the other hand, we want to make sure that files that are clean
> > +  (to some well defined extent, such as passing checkpatch or sparse
> > +  with checks only for certain important types of issues) keep being so.
> > +
> > +This is the purpose of ``scripts/runchecks``.
> > +
> > +The ``runchecks`` program looks for files named ``runchecks.cfg`` in the
> > +``scripts`` directory, then in the directory hierarchy of the source file,
> > +starting where that source file is located, searching upwards. If at least
> > +one such file exists in the source tree, ``runchecks`` parses a set of
> > +rules from it, and use them to determine how to invoke a set of individual
> > +checker tools for a particular file. The kernel Makefile system supports using
> > +this feature as an integrated part of compiling the code, using the
> > +``C={1,2}`` option. With::
> > +
> > +	make C=1
> > +
> > +runchecks will be invoked if the file needs to be recompiled. With ::
> > +
> > +	make C=2
> > +
> > +runchecks will be invoked for all source files, even if they do not need
> > +recompiling. Based on the configuration, ``runchecks`` will invoke one or
> > +more checkers. The number and types of checkers to run are configurable and
> > +can also be selected on the command line::
> > +
> > +	make C=2 CF="--run:sparse,checkpatch"
> > +
> > +If only one checker is run, any parameter that is not recognized by
> > +runchecks itself will be forwarded to the checker. If more than one checker
> > +is enabled, parameters can be forwarded to a specific checker by means of
> > +this syntax::
> > +
> > +	make C=2 CF="--to-checkpatch:--terse"
> > +
> > +A comma separated list of parameters can be supplied if necessary.
> > +
> > +Supported syntax of the runchecks.cfg configuration file
> > +--------------------------------------------------------
> > +
> > +The runchecks configuration file chain can be used to set policies and "rein in"
> > +checker errors piece by piece for a particular subsystem or driver. It can
> > +also be used to mitigate and extend checkers that does not support
> 
> does -> do
> 
> > +selective suppression of all it's checks.
> 
> it's -> its
> 
> > +Two classes of configuration is available. The first class is configuration
> > +that defines what checkers are enabled, and also allow a limited form of
> > +pattern matching to extend checking, to mitigate for checks that cannot be
> > +selectively disabled by command line parameters to the underlying tool
> > +itself. This type of configuration should go into the first accessed
> > +configuration file, and has been preconfigured for the currently supported
> > +checkers in ``scripts/runchecks.cfg``. The second class is the features for
> > +configuring the output of the checkers by selectively suppressing checks on
> > +a per file or per check basis. These typically goes in the source tree in
> 
> goes -> go
> 
> > +the directory of the source file or above. Some of the syntax is generic
> > +and some is only supported by some checkers.
> > +
> > +For the first class of configuration the following syntax is supported::
> > +
> > +	# comments
> > +	checker checkpatch [command]
> > +	addflags <list of extra flags and parameters>
> > +	cflags
> > +	typedef NAME <regular expression>
> > +	run [checker list|all]
> > +
> > +The ``checker`` command switches ``runchecks``'s attention to a particular
> > +checker. The following commands until the next ``checker`` statement
> > +applies to that particular checker. The first occurrence of ``checker``
> 
> applies -> apply
> 
> > +also serves as a potentially defining operation, if the checker name
> > +has not been preconfigured. In that case, a second parameter can be used
> > +to provide the name of the command used to run the checker.
> > +A full checker integration into runchecks will typically require some
> > +additions to runchecks, and will then have been preconfigured,
> > +but simple checkers might just be configured on the fly.
> > +
> > +The ``addflags`` command incrementally adds more flags and parameters to
> > +the command line used to invoke the checker. This applies to all
> > +invocations of the checker from runchecks.
> > +The ``cflags`` command tells to forward all the flags and options passed to
> > +the compiler invocation to the checker. The default is to suppress these
> > +parameters when invoking the checker.
> > +
> > +The ``typedef`` command adds ``NAME`` and associates it with the given
> > +regular expression. This expression is used to match against standard error
> > +output from the checker and ``NAME`` can be used as a new named check that
> > +runchecks understands and that can be used with checker supported names
> > +below to selectively suppress that particular set of warning or error
> > +messages. This is useful to handle output checks for which the underlying
> > +checker is does not provide any suppression. Check type namespaces are
> > +separate for the individual checkers. You can list the state of the built in and
> > +configured checker and check types with::
> > +
> > +	scripts/runchecks --list
> > +
> > +The checker implementations of the ``typedef`` command also allows
> > +runchecks to perform some unification of output by rewriting the output
> > +lines, adding optional color support, and use of the new type names in the
> > +error output, to ease the process of updating the runchecks.cfg files.
> > +Having a unified representation of the error output also makes it much
> > +easier to do statistics or other operations on top of an aggregated output
> > +from several checkers.
> > +
> > +For the second class of configuration the following syntax is supported::
> > +
> > +	# comments
> > +	checker checker_name
> > +	line_len <n>
> > +	except check_type [files ...]
> > +	pervasive check_type1 [check_type2 ...]
> > +
> > +The ``line_len`` directive defines the upper bound of characters per line
> > +tolerated in this directory. Currently only ``checkpatch`` supports this
> > +command.  The ``except`` directive takes a check type such as for example
> > +``MACRO_ARG_REUSE``, and a set of files that should not be subject to this
> > +particular check type. The ``pervasive`` command disables the listed types
> > +of checks for all the files in the subtree.  The ``except`` and
> > +``pervasive`` directives can be used cumulatively to add more exceptions.
> > +
> > +Running checker programs from make
> > +----------------------------------
> > +
> > +You can run checkpatch subject to rules defined in ``runcheck.cfg`` in the
> > +directory of the source file by using "make P=1" to run checkpatch on all files
> > +that gets recompiled, or "make P=2" to run checkpatch on all source files.
> > +
> > +A make variable ``CF`` allows passing additional parameters to
> > +``runchecks``. You can for instance use::
> > +
> > +	make C=2 CF="--run:checkpatch --fix-inplace"
> > +
> > +to run only the ``checkpatch`` checker, and to have checkpatch trying to fix
> 
> 
> trying -> try
> 
> > +issues it finds - *make sure you have a clean git tree and carefully review
> > +the output afterwards!* Combine this with selectively enabling of types of
> > +errors via changes under ``checker checkpatch`` to the local
> > +``runchecks.cfg``, and you can focus on fixing up errors subsystem or
> > +driver by driver on a type by type basis.
> > +
> > +By default runchecks will skip all files if a ``runchecks.cfg`` file cannot
> > +be found in the directory of the file or in the tree above.  This is to
> > +allow builds with ``C=2`` to pass even for subsystems that has not yet done
> > +anything to rein in checker errors. At some point when all subsystems and
> > +drivers either have fixed all checker errors or added proper
> > +``runchecks.cfg`` files, this can be changed.
> > +
> > +To force runchecks to run a full run in directories/trees where runchecks
> > +does not find  a ``runchecks.cfg`` file as well, use::
> > +
> > +	make C=2 CF="-f"
> > +
> > +If you like to see all the warnings and errors produced by the checkers, ignoring
> > +any runchecks.cfg files except the one under ``scripts``, you can use::
> > +
> > +	make C=2 CF="-n"
> > +
> > +or for a specific module directory::
> > +
> > +	make C=2 M=drivers/infiniband/core CF="--color -n -w"
> > +
> > +with the -w option to ``runchecks`` to suppress errors from any of the
> > +checkers and just continue on, and the ``--color`` option to present errors
> > +with colors where supported.
> > +
> > +Ever tightening checker rules
> > +-----------------------------
> > +
> > +Commit the changes to the relevant ``runchecks.cfg`` together with the code
> > +changes that fixes a particular type of issue, this will allow automatic
> > +checker running by default. This way we can ensure that new errors of that
> > +particular type do not inadvertently sneak in again! This can be done at
> > +any subsystem or module maintainer's discretion and at the right time
> > +without having to do it all at the same time.
> > +
> > +Before submitting your changes, verify that a full "make C=2" passes with no
> > +errors.
> > +
> > +Extending and improving checker support in ``runchecks``
> > +--------------------------------------------------------
> > +
> > +The runchecks program has been written with extensibility in mind.
> > +If the checker starts it's reporting lines with filename:lineno, there's a
> 
> it's -> its
> 
> > +good chance that a new checker can simply be added by adding::
> > +
> > +	checker mychecker path_to_mychecker
> > +
> > +to ``scripts/runchecks.cfg`` and suitable ``typedef`` expressions to provide
> > +selective suppressions of output, however it is likely that some quirks are
> > +needed to make the new checker behave similar to the others, and to support
> 
> similar -> similarly
> 
> julia

Thanks for the review - I'll certainly fix these for v3.

Knut

> 
> > +the full set of features, such as the ``--list`` option. This is done by
> > +implementing a new subclass of the Checker class in ``runchecks``. This is the way
> > +all the available default supported checkers are implemented, and those
> > +relatively lean implementations could serve as examples.
> > diff --git a/Documentation/dev-tools/sparse.rst b/Documentation/dev-tools/sparse.rst
> > index 78aa00a..e3e8b27 100644
> > --- a/Documentation/dev-tools/sparse.rst
> > +++ b/Documentation/dev-tools/sparse.rst
> > @@ -101,5 +101,31 @@ recompiled, or use "make C=2" to run sparse on the files whether
> they need to
> >  be recompiled or not.  The latter is a fast way to check the whole tree if you
> >  have already built it.
> >
> > -The optional make variable CF can be used to pass arguments to sparse.  The
> > -build system passes -Wbitwise to sparse automatically.
> > +The "make C={1,2}" form of kernel make indirectly calls sparse via "runchecks",
> > +which dependent on configuration and command line options may dispatch calls to
> > +other checkers in addition to sparse. Details on how this works is covered
> > +in Documentation/dev-tools/runchecks.rst .
> > +
> > +The optional make variable CF can be used to pass arguments to runchecks for dispatch
> > +to sparse. If sparse is the only tool enabled, any option not recognized by
> > +runchecks will be forwarded to sparse. If more than one tool is active, you must
> > +add the parameters you want sparse to get as a comma separated list prefixed by
> > +``--to-sparse:``. If you want sparse to be the only checker run, and you want
> > +some nice colored output, you can specify this as::
> > +
> > +	make C=2 CF="--run:sparse --color"
> > +
> > +This will cause sparse to be called for all files which are supported by a valid
> > +runchecks configuration (again see Documentation/dev-tools/runchecks.rst for
> > +details). If you want to run sparse on all files and ignore any missing
> > +configuration files(s), just add ``-n`` to the list of options passed to
> > +runchecks. This will cause runchecks to call sparse with all errors enabled for
> > +all files even if no valid configuration is found in the tree for the source files.
> > +
> > +By default "runchecks" is set to enable all sparse errors, but you can
> > +configure what checks to be applied by sparse on a per file or per subsystem
> > +basis. With the above invocation, make will fail and stop on the first file
> > +encountered with sparse errors or warnings in it. If you want to continue
> > +anyway, you can use::
> > +
> > +	make C=2 CF="--run:sparse --color -w"
> > diff --git a/Documentation/kbuild/kbuild.txt b/Documentation/kbuild/kbuild.txt
> > index ac2363e..260e688 100644
> > --- a/Documentation/kbuild/kbuild.txt
> > +++ b/Documentation/kbuild/kbuild.txt
> > @@ -103,10 +103,13 @@ CROSS_COMPILE is also used for ccache in some setups.
> >
> >  CF
> >  --------------------------------------------------
> > -Additional options for sparse.
> > -CF is often used on the command-line like this:
> > +Additional options for runchecks, the generic checker runner.
> > +CF is often used on the command-line for instance like this:
> >
> > -    make CF=-Wbitwise C=2
> > +    make C=2 CF="--run:sparse --color -w"
> > +
> > +to run the sparse tool only, and to use colored output and continue on warnings
> > +or errors.
> >
> >  INSTALL_PATH
> >  --------------------------------------------------
> > --
> > git-series 0.9.1
> >

Powered by blists - more mailing lists