2:25 p.m.
"clang-tidy" is a static code analysis tool for c and c++ code. See
https://listman.redhat.com/archives/libvir-list/2021-January/msg01152.html
for some issues found by clang-tidy and more background information.
Meson has support for clang-tidy and generates target named "clang-tidy" if
it finds a ".clang-tidy" configuration file in the project's root
directory.
There are some problems with this approach though, with regards to inclusion
in GitLab's CI:
* Single-threaded runtime of a complete scan takes between 95 and 110 minutes,
depending on the enabled checks, which is significantly longer than GitLab's
maximum run time of 60 minutes, after which jobs are aborted.
* Even without this limit on runtime, this new check would double to triple
the run time of the libVirt pipeline in GitLab.
* clang-tidy finds a lot of false positives (see link above for explanation)
and has checks that contradict the libVirt code style (e.g. braces around
blocks). This makes a quite complex configuration in ".clang-tidy" neccessary.
* I was unable to make clang-tidy recognize the settings from the
configuration file for generated files, leading clang-tidy to always add some
checks. These checks were among those that produced false positives.
* The list of enabled / disabled checks in the yaml configuration file is a
quite long string, making it hard to weave in some comments / documentation
on which checks are enabled / disabled for what reason.
This series introduces a new script, "run-clang-tidy.py". This is a
replacement for the script of the same name from clang-tools-extra. It offers
parallel execution, caching of results and a configurable soft-timeout.
Please see the individual commits for more details. Comments welcome.
https://gitlab.com/twiederh/libvirt/-/pipelines/255321968 → "clang-tidy".
Tim
Tim Wiederhake (10):
clang-tidy: Add a simple runner
clang-tidy: Run in parallel
clang-tidy: Filter output
clang-tidy: Add cache
clang-tidy: Add timeout
clang-tidy: Allow timeouts
clang-tidy: Add shuffle
clang-tidy: Make list of checks explicit
clang-tidy: Disable irrelevant and failing checks
clang-tidy: Add CI integration
.gitlab-ci.yml | 88 ++++++
scripts/run-clang-tidy.py | 557 ++++++++++++++++++++++++++++++++++++++
2 files changed, 645 insertions(+)
create mode 100755 scripts/run-clang-tidy.py
--
2.26.2
2:25 p.m.
New subject: [libvirt PATCH 01/10] clang-tidy: Add a simple runner
Run clang-tidy with default configuration on all files listed in the
compilation database. Note that the generated files in the build directory
have to be built first. The simplest way to achieve this is to build libVirt
first.
Example:
$ meson build && ninja -C build
$ ./scripts/run-clang-tidy.py -p build
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
scripts/run-clang-tidy.py | 68 +++++++++++++++++++++++++++++++++++++++
1 file changed, 68 insertions(+)
create mode 100755 scripts/run-clang-tidy.py
diff --git a/scripts/run-clang-tidy.py b/scripts/run-clang-tidy.py
new file mode 100755
index 0000000000..10c8b80fe0
--- /dev/null
+++ b/scripts/run-clang-tidy.py
@@ -0,0 +1,68 @@
+#!/usr/bin/env python3
+
+import argparse
+import json
+import os
+import subprocess
+import sys
+
+
+def parse_args():
+ parser = argparse.ArgumentParser(description="caching clang-tidy runner")
+ parser.add_argument(
+ "-p",
+ dest="build_dir",
+ default=".",
+ help="Path to build directory")
+
+ return parser.parse_args()
+
+
+def run_clang_tidy(item):
+ cmd = (
+ "clang-tidy",
+ "--warnings-as-errors=*",
+ "-p",
+ item["directory"],
+ item["file"])
+ result = subprocess.run(
+ cmd,
+ stdout=subprocess.PIPE,
+ stderr=subprocess.PIPE,
+ universal_newlines=True)
+ return {
+ "returncode": result.returncode,
+ "stdout": result.stdout.strip(),
+ "stderr": result.stderr.strip(),
+ }
+
+
+def worker():
+ for item in items:
+ os.chdir(item["directory"])
+
+ print(item["file"])
+
+ result = run_clang_tidy(item)
+
+ if result["returncode"] != 0:
+ findings.append(item["file"])
+ if result["stdout"]:
+ print(result["stdout"])
+ if result["stderr"]:
+ print(result["stderr"])
+
+
+args = parse_args()
+findings = list()
+
+with open(os.path.join(args.build_dir, "compile_commands.json")) as f:
+ items = json.load(f)
+
+worker()
+
+if findings:
+ print("Findings in %s file(s):" % len(findings))
+ for finding in findings:
+ print(" %s" % finding)
+ sys.exit(1)
--
2.26.2
2:25 p.m.
New subject: [libvirt PATCH 02/10] clang-tidy: Run in parallel
Similar to the "official" run-clang-tidy script. Defaults to run one thread
per core, making the tool more pleasant to use locally.
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
scripts/run-clang-tidy.py | 38 +++++++++++++++++++++++++++++---------
1 file changed, 29 insertions(+), 9 deletions(-)
diff --git a/scripts/run-clang-tidy.py b/scripts/run-clang-tidy.py
index 10c8b80fe0..79d9fb38b2 100755
--- a/scripts/run-clang-tidy.py
+++ b/scripts/run-clang-tidy.py
@@ -2,9 +2,12 @@
import argparse
import json
+import multiprocessing
import os
+import queue
import subprocess
import sys
+import threading
def parse_args():
@@ -14,6 +17,12 @@ def parse_args():
dest="build_dir",
default=".",
help="Path to build directory")
+ parser.add_argument(
+ "-j",
+ dest="thread_num",
+ default=multiprocessing.cpu_count(),
+ type=int,
+ help="Number of threads to run")
return parser.parse_args()
@@ -38,28 +47,39 @@ def run_clang_tidy(item):
def worker():
- for item in items:
+ while True:
+ item = items.get()
os.chdir(item["directory"])
print(item["file"])
result = run_clang_tidy(item)
- if result["returncode"] != 0:
- findings.append(item["file"])
- if result["stdout"]:
- print(result["stdout"])
- if result["stderr"]:
- print(result["stderr"])
+ with lock:
+ if result["returncode"] != 0:
+ findings.append(item["file"])
+ if result["stdout"]:
+ print(result["stdout"])
+ if result["stderr"]:
+ print(result["stderr"])
+
+ items.task_done()
args = parse_args()
+items = queue.Queue()
+lock = threading.Lock()
findings = list()
+for _ in range(args.thread_num):
+ threading.Thread(target=worker, daemon=True).start()
+
with open(os.path.join(args.build_dir, "compile_commands.json")) as f:
- items = json.load(f)
+ compile_commands = json.load(f)
+ for compile_command in compile_commands:
+ items.put(compile_command)
-worker()
+items.join()
if findings:
print("Findings in %s file(s):" % len(findings))
--
2.26.2
2:25 p.m.
New subject: [libvirt PATCH 03/10] clang-tidy: Filter output
GitLab's CI output is capped at a certain size. Filter out all status
messages that do not add value.
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
scripts/run-clang-tidy.py | 25 +++++++++++++++++++++++--
1 file changed, 23 insertions(+), 2 deletions(-)
diff --git a/scripts/run-clang-tidy.py b/scripts/run-clang-tidy.py
index 79d9fb38b2..dc5880878b 100755
--- a/scripts/run-clang-tidy.py
+++ b/scripts/run-clang-tidy.py
@@ -5,11 +5,32 @@ import json
import multiprocessing
import os
import queue
+import re
import subprocess
import sys
import threading
+spam = [
+ re.compile("^[0-9]+ (warning|error)[s]? .*generated"),
+ re.compile("^[0-9]+ warning[s]? treated as error"),
+ re.compile("^Suppressed [0-9]+ warning[s]?"),
+ re.compile("^Use .* to display errors from all non-system headers"),
+ re.compile("Error while processing "),
+ re.compile("Found compiler error"),
+]
+
+
+def remove_spam(output):
+ retval = list()
+ for line in output.split("\n"):
+ if any([s.match(line) for s in spam]):
+ continue
+ retval.append(line)
+
+ return "\n".join(retval)
+
+
def parse_args():
parser = argparse.ArgumentParser(description="caching clang-tidy runner")
parser.add_argument(
@@ -41,8 +62,8 @@ def run_clang_tidy(item):
universal_newlines=True)
return {
"returncode": result.returncode,
- "stdout": result.stdout.strip(),
- "stderr": result.stderr.strip(),
+ "stdout": remove_spam(result.stdout.strip()),
+ "stderr": remove_spam(result.stderr.strip()),
}
--
2.26.2
2:25 p.m.
New subject: [libvirt PATCH 04/10] clang-tidy: Add cache
Cache the results of clang-tidy. The cache is keyed, for each file, on:
* the file name,
* the exact command used to compile the file to detect changes in arguments,
* the hash of the preprocessor stage output to detect changes in includes.
A later patch also adds the list of enabled checks to the cache key.
Running clang-tidy uncached takes between 95 and 110 minutes single threaded
(just over 9 minutes wall time on a 12 core builder), depending on the set of
enabled checks. In the ideal case, where no source files changes, this number
is reduced to 80 seconds (9 seconds on a 12 core builder), when caching is
enabled.
This makes clang-tidy much more pleasant to work with locally, but is not
enough to guarantee painless CI operation: While GitLab does support caching
between builds and can be configured to retain the cache even when the job
fails, this does not happen when the job times out after 60 minutes or the
job is manually aborted.
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
scripts/run-clang-tidy.py | 83 ++++++++++++++++++++++++++++++++++++++-
1 file changed, 81 insertions(+), 2 deletions(-)
diff --git a/scripts/run-clang-tidy.py b/scripts/run-clang-tidy.py
index dc5880878b..cc9c20ea32 100755
--- a/scripts/run-clang-tidy.py
+++ b/scripts/run-clang-tidy.py
@@ -1,14 +1,17 @@
#!/usr/bin/env python3
import argparse
+import hashlib
import json
import multiprocessing
import os
import queue
import re
+import shlex
import subprocess
import sys
import threading
+import time
spam = [
@@ -44,6 +47,10 @@ def parse_args():
default=multiprocessing.cpu_count(),
type=int,
help="Number of threads to run")
+ parser.add_argument(
+ "--cache",
+ dest="cache",
+ help="Path to cache directory")
return parser.parse_args()
@@ -67,14 +74,75 @@ def run_clang_tidy(item):
}
+def cache_name(item):
+ if not args.cache:
+ return None
+
+ cmd = shlex.split(item["command"])
+ for index, element in enumerate(cmd):
+ if element == "-o":
+ cmd[index + 1] = "/dev/stdout"
+ continue
+ if element == "-MD":
+ cmd[index] = None
+ if element in ("-MQ", "-MF"):
+ cmd[index] = None
+ cmd[index + 1] = None
+ cmd = [c for c in cmd if c is not None]
+ cmd.append("-E")
+
+ result = subprocess.run(
+ cmd,
+ stdout=subprocess.PIPE,
+ universal_newlines=True)
+
+ if result.returncode != 0:
+ return None
+
+ hashsum = hashlib.sha256()
+ hashsum.update(item["command"].encode())
+ hashsum.update(result.stdout.encode())
+
+ basename = "".join([c if c.isalnum() else "_" for c in
item["output"]])
+ return os.path.join(args.cache, "%s-%s" % (basename, hashsum.hexdigest()))
+
+
+def cache_read(filename):
+ if filename is None:
+ return None
+
+ try:
+ with open(filename) as f:
+ return json.load(f)
+ except FileNotFoundError:
+ pass
+ except json.decoder.JSONDecodeError:
+ pass
+ return None
+
+
+def cache_write(filename, result):
+ if filename is None:
+ return
+
+ with open(filename, "w") as f:
+ json.dump(result, f)
+
+
def worker():
while True:
item = items.get()
os.chdir(item["directory"])
- print(item["file"])
+ cache = cache_name(item)
+ result = cache_read(cache)
+ with lock:
+ print(item["file"], "" if result is None else "(from
cache)")
+
+ if result is None:
+ result = run_clang_tidy(item)
- result = run_clang_tidy(item)
+ cache_write(cache, result)
with lock:
if result["returncode"] != 0:
@@ -92,6 +160,10 @@ items = queue.Queue()
lock = threading.Lock()
findings = list()
+if args.cache:
+ args.cache = os.path.abspath(args.cache)
+ os.makedirs(args.cache, exist_ok=True)
+
for _ in range(args.thread_num):
threading.Thread(target=worker, daemon=True).start()
@@ -102,6 +174,13 @@ with open(os.path.join(args.build_dir,
"compile_commands.json")) as f:
items.join()
+if args.cache:
+ cutoffdate = time.time() - 7 * 24 * 60 * 60
+ for filename in os.listdir(args.cache):
+ pathname = os.path.join(args.cache, filename)
+ if os.path.getmtime(pathname) < cutoffdate:
+ os.remove(pathname)
+
if findings:
print("Findings in %s file(s):" % len(findings))
for finding in findings:
--
2.26.2
2:25 p.m.
New subject: [libvirt PATCH 05/10] clang-tidy: Add timeout
Defining a custom timeout allows the scan to finish (albeit unsuccessfully)
before GitLab's 60 minute limit and thus preserve the cache of already
scanned files.
A successive run, e.g. when the "rerun" button in GitLab's web interface is
clicked, roughly picks up where the previous run stopped, gradually increasing
the amount of cached results and eventually succeeds.
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
scripts/run-clang-tidy.py | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/scripts/run-clang-tidy.py b/scripts/run-clang-tidy.py
index cc9c20ea32..19b8640982 100755
--- a/scripts/run-clang-tidy.py
+++ b/scripts/run-clang-tidy.py
@@ -51,6 +51,11 @@ def parse_args():
"--cache",
dest="cache",
help="Path to cache directory")
+ parser.add_argument(
+ "--timeout",
+ dest="timeout",
+ type=int,
+ help="Timeout in minutes")
return parser.parse_args()
@@ -132,6 +137,11 @@ def cache_write(filename, result):
def worker():
while True:
item = items.get()
+ if args.timeout and args.timeout < time.time():
+ findings.append("%s (timeout)" % item["file"])
+ items.task_done()
+ continue
+
os.chdir(item["directory"])
cache = cache_name(item)
@@ -164,6 +174,9 @@ if args.cache:
args.cache = os.path.abspath(args.cache)
os.makedirs(args.cache, exist_ok=True)
+if args.timeout:
+ args.timeout = time.time() + args.timeout * 60
+
for _ in range(args.thread_num):
threading.Thread(target=worker, daemon=True).start()
--
2.26.2
2:25 p.m.
New subject: [libvirt PATCH 06/10] clang-tidy: Allow timeouts
With an empty cache, the first run of the clang-tidy scan in the CI will
fail. While the instinctive reaction "press the rerun button" will eventually
lead to the situation where enough files are cached that the entire scan fits
within the time window, this creates friction for e.g. new contributors.
By allowing timeouts to be non-fatal events, we reduce scanning to a "best
effort" approach, that might miss newly introduced regressions.
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
scripts/run-clang-tidy.py | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/scripts/run-clang-tidy.py b/scripts/run-clang-tidy.py
index 19b8640982..54eb0ea584 100755
--- a/scripts/run-clang-tidy.py
+++ b/scripts/run-clang-tidy.py
@@ -56,6 +56,11 @@ def parse_args():
dest="timeout",
type=int,
help="Timeout in minutes")
+ parser.add_argument(
+ "--allow-timeout",
+ dest="allow_timeout",
+ action="store_true",
+ help="Do not treat timeout as failure if set")
return parser.parse_args()
@@ -138,7 +143,8 @@ def worker():
while True:
item = items.get()
if args.timeout and args.timeout < time.time():
- findings.append("%s (timeout)" % item["file"])
+ if not args.allow_timeout:
+ findings.append("%s (timeout)" % item["file"])
items.task_done()
continue
--
2.26.2
2:25 p.m.
New subject: [libvirt PATCH 07/10] clang-tidy: Add shuffle
Randomizing the order of files to scan has no impact for local use of the
script. The same holds true for use in the CI, if the amount of cached
files is big enough for the entire scan to finish before timeout.
If the cache is empty or not filled enough to ensure timely completion,
randomizing the order of files makes it more likely to spent time on caching
new files rather than hashing already cached files to check for the presence
of a cache file.
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
scripts/run-clang-tidy.py | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/scripts/run-clang-tidy.py b/scripts/run-clang-tidy.py
index 54eb0ea584..1d1038df0f 100755
--- a/scripts/run-clang-tidy.py
+++ b/scripts/run-clang-tidy.py
@@ -6,6 +6,7 @@ import json
import multiprocessing
import os
import queue
+import random
import re
import shlex
import subprocess
@@ -61,6 +62,11 @@ def parse_args():
dest="allow_timeout",
action="store_true",
help="Do not treat timeout as failure if set")
+ parser.add_argument(
+ "--shuffle-input",
+ dest="shuffle_input",
+ action="store_true",
+ help="Randomize order of files to check")
return parser.parse_args()
@@ -188,6 +194,8 @@ for _ in range(args.thread_num):
with open(os.path.join(args.build_dir, "compile_commands.json")) as f:
compile_commands = json.load(f)
+ if args.shuffle_input:
+ random.shuffle(compile_commands)
for compile_command in compile_commands:
items.put(compile_command)
--
2.26.2
2:25 p.m.
New subject: [libvirt PATCH 08/10] clang-tidy: Make list of checks explicit
Preparation for next patch.
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
scripts/run-clang-tidy.py | 24 ++++++++++++++++++++----
1 file changed, 20 insertions(+), 4 deletions(-)
diff --git a/scripts/run-clang-tidy.py b/scripts/run-clang-tidy.py
index 1d1038df0f..945a1f75d4 100755
--- a/scripts/run-clang-tidy.py
+++ b/scripts/run-clang-tidy.py
@@ -75,6 +75,7 @@ def run_clang_tidy(item):
cmd = (
"clang-tidy",
"--warnings-as-errors=*",
+ "--checks=-*,%s" % ",".join(checks),
"-p",
item["directory"],
item["file"])
@@ -90,7 +91,7 @@ def run_clang_tidy(item):
}
-def cache_name(item):
+def cache_name(item, checks):
if not args.cache:
return None
@@ -117,6 +118,7 @@ def cache_name(item):
hashsum = hashlib.sha256()
hashsum.update(item["command"].encode())
+ hashsum.update("\n".join(checks).encode())
hashsum.update(result.stdout.encode())
basename = "".join([c if c.isalnum() else "_" for c in
item["output"]])
@@ -145,7 +147,7 @@ def cache_write(filename, result):
json.dump(result, f)
-def worker():
+def worker(checks):
while True:
item = items.get()
if args.timeout and args.timeout < time.time():
@@ -156,7 +158,7 @@ def worker():
os.chdir(item["directory"])
- cache = cache_name(item)
+ cache = cache_name(item, checks)
result = cache_read(cache)
with lock:
print(item["file"], "" if result is None else "(from
cache)")
@@ -177,9 +179,19 @@ def worker():
items.task_done()
+def list_checks():
+ output = subprocess.check_output(
+ ["clang-tidy", "-checks=*", "-list-checks"],
+ universal_newlines=True).split("\n")[1:]
+
+ output = [line.strip() for line in output]
+ return output
+
+
args = parse_args()
items = queue.Queue()
lock = threading.Lock()
+checks = list_checks()
findings = list()
if args.cache:
@@ -189,8 +201,12 @@ if args.cache:
if args.timeout:
args.timeout = time.time() + args.timeout * 60
+print("Enabled checks:")
+for check in checks:
+ print(" %s" % check)
+
for _ in range(args.thread_num):
- threading.Thread(target=worker, daemon=True).start()
+ threading.Thread(target=worker, daemon=True, args=[checks]).start()
with open(os.path.join(args.build_dir, "compile_commands.json")) as f:
compile_commands = json.load(f)
--
2.26.2
2:25 p.m.
New subject: [libvirt PATCH 09/10] clang-tidy: Disable irrelevant and failing checks
clang-tidy's focus is on c++. Disable all checks that do not apply to the
libVirt code base. Also disable all checks that are currently failing, to
prevent introduction of new issues, while we work down the list of existing
issues and / or decide on disabling some checks permanently.
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
scripts/run-clang-tidy.py | 326 ++++++++++++++++++++++++++++++++++++++
1 file changed, 326 insertions(+)
diff --git a/scripts/run-clang-tidy.py b/scripts/run-clang-tidy.py
index 945a1f75d4..24bc034682 100755
--- a/scripts/run-clang-tidy.py
+++ b/scripts/run-clang-tidy.py
@@ -25,6 +25,331 @@ spam = [
]
+disabled_checks = [
+ # aliases for other checks
+ "bugprone-narrowing-conversions",
+ "cert-dcl03-c",
+ "cert-dcl16-c",
+ "cert-dcl54-cpp",
+ "cert-dcl59-cpp",
+ "cert-err61-cpp",
+ "cert-fio38-c",
+ "cert-msc30-c",
+ "cert-msc32-c",
+ "cert-oop11-cpp",
+ "cert-oop54-cpp",
+ "cert-pos44-c",
+ "cppcoreguidelines-avoid-c-arrays",
+ "cppcoreguidelines-avoid-magic-numbers",
+ "cppcoreguidelines-c-copy-assignment-signature",
+ "cppcoreguidelines-explicit-virtual-functions",
+ "cppcoreguidelines-non-private-member-variables-in-classes",
+ "fuchsia-header-anon-namespaces",
+ "google-readability-braces-around-statements",
+ "google-readability-function-size",
+ "google-readability-namespace-comments",
+ "hicpp-avoid-c-arrays",
+ "hicpp-avoid-goto",
+ "hicpp-braces-around-statements",
+ "hicpp-deprecated-headers",
+ "hicpp-explicit-conversions",
+ "hicpp-function-size",
+ "hicpp-invalid-access-moved",
+ "hicpp-member-init",
+ "hicpp-move-const-arg",
+ "hicpp-named-parameter",
+ "hicpp-new-delete-operators",
+ "hicpp-no-array-decay",
+ "hicpp-no-malloc",
+ "hicpp-special-member-functions",
+ "hicpp-static-assert",
+ "hicpp-uppercase-literal-suffix",
+ "hicpp-use-auto",
+ "hicpp-use-emplace",
+ "hicpp-use-equals-default",
+ "hicpp-use-equals-delete",
+ "hicpp-use-noexcept",
+ "hicpp-use-nullptr",
+ "hicpp-use-override",
+ "hicpp-vararg",
+ "llvm-qualified-auto",
+
+ # only relevant for c++
+ "bugprone-copy-constructor-init",
+ "bugprone-dangling-handle",
+ "bugprone-exception-escape",
+ "bugprone-fold-init-type",
+ "bugprone-forward-declaration-namespace",
+ "bugprone-forwarding-reference-overload",
+ "bugprone-inaccurate-erase",
+ "bugprone-lambda-function-name",
+ "bugprone-move-forwarding-reference",
+ "bugprone-parent-virtual-call",
+ "bugprone-sizeof-container",
+ "bugprone-string-constructor",
+ "bugprone-string-integer-assignment",
+ "bugprone-swapped-arguments",
+ "bugprone-throw-keyword-missing",
+ "bugprone-undelegated-constructor",
+ "bugprone-unhandled-self-assignment",
+ "bugprone-unused-raii",
+ "bugprone-unused-return-value",
+ "bugprone-use-after-move",
+ "bugprone-virtual-near-miss",
+ "cert-dcl21-cpp",
+ "cert-dcl50-cpp",
+ "cert-dcl58-cpp",
+ "cert-err09-cpp",
+ "cert-err52-cpp",
+ "cert-err58-cpp",
+ "cert-err60-cpp",
+ "cert-mem57-cpp",
+ "cert-msc50-cpp",
+ "cert-msc51-cpp",
+ "cert-oop58-cpp",
+ "clang-analyzer-cplusplus.InnerPointer",
+ "clang-analyzer-cplusplus.Move",
+ "clang-analyzer-cplusplus.NewDelete",
+ "clang-analyzer-cplusplus.NewDeleteLeaks",
+ "clang-analyzer-cplusplus.PureVirtualCall",
+ "clang-analyzer-cplusplus.SelfAssignment",
+ "clang-analyzer-cplusplus.SmartPtr",
+ "clang-analyzer-cplusplus.VirtualCallModeling",
+ "clang-analyzer-optin.cplusplus.UninitializedObject",
+ "clang-analyzer-optin.cplusplus.VirtualCall",
+ "cppcoreguidelines-no-malloc",
+ "cppcoreguidelines-owning-memory",
+ "cppcoreguidelines-pro-bounds-array-to-pointer-decay",
+ "cppcoreguidelines-pro-bounds-constant-array-index",
+ "cppcoreguidelines-pro-bounds-pointer-arithmetic",
+ "cppcoreguidelines-pro-type-const-cast",
+ "cppcoreguidelines-pro-type-cstyle-cast",
+ "cppcoreguidelines-pro-type-member-init",
+ "cppcoreguidelines-pro-type-reinterpret-cast",
+ "cppcoreguidelines-pro-type-static-cast-downcast",
+ "cppcoreguidelines-pro-type-union-access",
+ "cppcoreguidelines-pro-type-vararg",
+ "cppcoreguidelines-slicing",
+ "cppcoreguidelines-special-member-functions",
+ "fuchsia-default-arguments-calls",
+ "fuchsia-default-arguments-declarations",
+ "fuchsia-multiple-inheritance",
+ "fuchsia-overloaded-operator",
+ "fuchsia-statically-constructed-objects",
+ "fuchsia-trailing-return",
+ "fuchsia-virtual-inheritance",
+ "google-build-explicit-make-pair",
+ "google-build-namespaces",
+ "google-build-using-namespace",
+ "google-default-arguments",
+ "google-explicit-constructor",
+ "google-global-names-in-headers",
+ "google-readability-casting",
+ "google-runtime-operator",
+ "google-runtime-references",
+ "hicpp-exception-baseclass",
+ "hicpp-noexcept-move",
+ "hicpp-undelegated-constructor",
+ "llvm-namespace-comment",
+ "llvm-prefer-isa-or-dyn-cast-in-conditionals",
+ "llvm-prefer-register-over-unsigned",
+ "llvm-twine-local",
+ "misc-new-delete-overloads",
+ "misc-non-private-member-variables-in-classes",
+ "misc-throw-by-value-catch-by-reference",
+ "misc-unconventional-assign-operator",
+ "misc-uniqueptr-reset-release",
+ "misc-unused-using-decls",
+ "modernize-avoid-bind",
+ "modernize-avoid-c-arrays",
+ "modernize-concat-nested-namespaces",
+ "modernize-deprecated-headers",
+ "modernize-deprecated-ios-base-aliases",
+ "modernize-loop-convert",
+ "modernize-make-shared",
+ "modernize-make-unique",
+ "modernize-pass-by-value",
+ "modernize-raw-string-literal",
+ "modernize-redundant-void-arg",
+ "modernize-replace-auto-ptr",
+ "modernize-replace-random-shuffle",
+ "modernize-return-braced-init-list",
+ "modernize-shrink-to-fit",
+ "modernize-unary-static-assert",
+ "modernize-use-auto",
+ "modernize-use-bool-literals",
+ "modernize-use-default-member-init",
+ "modernize-use-emplace",
+ "modernize-use-equals-default",
+ "modernize-use-equals-delete",
+ "modernize-use-nodiscard",
+ "modernize-use-noexcept",
+ "modernize-use-nullptr",
+ "modernize-use-override",
+ "modernize-use-trailing-return-type",
+ "modernize-use-transparent-functors",
+ "modernize-use-uncaught-exceptions",
+ "modernize-use-using",
+ "performance-faster-string-find",
+ "performance-for-range-copy",
+ "performance-implicit-conversion-in-loop",
+ "performance-inefficient-algorithm",
+ "performance-inefficient-string-concatenation",
+ "performance-inefficient-vector-operation",
+ "performance-move-const-arg",
+ "performance-move-constructor-init",
+ "performance-no-automatic-move",
+ "performance-noexcept-move-constructor",
+ "performance-trivially-destructible",
+ "performance-type-promotion-in-math-fn",
+ "performance-unnecessary-copy-initialization",
+ "performance-unnecessary-value-param",
+ "portability-simd-intrinsics",
+ "readability-container-size-empty",
+ "readability-convert-member-functions-to-static",
+ "readability-deleted-default",
+ "readability-make-member-function-const",
+ "readability-qualified-auto",
+ "readability-redundant-access-specifiers",
+ "readability-redundant-member-init",
+ "readability-redundant-smartptr-get",
+ "readability-redundant-string-cstr",
+ "readability-redundant-string-init",
+ "readability-simplify-subscript-expr",
+ "readability-static-accessed-through-instance",
+ "readability-static-definition-in-anonymous-namespace",
+ "readability-string-compare",
+ "readability-uniqueptr-delete-release",
+ "zircon-temporary-objects",
+
+ # only relevant for objective c
+ "clang-analyzer-nullability.NullableDereferenced",
+ "clang-analyzer-nullability.NullablePassedToNonnull",
+ "clang-analyzer-nullability.NullableReturnedFromNonnull",
+ "clang-analyzer-nullability.NullPassedToNonnull",
+ "clang-analyzer-nullability.NullReturnedFromNonnull",
+ "clang-analyzer-optin.osx.cocoa.localizability." +
+ "EmptyLocalizationContextChecker",
+ "clang-analyzer-optin.osx.cocoa.localizability." +
+ "NonLocalizedStringChecker",
+ "clang-analyzer-optin.osx.OSObjectCStyleCast",
+ "clang-analyzer-optin.performance.GCDAntipattern",
+ "google-objc-avoid-nsobject-new",
+ "google-objc-avoid-throwing-exception",
+ "google-objc-function-naming",
+ "google-objc-global-variable-declaration",
+ "objc-avoid-nserror-init",
+ "objc-forbidden-subclassing",
+ "objc-missing-hash",
+ "objc-property-declaration",
+ "objc-super-self",
+
+ # only relevant for certain libraries
+ "abseil-duration-addition",
+ "abseil-duration-comparison",
+ "abseil-duration-conversion-cast",
+ "abseil-duration-division",
+ "abseil-duration-factory-float",
+ "abseil-duration-factory-scale",
+ "abseil-duration-subtraction",
+ "abseil-duration-unnecessary-conversion",
+ "abseil-faster-strsplit-delimiter",
+ "abseil-no-internal-dependencies",
+ "abseil-no-namespace",
+ "abseil-redundant-strcat-calls",
+ "abseil-str-cat-append",
+ "abseil-string-find-startswith",
+ "abseil-time-comparison",
+ "abseil-time-subtraction",
+ "abseil-upgrade-duration-conversions",
+ "boost-use-to-string",
+ "clang-analyzer-optin.mpi.MPI-Checker",
+ "google-readability-avoid-underscore-in-googletest-name",
+ "google-upgrade-googletest-case",
+ "mpi-buffer-deref",
+ "mpi-type-mismatch",
+ "openmp-exception-escape",
+ "openmp-use-default-none",
+
+ # only relevant for osx
+ "clang-analyzer-osx.API",
+ "clang-analyzer-osx.cocoa.AtSync",
+ "clang-analyzer-osx.cocoa.AutoreleaseWrite",
+ "clang-analyzer-osx.cocoa.ClassRelease",
+ "clang-analyzer-osx.cocoa.Dealloc",
+ "clang-analyzer-osx.cocoa.IncompatibleMethodTypes",
+ "clang-analyzer-osx.cocoa.Loops",
+ "clang-analyzer-osx.cocoa.MissingSuperCall",
+ "clang-analyzer-osx.cocoa.NilArg",
+ "clang-analyzer-osx.cocoa.NonNilReturnValue",
+ "clang-analyzer-osx.cocoa.NSAutoreleasePool",
+ "clang-analyzer-osx.cocoa.NSError",
+ "clang-analyzer-osx.cocoa.ObjCGenerics",
+ "clang-analyzer-osx.cocoa.RetainCount",
+ "clang-analyzer-osx.cocoa.RetainCountBase",
+ "clang-analyzer-osx.cocoa.RunLoopAutoreleaseLeak",
+ "clang-analyzer-osx.cocoa.SelfInit",
+ "clang-analyzer-osx.cocoa.SuperDealloc",
+ "clang-analyzer-osx.cocoa.UnusedIvars",
+ "clang-analyzer-osx.cocoa.VariadicMethodTypes",
+ "clang-analyzer-osx.coreFoundation.CFError",
+ "clang-analyzer-osx.coreFoundation.CFNumber",
+ "clang-analyzer-osx.coreFoundation.CFRetainRelease",
+ "clang-analyzer-osx.coreFoundation.containers.OutOfBounds",
+ "clang-analyzer-osx.coreFoundation.containers.PointerSizedValues",
+ "clang-analyzer-osx.MIG",
+ "clang-analyzer-osx.NSOrCFErrorDerefChecker",
+ "clang-analyzer-osx.NumberObjectConversion",
+ "clang-analyzer-osx.ObjCProperty",
+ "clang-analyzer-osx.OSObjectRetainCount",
+ "clang-analyzer-osx.SecKeychainAPI",
+ "darwin-avoid-spinlock",
+ "darwin-dispatch-once-nonstatic",
+
+ # failing checks
+ "android-cloexec-dup",
+ "android-cloexec-fopen",
+ "android-cloexec-open",
+ "android-cloexec-pipe",
+ "bugprone-branch-clone",
+ "bugprone-integer-division",
+ "bugprone-macro-parentheses",
+ "bugprone-signed-char-misuse",
+ "bugprone-sizeof-expression",
+ "bugprone-suspicious-missing-comma",
+ "bugprone-suspicious-string-compare",
+ "cert-err34-c",
+ "clang-analyzer-core.CallAndMessage",
+ "clang-analyzer-core.NonNullParamChecker",
+ "clang-analyzer-core.NullDereference",
+ "clang-analyzer-core.UndefinedBinaryOperatorResult",
+ "clang-analyzer-core.uninitialized.Branch",
+ "clang-analyzer-deadcode.DeadStores",
+ "clang-analyzer-optin.performance.Padding",
+ "clang-analyzer-security.insecureAPI.strcpy",
+ "clang-analyzer-unix.Malloc",
+ "cppcoreguidelines-init-variables",
+ "cppcoreguidelines-interfaces-global-init",
+ "cppcoreguidelines-narrowing-conversions",
+ "hicpp-multiway-paths-covered",
+ "hicpp-no-assembler",
+ "hicpp-signed-bitwise",
+ "llvm-include-order",
+ "readability-braces-around-statements",
+ "readability-else-after-return",
+ "readability-function-size",
+ "readability-inconsistent-declaration-parameter-name",
+ "readability-isolate-declaration",
+ "readability-magic-numbers",
+ "readability-misleading-indentation",
+ "readability-named-parameter",
+ "readability-non-const-parameter",
+ "readability-redundant-control-flow",
+ "readability-redundant-declaration",
+ "readability-uppercase-literal-suffix",
+]
+
+
def remove_spam(output):
retval = list()
for line in output.split("\n"):
@@ -185,6 +510,7 @@ def list_checks():
universal_newlines=True).split("\n")[1:]
output = [line.strip() for line in output]
+ output = [line for line in output if line not in disabled_checks]
return output
--
2.26.2
2:25 p.m.
New subject: [libvirt PATCH 10/10] clang-tidy: Add CI integration
A better solution would be to have an explicit target that creates all
generated files, similar to libvirt-pot-dep.
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
.gitlab-ci.yml | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 88 insertions(+)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 5779b1b8b2..3135632222 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -617,3 +617,91 @@ coverity:
- curl https://scan.coverity.com/builds?project=$COVERITY_SCAN_PROJECT_NAME --form
token=$COVERITY_SCAN_TOKEN --form email=$GITLAB_USER_EMAIL --form file=(a)cov-int.tar.gz
--form version="$(git describe --tags)" --form description="$(git describe
--tags) / $CI_COMMIT_TITLE / $CI_COMMIT_REF_NAME:$CI_PIPELINE_ID"
rules:
- if: "$CI_PIPELINE_SOURCE == 'schedule' &&
$COVERITY_SCAN_PROJECT_NAME && $COVERITY_SCAN_TOKEN"
+
+clang-tidy:
+ image: $CI_REGISTRY_IMAGE/ci-fedora-32:latest
+ needs:
+ - x64-fedora-32-container
+ stage: builds
+ cache:
+ key: "$CI_JOB_NAME-$CI_COMMIT_REF_SLUG"
+ when: always
+ paths:
+ - clang-tidy-cache
+ script:
+ - meson build
+ - ninja -C build src/access/viraccessapicheck.c
+ - ninja -C build src/access/viraccessapicheck.h
+ - ninja -C build src/access/viraccessapichecklxc.c
+ - ninja -C build src/access/viraccessapichecklxc.h
+ - ninja -C build src/access/viraccessapicheckqemu.c
+ - ninja -C build src/access/viraccessapicheckqemu.h
+ - ninja -C build src/admin/admin_client.h
+ - ninja -C build src/admin/admin_protocol.c
+ - ninja -C build src/admin/admin_protocol.h
+ - ninja -C build src/admin/admin_server_dispatch_stubs.h
+ - ninja -C build src/esx/esx_vi.generated.c
+ - ninja -C build src/esx/esx_vi.generated.h
+ - ninja -C build src/esx/esx_vi_methods.generated.c
+ - ninja -C build src/esx/esx_vi_methods.generated.h
+ - ninja -C build src/esx/esx_vi_methods.generated.macro
+ - ninja -C build src/esx/esx_vi_types.generated.c
+ - ninja -C build src/esx/esx_vi_types.generated.h
+ - ninja -C build src/esx/esx_vi_types.generated.typedef
+ - ninja -C build src/esx/esx_vi_types.generated.typeenum
+ - ninja -C build src/esx/esx_vi_types.generated.typefromstring
+ - ninja -C build src/esx/esx_vi_types.generated.typetostring
+ - ninja -C build src/hyperv/hyperv_wmi_classes.generated.c
+ - ninja -C build src/hyperv/hyperv_wmi_classes.generated.h
+ - ninja -C build src/hyperv/hyperv_wmi_classes.generated.typedef
+ - ninja -C build src/libvirt_functions.stp
+ - ninja -C build src/libvirt_probes.h
+ - ninja -C build src/libvirt_probes.stp
+ - ninja -C build src/locking/lock_daemon_dispatch_stubs.h
+ - ninja -C build src/locking/lock_protocol.c
+ - ninja -C build src/locking/lock_protocol.h
+ - ninja -C build src/logging/log_daemon_dispatch_stubs.h
+ - ninja -C build src/logging/log_protocol.c
+ - ninja -C build src/logging/log_protocol.h
+ - ninja -C build src/lxc/lxc_controller_dispatch.h
+ - ninja -C build src/lxc/lxc_monitor_dispatch.h
+ - ninja -C build src/lxc/lxc_monitor_protocol.c
+ - ninja -C build src/lxc/lxc_monitor_protocol.h
+ - ninja -C build src/qemu/libvirt_qemu_probes.h
+ - ninja -C build src/qemu/libvirt_qemu_probes.stp
+ - ninja -C build src/remote/lxc_client_bodies.h
+ - ninja -C build src/remote/lxc_daemon_dispatch_stubs.h
+ - ninja -C build src/remote/lxc_protocol.c
+ - ninja -C build src/remote/lxc_protocol.h
+ - ninja -C build src/remote/qemu_client_bodies.h
+ - ninja -C build src/remote/qemu_daemon_dispatch_stubs.h
+ - ninja -C build src/remote/qemu_protocol.c
+ - ninja -C build src/remote/qemu_protocol.h
+ - ninja -C build src/remote/remote_client_bodies.h
+ - ninja -C build src/remote/remote_daemon_dispatch_stubs.h
+ - ninja -C build src/remote/remote_protocol.c
+ - ninja -C build src/remote/remote_protocol.h
+ - ninja -C build src/rpc/virkeepaliveprotocol.c
+ - ninja -C build src/rpc/virkeepaliveprotocol.h
+ - ninja -C build src/rpc/virnetprotocol.c
+ - ninja -C build src/rpc/virnetprotocol.h
+ - ninja -C build src/util/virkeycodetable_atset1.h
+ - ninja -C build src/util/virkeycodetable_atset2.h
+ - ninja -C build src/util/virkeycodetable_atset3.h
+ - ninja -C build src/util/virkeycodetable_linux.h
+ - ninja -C build src/util/virkeycodetable_osx.h
+ - ninja -C build src/util/virkeycodetable_qnum.h
+ - ninja -C build src/util/virkeycodetable_usb.h
+ - ninja -C build src/util/virkeycodetable_win32.h
+ - ninja -C build src/util/virkeycodetable_xtkbd.h
+ - ninja -C build src/util/virkeynametable_linux.h
+ - ninja -C build src/util/virkeynametable_osx.h
+ - ninja -C build src/util/virkeynametable_win32.h
+ - ninja -C build tools/wireshark/src/libvirt/keepalive.h
+ - ninja -C build tools/wireshark/src/libvirt/lxc.h
+ - ninja -C build tools/wireshark/src/libvirt/protocol.h
+ - ninja -C build tools/wireshark/src/libvirt/qemu.h
+ - ninja -C build tools/wireshark/src/libvirt/remote.h
+ - scripts/run-clang-tidy.py -p build --cache clang-tidy-cache --timeout 30
--allow-timeout --shuffle-input
+ variables:
+ CC: clang
--
2.26.2
2:48 p.m.
On a Friday in 2021, Tim Wiederhake wrote:
"clang-tidy" is a static code analysis tool for c and c++
code. See
https://listman.redhat.com/archives/libvir-list/2021-January/msg01152.html
for some issues found by clang-tidy and more background information.
Meson has support for clang-tidy and generates target named "clang-tidy" if
it finds a ".clang-tidy" configuration file in the project's root
directory.
There are some problems with this approach though, with regards to inclusion
in GitLab's CI:
* Single-threaded runtime of a complete scan takes between 95 and 110 minutes,
depending on the enabled checks, which is significantly longer than GitLab's
maximum run time of 60 minutes, after which jobs are aborted.
That does seem like a deal-breaker. Did you manage to do a successful
run by iteratively building up the cache over multiple jobs? Or by doing
a multi-threaded run?
* Even without this limit on runtime, this new check would double to
triple
the run time of the libVirt pipeline in GitLab.
Running it for every job sounds wasteful - it can be run on schedule
like the coverity job - daily or even weekly.
Also, please never write libvirt like that again.
Jano
* clang-tidy finds a lot of false positives (see link above for
explanation)
and has checks that contradict the libVirt code style (e.g. braces around
blocks). This makes a quite complex configuration in ".clang-tidy" neccessary.
* I was unable to make clang-tidy recognize the settings from the
configuration file for generated files, leading clang-tidy to always add some
checks. These checks were among those that produced false positives.
* The list of enabled / disabled checks in the yaml configuration file is a
quite long string, making it hard to weave in some comments / documentation
on which checks are enabled / disabled for what reason.
This series introduces a new script, "run-clang-tidy.py". This is a
replacement for the script of the same name from clang-tools-extra. It offers
parallel execution, caching of results and a configurable soft-timeout.
Please see the individual commits for more details. Comments welcome.
https://gitlab.com/twiederh/libvirt/-/pipelines/255321968 → "clang-tidy".
Tim
Tim Wiederhake (10):
clang-tidy: Add a simple runner
clang-tidy: Run in parallel
clang-tidy: Filter output
clang-tidy: Add cache
clang-tidy: Add timeout
clang-tidy: Allow timeouts
clang-tidy: Add shuffle
clang-tidy: Make list of checks explicit
clang-tidy: Disable irrelevant and failing checks
clang-tidy: Add CI integration
.gitlab-ci.yml | 88 ++++++
scripts/run-clang-tidy.py | 557 ++++++++++++++++++++++++++++++++++++++
2 files changed, 645 insertions(+)
create mode 100755 scripts/run-clang-tidy.py
--
2.26.2
4:26 p.m.
On Fri, 2021-02-12 at 14:48 +0100, Ján Tomko wrote:
On a Friday in 2021, Tim Wiederhake wrote:
> "clang-tidy" is a static code analysis tool for c and c++ code. See
> https://listman.redhat.com/archives/libvir-list/2021-January/msg01152.html
> for some issues found by clang-tidy and more background
> information.
>
> Meson has support for clang-tidy and generates target named "clang-
> tidy" if
> it finds a ".clang-tidy" configuration file in the project's root
> directory.
>
> There are some problems with this approach though, with regards to
> inclusion
> in GitLab's CI:
>
> * Single-threaded runtime of a complete scan takes between 95 and
> 110 minutes,
> depending on the enabled checks, which is significantly longer than
> GitLab's
> maximum run time of 60 minutes, after which jobs are aborted.
>
That does seem like a deal-breaker. Did you manage to do a successful
run by iteratively building up the cache over multiple jobs? Or by
doing
a multi-threaded run?
I combined the soft-timeout (./scripts/run-clang-tidy.py --timeout ...)
together with caching. What this does is to stop scanning after the
specified amount of time, but will keep the cache.
Consecutive runs will pick up the cache and hopefully get further down
the list of scanned files before running out of time. This process
converges at a point where there are so few (if any) uncached results
left, that the scan completes.
The benefit of this approach is, that it actually works, takes no
longer than 30 minutes and does not require one to click "re-run"
several times until the job becomes green. And in the best case where
no code file changed aka. all cache files are recent, this reduces the
run time of the clang-tidy job to roughly three minutes; see link to
pipeline below. The first iteration did run into timeout, the second
one finished within the time window. I clicked "rerun" a second time to
get the minimum required time I quoted above.
The downside of this approach is, that for "incomplete" runs, aka when
we hit the timeout, not all files are scanned and the code may still
contain issues that would be flagged in a consecutive run.
> * Even without this limit on runtime, this new check would
double
> to triple
> the run time of the libVirt pipeline in GitLab.
>
Running it for every job sounds wasteful - it can be run on schedule
like the coverity job - daily or even weekly.
Also, please never write libvirt like that again.
Jano
I have no idea what I mixed up libvirt with, which is capitalized this
way. Good thing it's Friday.
Tim
> * clang-tidy finds a lot of false positives (see link above for
> explanation)
> and has checks that contradict the libVirt code style (e.g. braces
> around
> blocks). This makes a quite complex configuration in ".clang-tidy"
> neccessary.
>
> * I was unable to make clang-tidy recognize the settings from the
> configuration file for generated files, leading clang-tidy to
> always add some
> checks. These checks were among those that produced false
> positives.
>
> * The list of enabled / disabled checks in the yaml configuration
> file is a
> quite long string, making it hard to weave in some comments /
> documentation
> on which checks are enabled / disabled for what reason.
>
> This series introduces a new script, "run-clang-tidy.py". This is a
> replacement for the script of the same name from clang-tools-extra.
> It offers
> parallel execution, caching of results and a configurable soft-
> timeout.
>
> Please see the individual commits for more details. Comments
> welcome.
>
> https://gitlab.com/twiederh/libvirt/-/pipelines/255321968 → "clang-
> tidy".
>
> Tim
>
> Tim Wiederhake (10):
> clang-tidy: Add a simple runner
> clang-tidy: Run in parallel
> clang-tidy: Filter output
> clang-tidy: Add cache
> clang-tidy: Add timeout
> clang-tidy: Allow timeouts
> clang-tidy: Add shuffle
> clang-tidy: Make list of checks explicit
> clang-tidy: Disable irrelevant and failing checks
> clang-tidy: Add CI integration
>
> .gitlab-ci.yml | 88 ++++++
> scripts/run-clang-tidy.py | 557
> ++++++++++++++++++++++++++++++++++++++
> 2 files changed, 645 insertions(+)
> create mode 100755 scripts/run-clang-tidy.py
>
> --
> 2.26.2
>
>
3:12 p.m.
On Fri, Feb 12, 2021 at 02:25:24PM +0100, Tim Wiederhake wrote:
"clang-tidy" is a static code analysis tool for c and c++
code. See
https://listman.redhat.com/archives/libvir-list/2021-January/msg01152.html
for some issues found by clang-tidy and more background information.
Meson has support for clang-tidy and generates target named "clang-tidy" if
it finds a ".clang-tidy" configuration file in the project's root
directory.
There are some problems with this approach though, with regards to inclusion
in GitLab's CI:
* Single-threaded runtime of a complete scan takes between 95 and 110 minutes,
depending on the enabled checks, which is significantly longer than GitLab's
maximum run time of 60 minutes, after which jobs are aborted.
IIUC, you can override the timeout setting per job...
* Even without this limit on runtime, this new check would double to triple
the run time of the libVirt pipeline in GitLab.
Yeah that's a problem, but bear in mind there are two separate scenarios
Running post merge we need to check all files, but the length of time
is a non-issue since no one is waiting for a post-merge check to
complete.
Running pre- merge we only need to check files which are modified by
the patches on the current branch. That ought to be orders of magnitude
faster.
* clang-tidy finds a lot of false positives (see link above for explanation)
and has checks that contradict the libVirt code style (e.g. braces around
blocks). This makes a quite complex configuration in ".clang-tidy" neccessary.
IMHO for code checkers to add real value to people you need the existing
codebase in git to be 100% clean.
If you're not going to make the existing code compliant, then users will
never be sure of what they should do for new code. We see this all the
time in QEMU where its patch checker complains about problems that were
pre-existing.
If we could get clang-tidy to do a subset of checks where we can malke
libvirt 100% clean
* I was unable to make clang-tidy recognize the settings from the
configuration file for generated files, leading clang-tidy to always add some
checks. These checks were among those that produced false positives.
* The list of enabled / disabled checks in the yaml configuration file is a
quite long string, making it hard to weave in some comments / documentation
on which checks are enabled / disabled for what reason.
This series introduces a new script, "run-clang-tidy.py". This is a
replacement for the script of the same name from clang-tools-extra. It offers
parallel execution, caching of results and a configurable soft-timeout.
Please see the individual commits for more details. Comments welcome.
https://gitlab.com/twiederh/libvirt/-/pipelines/255321968 → "clang-tidy".
What am I missing here - that job just seems to show a list of
filenames, but isn't reporting any issues for them ?
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
4:26 p.m.
On Fri, 2021-02-12 at 14:12 +0000, Daniel P. Berrangé wrote:
On Fri, Feb 12, 2021 at 02:25:24PM +0100, Tim Wiederhake wrote:
> "clang-tidy" is a static code analysis tool for c and c++ code. See
> https://listman.redhat.com/archives/libvir-list/2021-January/msg01152.html
> for some issues found by clang-tidy and more background
> information.
>
> Meson has support for clang-tidy and generates target named "clang-
> tidy" if
> it finds a ".clang-tidy" configuration file in the project's root
> directory.
>
> There are some problems with this approach though, with regards to
> inclusion
> in GitLab's CI:
>
> * Single-threaded runtime of a complete scan takes between 95 and
> 110 minutes,
> depending on the enabled checks, which is significantly longer than
> GitLab's
> maximum run time of 60 minutes, after which jobs are aborted.
IIUC, you can override the timeout setting per job...
IMO, extending the "hard" timeout for the job is not an option anyway,
for the reason presented in the next point:
> * Even without this limit on runtime, this new check would double
> to triple
> the run time of the libVirt pipeline in GitLab.
↑ This one.
Yeah that's a problem, but bear in mind there are two separate
scenarios
Running post merge we need to check all files, but the length of time
is a non-issue since no one is waiting for a post-merge check to
complete.
I am vary of the possibility of the pre-merge job running successfully
(e.g. because it ran into timeout before finding a certain issue) and
then failing for the post-merge job. One idea would be to set the post-
merge job to "allow-failure: true", i.e. making it a "notification"
only. But I am absolutely unsure on what the best approach is here,
hence the "RFC".
Running pre- merge we only need to check files which are modified by
the patches on the current branch. That ought to be orders of
magnitude
faster.
I see where you are coming from, but I believe this does not catch
cases where e.g. a change in a header file leads to a clang-tidy issue
in an otherwise untouched .c file, or vice versa.
The approach to cache files keyed on the result of the preprocessor
stage ("hash(gcc -E file.c)") on the other hand does catch this kind of
changes and reruns clang-tidy for all affected files.
> * clang-tidy finds a lot of false positives (see link above for
> explanation)
> and has checks that contradict the libVirt code style (e.g. braces
> around
> blocks). This makes a quite complex configuration in ".clang-tidy"
> necessary.
IMHO for code checkers to add real value to people you need the
existing
codebase in git to be 100% clean.
If you're not going to make the existing code compliant, then users
will
never be sure of what they should do for new code. We see this all
the
time in QEMU where its patch checker complains about problems that
were
pre-existing.
If we could get clang-tidy to do a subset of checks where we can make
libvirt 100% clean
I believe there is a misunderstanding here, please see explanation
below.
> * I was unable to make clang-tidy recognize the settings from
the
> configuration file for generated files, leading clang-tidy to
> always add some
> checks. These checks were among those that produced false
> positives.
>
> * The list of enabled / disabled checks in the yaml configuration
> file is a
> quite long string, making it hard to weave in some comments /
> documentation
> on which checks are enabled / disabled for what reason.
>
> This series introduces a new script, "run-clang-tidy.py". This is a
> replacement for the script of the same name from clang-tools-extra.
> It offers
> parallel execution, caching of results and a configurable soft-
> timeout.
>
> Please see the individual commits for more details. Comments
> welcome.
>
> https://gitlab.com/twiederh/libvirt/-/pipelines/255321968 → "clang-
> tidy".
What am I missing here - that job just seems to show a list of
filenames, but isn't reporting any issues for them ?
There are three "types" of checks:
* Irrelevant checks that we want to disable permanently, e.g. checks
for some c++ specific issue that does not apply to c code,
* Checks that the libvirt code base currently passes,
* Checks that the libvirt code base currently fails.
Path #09 disables the first and last category of checks. Note that the
list is not defined in terms of "enable these checks" but "disable
these checks". The idea is to have checks that are introduced in new
versions of clang-tidy enabled by default and disabled checks commented
on and justified.
This is why you see no findings in the output -- there are none,
currently only "passing" checks are enabled. I disabled all failing
checks indiscriminately, as a discussion of which particular checks are
actually useful and which are not would distract from the general
question of "Do we want clang-tidy integration in the CI and if so,
how".
See for example the check "readability-braces-around-statements" [1],
which mandates braces around single-line "if" or "while" blocks,
which
contradicts the libvirt code style. I believe that the verdict for this
check will be permanent disablement. Other checks, for example "cert-
err34-c" [2] are issues that in my opinion should be fixed and I
started doing so already, see [3].
For comparison, have a look at [4]: This is the output of run-clang-
tidy.py with only the "irrelevant" checks disabled.
Regards,
Daniel
Regards,
Tim
[1]
https://clang.llvm.org/extra/clang-tidy/checks/readability-braces-around-...
[2] https://clang.llvm.org/extra/clang-tidy/checks/cert-err34-c.html
[3]
https://listman.redhat.com/archives/libvir-list/2021-January/msg01152.html
[4] https://gitlab.com/twiederh/libvirt/-/jobs/1026797599
1315
days inactive
1315
days old
14 comments
3 participants
participants (3)
-
Daniel P. Berrangé -
Ján Tomko -
Tim Wiederhake