[bazel] Add aspects to run clang-tidy

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 <[email protected]>

Author: dmcardle

.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.

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,46 @@ 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",
+        #"//sw/device/lib/base:memory_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/**",

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,218 @@ 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")
+
+        # For debugging the args passed to clang-tidy:
+        #args.add("--print-args")
+
+        # Perform our best effort to analyze and fix errors, but do not fail the
+        # build when an error occurrs.
+
+        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")
+
+        # 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")
+
+        # Use the nearest .clang_format file.
+        args.add("--format-style=file")
+
+        for arg in command_line:
+            # Skip the src file argument. We give that to clang-tidy separately.
+            if arg == src.path:
+                continue
+            elif arg == "":
+                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 = {}