[bazel] Add aspects to run clang-tidy

dmcardle · dmcardle · commit f7bc9c398e21 · 2023-05-15T10:07:09.000-04:00
This commit adds "fix" and "check" aspects that run clang-tidy.

Example usage:

    ./bazelisk.sh build --config=clang_tidy_fix \
      --config=riscv32 //sw/device/lib/base:memory

    ./bazelisk.sh build --config=clang_tidy_check \
      --config=riscv32 //sw/device/lib/base:memory

This also adds a new target, //quality:clang_tidy_check, which runs
clang-tidy against a very incomplete list of hand-picked targets.

Signed-off-by: Dan McArdle &lt;dmcardle@opentitan.org&gt;
diff --git a/.bazelrc b/.bazelrc
@@ -72,6 +72,20 @@ test:local_test_jobs_per_cpus --local_test_jobs=HOST_CPUS*0.22
 # We have verilator tests that take more than an hour to complete
 test --test_timeout=60,300,1500,7200
 
+# Configuration to run clang-tidy alongside build actions. This is a strict
+# analysis, which will fail the build when any of its checks fail. Enable with
+# `--config=clang_tidy_check`.
+build:clang_tidy_check --aspects rules/quality.bzl%clang_tidy_check_aspect
+build:clang_tidy_check --output_groups=clang_tidy
+
+# Configuration to run clang-tidy alongside build actions. In this mode, fixes
+# will be applied automatically when possible. This is not a strict analysis;
+# clang-tidy errors will not fail the build. Enable with
+# `--config=clang_tidy_check`.
+build:clang_tidy_fix --aspects rules/quality.bzl%clang_tidy_fix_aspect
+build:clang_tidy_fix --output_groups=clang_tidy
+build:clang_tidy_fix --spawn_strategy=local
+
 # AddressSanitizer (ASan) catches runtime out-of-bounds accesses to globals, the
 # stack, and (less importantly for OpenTitan) the heap. ASan instruments
 # programs at compile time and also requires a runtime library.
diff --git a/quality/BUILD.bazel b/quality/BUILD.bazel
@@ -4,7 +4,7 @@
 
 load("@ot_hack_bazelbuild_buildtools//buildifier:def.bzl", "buildifier", "buildifier_test")
 load("@lowrisc_lint//rules:rules.bzl", "licence_test")
-load("//rules:quality.bzl", "clang_format_check", "clang_format_test", "html_coverage_report")
+load("//rules:quality.bzl", "clang_format_check", "clang_format_test", "clang_tidy_check", "clang_tidy_check_rv", "html_coverage_report")
 load("@rules_rust//rust:defs.bzl", "rustfmt_test")
 
 package(default_visibility = ["//visibility:public"])
@@ -103,6 +103,45 @@ clang_format_check(
     mode = "fix",
 )
 
+# TODO(dmcardle) Add more targets that should be automatically checked with
+# clang-tidy. Find a way to automate this.
+clang_tidy_check_rv(
+    name = "clang_tidy_check_rv",
+    testonly = True,
+    deps = [
+        "//sw/device/lib/base:memory",
+        "//sw/device/lib/base:memory_perftest",
+        "//sw/device/silicon_creator/rom:boot_policy",
+        "//sw/device/silicon_creator/rom:bootstrap",
+        "//sw/device/silicon_creator/rom:rom_common",
+        "//sw/device/silicon_creator/rom:rom_epmp",
+        "//sw/device/silicon_creator/rom:rom_start",
+        "//sw/device/silicon_creator/rom:sigverify_keys",
+        "//sw/device/silicon_creator/rom:sigverify_keys_rsa",
+        "//sw/device/silicon_creator/rom:sigverify_keys_spx",
+        "//sw/device/silicon_creator/rom/keys/fake",
+        "//sw/device/silicon_creator/rom/keys/fake/spx:fake",
+    ],
+)
+
+clang_tidy_check(
+    name = "clang_tidy_check_host",
+    testonly = True,
+    deps = [
+        "//sw/device/silicon_creator/rom:boot_policy_unittest",
+        "//sw/device/silicon_creator/rom:bootstrap_unittest",
+    ],
+)
+
+filegroup(
+    name = "clang_tidy_check",
+    testonly = True,
+    srcs = [
+        ":clang_tidy_check_host",
+        ":clang_tidy_check_rv",
+    ],
+)
+
 buildifier_exclude = [
     "./WORKSPACE",  # Prevent Buildifier from inserting unnecessary newlines.
     "./**/vendor/**",
diff --git a/rules/quality.bzl b/rules/quality.bzl
@@ -5,7 +5,10 @@
 """Quality check rules for OpenTitan.
 """
 
+load("@rules_cc//cc:action_names.bzl", "ACTION_NAMES", "C_COMPILE_ACTION_NAME")
 load("@bazel_skylib//lib:shell.bzl", "shell")
+load("@rules_cc//cc:find_cc_toolchain.bzl", "find_cc_toolchain")
+load("//rules:rv.bzl", "rv_rule")
 
 def _ensure_tag(tags, *tag):
     for t in tag:
@@ -95,6 +98,208 @@ def clang_format_test(**kwargs):
     kwargs["tags"] = _ensure_tag(tags, "no-sandbox", "no-cache", "external")
     _clang_format_test(**kwargs)
 
+# To see which checks clang-tidy knows about, run this command:
+#
+#  ./bazelisk.sh run @lowrisc_rv32imcb_files//:bin/clang-tidy -- --checks='*' --list-checks
+_CLANG_TIDY_CHECKS = [
+    "clang-analyzer-core.*",
+]
+
+def _clang_tidy_aspect_impl(target, ctx):
+    """Aspect implementation that runs clang-tidy on C/C++."""
+
+    if ctx.rule.kind not in ["cc_library", "cc_binary", "cc_test"]:
+        return [OutputGroupInfo(clang_tidy = [])]
+
+    if CcInfo in target:
+        cc_info = target[CcInfo]
+    elif hasattr(ctx.rule.attr, "deps"):
+        # Some rules, like cc_binary, do NOT produce a CcInfo provider. Therefore,
+        # we need to build one from its dependencies.
+        cc_info = cc_common.merge_cc_infos(
+            direct_cc_infos = [dep[CcInfo] for dep in ctx.rule.attr.deps if CcInfo in dep],
+        )
+    else:
+        return [OutputGroupInfo(clang_tidy = [])]
+    cc_compile_ctx = cc_info.compilation_context
+
+    cc_toolchain = find_cc_toolchain(ctx).cc
+    feature_configuration = cc_common.configure_features(
+        ctx = ctx,
+        cc_toolchain = cc_toolchain,
+        requested_features = ctx.features,
+        unsupported_features = ctx.disabled_features,
+    )
+
+    if not hasattr(ctx.rule.attr, "srcs"):
+        return [OutputGroupInfo(clang_tidy = [])]
+    all_srcs = []
+    for src in ctx.rule.attr.srcs:
+        all_srcs += [src for src in src.files.to_list() if src.is_source]
+
+    outputs = []
+    for src in all_srcs:
+        if src.path.startswith("external/"):
+            continue
+        if not src.extension in ["c", "cc", "h"]:
+            continue
+
+        generated_file = ctx.actions.declare_file("{}.{}.clang-tidy.out".format(src.basename, target.label.name))
+        outputs.append(generated_file)
+
+        opts = ctx.fragments.cpp.copts
+        if hasattr(ctx.rule.attr, "copts"):
+            opts += ctx.rule.attr.copts
+
+        # TODO(dmcardle) What if an .h file should be compiled for C++? Perhaps
+        # this should match the behavior in rules/cc_side_outputs.bzl.
+        if src.extension in ["c", "h"]:
+            opts += ctx.fragments.cpp.conlyopts
+        else:
+            opts += ctx.fragments.cpp.cxxopts
+            if hasattr(ctx.rule.attr, "cxxopts"):
+                opts += ctx.rule.attr.cxxopts
+
+        c_compile_variables = cc_common.create_compile_variables(
+            feature_configuration = feature_configuration,
+            cc_toolchain = cc_toolchain,
+            source_file = src.path,
+            user_compile_flags = opts,
+            include_directories = depset(
+                direct = [src.dirname for src in cc_compile_ctx.headers.to_list()],
+                transitive = [cc_compile_ctx.includes],
+            ),
+            quote_include_directories = cc_compile_ctx.quote_includes,
+            system_include_directories = cc_compile_ctx.system_includes,
+            framework_include_directories = cc_compile_ctx.framework_includes,
+            preprocessor_defines = depset(
+                direct = ctx.rule.attr.local_defines,
+                transitive = [cc_compile_ctx.defines],
+            ),
+        )
+
+        command_line = cc_common.get_memory_inefficient_command_line(
+            feature_configuration = feature_configuration,
+            action_name = ACTION_NAMES.c_compile,
+            variables = c_compile_variables,
+        )
+
+        args = ctx.actions.args()
+
+        # Add args that are consumed by the wrapper script.
+        if ctx.attr._enable_fix:
+            args.add("--ignore-clang-tidy-error")
+        args.add(".clang-tidy.lock")
+        args.add(generated_file)
+        args.add_all(ctx.attr._clang_tidy.files)
+
+        # Add args for clang-tidy.
+        if len(_CLANG_TIDY_CHECKS) > 0:
+            checks_pattern = ",".join(_CLANG_TIDY_CHECKS)
+            args.add("--checks=" + checks_pattern)
+
+            # Treat warnings from every enabled check as errors.
+            args.add("--warnings-as-errors=" + checks_pattern)
+        if ctx.attr._enable_fix:
+            args.add("--fix")
+            args.add("--fix-errors")
+
+            # Use the nearest .clang_format file to format code adjacent to fixes.
+            args.add("--format-style=file")
+
+        # Specify a regex header filter. Without this, clang-tidy will not
+        # report or fix errors in header files.
+        args.add("--header-filter=.*\\.h$")
+        args.add("--use-color")
+
+        for arg in command_line:
+            # Skip the src file argument. We give that to clang-tidy separately.
+            if arg == src.path:
+                continue
+            elif arg == "-fno-canonical-system-headers":
+                continue
+            args.add("--extra-arg=" + arg)
+
+        # Tell clang-tidy which source file to analyze.
+        args.add(src)
+
+        ctx.actions.run(
+            executable = ctx.attr._clang_tidy_wrapper.files_to_run,
+            arguments = [args],
+            inputs = depset(
+                direct = [src],
+                transitive = [
+                    cc_toolchain.all_files,
+                    cc_compile_ctx.headers,
+                ],
+            ),
+            tools = [ctx.attr._clang_tidy.files_to_run],
+            outputs = [generated_file],
+            progress_message = "Running clang tidy on {}".format(src.path),
+        )
+
+    return [
+        OutputGroupInfo(
+            clang_tidy = depset(direct = outputs),
+        ),
+    ]
+
+def _make_clang_tidy_aspect(enable_fix):
+    return aspect(
+        implementation = _clang_tidy_aspect_impl,
+        attr_aspects = ["deps"],
+        attrs = {
+            "_clang_tidy_wrapper": attr.label(
+                default = "//rules/scripts:clang_tidy.py",
+                allow_single_file = True,
+                cfg = "host",
+                executable = True,
+            ),
+            "_clang_tidy": attr.label(
+                default = "@lowrisc_rv32imcb_files//:bin/clang-tidy",
+                allow_single_file = True,
+                cfg = "host",
+                executable = True,
+                doc = "The clang-tidy executable",
+            ),
+            "_enable_fix": attr.bool(default = enable_fix),
+        },
+        incompatible_use_toolchain_transition = True,
+        fragments = ["cpp"],
+        host_fragments = ["cpp"],
+        toolchains = ["@rules_cc//cc:toolchain_type"],
+    )
+
+clang_tidy_fix_aspect = _make_clang_tidy_aspect(True)
+clang_tidy_check_aspect = _make_clang_tidy_aspect(False)
+
+def _clang_tidy_check_impl(ctx):
+    return [
+        DefaultInfo(
+            files = depset(
+                transitive = [dep[OutputGroupInfo].clang_tidy for dep in ctx.attr.deps],
+            ),
+        ),
+    ]
+
+clang_tidy_check_rv = rv_rule(
+    implementation = _clang_tidy_check_impl,
+    attrs = {
+        "deps": attr.label_list(
+            aspects = [clang_tidy_check_aspect],
+        ),
+    },
+)
+
+clang_tidy_check = rule(
+    implementation = _clang_tidy_check_impl,
+    attrs = {
+        "deps": attr.label_list(
+            aspects = [clang_tidy_check_aspect],
+        ),
+    },
+)
+
 def _html_coverage_report_impl(ctx):
     out_file = ctx.actions.declare_file(ctx.label.name + ".bash")
     substitutions = {}
diff --git a/rules/scripts/clang_tidy.py b/rules/scripts/clang_tidy.py
@@ -0,0 +1,98 @@
+#!/usr/bin/env python3
+#
+# Copyright lowRISC contributors.
+# Licensed under the Apache License, Version 2.0, see LICENSE for details.
+# SPDX-License-Identifier: Apache-2.0
+"""
+This wrapper script acts as a shim between Bazel and clang-tidy.
+"""
+
+import argparse
+import fcntl
+from pathlib import Path
+import subprocess
+import sys
+import time
+
+
+def maybe_rename_path(orig: Path, new: Path):
+    """Tries to rename `orig` to `new`. Returns an "undo" lambda.
+    """
+    try:
+        orig.rename(new)
+    except FileNotFoundError:
+        pass
+    return lambda: maybe_rename_path(new, orig)
+
+
+def acquire_lock(path: Path):
+    """Blocks until acquiring an exclusive lock on `path`.
+    """
+    with open(path, "a+") as f:
+        while True:
+            try:
+                fcntl.flock(f, fcntl.LOCK_EX)
+                return
+            except IOError:
+                time.sleep(0.1)
+
+
+def main() -> int:
+    parser = argparse.ArgumentParser()
+    parser.add_argument('--ignore-clang-tidy-error', action='store_true')
+    parser.add_argument('--print-args', action='store_true')
+    parser.add_argument('lock_file', type=Path)
+    parser.add_argument('out_file', type=Path)
+    parser.add_argument('clang_tidy')
+    parser.add_argument('clang_tidy_args', nargs=argparse.REMAINDER)
+
+    args = parser.parse_args()
+
+    if args.print_args:
+        print("Args that will be passed to clang-tidy:")
+        for arg in args.clang_tidy_args:
+            print(" ", arg)
+
+    # Acquire an exclusive lock before proceeding. There are two reasons for locking.
+    #
+    # (1) We need to ensure that the compilation database is not present when
+    #     clang-tidy runs. If compile_commands.json exists, clang-tidy will read
+    #     it and ignore the compiler arguments we give it. There does not appear
+    #     to be a command-line flag that controls this behavior, so all we can
+    #     do is ensure the file isn't present when clang-tidy runs.
+    #
+    # (2) Without locking, headers may receive the same fix multiple times. For
+    #     instance, strict prototype fixes might pile up on `void Foo();` to
+    #     create something like `void Foo(voidvoidvoid);`. This happens because
+    #     Bazel runs many instances of clang-tidy in parallel, and clang-tidy
+    #     has no synchronization mechanism.
+    #
+    # To run clang-tidy in parallel without locking, we would need to export
+    # fixes and deduplicate before applying them. Unfortunately, we cannot apply
+    # the fixes that `--export-fixes` creates because our toolchain doesn't
+    # include the `clang-apply-replacements` binary.
+    acquire_lock(args.lock_file)
+
+    compile_commands = Path("compile_commands.json")
+    cleanup_func = maybe_rename_path(
+        compile_commands, compile_commands.with_suffix(".tmp-clang-tidy"))
+
+    assert not compile_commands.exists()
+
+    clang_tidy_command = [args.clang_tidy] + args.clang_tidy_args
+    proc = subprocess.run(clang_tidy_command)
+    if proc.returncode != 0:
+        print(f"clang-tidy exited with {proc.returncode}")
+
+        if not args.ignore_clang_tidy_error:
+            cleanup_func()
+            return proc.returncode
+
+    args.out_file.touch()
+
+    cleanup_func()
+    return 0
+
+
+if __name__ == '__main__':
+    sys.exit(main())
