[bazel] Enable layering_check for llvm and clang

[MaskRay]

Summary

Similar to D113952 for mlir.

I have added many missing dependencies so that
bazel-5.0.0 build --config=generic_clang --features=layering_check @llvm-project//llvm:all @llvm-project//clang:all
work now.
Enable the feature to ensure layering and catch circular dependencies
(https://llvm.org/docs/CodingStandards.html#library-layering).

[MaskRay]

buildifier

[GMNGeoffrey]

I'm seeing a bunch of layering failures when trying to integrate this into IREE. We're building with clang-16 which seems to have gotten much better at finding these issues (IIRC there used to be a bug that prevented this). I'm surprised Google hasn't hit this issue with the internal build as well, since I thought we were using a very new clang. Maybe we could use a newer clang in the Bazel build bot? Disadvantage is that it might stop working with an older Clang

[shahms]

This breaks users of llvm_external_zlib because https://github.com/llvm/llvm-project/blob/main/utils/bazel/deps_impl/zlib_external.BUILD doesn't export the zlib.h header.

[MaskRay]

In D141553#4067007, @GMNGeoffrey wrote:

I'm seeing a bunch of layering failures when trying to integrate this into IREE. We're building with clang-16 which seems to have gotten much better at finding these issues (IIRC there used to be a bug that prevented this). I'm surprised Google hasn't hit this issue with the internal build as well, since I thought we were using a very new clang. Maybe we could use a newer clang in the Bazel build bot? Disadvantage is that it might stop working with an older Clang

Do you manage to address the IREE issues? I don't recall a layering_check bug but before D132779 the check was less strict (false negatives for some textual_hdrs uses: https://maskray.me/blog/2022-09-25-layering-check-with-clang#bazel)

In D141553#4074337, @shahms wrote:

This breaks users of llvm_external_zlib because https://github.com/llvm/llvm-project/blob/main/utils/bazel/deps_impl/zlib_external.BUILD doesn't export the zlib.h header.

Do you have a build instruction? The fix is likely an addition of features = ["-layering_check"], somewhere.

[shahms]

https://github.com/kythe/kythe/pull/5511

Is the relevant PR with the workaround for this (which is, indeed, "just" patching the LLVM BUILD files to remove the feature), alongside my workaround for the empty globs.

[MaskRay]

In D141553#4074491, @shahms wrote:

https://github.com/kythe/kythe/pull/5511

Is the relevant PR with the workaround for this (which is, indeed, "just" patching the LLVM BUILD files to remove the feature), alongside my workaround for the empty globs.

I don't know how to build kythe and I cannot find any occurrence of llvm_external_zlib...
https://github.com/kythe/kythe/pull/5511 looks reasonable. features = ["layering_check"], in the upstream helps catch problems.
If a downstream project patches something and runs into failures (please give detailed instructions; bazel is an unsupported build system anyway), removing layering_check is reasonable.

[shahms]

Sorry, it's llvm_zlib_external https://github.com/llvm/llvm-project/blob/7718422d3b788a52b751773f464bfa52a6461fb7/utils/bazel/zlib.bzl#L32

[GMNGeoffrey]

Hmm breaking zlib_external isn't great. I actually ran into another issue when I tried to repro this:

bazel build --config=generic_clang --config=zlib_external @llvm-project//...

ERROR: An error occurred during the fetch of repository 'llvm_zlib':
   Traceback (most recent call last):
        File "/usr/local/google/home/gcmn/.cache/bazel/_bazel_gcmn/7979a4df276c1366186cbb0309ad0645/external/llvm-raw/utils/bazel/zlib.bzl", line 85, column 33, in _llvm_zlib_from_env_impl
                _llvm_zlib_external_impl(repository_ctx)
        File "/usr/local/google/home/gcmn/.cache/bazel/_bazel_gcmn/7979a4df276c1366186cbb0309ad0645/external/llvm-raw/utils/bazel/zlib.bzl", line 23, column 28, in _llvm_zlib_external_impl
                repository_ctx.template(
Error in template: got dict<string, Label> for 'substitutions', want dict<string, string>

So that appears to be broken also (need an explicit string conversion). Presumably that means that no one is using the environment variable method of configuring these.

We could add features = ["-layering_check"] to the llvm:Support rule only, which would at least scope it. Other options:

1. add a header file to the zlib_external build rule that re-exports the zlib header. That's a bit gross
2. Move the define to the zlib.BUILD build rule and make the zlib_external build rule just an alias. That requires dependent projects to use that rule rather than a zlib dep they've already set up, so also not great.
3. Find another way to remap repo names. I think Bazel might have better functionality for it now

[GMNGeoffrey]

Well I just spent quite a while fiddling with the zlib setup and didn't come up with something better than disabling layering check on Support. The issue is that we want the zlib rule to add in LLVM's define as well (at least as currently configured). Otherwise we could do an alias or cc_import rule. The latter might even be nicer for system zlib than the current hardcoding of linkopts, which I think is unfriendly to Windows. Trying to create a header to re-export the zlib header doesn't work because it needs to be called "zlib.h" and then it just recursively includes itself. I'm annoyed (but not that shocked) that Bazel doesn't have a way to declare a header re-exported from the build rule. I think a better solution than disabling layering would involve reworking the way we do configurable dependencies here in general.

[aaronmondal]

Proposing D143320 to address the zlib breakages with recent Clang versions.

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("//llvm:tblgen.bzl", "gentbl")
 load("//llvm:binary_alias.bzl", "binary_alias")
 load("//llvm:cc_plugin_library.bzl", "cc_plugin_library")
 
 package(
     default_visibility = ["//visibility:public"],
+    features = ["layering_check"],
 )
 
 licenses(["notice"])
 
 exports_files([
     "tools/clang-format/clang-format.el",
     "tools/clang-format/clang-format-test.el",
     "tools/clang-format/clang-format.py",

utils/bazel/llvm-project-overlay/llvm/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:common_settings.bzl", "string_flag")
 load("@bazel_skylib//rules:expand_template.bzl", "expand_template")
 load(":tblgen.bzl", "gentbl")
 load(":config.bzl", "llvm_config_defines")
 load(":targets.bzl", "llvm_targets")
 load(":enum_targets_gen.bzl", "enum_targets_gen")
 load(":binary_alias.bzl", "binary_alias")
 
-package(default_visibility = ["//visibility:public"])
+package(
+    default_visibility = ["//visibility:public"],
+    features = ["layering_check"],
+)
 
 licenses(["notice"])
 
 exports_files(["LICENSE.TXT"])
 
 # It may be tempting to add compiler flags here, but that should be avoided.
 # The necessary warnings and other compile flags should be provided by the
 # toolchain or the `.bazelrc` file. This is just a workaround until we have a