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: <20240802223509.GA781199@thelio-3990X>
Date: Fri, 2 Aug 2024 15:35:09 -0700
From: Nathan Chancellor <nathan@...nel.org>
To: Théo Lebrun <theo.lebrun@...tlin.com>
Cc: Masahiro Yamada <masahiroy@...nel.org>,
	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

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.

> 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>
>