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 <[email protected]>
---
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
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 <[email protected]>
+#
+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 static-analyzer compile_commands.json
+else
+ $(error Clang-analyzer requires CC=clang)
+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 <[email protected]>
+#
+"""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)
+
+ args = parser.parse_args()
+
+ return args
+
+def init(l,t):
+ global lock
+ global analysis_type
+ lock = l
+ analysis_type = t
+
+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)
+ lock.acquire()
+ print(entry['file'])
+ os.write(1, p.stdout)
+ os.write(2, p.stderr)
+ lock.release()
+
+
+def main():
+ args = parse_arguments()
+ filename = args.file
+
+ #Read JSON data into the datastore variable
+ if filename:
+ 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)
+
+if __name__ == '__main__':
+ main()
--
2.27.0.383.g050319c2ae-goog
On Wed, Jul 8, 2020 at 11:21 AM 'Nathan Huckleberry' via Clang Built
Linux <[email protected]> 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 <[email protected]>
> ---
> 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.
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 <[email protected]>
> +#
> +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 <[email protected]>
> +#
> +"""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.
> +
> +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.
> + 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
On Wed, Jul 8, 2020 at 2:11 PM Nick Desaulniers <[email protected]> wrote:
>
> On Wed, Jul 8, 2020 at 11:21 AM 'Nathan Huckleberry' via Clang Built
> Linux <[email protected]> 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 <[email protected]>
> > ---
> > 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.
>
> 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 <[email protected]>
> > +#
> > +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 <[email protected]>
> > +#
> > +"""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
(bumping a few points for v3)
On Thu, Jul 9, 2020 at 10:56 AM Nathan Huckleberry <[email protected]> wrote:
>
> On Wed, Jul 8, 2020 at 2:11 PM Nick Desaulniers <[email protected]> wrote:
> >
> > On Wed, Jul 8, 2020 at 11:21 AM 'Nathan Huckleberry' via Clang Built
> > Linux <[email protected]> wrote:
> > >
> > 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).
bump
> > > --- /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 <[email protected]>
> > > +#
> > > +"""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
Is `re` being used anywhere?
> > > +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.
double quotes.
> > > +
> > > + 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
Ah, ok, I see the problem. In that case, I'm less worried about this.
`global` just sets off red flags unless there is a very good reason to
use it.
>
> >
> > > +
> > > +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],
Is it worthwhile to NOT run `-*` passes and only run
`clang-analyzer-*`? Otherwise `make clang-analyzer` and `make
clang-tidy` contain a ton of duplicate info.
> > > + 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.
Cool, I still think the ternary can simplify a v3.
>
> >
> > > + 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.
Also, I'm not a fan of how clang-tidy writes the errors to stdout.
$ make LLVM=1 -j71 defconfig clang-tidy 2> log.txt
write part of the log, and spews to stdout. Do you think it would
make sense to redirect stdout from clang-tidy to stderr for this
script?
$ grep error: log.txt | sort -u
$ grep clang-analyzer log.txt | sort -u
Checking some of the clang-tidy warnings, some seem harmless.
linux-next/net/core/devlink.c:9527:6: error: redefinition of
'devlink_compat_running_version' [clang-diagnostic-error]
looks legit, though not terribly important to fix ASAP. So that's cool.
The clang-analyzer report is a little beefier, once the traces start
getting long they become fairly hard to follow. Is it possible to dump
the html report of these? I guess the issue with that is that we
wouldn't be able to join them in the python script.
$ grep clang-analyzer log.txt | sort -u | cut -d '[' -f 2 | sort -u
clang-analyzer-core.CallAndMessage]
clang-analyzer-core.DivideZero]
clang-analyzer-core.NonNullParamChecker]
clang-analyzer-core.NullDereference]
clang-analyzer-core.StackAddressEscape]
clang-analyzer-core.UndefinedBinaryOperatorResult]
clang-analyzer-core.uninitialized.ArraySubscript]
clang-analyzer-core.uninitialized.Assign]
clang-analyzer-core.uninitialized.Branch]
clang-analyzer-core.uninitialized.UndefReturn]
clang-analyzer-deadcode.DeadStores]
clang-analyzer-optin.performance.Padding]
clang-analyzer-optin.portability.UnixAPI]
clang-analyzer-security.insecureAPI.bcmp]
clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling]
clang-analyzer-security.insecureAPI.strcpy]
clang-analyzer-unix.cstring.NullArg]
clang-analyzer-unix.Malloc]
clang-analyzer-valist.Uninitialized]
interesting. I like how clang-analyzer warns about bcmp and yet llvm
will generate calls to bcmp...
--
Thanks,
~Nick Desaulniers
On Thu, Jul 09, 2020 at 12:56:07PM -0500, Nathan Huckleberry wrote:
>On Wed, Jul 8, 2020 at 2:11 PM Nick Desaulniers <[email protected]> wrote:
>>
>> On Wed, Jul 8, 2020 at 11:21 AM 'Nathan Huckleberry' via Clang Built
>> Linux <[email protected]> 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 <[email protected]>
>> > ---
>> > 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 <[email protected]>
>> > +#
>> > +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 <[email protected]>
>> > +#
>> > +"""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
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 <[email protected]>
---
Changes v2 -> v3
* Redirect clang-tidy output to stderr
* Style fixes
* Add directory to MAINTAINERS
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 | 69 +++++++++++++++++++
5 files changed, 96 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..7ad3308c1937
--- /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 <[email protected]>
+#
+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 static-analyzer compile_commands.json
+else
+ $(error clang-analyzer requires CC=clang)
+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..00b8532c1729
--- /dev/null
+++ b/scripts/clang-tools/run-clang-tools.py
@@ -0,0 +1,69 @@
+#!/usr/bin/env python
+# SPDX-License-Identifier: GPL-2.0
+#
+# Copyright (C) Google LLC, 2020
+#
+# Author: Nathan Huckleberry <[email protected]>
+#
+"""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 subprocess
+import sys
+
+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)
+
+ args = parser.parse_args()
+
+ return args
+
+def init(l,t):
+ global lock
+ global analysis_type
+ lock = l
+ analysis_type = t
+
+def run_analysis(entry):
+ filename = entry["file"]
+ checks = "-checks=-*,linuxkernel-*" if (analysis_type == "clang-tidy") else "-checks=-*,clang-analyzer-*"
+ p = subprocess.run(["clang-tidy", "-p", os.getcwd(),
+ checks, filename],
+ stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
+ lock.acquire()
+ print(filename, file=sys.stderr)
+ sys.stderr.buffer.write(p.stdout)
+ lock.release()
+
+
+def main():
+ args = parse_arguments()
+ filename = args.file
+
+ #Read JSON data into the datastore variable
+ 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)
+
+if __name__ == "__main__":
+ main()
--
2.27.0.389.gc38d7665816-goog
On Tue, Jul 14, 2020 at 4:24 PM 'Nathan Huckleberry' via Clang Built
Linux <[email protected]> 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 <[email protected]>
> ---
> Changes v2 -> v3
> * Redirect clang-tidy output to stderr
> * Style fixes
> * Add directory to MAINTAINERS
> 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 | 69 +++++++++++++++++++
> 5 files changed, 96 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..7ad3308c1937
> --- /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 <[email protected]>
> +#
> +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 static-analyzer compile_commands.json
^ note below on `static-analyzer`
> +else
> + $(error clang-analyzer requires CC=clang)
> +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..00b8532c1729
> --- /dev/null
> +++ b/scripts/clang-tools/run-clang-tools.py
> @@ -0,0 +1,69 @@
> +#!/usr/bin/env python
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Copyright (C) Google LLC, 2020
> +#
> +# Author: Nathan Huckleberry <[email protected]>
> +#
> +"""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 subprocess
> +import sys
> +
> +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"],
Rather than "static-analyzer", how about "clang-analyzer" to be
consistent with the `make` target? Top level Makefile would need to
pass that here, too.
> + help=type_help)
> + file_path_help = ("Path to the compilation database to parse")
> + parser.add_argument("file", type=str, help=file_path_help)
> +
> + args = parser.parse_args()
> +
> + return args
> +
> +def init(l,t):
> + global lock
> + global analysis_type
> + lock = l
> + analysis_type = t
> +
> +def run_analysis(entry):
> + filename = entry["file"]
> + checks = "-checks=-*,linuxkernel-*" if (analysis_type == "clang-tidy") else "-checks=-*,clang-analyzer-*"
I'm not sure that the parens are necessary ^, but it's not a big
enough deal to necessitate a v4, IMO.
Though this is still running `-*,` for static-analyzer which I would
like removed.
> + p = subprocess.run(["clang-tidy", "-p", os.getcwd(),
> + checks, filename],
> + stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
> + lock.acquire()
> + print(filename, file=sys.stderr)
Should we drop printing the filename? Analyzing the output of `make
LLVM=1 -j71 defconfig clang-tidy 2> log.txt`, for example I see:
Error while processing
/linux-next/drivers/net/ethernet/freescale/fman/fman_sp.c.
...
<hundreds of lines from different files>
drivers/net/ethernet/freescale/fman/fman_sp.c
It's surprising to me how these appear out of order; maybe buffering
or not has something to do with it?
Anyways, if we print the filename anyways per error, and files with no
errors are just printed kind of meaninglessly, then I think we don't
need to print the filename being analyzed again. For clang-analyzer,
the errors also have the filename per line of the warning printed.
> + sys.stderr.buffer.write(p.stdout)
> + lock.release()
> +
> +
> +def main():
> + args = parse_arguments()
> + filename = args.file
> +
> + #Read JSON data into the datastore variable
> + 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)
> +
> +if __name__ == "__main__":
> + main()
> --
--
Thanks,
~Nick Desaulniers
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 <[email protected]>
---
Changes v3->v4
* Update usages of static-analyzer to clang-analyzer
* Clarify -* explicitly in comment
* Remove filename printing
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 | 69 +++++++++++++++++++
5 files changed, 96 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 <[email protected]>
+#
+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
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..44527b3663e9
--- /dev/null
+++ b/scripts/clang-tools/run-clang-tools.py
@@ -0,0 +1,69 @@
+#!/usr/bin/env python
+# SPDX-License-Identifier: GPL-2.0
+#
+# Copyright (C) Google LLC, 2020
+#
+# Author: Nathan Huckleberry <[email protected]>
+#
+"""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 subprocess
+import sys
+
+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", "clang-analyzer"],
+ help=type_help)
+ file_path_help = ("Path to the compilation database to parse")
+ parser.add_argument("file", type=str, help=file_path_help)
+
+ args = parser.parse_args()
+
+ return args
+
+def init(l,t):
+ global lock
+ global analysis_type
+ lock = l
+ analysis_type = t
+
+def run_analysis(entry):
+ filename = entry["file"]
+ #Disable all checks, then re-enable the ones we want
+ checks = "-checks=-*,linuxkernel-*" if (analysis_type == "clang-tidy") else "-checks=-*,clang-analyzer-*"
+ p = subprocess.run(["clang-tidy", "-p", os.getcwd(),
+ checks, filename],
+ stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
+ lock.acquire()
+ sys.stderr.buffer.write(p.stdout)
+ lock.release()
+
+
+def main():
+ args = parse_arguments()
+ filename = args.file
+
+ #Read JSON data into the datastore variable
+ 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)
+
+if __name__ == "__main__":
+ main()
--
2.28.0.rc0.105.gf9edc3c819-goog