[7.2.0] Add parse_headers support to the default Unix toolchain (#2…

…2369)

Also remove Ubuntu 16.04 workarounds from layering_check tests.

RELNOTES: The default Unix C++ toolchain now supports the
`parse_headers` feature to validate header files with
`--process_headers_in_dependencies`.

Closes #21560.

PiperOrigin-RevId: 633657012
Change-Id: Iaaa2623bcc86b219b02567eca1c7bf9e1a77ae7d

Fixes #22361

Commit
231dfc2

site/en/docs/bazel-and-cpp.md

@@ -82,3 +82,22 @@ Follow these guidelines for include paths:
    use the [`include_prefix`](/reference/be/c-cpp#cc_library.include_prefix) and
    [`strip_include_prefix`](/reference/be/c-cpp#cc_library.strip_include_prefix)
    arguments on the `cc_library` rule target.
+
+### Toolchain features {:#toolchain-features}
+
+The following optional [features](/docs/cc-toolchain-config-reference#features)
+can improve the hygiene of a C++ project. They can be enabled using the
+`--features` command-line flag or the `features` attribute of
+[`repo`](/external/overview#repo.bazel),
+[`package`](/reference/be/functions#package) or `cc_*` rules:
+
+* The `parse_headers` feature makes it so that the C++ compiler is used to parse
+  (but not compile) all header files in the built targets and their dependencies
+  when using the
+  [`--process_headers_in_dependencies`](/reference/command-line-reference#flag--process_headers_in_dependencies)
+  flag. This can help catch issues in header-only libraries and ensure that
+  headers are self-contained and independent of the order in which they are
+  included.
+* The `layering_check` feature enforces that targets only include headers
+  provided by their direct dependencies. The default toolchain supports this
+  feature on Linux with `clang` as the compiler.

src/test/shell/bazel/bazel_layering_check_test.sh

@@ -30,6 +30,12 @@ cc_binary(
   deps = [":hello_lib"],
 )
 
+cc_library(
+  name = 'hello_header_only',
+  hdrs = ['hello_header_only.h'],
+  deps = [":hello_lib"],
+)
+
 cc_library(
   name = "hello_lib",
   srcs = ["hello_private.h", "hellolib.cc"],
@@ -115,11 +121,28 @@ int main() {
   return hello() - 42;
 }
 #endif
+EOF
+
+  cat >hello/hello_header_only.h <<EOF
+#ifdef private_header
+#include "hello_private.h"
+int func() {
+  return helloPrivate() - 42;
+}
+#elif defined layering_violation
+#include "base.h"
+int func() {
+  return base() - 42;
+}
+#else
+#include "hello.h"
+int func() {
+  return hello() - 42;
+}
+#endif
 EOF
 }
 
-# TODO(hlopko): Add a test for a "toplevel" header-only library
-#   once we have parse_headers support in cc_configure.
 function test_bazel_layering_check() {
   if is_darwin; then
     echo "This test doesn't run on Darwin. Skipping."
@@ -134,8 +157,8 @@ function test_bazel_layering_check() {
 
   write_files
 
-  CC="${clang_tool}" bazel build --experimental_cc_implementation_deps \
-    //hello:hello --linkopt=-fuse-ld=gold --features=layering_check \
+  CC="${clang_tool}" bazel build \
+    //hello:hello --features=layering_check \
     &> "${TEST_log}" || fail "Build with layering_check failed"
 
   bazel-bin/hello/hello || fail "the built binary failed to run"
@@ -148,23 +171,65 @@ function test_bazel_layering_check() {
     fail "module map files were not generated"
   fi
 
-  # Specifying -fuse-ld=gold explicitly to override -fuse-ld=/usr/bin/ld.gold
-  # passed in by cc_configure because Ubuntu-16.04 ships with an old
-  # clang version that doesn't accept that.
-  CC="${clang_tool}" bazel build --experimental_cc_implementation_deps \
+  CC="${clang_tool}" bazel build \
     --copt=-D=private_header \
-    //hello:hello --linkopt=-fuse-ld=gold --features=layering_check \
+    //hello:hello --features=layering_check \
     &> "${TEST_log}" && fail "Build of private header violation with "\
     "layering_check should have failed"
   expect_log "use of private header from outside its module: 'hello_private.h'"
 
   CC="${clang_tool}" bazel build --experimental_cc_implementation_deps \
     --copt=-D=layering_violation \
-    //hello:hello --linkopt=-fuse-ld=gold --features=layering_check \
+    //hello:hello --features=layering_check \
     &> "${TEST_log}" && fail "Build of private header violation with "\
     "layering_check should have failed"
   expect_log "module //hello:hello does not depend on a module exporting "\
     "'base.h'"
 }
 
+function test_bazel_layering_check_header_only() {
+  if is_darwin; then
+    echo "This test doesn't run on Darwin. Skipping."
+    return
+  fi
+
+  local -r clang_tool=$(which clang)
+  if [[ ! -x ${clang_tool:-/usr/bin/clang_tool} ]]; then
+    echo "clang not installed. Skipping test."
+    return
+  fi
+
+  write_files
+
+  CC="${clang_tool}" bazel build \
+    //hello:hello_header_only --features=layering_check --features=parse_headers \
+    -s --process_headers_in_dependencies \
+    &> "${TEST_log}" || fail "Build with layering_check + parse_headers failed"
+
+  if [[ ! -e bazel-bin/hello/hello_header_only.cppmap ]]; then
+    fail "module map file for hello_header_only was not generated"
+  fi
+
+  if [[ ! -e bazel-bin/hello/hello_lib.cppmap ]]; then
+    fail "module map file for hello_lib was not generated"
+  fi
+
+  CC="${clang_tool}" bazel build \
+    --copt=-D=private_header \
+    //hello:hello_header_only --features=layering_check --features=parse_headers \
+    --process_headers_in_dependencies \
+    &> "${TEST_log}" && fail "Build of private header violation with "\
+    "layering_check + parse_headers should have failed"
+  expect_log "use of private header from outside its module: 'hello_private.h'"
+
+  CC="${clang_tool}" bazel build \
+    --copt=-D=layering_violation \
+    //hello:hello_header_only --features=layering_check --features=parse_headers \
+    --process_headers_in_dependencies \
+    &> "${TEST_log}" && fail "Build of private header violation with "\
+    "layering_check + parse_headers should have failed"
+  expect_log "module //hello:hello_header_only does not depend on a module exporting "\
+    "'base.h'"
+}
+
 run_suite "test layering_check"

src/test/shell/bazel/cc_integration_test.sh

@@ -1976,4 +1976,39 @@ EOF
   fi
 }
 
+function test_parse_headers_unclean() {
+  mkdir pkg
+  cat > pkg/BUILD <<'EOF'
+cc_library(name = "lib", hdrs = ["lib.h"])
+EOF
+  cat > pkg/lib.h <<'EOF'
+// Missing include of cstdint, which defines uint8_t.
+uint8_t foo();
+EOF
+
+  bazel build -s --process_headers_in_dependencies --features parse_headers \
+    //pkg:lib &> "$TEST_log" && fail "Build should have failed due to unclean headers"
+  expect_log "Compiling pkg/lib.h"
+  expect_log "error:.*'uint8_t'"
+
+  bazel build -s --process_headers_in_dependencies \
+    //pkg:lib &> "$TEST_log" || fail "Build should have passed"
+}
+
+function test_parse_headers_clean() {
+  mkdir pkg
+  cat > pkg/BUILD <<'EOF'
+package(features = ["parse_headers"])
+cc_library(name = "lib", hdrs = ["lib.h"])
+EOF
+  cat > pkg/lib.h <<'EOF'
+#include <cstdint>
+uint8_t foo();
+EOF
+
+  bazel build -s --process_headers_in_dependencies \
+    //pkg:lib &> "$TEST_log" || fail "Build should have passed"
+  expect_log "Compiling pkg/lib.h"
+}
+
 run_suite "cc_integration_test"

tools/cpp/BUILD.tpl

@@ -84,6 +84,7 @@ cc_toolchain(
     linker_files = ":compiler_deps",
     objcopy_files = ":empty",
     strip_files = ":empty",
+    supports_header_parsing = 1,
     supports_param_files = 1,
     module_map = %{modulemap},
 )

tools/cpp/linux_cc_wrapper.sh.tpl

@@ -18,8 +18,37 @@
 #
 set -eu
 
+OUTPUT=
+
+function parse_option() {
+    local -r opt="$1"
+    if [[ "${OUTPUT}" = "1" ]]; then
+        OUTPUT=$opt
+    elif [[ "$opt" = "-o" ]]; then
+        # output is coming
+        OUTPUT=1
+    fi
+}
+
+# let parse the option list
+for i in "$@"; do
+    if [[ "$i" = @* && -r "${i:1}" ]]; then
+        while IFS= read -r opt
+        do
+            parse_option "$opt"
+        done < "${i:1}" || exit 1
+    else
+        parse_option "$i"
+    fi
+done
+
 # Set-up the environment
 %{env}
 
 # Call the C++ compiler
 %{cc} "$@"
+
+# Generate an empty file if header processing succeeded.
+if [[ "${OUTPUT}" == *.h.processed ]]; then
+  echo -n > "${OUTPUT}"
+fi

tools/cpp/osx_cc_wrapper.sh.tpl

@@ -66,6 +66,11 @@ done
 # Call the C++ compiler
 %{cc} "$@"
 
+# Generate an empty file if header processing succeeded.
+if [[ "${OUTPUT}" == *.h.processed ]]; then
+  echo -n > "${OUTPUT}"
+fi
+
 function get_library_path() {
     for libdir in ${LIB_DIRS}; do
         if [ -f ${libdir}/lib$1.so ]; then

tools/cpp/unix_cc_configure.bzl

@@ -387,6 +387,10 @@ def configure_unix_toolchain(repository_ctx, cpu_value, overriden_tools):
         overriden_tools["ar"] = _find_generic(repository_ctx, "libtool", "LIBTOOL", overriden_tools)
     auto_configure_warning_maybe(repository_ctx, "CC used: " + str(cc))
     tool_paths = _get_tool_paths(repository_ctx, overriden_tools)
+
+    # The parse_header tool needs to be a wrapper around the compiler as it has
+    # to touch the output file.
+    tool_paths["parse_headers"] = "cc_wrapper.sh"
     cc_toolchain_identifier = escape_string(get_env_var(
         repository_ctx,
         "CC_TOOLCHAIN_NAME",
@@ -546,9 +550,10 @@ def configure_unix_toolchain(repository_ctx, cpu_value, overriden_tools):
             "%{cc_toolchain_identifier}": cc_toolchain_identifier,
             "%{name}": cpu_value,
             "%{modulemap}": ("\":module.modulemap\"" if generate_modulemap else "None"),
-            "%{cc_compiler_deps}": get_starlark_list([":builtin_include_directory_paths"] + (
-                [":cc_wrapper"] if darwin else []
-            )),
+            "%{cc_compiler_deps}": get_starlark_list([
+                ":builtin_include_directory_paths",
+                ":cc_wrapper",
+            ]),
             "%{compiler}": escape_string(get_env_var(
                 repository_ctx,
                 "BAZEL_COMPILER",

tools/cpp/unix_cc_toolchain_config.bzl

@@ -96,6 +96,47 @@ def layering_check_features(compiler):
         ),
     ]
 
+def parse_headers_support(parse_headers_tool_path):
+    if not parse_headers_tool_path:
+        return [], []
+    action_configs = [
+        action_config(
+            action_name = ACTION_NAMES.cpp_header_parsing,
+            tools = [
+                tool(path = parse_headers_tool_path),
+            ],
+            flag_sets = [
+                flag_set(
+                    flag_groups = [
+                        flag_group(
+                            flags = [
+                                # Note: This treats all headers as C++ headers, which may lead to
+                                # parsing failures for C headers that are not valid C++.
+                                # For such headers, use features = ["-parse_headers"] to selectively
+                                # disable parsing.
+                                "-xc++-header",
+                                "-fsyntax-only",
+                            ],
+                        ),
+                    ],
+                ),
+            ],
+            implies = [
+                # Copied from the legacy feature definition in CppActionConfigs.java.
+                "legacy_compile_flags",
+                "user_compile_flags",
+                "sysroot",
+                "unfiltered_compile_flags",
+                "compiler_input_flags",
+                "compiler_output_flags",
+            ],
+        ),
+    ]
+    features = [
+        feature(name = "parse_headers"),
+    ]
+    return action_configs, features
+
 all_compile_actions = [
     ACTION_NAMES.c_compile,
     ACTION_NAMES.cpp_compile,
@@ -1485,6 +1526,12 @@ def _impl(ctx):
             generate_linkmap_feature,
         ]
 
+    parse_headers_action_configs, parse_headers_features = parse_headers_support(
+        parse_headers_tool_path = ctx.attr.tool_paths.get("parse_headers"),
+    )
+    action_configs += parse_headers_action_configs
+    features += parse_headers_features
+
     return cc_common.create_cc_toolchain_config_info(
         ctx = ctx,
         features = features,