This uses a header-only extraction of the Boost unordered hashtable
project to allow a trivial Bazel build and for us to benchmark against
it effectively.
This takes the installation layout and replicates it using `rules_pkg`
to build either a tarball or a zip file of the toolchain. Correctly
manages file permissions and symlinks, etc.
There are some big remaining things here:
- Figure out how we want to test this. We can add shell tests maybe?
A bit awkward. Nicer would be to make the `//examples` tree build
using this rather than the more native-bazel install data, however
building these is quite slow and it seems bad to pay that cost
constantly so dedicated testing is probably better. For now, I've
tested these manually.
- Need to add versions to the toolchain and then thread them through
here so they install properly as a versioned release.
But my primary goal for now is just to be able to validate that the
install tree is working outside of Bazel and this does enough for that.
The above will be longer-term things.
I'd kept these in to separate the bazel module update from the BUILD
file changes, then forgot about it. I think all of these can be cleanly
removed now. I think it's something we should clean up for consistency
with the bazel central repository names; I think it's best to reduce
that divergence.
llvm_zlib and llvm_zstd remain because of how llvm depends on the
particular names.
The patches to bazel_clang_tidy were adopted upstream and are no longer
necessary, so I think we can simplify to git_override.
This fixes an existing issue where bazel was complaining that some rules
indirectly requested rules_python 0.29.0.
Note I'm not updating protobuf or com_google_libprotobuf_mutator. This
is because I get build errors with a protobuf -> abseil dependency if
updating.
Co-authored-by: josh11b <github-llvm@technomagi.com>
Update the compiler-rt patch (maybe this could be upstreamed, or try out
https://github.com/google/fuzztest?) and handle the ThreadPool ->
DefaultThreadPool rename.
C++20 mode exposes bugs in `clang-tidy` 16 that are challenging to work
around, but this should manage to do so. The patch to `bazel_clang_tidy`
has been sent upstream but no need to wait for it.
We don't actually specify a specific version of C++ in our style guide
docs, and the Google style guide has already been updated to be based on
C++20 so nothing to be done there.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
This should also unblock our switch to C++20 and other improvements.
There are three core parts of the change --
1) Updating our infrastructure to fetch and find Clang-16.
2) Updating our documentation to reflect this and help folks with any
system issues they encounter.
The infrastructure change is unfortunately tricky. We can't get Clang 16
easily on GitHub's runner images, and in the past we've had persistent
problems with flakiness when our actions download this much during their
runs. Due to the flakiness, we've previously removed all downloading of
dependencies outside of Bazel itself, and added retry loops around Bazel
specifically to overcome flaky downloads.
This change tries to address these problems by populating the Clang and
LLVM toolchain in a place that we can then cache using the built-in
GitHub action caching infrastructure. This seems like by far the least
likely to flake way of downloading extra things into our runs. And since
these are relatively slow moving dependencies, we should populate this
cache very, very rarely.
For Linux, this downloads the binary release artifact from GitHub,
prunes out large parts of it that we don't need, and then caches this as
a local toolchain. This proves both small and fast.
For macOS, this uses a trick to cache the destination of Homebrew
installs. It unfortunately caches the *entire* Homebrew installation
though, and so it also goes to some lengths to prune and minimize how
much is installed from Homebrew. The result is "only" a 2gb cache image.
Because of the size and slower download and filesystem, the macOS runs
see a 1 - 2 minute slowdown.
We might extend the Linux infrastructure here usefully if we want to
test multiple LLVM versions. We might also extend the macOS version to
get a cheaper way to prune parts of the system and free up disk space,
or to cache other Homebrew installed tools if needed.
Last but not least, this brought to the forefront an issue with our C++
toolchain integration which relied on a specific CMake build option
being set in the LLVM toolchain install. This option isn't used in the
official release artifacts. Instead, switch to a more robust approach to
linking libc++abi statically that shouldn't have these problems.
Beyond the infrastructure changes, this also updates the documentation
to reflect requiring Clang 16 or newer, and adds some extra tips for
folks that are missing this.
The documentation is also updated to address a problem with getting the
right libc++abi files installed to support the more robust linking
strategy. This may reduce the problems we've seen in the past around
libc++abi and linking on other Linux distros as well.
---------
Co-authored-by: josh11b <josh11b@users.noreply.github.com>
The pip dependencies in github_tools are the reason the
MODULE.bazel.lock is platform-dependent. Following complaints about the
platform-dependence, split apart github_tools from the rest of the bazel
repo and make it not track the lockfile: while there's an incremental
safety risk due to not tracking checksums, it's unlikely the tools there
would ever be part of a Carbon release process. If we eventually add
Python tools that need pip to the release, it might be desirable to go
back to re-unify the bazezl repos.
This does make running pr_comments incrementally more inconvenient
because a "bazel run" needs to be run from the github_tools subdir.
As a consequence of separating the dependency, this means tests will not
be continuously run in github_tools. They're now a separate repo, and we
cannot add a dependency without restoring the platform-dependent issue.
I think pr_comments is sufficiently low value and unchanging that it is
not worth building separate CI for it.
Cleans up some legacy references to third_party/llvm-project, since now
github_tools needs to be added to the main bazelignore.
The `libpfm` in the Bazel central repository uses `make` to build it,
which is difficult to integrate with our toolchain. Rather than try to
fix all the issues there, it's easy to just add a native Bazel build for
the library. I don't know that any of the relevant upstream folks are
interested in this kind of build, but it seems easy for us to maintain
as a Carbon project build configuration. I've also not tried to port all
of the different configurations as a consequence, and only 64-bit x86
and Arm as that seems the only likely architectures we'll care about in
the near term.
I've kept this using the `bzlmod` stuff as best I can, and I *think* I'm
holding all of those pieces correctly, but if not, happy for suggestions
on adjustments.
The `google_benchmark` package also has an awkward way of enabling
`libpfm` support using a top-level `bazel` command line flag. I think
this is because of how brittle the Bazel build of `libpfm` is, but I'm
not sure. With the new build, it seems easy to patch `google_benchmark`
to detect the same conditions as we build `libpfm` under, and enable it
there. So I've done this to avoid folks having to pass a command line
flag on platforms where it is supported.
The result is that we now get really nice CPU counter support in our
benchmarks out-of-the-box on Linux x86-64 and AArch64. For example on my
Fedora Asahi install on a Mac Mini I get:
```console
$ bazel run -c opt --copt=-gmlt //common:hashing_benchmark --run_under="taskset -c 4" -- --benchmark_counters_tabular=true --benchmark_perf_counters=CYCLES,INSTRUCTIONS
INFO: Invocation ID: 4aaeb9e9-7df5-4f1f-b56b-c03411790268
INFO: Analyzed target //common:hashing_benchmark (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
Target //common:hashing_benchmark up-to-date:
bazel-bin/common/hashing_benchmark
INFO: Elapsed time: 0.360s, Critical Path: 0.02s
INFO: 1 process: 1 internal.
INFO: Build completed successfully, 1 total action
INFO: Running command line: /bin/bash -c 'taskset -c 4 bazel-bin/common/hashing_benchmark '\''--benchmark_counters_tabular=true'\'' '\''--benchmark_perf_counters=CYCLES,INSTRUCTIONS'\'''
2024-01-15T00:10:50-08:00
Running /home/chandlerc/.cache/bazel/_bazel_chandlerc/b686aa8910e0845b88c21d715819b076/execroot/_main/bazel-out/aarch64-opt/bin/common/hashing_benchmark
Run on (8 X 2064 MHz CPU s)
CPU Caches:
L1 Data 64 KiB (x8)
L1 Instruction 128 KiB (x8)
L2 Unified 4096 KiB (x2)
Load Average: 0.01, 0.08, 0.08
--------------------------------------------------------------------------------------------------------------------------------------------------------------
Benchmark Time CPU Iterations CYCLES INSTRUCTIONS bytes_per_second
--------------------------------------------------------------------------------------------------------------------------------------------------------------
BM_LatencyHash<RandValues<uint8_t>, CarbonHashBench> 4.11 ns 4.11 ns 170200064 13.1321 9.00587 232.116Mi/s
BM_LatencyHash<RandValues<uint8_t>, AbseilHashBench> 4.82 ns 4.82 ns 145643520 15.3657 12.0059 197.946Mi/s
BM_LatencyHash<RandValues<uint8_t>, LLVMHashBench> 7.96 ns 7.95 ns 87956480 25.3737 17.0068 119.991Mi/s
BM_LatencyHash<RandValues<uint16_t>, CarbonHashBench> 4.11 ns 4.11 ns 170365952 13.1247 9.00587 464.573Mi/s
BM_LatencyHash<RandValues<uint16_t>, AbseilHashBench> 5.51 ns 5.51 ns 127568896 17.5578 14.0059 346.225Mi/s
BM_LatencyHash<RandValues<uint16_t>, LLVMHashBench> 8.00 ns 7.99 ns 87085056 25.377 17.0068 238.834Mi/s
BM_LatencyHash<RandValues<std::pair<uint8_t, uint8_t>>, CarbonHashBench> 4.91 ns 4.90 ns 136013824 15.6456 14.0059 389.006Mi/s
BM_LatencyHash<RandValues<std::pair<uint8_t, uint8_t>>, AbseilHashBench> 6.85 ns 6.85 ns 102630400 21.8041 18.0059 278.637Mi/s
BM_LatencyHash<RandValues<std::pair<uint8_t, uint8_t>>, LLVMHashBench> 7.57 ns 7.56 ns 92798976 24.1437 20.0068 252.151Mi/s
BM_LatencyHash<RandValues<uint32_t>, CarbonHashBench> 4.12 ns 4.12 ns 170229760 13.1272 9.00587 926.444Mi/s
BM_LatencyHash<RandValues<uint32_t>, AbseilHashBench> 4.93 ns 4.92 ns 145304576 15.3738 12.0059 775.224Mi/s
BM_LatencyHash<RandValues<uint32_t>, LLVMHashBench> 8.11 ns 8.10 ns 87127040 25.373 17.0068 470.98Mi/s
```
This patches bazel_clang_tidy handling of headers. I found an equivalent
change at https://github.com/erenon/bazel_clang_tidy/pull/13, but that
was [already
rejected](https://github.com/erenon/bazel_clang_tidy/pull/13#issuecomment-1047007424).
Per the criticism, this will result in redundant processing of headers.
The project instead uses `HeaderFilterRegex: ".*"`, but that results in
two problems:
1. When running with `-k`, errors are repeated when a header is included
more than once, which is common.
2. clang-tidy including errors from headers that are included from other
modules (e.g., abseil-cpp); filtering correctly is difficult.
Given the trade-offs and options (including forking), I thought patching
was preferable so long as it remains narrow.
This sets things up to use `bazel` to run `clang-tidy` using
https://github.com/erenon/bazel_clang_tidy.
I'm fixing issues outside of explorer, and disabling clang-tidy for
targets in explorer that have legacy issues. I was going to disable
clang-tidy for targets in explorer such as interpreter anyways, because
they're slow to parse, and just extended that to the currently failing
targets.
Directories in `external/` now include versions of dependencies, so
continuing to make that work would require version-locking the
compile_flags.txt. We've previously seen issues with directories being
forgotten; this switches to letting bazel generate everything.
Setup follows instructions at
https://github.com/hedronvision/bazel-compile-commands-extractor?tab=readme-ov-file#first-add-this-tool-to-your-bazel-setup
Maintains `./scripts/create_compdb.py`. Per discussion, new files seem
to work okay -- I'm getting #include completion that clearly detects
files.
Note this creates an `external` directory:
```
╚╡ls -ld external
lrwxrwxrwx 1 jperkins primarygroup 27 Dec 19 14:02 external -> bazel-out/../../../external
```
I felt sort of weird checking that in though, at least alongside
`bazel-execroot` it feels like it should have a `bazel-` prefix.
However, I can't rename it. So omitting it, and leaving it to the script
to generate, felt like a reasonable compromise.
Running takes a few minutes (about 3 for me), and should be faster on
reruns. It does print a couple warnings, just tree_sitter missing
parser.c and _GNUC_PREREQ errors from m4.
The use of zlib-ng seemed to work previously, but may have only done so
because of some combination of an implementation detail of
`strip_include_prefix` in older versions of Bazel and some happenstance
of a system installed `zlib.h` being possible to find. With Bazel 7 it
started breaking on my machine which doesn't have a system `zlib.h`.
Using the "normal" zlib and the bzlmod rigging for it is simple and
seems to work.
Also adds the Linux AArch64 component of the Bazel lock file which was
necessary to test this on my Arm Linux machine.
Building on #3505, the toolchain and llvm rules require a little more
special-casing to get them to work well. This also moves
libprotobuffer_mutator, but that one's more minor. The migration
encounters more quirks in repo naming as seen by various queries.
This changes some of the toolchain work that was recently done for bazel
7 in #3496, dropping a bzl file I'd suggested to add, instead using
`:all` for toolchain registration. (somewhat as an improvement, somewhat
just to avoid a `load`)
Remaining in the WORKSPACE are example code repos and tree sitter rules.
Neither of these are part of the main toolchain builds, and so will
probably be lower impact if there's a good solution for them.
Some background information is at https://bazel.build/external/migration
Trying to handle the simple cases first. This adds a requirement for
bazel 7 due to differences in bzlmod handling between 6 and 7 (also
discussed on
[#infra](https://discord.com/channels/655572317891461132/707150492370862090/1184942191412510720)).
Bazel seems to be okay with a partial migration such as ths.
The python import behavior has subtly shifted, so `carbon.` is no longer
part of import paths. There's a version-incompatible change for `@@`.
bzlmod makes repos sometimes show as `name~version`.
`target-determinator` seems to be okay with `@@` after a version update.
Things not moved here are things that basically need more dep work:
- clang_register_toolchains because I need to dive into its format.
- llvm-project because we need something slightly atypical, I need to
make sure patching and the repo work carries over.
- com_google_libprotobuf_mutator is sufficiently atypical that it
doesn't have a module already, but should be one of the easier things to
fix.
- brotli/woff2: I think we should actually consider removing these. But
again, they're not trivial moves.
- treesitter due to toolchain registration, which has shifted a bit.
- rules_nodejs because treesitter depends on it in an awkward way to
migrate.
Chandler Carruth
Jon Ross-Perkins
