From c76a681a88d1c043699b26386e9a3a44bf2149dc Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 24 Mar 2022 16:52:54 -0400 Subject: [PATCH] Adding a clang-tidy helper script that is outside the build process. (#16382) * Make clang variant as a requirement for asan/tsan builds, add support for required variants for build systems * Restyle * Also update workflows * Adding a clang-tidy helper script that is outside the build process. Replaces the pw_command_runner path we had before and allows independent execution of the checker. * Remove pw command launcher from darwin as well. clang tidy on linux should be sufficient for now * Use clang builds and validate more compile databases * Adjust test group ordering since we have placed clang as a last variant * More moving of chip tool variants to make the options in yaml file make sense * add missin $ for var specifier * Add clang variant to tsan * Asan/tsan not limited by clang, so updated as such * Restyle * Ensure darwin clang-tidy is also run * Ensure compile commands are exported * Update to use same coverage for tidy for linux as well as before * Undo changes to TestCommand * Remove modernize-nullptr for now: it is still too strict * Select individual OS compilations and do not compile gcc variants on mac * It looks like IOS is always compiled with gcc - see build/toolchain/build/ios. Do not attempt to clang-enable it nor clang-tidy * Tidy gcc/g++ as well * fix typo * Remove PWcommandlauncher from default build as well * Bump up the timeout value for clang validation a lot just in case * Make code easier to read * Fix darwin paths: when using gcc/g++, sysroot is required * More robust gcc finding of sysroot * Typo fix and restyle * Disabled optin-osx-cocoa-localizability-emptylocalizationcontextchecker-objc for clang tidy default * Fix optin to be case correct: clang-analyzer-optin.osx.cocoa.localizability.EmptyLocalizationContextChecker --- .clang-tidy | 2 +- .github/workflows/build.yaml | 41 ++- scripts/helpers/clang-tidy-launcher.py | 74 ---- scripts/run-clang-tidy-on-compile-commands.py | 341 ++++++++++++++++++ 4 files changed, 371 insertions(+), 87 deletions(-) delete mode 100755 scripts/helpers/clang-tidy-launcher.py create mode 100755 scripts/run-clang-tidy-on-compile-commands.py diff --git a/.clang-tidy b/.clang-tidy index cda8ea53a5f4cc..1998473bf95879 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -1,4 +1,4 @@ --- -Checks: 'bugprone-*,-bugprone-not-null-terminated-result,-bugprone-suspicious-memory-comparison,-bugprone-argument-comment,-bugprone-unused-return-value,-bugprone-branch-clone,-bugprone-easily-swappable-parameters,-bugprone-reserved-identifier,-bugprone-macro-parentheses,-bugprone-forward-declaration-namespace,-bugprone-forwarding-reference-overload,-bugprone-undelegated-constructor,-bugprone-sizeof-expression,-bugprone-implicit-widening-of-multiplication-result,-bugprone-too-small-loop-variable,-bugprone-narrowing-conversions,-bugprone-misplaced-widening-cast,-bugprone-suspicious-include,-bugprone-signed-char-misuse,-bugprone-copy-constructor-init,-clang-analyzer-core.CallAndMessage,-clang-analyzer-core.UndefinedBinaryOperatorResult,-clang-analyzer-core.NullDereference,-clang-analyzer-optin.cplusplus.UninitializedObject,-clang-analyzer-core.uninitialized.Branch,-clang-analyzer-optin.performance,-clang-analyzer-deadcode.DeadStores,-clang-analyzer-cplusplus.Move,-clang-analyzer-optin.cplusplus.VirtualCall,-clang-analyzer-security.insecureAPI.strcpy,-clang-analyzer-nullability.NullablePassedToNonnull,-clang-analyzer-optin.performance.Padding,-clang-analyzer-security.insecureAPI.bzero,-clang-analyzer-unix.cstring.NullArg,-clang-analyzer-security.insecureAPI.rand,-clang-analyzer-core.NonNullParamChecker,-clang-analyzer-nullability.NullPassedToNonnull,-clang-analyzer-unix.Malloc,-clang-analyzer-valist.Unterminated,-clang-analyzer-cplusplus.NewDeleteLeaks,-clang-diagnostic-implicit-int-conversion' +Checks: 'bugprone-*,-bugprone-not-null-terminated-result,-bugprone-suspicious-memory-comparison,-bugprone-argument-comment,-bugprone-unused-return-value,-bugprone-branch-clone,-bugprone-easily-swappable-parameters,-bugprone-reserved-identifier,-bugprone-macro-parentheses,-bugprone-forward-declaration-namespace,-bugprone-forwarding-reference-overload,-bugprone-undelegated-constructor,-bugprone-sizeof-expression,-bugprone-implicit-widening-of-multiplication-result,-bugprone-too-small-loop-variable,-bugprone-narrowing-conversions,-bugprone-misplaced-widening-cast,-bugprone-suspicious-include,-bugprone-signed-char-misuse,-bugprone-copy-constructor-init,-clang-analyzer-core.CallAndMessage,-clang-analyzer-core.UndefinedBinaryOperatorResult,-clang-analyzer-core.NullDereference,-clang-analyzer-optin.cplusplus.UninitializedObject,-clang-analyzer-core.uninitialized.Branch,-clang-analyzer-optin.performance,-clang-analyzer-optin.osx.cocoa.localizability.EmptyLocalizationContextChecker,-clang-analyzer-deadcode.DeadStores,-clang-analyzer-cplusplus.Move,-clang-analyzer-optin.cplusplus.VirtualCall,-clang-analyzer-security.insecureAPI.strcpy,-clang-analyzer-nullability.NullablePassedToNonnull,-clang-analyzer-optin.performance.Padding,-clang-analyzer-security.insecureAPI.bzero,-clang-analyzer-unix.cstring.NullArg,-clang-analyzer-security.insecureAPI.rand,-clang-analyzer-core.NonNullParamChecker,-clang-analyzer-nullability.NullPassedToNonnull,-clang-analyzer-unix.Malloc,-clang-analyzer-valist.Unterminated,-clang-analyzer-cplusplus.NewDeleteLeaks,-clang-diagnostic-implicit-int-conversion' WarningsAsErrors: '*' HeaderFilterRegex: '(src|examples|zzz_generated|credentials)' diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index 6e220b7ab0bd47..92adeac81dd3cb 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -160,13 +160,22 @@ jobs: for BUILD_TYPE in gcc_release clang; do case $BUILD_TYPE in "gcc_release") GN_ARGS='is_debug=false';; - "clang") GN_ARGS='is_clang=true pw_command_launcher="`pwd`/../scripts/helpers/clang-tidy-launcher.py"';; + "clang") GN_ARGS='is_clang=true';; esac - scripts/build/gn_gen.sh --args="$GN_ARGS" - scripts/run_in_build_env.sh "ninja -C ./out" - scripts/tests/gn_tests.sh + BUILD_TYPE=$BUILD_TYPE scripts/build/gn_gen.sh --args="$GN_ARGS" --export-compile-commands + scripts/run_in_build_env.sh "ninja -C ./out/$BUILD_TYPE" + BUILD_TYPE=$BUILD_TYPE scripts/tests/gn_tests.sh done + - name: Clang-tidy validation + timeout-minutes: 45 + run: | + ./scripts/run_in_build_env.sh \ + "./scripts/run-clang-tidy-on-compile-commands.py \ + --no-log-timestamps \ + --compile-database out/clang/compile_commands.json \ + check \ + " - name: Run Tests with sanitizers timeout-minutes: 30 env: @@ -189,13 +198,12 @@ jobs: run: | ./scripts/run_in_build_env.sh \ "./scripts/build/build_examples.py --no-log-timestamps \ - --target linux-x64-all-clusters-ipv6only \ - --target linux-x64-chip-tool-ipv6only \ - --target linux-x64-minmdns-ipv6only \ + --target linux-x64-all-clusters-ipv6only-clang \ + --target linux-x64-chip-tool-ipv6only-clang \ + --target linux-x64-minmdns-ipv6only-clang \ --target linux-x64-rpc-console \ build \ " - - name: Run fake linux tests with build_examples timeout-minutes: 15 run: | @@ -335,13 +343,22 @@ jobs: # use that on Darwin. # * the "host clang" build, which uses the pigweed # clang. - "default") GN_ARGS='target_os="all" is_asan=true enable_host_clang_build=false enable_host_gcc_mbedtls_build=false pw_command_launcher="`pwd`/../scripts/helpers/clang-tidy-launcher.py"';; + "default") GN_ARGS='target_os="all" is_asan=true enable_host_clang_build=false enable_host_gcc_mbedtls_build=false';; "python_lib") GN_ARGS='enable_rtti=true enable_pylib=true';; esac - scripts/build/gn_gen.sh --args="$GN_ARGS" - scripts/run_in_build_env.sh "ninja -C ./out" - scripts/tests/gn_tests.sh + BUILD_TYPE=$BUILD_TYPE scripts/build/gn_gen.sh --args="$GN_ARGS" --export-compile-commands + scripts/run_in_build_env.sh "ninja -C ./out/$BUILD_TYPE" + BUILD_TYPE=$BUILD_TYPE scripts/tests/gn_tests.sh done + - name: Clang-tidy validation + timeout-minutes: 45 + run: | + ./scripts/run_in_build_env.sh \ + "./scripts/run-clang-tidy-on-compile-commands.py \ + --no-log-timestamps \ + --compile-database out/default/compile_commands.json \ + check \ + " - name: Uploading diagnostic logs uses: actions/upload-artifact@v2 if: ${{ failure() }} && ${{ !env.ACT }} diff --git a/scripts/helpers/clang-tidy-launcher.py b/scripts/helpers/clang-tidy-launcher.py deleted file mode 100755 index 157f354ce5addd..00000000000000 --- a/scripts/helpers/clang-tidy-launcher.py +++ /dev/null @@ -1,74 +0,0 @@ -#!/usr/bin/env -S python3 -B - -# Copyright (c) 2021 Project CHIP Authors -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -import subprocess -import sys - - -def consumeArgument(args): - if len(args) == 0: - return None - return args.pop(0) - - -def maybeRunClangTidy(args): - # If the command is not a clang command, do no try to run clang-tidy on it - cc = consumeArgument(args) - if cc is None or not cc.startswith("clang"): - return 0 - - clang_tidy_srcs = [] - clang_tidy_args = [] - - # If warnings comes from third_party, ignore it as there is little we can do. - ignored_list = ['third_party/mbedtls', 'third_party/lwip'] - - arg = consumeArgument(args) - while arg: - if arg == "-c": - sourceFile = consumeArgument(args) - if sourceFile is None: - return 1 - - for name in ignored_list: - if name in sourceFile: - return 0 - - clang_tidy_srcs.append(sourceFile) - - elif arg == "-o": - objectFile = consumeArgument(args) - if objectFile is None: - return 1 - else: - clang_tidy_args.append(arg) - - arg = consumeArgument(args) - - command = ["clang-tidy"] + clang_tidy_srcs + ["--"] + clang_tidy_args - return subprocess.run(command).returncode - - -def main(): - returnCode = maybeRunClangTidy(sys.argv[1:]) - if returnCode != 0: - return returnCode - - return subprocess.run(sys.argv[1:]).returncode - - -if __name__ == "__main__": - sys.exit(main()) diff --git a/scripts/run-clang-tidy-on-compile-commands.py b/scripts/run-clang-tidy-on-compile-commands.py new file mode 100755 index 00000000000000..2de0944671c7aa --- /dev/null +++ b/scripts/run-clang-tidy-on-compile-commands.py @@ -0,0 +1,341 @@ +#!/usr/bin/env python +# +# Runs clang-tidy on files based on a `compile_commands.json` file +# + +""" +Run clang-tidy in parallel on compile databases. + +Example run: + +# This prepares the build. NOTE this is `build` not `gen` because the build +# step generates required header files (this can be simplified if needed +# to invoke ninja to compile only generated files if needed) + +./scripts/build/build_examples.py --target linux-x64-chip-tool-clang build + +# Actually running clang-tidy to check status + +./scripts/run-clang-tidy-on-compile-commands.py check + +""" + +import build +import click +import coloredlogs +import glob +import json +import logging +import multiprocessing +import os +import re +import shlex +import subprocess +import sys +import threading +import traceback +import queue + + +class TidyResult: + def __init__(self, path: str, ok: bool): + self.path = path + self.ok = ok + + def __repr__(self): + if self.ok: + status = "OK" + else: + status = "FAIL" + + return "%s(%s)" % (status, self.path) + + def __str__(self): + return self.__repr__() + + +class ClangTidyEntry: + """Represents a single entry for running clang-tidy based + on a compile_commands.json item. + """ + + def __init__(self, json_entry, gcc_sysroot=None): + # Entries in compile_commands: + # - "directory": location to run the compile + # - "file": a relative path to directory + # - "command": full compilation command + + self.directory = json_entry["directory"] + self.file = json_entry["file"] + self.valid = False + self.clang_arguments = [] + self.tidy_arguments = [] + + command = json_entry["command"] + + command_items = shlex.split(command) + compiler = os.path.basename(command_items[0]) + + # Allow gcc/g++ invocations to also be tidied - arguments should be + # compatible and on darwin gcc/g++ is actually a symlink to clang + if compiler in ['clang++', 'clang', 'gcc', 'g++']: + self.valid = True + self.clang_arguments = command_items[1:] + else: + logging.warning( + "Cannot tidy %s - not a clang compile command", self.file) + return + + if compiler in ['gcc', 'g++'] and gcc_sysroot: + self.clang_arguments.insert(0, '--sysroot='+gcc_sysroot) + + @property + def full_path(self): + return os.path.abspath(os.path.join(self.directory, self.file)) + + def ExportFixesTo(self, f: str): + self.tidy_arguments.append("--export-fixes") + self.tidy_arguments.append(f) + + def Check(self): + logging.debug("Running tidy on %s from %s", self.file, self.directory) + try: + cmd = ["clang-tidy", self.file, "--"] + self.clang_arguments + logging.debug("Executing: %r" % cmd) + + proc = subprocess.Popen( + cmd, + cwd=self.directory, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + ) + output, err = proc.communicate() + if output: + logging.info("TIDY %s: %s", self.file, output.decode("utf-8")) + + if err: + logging.warning("TIDY %s: %s", self.file, err.decode("utf-8")) + + if proc.returncode != 0: + if proc.returncode < 0: + logging.error( + "Failed %s with signal %d", self.file, -proc.returncode + ) + else: + logging.warning( + "Tidy %s ended with code %d", self.file, proc.returncode + ) + return TidyResult(self.full_path, False) + except: + traceback.print_exc() + return TidyResult(self.full_path, False) + + return TidyResult(self.full_path, True) + + +class TidyState: + def __init__(self): + self.successes = 0 + self.failures = 0 + self.lock = threading.Lock() + self.failed_files = [] + + def Success(self): + with self.lock: + self.successes += 1 + + def Failure(self, path: str): + with self.lock: + self.failures += 1 + self.failed_files.append(path) + logging.error("Failed to process %s", path) + + +def find_darwin_gcc_sysroot(): + for line in subprocess.check_output('xcodebuild -sdk -version'.split()).decode('utf8').split('\n'): + if not line.startswith('Path: '): + continue + path = line[line.find(': ')+2:] + if not '/MacOSX.platform/' in path: + continue + logging.info("Found %s" % path) + return path + + # A hard-coded value that works on default installations + logging.warning("Using default platform sdk path. This may be incorrect.") + return '/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk' + + +class ClangTidyRunner: + """Handles running clang-tidy""" + + def __init__(self): + self.entries = [] + self.state = TidyState() + self.gcc_sysroot = None + + if sys.platform == 'darwin': + # Darwin gcc invocation will auto select a system root, however clang requires an explicit path since + # we are using the built-in pigweed clang-tidy. + logging.info( + 'Searching for a MacOS system root for gcc invocations...') + self.gcc_sysroot = find_darwin_gcc_sysroot() + logging.info(' Chose: %s' % self.gcc_sysroot) + + def AddDatabase(self, compile_commands_json): + database = json.load(open(compile_commands_json)) + + for entry in database: + item = ClangTidyEntry(entry, self.gcc_sysroot) + if not item.valid: + continue + + self.entries.append(item) + + def ExportFixesTo(self, f): + # use absolute path since running things will change working directories + f = os.path.abspath(f) + for e in self.entries: + e.ExportFixesTo(f) + + def FilterEntries(self, f): + for e in self.entries: + if not f(e): + logging.info("Skipping %s in %s", e.file, e.directory) + self.entries = [e for e in self.entries if f(e)] + + def CheckThread(self, task_queue): + while True: + entry = task_queue.get() + status = entry.Check() + + if status.ok: + self.state.Success() + else: + self.state.Failure(status.path) + + task_queue.task_done() + + def Check(self): + count = multiprocessing.cpu_count() + task_queue = queue.Queue(count) + + for _ in range(count): + t = threading.Thread(target=self.CheckThread, args=(task_queue,)) + t.daemon = True + t.start() + + for e in self.entries: + task_queue.put(e) + task_queue.join() + + logging.info("Successfully processed %d paths", self.state.successes) + logging.info("Failed to process %d paths", self.state.failures) + if self.state.failures: + for name in self.state.failed_files: + logging.warning("Failure reported for %s", name) + + sys.exit(1) + + +# Supported log levels, mapping string values required for argument +# parsing into logging constants +__LOG_LEVELS__ = { + "debug": logging.DEBUG, + "info": logging.INFO, + "warn": logging.WARN, + "fatal": logging.FATAL, +} + + +@click.group(chain=True) +@click.option( + "--compile-database", + default=[], + multiple=True, + help="Path to `compile_commands.json` to use for executing clang-tidy.", +) +@click.option( + "--file-include-regex", + default="/(src|examples)/", + help="regular expression to apply to the file paths for running.", +) +@click.option( + "--file-exclude-regex", + # NOTE: if trying '/third_party/' note that a lot of sources are routed through + # paths like `../../examples/chip-tool/third_party/connectedhomeip/src/` + default="/(repo|zzz_generated)/", + help="Regular expression to apply to the file paths for running. Skip overrides includes.", +) +@click.option( + "--log-level", + default="INFO", + type=click.Choice(__LOG_LEVELS__.keys(), case_sensitive=False), + help="Determines the verbosity of script output.", +) +@click.option( + "--no-log-timestamps", + default=False, + is_flag=True, + help="Skip timestaps in log output", +) +@click.option( + "--export-fixes", + default=None, + type=click.Path(), + help="Where to export fixes to apply. TODO(fix apply not yet implemented).", +) +@click.pass_context +def main( + context, + compile_database, + file_include_regex, + file_exclude_regex, + log_level, + no_log_timestamps, + export_fixes, +): + log_fmt = "%(asctime)s %(levelname)-7s %(message)s" + if no_log_timestamps: + log_fmt = "%(levelname)-7s %(message)s" + coloredlogs.install(level=__LOG_LEVELS__[log_level], fmt=log_fmt) + + if not compile_database: + logging.warning( + "Compilation database file not provided. Searching for first item in ./out" + ) + compile_database = next( + glob.iglob("./out/**/compile_commands.json", recursive=True) + ) + if not compile_database: + raise Exception("Could not find `compile_commands.json` in ./out") + logging.info("Will use %s for compile", compile_database) + + context.obj = ClangTidyRunner() + + for name in compile_database: + context.obj.AddDatabase(name) + + if file_include_regex: + r = re.compile(file_include_regex) + context.obj.FilterEntries(lambda e: r.search(e.file)) + + if file_exclude_regex: + r = re.compile(file_exclude_regex) + context.obj.FilterEntries(lambda e: not r.search(e.file)) + + if export_fixes: + context.obj.ExportFixesTo(export_fixes) + + for e in context.obj.entries: + logging.info("Will tidy %s", e.full_path) + + +@main.command("check", help="Run clang-tidy check") +@click.pass_context +def cmd_check(context): + context.obj.Check() + + +if __name__ == "__main__": + main()