This is an archive of the discontinued LLVM Phabricator instance.

[bazel] Include builtin headers with clang-tidy
Needs ReviewPublic

Authored by jathu on Aug 26 2023, 12:09 PM.

Details

Reviewers
njames93
GMNGeoffrey
keith
matt-sm
goncharov
gchatelet
Group Reviewers
Restricted Project
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.

Diff Detail

Event Timeline

jathu created this revision.Aug 26 2023, 12:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 26 2023, 12:09 PM
jathu requested review of this revision.Aug 26 2023, 12:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 26 2023, 12:09 PM
jathu added a reviewer: Restricted Project.
jathu added a reviewer: matt-sm.
jathu added subscribers: goncharov, gchatelet, Restricted Project.
gchatelet resigned from this revision.Aug 29 2023, 2:01 AM

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

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

+@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 added a comment.Aug 30 2023, 6:56 PM

@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

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

jathu added a comment.EditedSep 7 2023, 7:33 PM

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 added a comment.Sep 14 2023, 7:10 PM

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

jathu added a comment.Sep 27 2023, 6:26 PM

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