linux-kernel - Re: [PATCH] scripts: run-clang-tools: add file filtering option

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAK7LNAR6VFT+z377NURKZs5P4+Z9zE2oeN44zQE7PRfqV0JoZQ@mail.gmail.com>
Date: Sat, 3 Aug 2024 18:16:11 +0900
From: Masahiro Yamada <masahiroy@...nel.org>
To: Nathan Chancellor <nathan@...nel.org>
Cc: Théo Lebrun <theo.lebrun@...tlin.com>, 
	Nicolas Schier <nicolas@...sle.eu>, Nick Desaulniers <ndesaulniers@...gle.com>, 
	Bill Wendling <morbo@...gle.com>, Justin Stitt <justinstitt@...gle.com>, llvm@...ts.linux.dev, 
	linux-kbuild@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH] scripts: run-clang-tools: add file filtering option

On Sat, Aug 3, 2024 at 7:35 AM Nathan Chancellor <nathan@...nel.org> wrote:
>
> Hi Théo,
>
> First of all, apologies that it has taken me so long to review this!
>
> On Thu, Jul 04, 2024 at 11:28:21AM +0200, Théo Lebrun wrote:
> > Add file filtering feature. We take zero or more filters at the end as
> > positional arguments. If none are given, the default behavior is kept
> > and we run the tool on all files in the datastore. Else, files must
> > match one or more filter to be analysed.
> >
> > The below command runs clang-tidy on drivers/clk/clk.c and all C files
> > inside drivers/reset/.
> >
> >     ./scripts/clang-tools/run-clang-tools.py clang-tidy \
> >         compile_commands.json \
> >         'drivers/clk/clk.c' 'drivers/reset/*'
> >
> > The Python fnmatch builtin module is used. Matching is case-insensitive.
> > See its documentation for allowed syntax:
> > https://docs.python.org/3/library/fnmatch.html
> >
> > Signed-off-by: Théo Lebrun <theo.lebrun@...tlin.com>
> > ---
> > Currently, all files in the datastore are analysed. This is not
> > practical for grabbing errors in a subsystem, or relative to a patch
> > series. Add a file filtering feature with wildcard support.
>
> Sure, I think this is totally reasonable. In fact, I think some of this
> could be added to the commit message as further existence for this
> feature.
>
> The change itself looks good to me for the most part, I have some
> questions below just for my own understanding.
>
> Reviewed-by: Nathan Chancellor <nathan@...nel.org>
>
> One further question/comment now: Have you considered a way to
> integrate this into Kbuild with the clang-tidy and clang-analyzer
> commands? I don't think it is strictly necessary for the acceptance of
> this patch but it might be nice to have some variable that users could
> provide to do this with their regular make command + the clang-tidy
> target? Not sure if Masahiro has further thoughts on that.


We can do this in a separate patch if it is desired.

This script already supports multiple options.
I'd like to prefer a single generic option
like CLANG_TOOLS_FLAGS.





> > Have a nice day,
> > Théo
> > ---
> >  scripts/clang-tools/run-clang-tools.py | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/scripts/clang-tools/run-clang-tools.py b/scripts/clang-tools/run-clang-tools.py
> > index f31ffd09e1ea..b0b3a9c8cdec 100755
> > --- a/scripts/clang-tools/run-clang-tools.py
> > +++ b/scripts/clang-tools/run-clang-tools.py
> > @@ -10,6 +10,7 @@ compile_commands.json.
> >  """
> >
> >  import argparse
> > +import fnmatch
> >  import json
> >  import multiprocessing
> >  import subprocess
> > @@ -32,6 +33,8 @@ def parse_arguments():
> >                          help=type_help)
> >      path_help = "Path to the compilation database to parse"
> >      parser.add_argument("path", type=str, help=path_help)
> > +    file_filter_help = "Optional Unix shell-style wildcard file filters"
> > +    parser.add_argument("file_filter", type=str, nargs="*", help=file_filter_help)
> >
> >      checks_help = "Checks to pass to the analysis"
> >      parser.add_argument("-checks", type=str, default=None, help=checks_help)
> > @@ -48,6 +51,22 @@ def init(l, a):
> >      args = a
> >
> >
> > +def filter_entries(datastore, filters):
> > +    for entry in datastore:
> > +        if filters == []:
> > +            yield entry
> > +            continue
> > +
> > +        assert entry['file'].startswith(entry['directory'])
>
> What is the purpose of this assertion? Will it cause AssertionError
> under normal circumstances?
>
> > +        # filepath is relative to the directory, to avoid matching on the absolute path
> > +        filepath = entry['file'][len(entry['directory']):].lstrip('/')
> > +
> > +        for pattern in filters:
> > +            if fnmatch.fnmatch(filepath, pattern):
> > +                yield entry
> > +                break
> > +
> > +
> >  def run_analysis(entry):
> >      # Disable all checks, then re-enable the ones we want
> >      global args
> > @@ -87,6 +106,7 @@ def main():
> >          # Read JSON data into the datastore variable
> >          with open(args.path, "r") as f:
> >              datastore = json.load(f)
> > +            datastore = filter_entries(datastore, args.file_filter)
> >              pool.map(run_analysis, datastore)
> >      except BrokenPipeError:
> >          # Python flushes standard streams on exit; redirect remaining output
> >
> > ---
> > base-commit: 22a40d14b572deb80c0648557f4bd502d7e83826
> > change-id: 20240704-clang-tidy-filter-f470377cb2b4
> >
> > Best regards,
> > --
> > Théo Lebrun <theo.lebrun@...tlin.com>
> >
>


--
Best Regards
Masahiro Yamada