lkml.org 
[lkml]   [2020]   [Jul]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2] Makefile: Add clang-tidy and static analyzer support to makefile
On Thu, Jul 09, 2020 at 12:56:07PM -0500, Nathan Huckleberry wrote:
>On Wed, Jul 8, 2020 at 2:11 PM Nick Desaulniers <ndesaulniers@google.com> wrote:
>>
>> On Wed, Jul 8, 2020 at 11:21 AM 'Nathan Huckleberry' via Clang Built
>> Linux <clang-built-linux@googlegroups.com> 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@google.com>
>> > ---
>> > Changes V1 -> V2:
>> > * Remove dependencies on GNU Parallel
>> > * * Clang-tidy/analyzer now invoked directly from python
>> > Link: https://lkml.org/lkml/2019/8/6/941
>> >
>> > Makefile | 3 +
>> > scripts/clang-tools/Makefile.clang-tools | 23 ++++++
>> > .../{ => clang-tools}/gen_compile_commands.py | 0
>>
>> + Tom for the rename.

The rename is fine with me.

>>
>> I think we should add scripts/clang-tools/ to MAINTAINERS under
>> CLANG/LLVM SUPPORT:
>> ```
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index c87b94e6b2f6..42602231929c 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -4211,6 +4211,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
>> ```
>> that way we get cc'ed properly on proposed changes (should folks use
>> scripts/get_maintainer.pl).
>>
>> > scripts/clang-tools/run-clang-tools.py | 77 +++++++++++++++++++
>> > 4 files changed, 103 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/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..e09dc1a8efff
>> > --- /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@google.com>
>> > +#
>> > +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)
>>
>> s/Clang/clang/ to match the case of the target.
>>
>> > +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 static-analyzer compile_commands.json
>> > +else
>> > + $(error Clang-analyzer requires CC=clang)
>>
>> s/Clang/clang/ to match the case of the target.
>>
>> > +endif
>> > diff --git a/scripts/gen_compile_commands.py b/scripts/clang-tools/gen_compile_commands.py
>> > similarity index 100%
>> > rename from scripts/gen_compile_commands.py
>> > rename to scripts/clang-tools/gen_compile_commands.py
>> > diff --git a/scripts/clang-tools/run-clang-tools.py b/scripts/clang-tools/run-clang-tools.py
>> > new file mode 100755
>> > index 000000000000..d429a150e23a
>> > --- /dev/null
>> > +++ b/scripts/clang-tools/run-clang-tools.py
>> > @@ -0,0 +1,77 @@
>> > +#!/usr/bin/env python
>> > +# SPDX-License-Identifier: GPL-2.0
>> > +#
>> > +# Copyright (C) Google LLC, 2020
>> > +#
>> > +# Author: Nathan Huckleberry <nhuck@google.com>
>> > +#
>> > +"""A helper routine run clang-tidy and the clang static-analyzer on
>> > +compile_commands.json."""
>> > +
>> > +import argparse
>> > +import json
>> > +import logging
>> > +import multiprocessing
>> > +import os
>> > +import re
>> > +import subprocess
>> > +
>> > +def parse_arguments():
>> > + """Set up and parses command-line arguments.
>> > + Returns:
>> > + args: Dict of parsed args
>> > + Has keys 'file' and 'type'
>> > + """
>> > + usage = """Run clang-tidy or the clang static-analyzer on a
>> > + compilation database."""
>> > + parser = argparse.ArgumentParser(description=usage)
>> > +
>> > + type_help = ('Type of analysis to be performed')
>> > + parser.add_argument('type', choices=['clang-tidy', 'static-analyzer'],
>> > + help=type_help)
>> > + file_path_help = ('Path to the compilation database to parse')
>> > + parser.add_argument('file', type=str, help=file_path_help)
>>
>> I don't know if the kernel has a preferred style for Python, but I
>> think it would be good to be consistent in the use of single vs double
>> quotes for strings. My preference is for double quotes, but I don't
>> know enough about the various PEPs for style or if the kernel has a
>> preferred style for these.
>>
>> + Bill who knows a bit about Python style.
>>
>> > +
>> > + args = parser.parse_args()
>> > +
>> > + return args
>> > +
>> > +def init(l,t):
>> > + global lock
>> > + global analysis_type
>> > + lock = l
>> > + analysis_type = t
>>
>> Is this canonical Python? Maybe wrap these functions into methods of
>> an object you construct, that way you can assign these as instance
>> variables against `self`, rather than using global variables.
>
>I did this to allow shared locks between processes, see
>https://stackoverflow.com/questions/25557686/python-sharing-a-lock-between-processes
>
>>
>> > +
>> > +def run_analysis(entry):
>> > + filename = entry['file']
>> > + p = None
>> > + if(analysis_type == "clang-tidy"):
>> > + p = subprocess.run(["clang-tidy", "-p", os.getcwd(),
>> > + "-checks=-*,linuxkernel-*", filename],
>> > + stdout=subprocess.PIPE, stderr=subprocess.PIPE)
>> > + if(analysis_type == "static-analyzer"):
>> > + p = subprocess.run(["clang-tidy", "-p", os.getcwd(),
>> > + "-checks=-*,clang-analyzer-*", filename],
>> > + stdout=subprocess.PIPE, stderr=subprocess.PIPE)
>>
>> When you have a fair amount of duplication between two branches of an
>> if/else (for instance, same method invocation and number of
>> parameters, just slight differences in parameter values), consider if
>> you can use a ternary to simplify or make the code more concise. That
>> would also help avoid initializing `p` to `None`:
>>
>> checks = "-checks=-*,linuxkernel-*" if analysis_type == "clang-tidy"
>> else "-checks=-*,clang-analyzer-*"
>> p = subprocess.run(["clang-tidy", "-p", os.getcwd(), checks,
>> stdout=subprocess.PIPE, stderr=subprocess.PIPE]
>>
>> then maybe do some validation of the analysis_type when validating
>> command line arguments earlier.
>
>Argparse should already handle validation of the analysis type.
>
>>
>> > + lock.acquire()
>> > + print(entry['file'])
>> > + os.write(1, p.stdout)
>> > + os.write(2, p.stderr)
>>
>> Please use sys.stdout and sys.stderr rather than magic constants for
>> their file descriptors.
>>
>> > + lock.release()
>> > +
>> > +
>> > +def main():
>> > + args = parse_arguments()
>> > + filename = args.file
>> > +
>> > + #Read JSON data into the datastore variable
>> > + if filename:
>>
>> Isn't there a way to make command line arguments required with
>> Argparse? In that case, would you still need the conditional?
>>
>> > + with open(filename, 'r') as f:
>> > + datastore = json.load(f)
>> > +
>> > + lock = multiprocessing.Lock()
>> > + pool = multiprocessing.Pool(initializer=init, initargs=(lock,args.type,))
>> > + pool.map(run_analysis,datastore)
>>
>> Please use a space to separate parameters in a parameter list.
>>
>> > +
>> > +if __name__ == '__main__':
>> > + main()
>>
>> So rather than call a function named main, you could simply construct
>> an object, then call a method on it or have the constructor simply
>> kick off the analysis (essentially a mix of `main` and `init`).
>>
>> --
>> Thanks,
>> ~Nick Desaulniers
>
>Thanks,
>Nathan Huckleberry

\
 
 \ /
  Last update: 2020-07-14 00:51    [W:0.090 / U:0.516 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site