Re: [PATCH v7] Makefile: Add clang-tidy and static analyzer support to makefile

[Masahiro Yamada]

On Fri, Aug 7, 2020 at 6:42 AM 'Nathan Huckleberry' via Clang Built
Linux <clang-built-linux@xxxxxxxxxxxxxxxx> wrote:
>
> On Thu, Aug 6, 2020 at 3:44 AM Masahiro Yamada <masahiroy@xxxxxxxxxx> wrote:
> >
> > On Tue, Jul 28, 2020 at 9:47 AM Nathan Huckleberry <nhuck@xxxxxxxxxx> wrote:
> > >
> > > This patch adds clang-tidy and the clang static-analyzer as make
> > > targets. The goal of this patch is to make static analysis tools
> > > usable and extendable by any developer or researcher who is familiar
> > > with basic c++.
> > >
> > > The current static analysis tools require intimate knowledge of the
> > > internal workings of the static analysis. Clang-tidy and the clang
> > > static analyzers expose an easy to use api and allow users unfamiliar
> > > with clang to write new checks with relative ease.
> > >
> > > ===Clang-tidy===
> > >
> > > Clang-tidy is an easily extendable 'linter' that runs on the AST.
> > > Clang-tidy checks are easy to write and understand. A check consists of
> > > two parts, a matcher and a checker. The matcher is created using a
> > > domain specific language that acts on the AST
> > > (https://clang.llvm.org/docs/LibASTMatchersReference.html).  When AST
> > > nodes are found by the matcher a callback is made to the checker. The
> > > checker can then execute additional checks and issue warnings.
> > >
> > > Here is an example clang-tidy check to report functions that have calls
> > > to local_irq_disable without calls to local_irq_enable and vice-versa.
> > > Functions flagged with __attribute((annotation("ignore_irq_balancing")))
> > > are ignored for analysis. (https://reviews.llvm.org/D65828)
> > >
> > > ===Clang static analyzer===
> > >
> > > The clang static analyzer is a more powerful static analysis tool that
> > > uses symbolic execution to find bugs. Currently there is a check that
> > > looks for potential security bugs from invalid uses of kmalloc and
> > > kfree. There are several more general purpose checks that are useful for
> > > the kernel.
> > >
> > > The clang static analyzer is well documented and designed to be
> > > extensible.
> > > (https://clang-analyzer.llvm.org/checker_dev_manual.html)
> > > (https://github.com/haoNoQ/clang-analyzer-guide/releases/download/v0.1/clang-analyzer-guide-v0.1.pdf)
> > >
> > > The main draw of the clang tools is how accessible they are. The clang
> > > documentation is very nice and these tools are built specifically to be
> > > easily extendable by any developer. They provide an accessible method of
> > > bug-finding and research to people who are not overly familiar with the
> > > kernel codebase.
> > >
> > > Signed-off-by: Nathan Huckleberry <nhuck@xxxxxxxxxx>
> > > ---
> > > Changes v6->v7
> > > * Fix issues with relative paths
> > > * Additional style fixes
> > >  MAINTAINERS                                   |  1 +
> > >  Makefile                                      |  3 +
> > >  scripts/clang-tools/Makefile.clang-tools      | 23 ++++++
> > >  .../{ => clang-tools}/gen_compile_commands.py |  0
> > >  scripts/clang-tools/run-clang-tools.py        | 74 +++++++++++++++++++
> > >  5 files changed, 101 insertions(+)
> > >  create mode 100644 scripts/clang-tools/Makefile.clang-tools
> > >  rename scripts/{ => clang-tools}/gen_compile_commands.py (100%)
> > >  create mode 100755 scripts/clang-tools/run-clang-tools.py
> > >
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 1d4aa7f942de..a444564e5572 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -4198,6 +4198,7 @@ W:        https://clangbuiltlinux.github.io/
> > >  B:     https://github.com/ClangBuiltLinux/linux/issues
> > >  C:     irc://chat.freenode.net/clangbuiltlinux
> > >  F:     Documentation/kbuild/llvm.rst
> > > +F:     scripts/clang-tools/
> > >  K:     \b(?i:clang|llvm)\b
> > >
> > >  CLEANCACHE API
> > > diff --git a/Makefile b/Makefile
> > > index fe0164a654c7..3e2df010b342 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -747,6 +747,7 @@ KBUILD_CFLAGS       += $(call cc-option,-fno-allow-store-data-races)
> > >
> > >  include scripts/Makefile.kcov
> > >  include scripts/Makefile.gcc-plugins
> > > +include scripts/clang-tools/Makefile.clang-tools
> > >
> > >  ifdef CONFIG_READABLE_ASM
> > >  # Disable optimizations that make assembler listings hard to read.
> > > @@ -1543,6 +1544,8 @@ help:
> > >         @echo  '  export_report   - List the usages of all exported symbols'
> > >         @echo  '  headerdep       - Detect inclusion cycles in headers'
> > >         @echo  '  coccicheck      - Check with Coccinelle'
> > > +       @echo  '  clang-analyzer  - Check with clang static analyzer'
> > > +       @echo  '  clang-tidy      - Check with clang-tidy'
> > >         @echo  ''
> > >         @echo  'Tools:'
> > >         @echo  '  nsdeps          - Generate missing symbol namespace dependencies'
> > > diff --git a/scripts/clang-tools/Makefile.clang-tools b/scripts/clang-tools/Makefile.clang-tools
> > > new file mode 100644
> > > index 000000000000..5c9d76f77595
> > > --- /dev/null
> > > +++ b/scripts/clang-tools/Makefile.clang-tools
> > > @@ -0,0 +1,23 @@
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +#
> > > +# Copyright (C) Google LLC, 2020
> > > +#
> > > +# Author: Nathan Huckleberry <nhuck@xxxxxxxxxx>
> > > +#
> > > +PHONY += clang-tidy
> > > +clang-tidy:
> > > +ifdef CONFIG_CC_IS_CLANG
> > > +       $(PYTHON3) scripts/clang-tools/gen_compile_commands.py
> > > +       $(PYTHON3) scripts/clang-tools/run-clang-tools.py clang-tidy compile_commands.json
> > > +else
> > > +       $(error clang-tidy requires CC=clang)
> > > +endif
> > > +
> > > +PHONY += clang-analyzer
> > > +clang-analyzer:
> > > +ifdef CONFIG_CC_IS_CLANG
> > > +       $(PYTHON3) scripts/clang-tools/gen_compile_commands.py
> > > +       $(PYTHON3) scripts/clang-tools/run-clang-tools.py clang-analyzer compile_commands.json
> > > +else
> > > +       $(error clang-analyzer requires CC=clang)
> > > +endif
> >
> >
> >
> > You can unify the almost same two rules.
> >
> > PHONY += clang-tidy clang-analyzer
> > clang-tidy clang-analyzer:
> > ifdef CONFIG_CC_IS_CLANG
> >         $(PYTHON3) scripts/clang-tools/gen_compile_commands.py
> >         $(PYTHON3) scripts/clang-tools/run-clang-tools.py $@
> > compile_commands.json
> > else
> >         $(error $@ requires CC=clang)
> > endif
> >
>
> I like this.
>
> >
> >
> >
> > But, before we proceed, please tell me
> > what this check is intended for.
> >
>
> Clang-tidy invokes clang using the command line
> options specified in the compile_commands.json file.
> Using gcc command line options causes a bunch of
> errors for unknown options.
>
> >
> >
> >
> >
> > Case 1)
> > Build the kernel with CC=clang,
> > and then run clang-tidy without CC=clang.
> >
> > $ make CC=clang defconfig
> > $ make CC=clang -j$(nproc)
> > $ make clang-tidy
> >
> > scripts/clang-tools/Makefile.clang-tools:13: *** clang-tidy requires
> > CC=clang.  Stop.
> >
>
> I suppose this case could allow clang-tidy to
> be run.
>
> >
> >
> >
> > Case 2)
> > Build the kernel using GCC,
> > and then run clang-tidy with CC=clang.
> >
> > $ make defconfig
> > $ make -j$(nproc)
> > $ make CC=clang clang-tidy
> >
> > This patch happily runs clang-tidy
> > although compile_commands.json
> > contains GCC commands.
> >
>
> This is the worst of the two cases. I'm not
> sure how to prevent this other than parsing the
> compiler invocation in run-clang-tools.py.
>
> I'm open to better suggestions.
>
> >
> >
> >
> >
> > So, it checks if you have passed CC=clang
> > to "make clang-tidy", where I do not see
> > any user of the $(CC) variable.
> >
> > It does not care whether you have built
> > the kernel with GCC or Clang.
> >
> >
> >
> > What happens if you run clang-tidy against
> > compile_commands.json that contains GCC
> > commands?
>
> Clang-tidy itself uses the command line options from
> compile_commands.json to invoke clang. If you run
> clang-tidy against GCC commands you get lots of
> errors similar to this
>
> Found compiler error(s).
> 12 warnings and 8 errors generated.
> Error while processing /usr/local/google/home/nhuck/linux/arch/x86/lib/iomem.c.
> error: unknown argument: '-fconserve-stack' [clang-diagnostic-error]
> error: unknown argument: '-fno-var-tracking-assignments'
> [clang-diagnostic-error]
> error: unknown argument: '-mindirect-branch-register' [clang-diagnostic-error]
> error: unknown argument: '-mindirect-branch=thunk-extern'
> [clang-diagnostic-error]
> error: unknown argument: '-mno-fp-ret-in-387' [clang-diagnostic-error]
> error: unknown argument: '-mpreferred-stack-boundary=3' [clang-diagnostic-error]
> error: unknown argument: '-mskip-rax-setup' [clang-diagnostic-error]
>
> >
> >
> > I also care about stale commands
> > in compile_commands.json.
> >
>
> I agree with this point, but it's more of a bug with
> gen_compile_commands.py. Maybe gen_compile_commands.py
> could emit a warning when stale commands are detected in the
> .*.cmd files.


Nathan, thanks for your comments.

I can improve this
so compile_commands.json contains
only commands from the last build.

Working on a patch.

-- 
Best Regards
Masahiro Yamada