Add support for layering_check (#325)

This feature currently applies to C/C++ compiles. It validates that for
every header you import you have a corresponding target in your deps
that surfaces that header. Bazel handles a lot of this logic internally
in the CC rules but it's up to us to provide a system-level modulemap
file that contains all system headers, so that non-system headers that
aren't in your deps are correctly flagged as a layering check violation.

To do this we use find to discover all headers in the current
DEVELOPER_DIR. There are a few potential caching issues surfaced with
this:

1. If the paths were absolute, the Xcode path would be an input. We
   avoid this by storing relative paths, and then using a vfsoverlay to
   make it "look" like the modulemap is inside of Xcode itself, which is
   the only way to make relative paths work in modulemaps.
2. If different Xcode versions have different headers, and you support
   multiple Xcode versions in your build, the modulemap file is an input
   and will cause caching issues. I think the future solution to this
   would be allow users to provide their own modulemap with a constant
   set of headers, since you likely won't care about the new or removed
   ones (and if you do you have to only support 1 of the Xcode verisons)

This is currently disabled by default and can be enabled with
`--repo_env=APPLE_SUPPORT_LAYERING_CHECK_BETA=1`. This is primarily
because of the issues above, and that generating the modulemap file
takes ~5 seconds.

.bazelci/presubmit.yml

@@ -32,6 +32,14 @@ tasks:
     bazel: latest
     <<: *common
 
+  macos_latest_layering_check:
+    name: "Current layering_check"
+    platform: macos_arm64
+    xcode_version: "15.2"
+    bazel: latest
+    shell_commands:
+      - test/shell/layering_check_test.sh
+
   macos_last_green:
     name: "Last Green Bazel"
     bazel: last_green

crosstool/BUILD.tpl

@@ -41,6 +41,13 @@ cc_toolchain_suite(
     toolchains = dict(CC_TOOLCHAINS),
 )
 
+filegroup(
+    name = "modulemap",
+    srcs = [
+%{layering_check_modulemap}
+    ],
+)
+
 [
     filegroup(
         name = "osx_tools_" + arch,
@@ -49,6 +56,7 @@ cc_toolchain_suite(
             ":libtool",
             ":libtool_check_unique",
             ":make_hashed_objlist.py",
+            ":modulemap",
             ":wrapped_clang",
             ":wrapped_clang_pp",
             ":xcrunwrapper.sh",
@@ -71,6 +79,7 @@ cc_toolchain_suite(
         supports_param_files = 1,
         toolchain_config = arch,
         toolchain_identifier = arch,
+        module_map = %{placeholder_modulemap},
     )
     for arch in _APPLE_ARCHS
 ]
@@ -86,6 +95,7 @@ cc_toolchain_suite(
 %{cxx_builtin_include_directories}
         ],
         tool_paths_overrides = {%{tool_paths_overrides}},
+        module_map = ":modulemap",
     )
     for arch in _APPLE_ARCHS
 ]

crosstool/cc_toolchain_config.bzl

@@ -2524,6 +2524,65 @@ please file an issue at https://github.com/bazelbuild/apple_support/issues/new
         ],
     )
 
+    modulemaps = ctx.attr.module_map.files.to_list()
+    if modulemaps:
+        if len(modulemaps) != 1:
+            fail("internal error: expected 1 modulemap got:", modulemaps)
+        layering_check_feature = feature(
+            name = "layering_check",
+            flag_sets = [
+                flag_set(
+                    actions = [
+                        ACTION_NAMES.c_compile,
+                        ACTION_NAMES.cpp_compile,
+                        ACTION_NAMES.cpp_header_parsing,
+                        ACTION_NAMES.cpp_module_compile,
+                        ACTION_NAMES.objc_compile,
+                        ACTION_NAMES.objcpp_compile,
+                    ],
+                    flag_groups = [
+                        flag_group(
+                            flags = [
+                                "-fmodules-strict-decluse",
+                                "-Wprivate-header",
+                                "-Xclang",
+                                "-fmodule-name=%{module_name}",
+                                "-Xclang",
+                                "-fmodule-map-file=%{module_map_file}",
+                            ],
+                        ),
+                        flag_group(
+                            iterate_over = "dependent_module_map_files",
+                            flags = [
+                                "-Xclang",
+                                "-fmodule-map-file=%{dependent_module_map_files}",
+                            ],
+                        ),
+                    ],
+                ),
+            ],
+            env_sets = [
+                env_set(
+                    actions = [
+                        ACTION_NAMES.c_compile,
+                        ACTION_NAMES.cpp_compile,
+                        ACTION_NAMES.cpp_header_parsing,
+                        ACTION_NAMES.cpp_module_compile,
+                        ACTION_NAMES.objc_compile,
+                        ACTION_NAMES.objcpp_compile,
+                    ],
+                    env_entries = [
+                        env_entry(
+                            key = "APPLE_SUPPORT_MODULEMAP",
+                            value = modulemaps[0].path,
+                        ),
+                    ],
+                ),
+            ],
+        )
+    else:
+        layering_check_feature = feature(name = "layering_check")
+
     features = [
         # Marker features
         feature(name = "archive_param_file"),
@@ -2605,6 +2664,7 @@ please file an issue at https://github.com/bazelbuild/apple_support/issues/new
         default_sanitizer_flags_feature,
         treat_warnings_as_errors_feature,
         no_warn_duplicate_libraries_feature,
+        layering_check_feature,
     ]
 
     if (ctx.attr.cpu == "darwin_x86_64" or
@@ -2678,6 +2738,7 @@ cc_toolchain_config = rule(
         "cxx_builtin_include_directories": attr.string_list(),
         "tool_paths_overrides": attr.string_dict(),
         "extra_env": attr.string_dict(),
+        "module_map": attr.label(),
         "_xcode_config": attr.label(default = configuration_field(
             fragment = "apple",
             name = "xcode_config_label",

crosstool/generate-modulemap.sh

@@ -0,0 +1,14 @@
+#!/bin/bash
+
+set -euo pipefail
+
+cd "$(xcode-select -p)"
+
+echo 'module "crosstool" [system] {'
+
+find . -type f \( -name "*.h" -o -name "*.def" -o -path "*/c++/*" \) \
+  | LANG=C sort -u | while read -r header; do
+    echo "  textual header \"${header}\""
+  done
+
+echo "}"

crosstool/osx_cc_configure.bzl

@@ -62,6 +62,29 @@ def _succeeds(repository_ctx, *args):
 
     return result.return_code == 0
 
+def _generate_system_modulemap(repository_ctx, script, output):
+    env = repository_ctx.os.environ
+    result = repository_ctx.execute([
+        "env",
+        "-i",
+        "DEVELOPER_DIR={}".format(env.get("DEVELOPER_DIR", default = "")),
+        script,
+    ])
+
+    if result.return_code != 0:
+        error_msg = (
+            "return code {code}, stderr: {err}, stdout: {out}"
+        ).format(
+            code = result.return_code,
+            err = result.stderr,
+            out = result.stdout,
+        )
+        fail(output + " failed to generate. Please file an issue at " +
+             "https://github.com/bazelbuild/apple_support/issues with the following:\n" +
+             error_msg)
+
+    repository_ctx.file(output, result.stdout)
+
 def _compile_cc_file(repository_ctx, src_name, out_name):
     env = repository_ctx.os.environ
     xcrun_result = repository_ctx.execute([
@@ -149,6 +172,9 @@ def configure_osx_toolchain(repository_ctx):
     wrapped_clang_src_path = str(repository_ctx.path(
         Label("@build_bazel_apple_support//crosstool:wrapped_clang.cc"),
     ))
+    generate_modulemap_path = str(repository_ctx.path(
+        Label("@build_bazel_apple_support//crosstool:generate-modulemap.sh"),
+    ))
 
     xcode_toolchains = []
     xcodeloc_err = ""
@@ -181,6 +207,15 @@ def configure_osx_toolchain(repository_ctx):
     _compile_cc_file(repository_ctx, wrapped_clang_src_path, "wrapped_clang")
     repository_ctx.symlink("wrapped_clang", "wrapped_clang_pp")
 
+    layering_check_modulemap = None
+    if repository_ctx.os.environ.get("APPLE_SUPPORT_LAYERING_CHECK_BETA") == "1":
+        layering_check_modulemap = "layering_check.modulemap"
+        _generate_system_modulemap(repository_ctx, generate_modulemap_path, layering_check_modulemap)
+        repository_ctx.file(
+            "module.modulemap",
+            "// Placeholder to satisfy API requirements. See apple_support for usage",
+        )
+
     tool_paths = {}
     gcov_path = repository_ctx.os.environ.get("GCOV")
     if gcov_path != None:
@@ -204,6 +239,8 @@ def configure_osx_toolchain(repository_ctx):
         {
             "%{cxx_builtin_include_directories}": "\n".join(escaped_cxx_include_directories),
             "%{features}": "\n".join(['"{}"'.format(x) for x in features]),
+            "%{layering_check_modulemap}": "\":{}\",".format(layering_check_modulemap) if layering_check_modulemap else "",
+            "%{placeholder_modulemap}": "\":module.modulemap\"" if layering_check_modulemap else "None",
             "%{tool_paths_overrides}": ",\n            ".join(
                 ['"%s": "%s"' % (k, v) for k, v in tool_paths.items()],
             ),

crosstool/setup.bzl

@@ -56,6 +56,7 @@ _apple_cc_autoconf = repository_rule(
     environ = [
         _DISABLE_ENV_VAR,
         _OLD_DISABLE_ENV_VAR,
+        "APPLE_SUPPORT_LAYERING_CHECK_BETA",
         "BAZEL_ALLOW_NON_APPLICATIONS_XCODE",  # Signals to configure_osx_toolchain that some Xcodes may live outside of /Applications and we need to probe further when detecting/configuring them.
         "DEVELOPER_DIR",  # Used for making sure we use the right Xcode for compiling toolchain binaries
         "GCOV",  # TODO: Remove this

crosstool/wrapped_clang.cc

@@ -377,6 +377,22 @@ void ProcessArgument(const std::string arg, const std::string developer_dir,
   consumer(new_arg);
 }
 
+void AddLayeringCheckVFS(const std::string vfs_overlay_file,
+                         const char *modulemap, const std::string developer_dir,
+                         std::function<void(const std::string &)> consumer) {
+  std::ofstream vfs_overlay_file_stream(vfs_overlay_file);
+  vfs_overlay_file_stream
+      << R"EOF({"case-sensitive":true,"overlay-relative":false,"roots":[{"contents":[{"external-contents":")EOF"
+      << modulemap
+      << R"EOF(","name":"vfs.modulemap","type":"file"}],"name":")EOF"
+      << developer_dir
+      << R"EOF(","type":"directory"}],"use-external-names":false,"version":0})EOF"
+      << std::endl;
+  consumer("-ivfsoverlay" + vfs_overlay_file);
+  consumer("-Xclang");
+  consumer("-fmodule-map-file=" + developer_dir + "/vfs.modulemap");
+}
+
 }  // namespace
 
 int main(int argc, char *argv[]) {
@@ -419,6 +435,14 @@ int main(int argc, char *argv[]) {
                     linked_binary, dsym_path, toolchain_path, consumer);
   }
 
+  char *modulemap = getenv("APPLE_SUPPORT_MODULEMAP");
+  std::unique_ptr<TempFile> vfs_overlay_file;
+  if (modulemap != nullptr) {
+    vfs_overlay_file = TempFile::Create("modules-vfs-overlay.XXXXXX");
+    AddLayeringCheckVFS(vfs_overlay_file->GetPath(), modulemap, developer_dir,
+                        consumer);
+  }
+
   // Special mode that only prints the command. Used for testing.
   if (getenv("__WRAPPED_CLANG_LOG_ONLY")) {
     for (const std::string &arg : invocation_args) std::cout << arg << ' ';

test/layering_check/BUILD

@@ -0,0 +1,74 @@
+load("@bazel_skylib//rules:build_test.bzl", "build_test")
+
+package(features = ["layering_check"])
+
+cc_library(
+    name = "a",
+    hdrs = ["a.h"],
+)
+
+cc_library(
+    name = "b",
+    hdrs = ["b.h"],
+    deps = [":a"],
+)
+
+cc_test(
+    name = "bad_layering_check",
+    srcs = ["c.cpp"],
+    deps = [":b"],
+)
+
+cc_test(
+    name = "disabled_bad_layering_check",
+    srcs = ["c.cpp"],
+    features = ["-layering_check"],
+    deps = [":b"],
+)
+
+cc_test(
+    name = "good_layering_check",
+    srcs = ["c.cpp"],
+    deps = [
+        ":a",
+        ":b",
+    ],
+)
+
+objc_library(
+    name = "bad_layering_check_objc",
+    srcs = ["c.m"],
+    tags = ["manual"],
+    deps = [":b"],
+)
+
+build_test(
+    name = "bad_layering_check_objc_test",
+    targets = [":bad_layering_check_objc"],
+)
+
+objc_library(
+    name = "disabled_bad_layering_check_objc",
+    srcs = ["c.m"],
+    features = ["-layering_check"],
+    deps = [":b"],
+)
+
+build_test(
+    name = "disabled_bad_layering_check_objc_test",
+    targets = [":disabled_bad_layering_check_objc"],
+)
+
+objc_library(
+    name = "good_layering_check_objc",
+    srcs = ["c.m"],
+    deps = [
+        ":a",
+        ":b",
+    ],
+)
+
+build_test(
+    name = "good_layering_check_objc_test",
+    targets = [":good_layering_check_objc"],
+)

test/layering_check/a.h

@@ -0,0 +1 @@
+int from_a_library = 1;

test/layering_check/b.h

@@ -0,0 +1 @@
+int from_b_library = 2;

test/layering_check/c.cpp

@@ -0,0 +1,7 @@
+#include "a.h"
+#include "b.h"
+#include <iostream>
+
+int main() {
+  std::cout << from_a_library << from_b_library << std::endl;
+}

test/layering_check/c.m

@@ -0,0 +1,8 @@
+#include "a.h"
+#include "b.h"
+#import <Foundation/Foundation.h>
+#include <stdio.h>
+
+int main() {
+    NSLog(@"%d %d", from_a_library, from_b_library);
+}

test/shell/layering_check_test.sh

@@ -0,0 +1,28 @@
+#!/bin/bash
+
+set -euo pipefail
+
+script_path="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+source "$script_path"/unittest.bash
+
+bazel="${BAZEL:-bazel}"
+
+function test_good_layering_checks() {
+  "$bazel" test --repo_env=APPLE_SUPPORT_LAYERING_CHECK_BETA=1 -- //test/layering_check/... -//test/layering_check:bad_layering_check -//test/layering_check:bad_layering_check_objc_test &>"$TEST_log"
+}
+
+function test_bad_layering_checks() {
+  ! "$bazel" test --repo_env=APPLE_SUPPORT_LAYERING_CHECK_BETA=1 -- //test/layering_check:bad_layering_check &> "$TEST_log" || fail "Expected build failure"
+
+  expect_log_once "does not depend on a module exporting"
+  expect_log "test/layering_check/c.cpp:1:10: error: module //test/layering_check:bad_layering_check does not depend on a module exporting 'a.h'" "failed wrong layering_check"
+}
+
+function test_bad_layering_checks_objc() {
+  ! "$bazel" test --repo_env=APPLE_SUPPORT_LAYERING_CHECK_BETA=1 -- //test/layering_check:bad_layering_check_objc_test &> "$TEST_log" || fail "Expected build failure"
+
+  expect_log_once "does not depend on a module exporting"
+  expect_log "test/layering_check/c.m:1:10: error: module //test/layering_check:bad_layering_check_objc does not depend on a module exporting 'a.h'" "failed wrong layering_check"
+}
+
+run_suite "layering_check tests"