From adfe3fe2c176f65dc4216ab1f3179e1022572022 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Tue, 14 May 2024 11:44:09 -0700 Subject: [PATCH] Add `parse_headers` support to the default Unix toolchain 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 --- site/en/docs/bazel-and-cpp.md | 19 +++++ .../shell/bazel/bazel_layering_check_test.sh | 85 ++++++++++++++++--- src/test/shell/bazel/cc_integration_test.sh | 35 ++++++++ tools/cpp/BUILD.tpl | 1 + tools/cpp/linux_cc_wrapper.sh.tpl | 29 +++++++ tools/cpp/osx_cc_wrapper.sh.tpl | 5 ++ tools/cpp/unix_cc_configure.bzl | 11 ++- tools/cpp/unix_cc_toolchain_config.bzl | 47 ++++++++++ 8 files changed, 219 insertions(+), 13 deletions(-) diff --git a/site/en/docs/bazel-and-cpp.md b/site/en/docs/bazel-and-cpp.md index aab47cdcccd08d..866f0f7beac0aa 100644 --- a/site/en/docs/bazel-and-cpp.md +++ b/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. diff --git a/src/test/shell/bazel/bazel_layering_check_test.sh b/src/test/shell/bazel/bazel_layering_check_test.sh index 4942f462944e99..45154937837efe 100755 --- a/src/test/shell/bazel/bazel_layering_check_test.sh +++ b/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 < "${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" diff --git a/src/test/shell/bazel/cc_integration_test.sh b/src/test/shell/bazel/cc_integration_test.sh index 2512dc300c3bb5..ccb926538fb0ee 100755 --- a/src/test/shell/bazel/cc_integration_test.sh +++ b/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 +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" diff --git a/tools/cpp/BUILD.tpl b/tools/cpp/BUILD.tpl index 384b63a12c0c53..f99995d2b923eb 100644 --- a/tools/cpp/BUILD.tpl +++ b/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}, ) diff --git a/tools/cpp/linux_cc_wrapper.sh.tpl b/tools/cpp/linux_cc_wrapper.sh.tpl index a83be50655c3d9..629741e55d181d 100644 --- a/tools/cpp/linux_cc_wrapper.sh.tpl +++ b/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 diff --git a/tools/cpp/osx_cc_wrapper.sh.tpl b/tools/cpp/osx_cc_wrapper.sh.tpl index 8264090c29604d..ff762cc40b4047 100644 --- a/tools/cpp/osx_cc_wrapper.sh.tpl +++ b/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 diff --git a/tools/cpp/unix_cc_configure.bzl b/tools/cpp/unix_cc_configure.bzl index ca8c1ba8ff7763..d123a3d1aaa099 100644 --- a/tools/cpp/unix_cc_configure.bzl +++ b/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", diff --git a/tools/cpp/unix_cc_toolchain_config.bzl b/tools/cpp/unix_cc_toolchain_config.bzl index 501d904ee693c2..ce4406aea12058 100644 --- a/tools/cpp/unix_cc_toolchain_config.bzl +++ b/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,