[bazel] Include builtin headers with clang-tidy

[jathu]

Summary

Context: https://github.com/llvm/llvm-project/issues/63890

These builtin headers are required to exist alongside clang-tidy for it to work. Specifically, the tool looks for headers relatively in ../lib/clang/{major version}/include.

[gchatelet]

I've only touched the bazel config for LLVM libc, I don't feel qualified to review this patch.

[rnk]

+@rupprecht @aeubanks, can you review this Bazel change?

[rupprecht]

In D158942#4626240, @rnk wrote:

+@rupprecht @aeubanks, can you review this Bazel change?

Sure, I can take a look, although I'm out at least the rest of the week. @alexfh, do you have any ideas here?

The root cause of the bug seems to be that clang-based tooling looks for headers relative to where it is, assuming it doesn't find it in isystem or similar flags. There are some brief docs that mention cmake-related things: https://clang.llvm.org/docs/LibTooling.html#builtin-includes. clang works because it's defined in the same build file as the clang builtin headers, and it has some transitive dep onto it, but that might actually be accidental.

I'm not sure the approach here makes sense, since it only fixes the case of using this tool directly from the blaze tree since now the directory will be placed in the right spot adjacent to clang-tidy. As soon as you copy the clang-tidy binary to install it somewhere, it will fail to find the headers if you don't also know that you have to copy that too.

But, I'm also not aware of any kind of well lit path for this kind of problem. The build file tree defines lots of targets to build and produce a bunch of build artifacts, but there isn't really an equivalent to running install targets that place all those build artifacts together and in the right configuration (e.g. clang needs to go next to clang headers). That's usually done by external means.

utils/bazel/llvm-project-overlay/clang-tools-extra/BUILD.bazel

19

Why does the symlink destination need the version in it?

[jathu]

@rnk @rupprecht thanks for the review!

I'm not sure the approach here makes sense, since it only fixes the case of using this tool directly from the blaze tree since now the directory will be placed in the right spot adjacent to clang-tidy. As soon as you copy the clang-tidy binary to install it somewhere, it will fail to find the headers if you don't also know that you have to copy that too.

It is true that if we copy just the binary outside of the bazel sandbox, the tool will not work. However, this is the case with installing clang-tidy from any package manager. For example, try moving your installed clang-tidy anywhere in your system — it will fail to run if the adjacent headers are not found. So, if someone does copy the binary out, I guess they will have to follow the standard of bundling the runtime headers too. Note that we are making the headers a runtime data file, so I'm assuming if you use Bazel to package the binary, it will include the headers?

But, I'm also not aware of any kind of well lit path for this kind of problem. The build file tree defines lots of targets to build and produce a bunch of build artifacts, but there isn't really an equivalent to running install targets that place all those build artifacts together and in the right configuration (e.g. clang needs to go next to clang headers). That's usually done by external means.

I assumed the existence of a runtime data attribute in cc_binary meant this is an accepted pattern. I've configured clang-tidy to run using Bazel and the data files are successfully included in the runfiles when it runs

utils/bazel/llvm-project-overlay/clang-tools-extra/BUILD.bazel

19

Clang seems to look for it in this pattern ../lib/clang/{major version}/include. You can see something similar in the docs here: https://clang.llvm.org/docs/LibTooling.html#builtin-includes.

Running clang-tidy with verbose logging also includes this error:

ignoring nonexistent directory "/private/var/tmp/_bazel_jathu/d7c345aded7a3e75ef6039df0c27203f/execroot/starfig/bazel-out/darwin_arm64-opt-exec-2B5CBBC6/bin/external/llvm-project/clang-tools-extra/lib/clang/17/include

[chapuni]

FYI, I was working for introducing clang-resources in D141755. Could we unify clang-resources in the clang?

[jathu]

In D158942#4633201, @chapuni wrote:

FYI, I was working for introducing clang-resources in D141755. Could we unify clang-resources in the clang?

@chapuni this will not work since it needs to be located in the parent folder of where the binary is built — so it must exist in utils/bazel/llvm-project-overlay/clang-tools-extra/lib/clang/{major version}/include. FWIW also tried it by rebasing on your diff and get a runtime error (headers not found)

[jathu]

bump @rupprecht — hoping to get a decision on this before phabricator shuts down, otherwise I'll have to recreate the PR on GitHub

[jathu]

Moving this diff to GitHub since Phabricator will be shutdown Oct 1: https://github.com/llvm/llvm-project/pull/67626 cc @rupprecht

utils/bazel/llvm-project-overlay/clang-tools-extra/BUILD.bazel

+# This file is licensed under the Apache License v2.0 with LLVM Exceptions.
+# See https://llvm.org/LICENSE.txt for license information.
+# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+load("//:vars.bzl", "LLVM_VERSION_MAJOR")
+load("//clang:defs.bzl", "BUILTIN_HEADERS_GEN_DIR")
+load("defs.bzl", "symlink")
+
+package(
+    default_visibility = ["//visibility:public"],
+    features = ["layering_check"],
+)
+
+licenses(["notice"])
+
+symlink(
+    name = "builtin_headers",
+    srcs = ["//clang:builtin_headers_gen"],
+    destination = "lib/clang/{0}/include".format(LLVM_VERSION_MAJOR),
+    partition = BUILTIN_HEADERS_GEN_DIR,
+)

utils/bazel/llvm-project-overlay/clang-tools-extra/clang-tidy/BUILD.bazel

deps = CHECKS + [
         "//clang:tooling",
         "//llvm:Support",
     ],
 )
 
 cc_binary(
     name = "clang-tidy",
     srcs = ["tool/ClangTidyToolMain.cpp"],
+    data = ["//clang-tools-extra:builtin_headers"],
     stamp = 0,
     deps = [":tool"],
 )
 
 native_binary(
     name = "run-clang-tidy",
     src = "tool/run-clang-tidy.py",
     out = "run-clang-tidy",
     data = [":clang-tidy"],
 )

utils/bazel/llvm-project-overlay/clang-tools-extra/defs.bzl

+# This file is licensed under the Apache License v2.0 with LLVM Exceptions.
+# See https://llvm.org/LICENSE.txt for license information.
+# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+load("@bazel_skylib//lib:paths.bzl", "paths")
+
+def _symlink_impl(ctx):
+    copied_files = []
+    for input_file in ctx.files.srcs:
+        (_, _, relative_filename) = input_file.path.rpartition(ctx.attr.partition)
+        output_file = ctx.actions.declare_file(paths.join(ctx.attr.destination, relative_filename))
+        ctx.actions.symlink(
+            target_file = input_file,
+            output = output_file,
+        )
+        copied_files.append(output_file)
+    return DefaultInfo(files = depset(copied_files))
+
+symlink = rule(
+    implementation = _symlink_impl,
+    attrs = {
+        "destination": attr.string(mandatory = True),
+        "partition": attr.string(mandatory = True),
+        "srcs": attr.label_list(allow_files = True),
+    },
+)

utils/bazel/llvm-project-overlay/clang/BUILD.bazel

 # This file is licensed under the Apache License v2.0 with LLVM Exceptions.
 # See https://llvm.org/LICENSE.txt for license information.
 # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 
 load("@bazel_skylib//rules:expand_template.bzl", "expand_template")
 load("//:workspace_root.bzl", "workspace_root")
 load("//llvm:tblgen.bzl", "gentbl")
 load("//llvm:binary_alias.bzl", "binary_alias")
 load("//llvm:cc_plugin_library.bzl", "cc_plugin_library")
 load(
     "//:vars.bzl",
     "LLVM_VERSION",
     "LLVM_VERSION_MAJOR",
     "LLVM_VERSION_MINOR",
     "LLVM_VERSION_PATCH",
 )
+load("defs.bzl", "BUILTIN_HEADERS_GEN_DIR")
 
 package(
     default_visibility = ["//visibility:public"],
     features = ["layering_check"],
 )
 
 licenses(["notice"])
 
) + [
     "lib/Headers/openmp_wrappers/cmath",
     "lib/Headers/openmp_wrappers/complex",
     "lib/Headers/openmp_wrappers/new",
 ]
 
 genrule(
     name = "builtin_headers_gen",
     srcs = builtin_headers,
-    outs = [hdr.replace("lib/Headers/", "staging/include/") for hdr in builtin_headers],
+    outs = [
+        header.replace("lib/Headers/", BUILTIN_HEADERS_GEN_DIR)
+        for header in builtin_headers
+    ],
     cmd = """
        for src in $(SRCS); do
-         relsrc=$${src#*"$(WORKSPACE_ROOT)"/clang/lib/Headers}
-         target=$(@D)/staging/include/$$relsrc
+         relsrc=$${{src#*"$(WORKSPACE_ROOT)"/clang/lib/Headers}}
+         target=$(@D)/{resource_dir}$$relsrc
          mkdir -p $$(dirname $$target)
          cp $$src $$target
-       done""",
+       done""".format(resource_dir = BUILTIN_HEADERS_GEN_DIR),
     output_to_bindir = 1,
     toolchains = [
         ":workspace_root",
     ],
 )
 
 cc_library(
     name = "frontend",

utils/bazel/llvm-project-overlay/clang/defs.bzl

+# This file is licensed under the Apache License v2.0 with LLVM Exceptions.
+# See https://llvm.org/LICENSE.txt for license information.
+# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+BUILTIN_HEADERS_GEN_DIR = "staging/include/"