Add layering check support to cc_configure on Linux with Clang This PR teaches the C++ toolchain autoconfiguration (`cc_configure`) about `layering_check`. Layering check is a feature of C++ rules in concert with clang that will fail the compilation action when a transitive header was depended on directly. The fix is to add a direct dependency on the target providing the header. The result is more explicit dependency graph where one can be sure that all targets that depend on a header depend on it directly. Layering check also validates that private headers are not depended on directly. To make your codebase satisfy the layering check incrementally, I suggest to enable layering check by default (e.g. by putting `build --features=layering_check` into your `.bazelrc` file), and then opting specific targets out temporarily by disabling the feature in the BUILD file by putting `features = ["-layering_check"]` attribute into the C++ rule. In order to do that we need to generate a module map for the toolchain. This is done by taking any file present transitively in one of the default include directories reported by the compiler. For system installed compilers this map can be quite big as they often look into directories such as `/usr/include` etc. As anecdata, the header map on my home machine is ~ 27K lines long. Computing this map takes too much time when implemented in idiomatic Starlark (~16s on my machine), therefore I've created a shell script that does the work faster (~2s on my machine). The map is only created when the compiler used is `clang`. If the compiler is `clang` is detected by running `$CC -v` and searching for string "clang" :) Closes #11440. PiperOrigin-RevId: 313173938
diff --git a/src/test/shell/bazel/BUILD b/src/test/shell/bazel/BUILD index d61629f..df59961 100644 --- a/src/test/shell/bazel/BUILD +++ b/src/test/shell/bazel/BUILD
@@ -550,6 +550,15 @@ ) sh_test( + name = "bazel_layering_check_test", + srcs = ["bazel_layering_check_test.sh"], + data = [":test-deps"], + tags = [ + "no_windows", + ], +) + +sh_test( name = "bazel_localtest_test", srcs = ["bazel_localtest_test.sh"], data = [":test-deps"],
diff --git a/src/test/shell/bazel/bazel_layering_check_test.sh b/src/test/shell/bazel/bazel_layering_check_test.sh new file mode 100755 index 0000000..8796478 --- /dev/null +++ b/src/test/shell/bazel/bazel_layering_check_test.sh
@@ -0,0 +1,149 @@ +#!/bin/bash +# +# Copyright 2020 The Bazel Authors. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +set -euo pipefail + +# Load the test setup defined in the parent directory +CURRENT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +source "${CURRENT_DIR}/../integration_test_setup.sh" \ + || { echo "integration_test_setup.sh not found!" >&2; exit 1; } + +function write_files { + mkdir -p hello || fail "mkdir hello failed" + cat >hello/BUILD <<EOF +cc_binary( + name = 'hello', + srcs = ['hello.cc'], + deps = [":hello_lib"], +) + +cc_library( + name = "hello_lib", + srcs = ["hello_private.h", "hellolib.cc"], + hdrs = ["hello.h"], + deps = [":base"], +) + +cc_library( + name = "base", + srcs = ["base.cc"], + hdrs = ["base.h"], +) +EOF + cat >hello/hello.h <<EOF +#include "hello_private.h" +int hello(); +EOF + + cat >hello/hello_private.h <<EOF +int helloPrivate(); +EOF + + cat >hello/base.h <<EOF +int base(); +EOF + + cat >hello/base.cc <<EOF +#include "base.h" + +int base() { + return 42; +} +EOF + + cat >hello/hellolib.cc <<EOF +#include "hello.h" +#include "base.h" + +int helloPrivate() { + return base(); +} + +int hello() { + return helloPrivate(); +} +EOF + + cat >hello/hello.cc <<EOF +#ifdef private_header +#include "hello_private.h" +int main() { + return helloPrivate() - 42; +} +#elif defined layering_violation +#include "base.h" +int main() { + return base() - 42; +} +#else +#include "hello.h" +int main() { + 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." + 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 --linkopt=-fuse-ld=gold --features=layering_check \ + &> "${TEST_log}" || fail "Build with layering_check failed" + + bazel-bin/hello/hello || fail "the built binary failed to run" + + if [[ ! -e bazel-bin/hello/hello.cppmap ]]; then + fail "module map files were not generated" + fi + + if [[ ! -e bazel-bin/hello/hello_lib.cppmap ]]; then + 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 \ + --copt=-D=private_header \ + //hello:hello --linkopt=-fuse-ld=gold --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 \ + --copt=-D=layering_violation \ + //hello:hello --linkopt=-fuse-ld=gold --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'" +} + +run_suite "test layering_check"
diff --git a/tools/cpp/BUILD.tpl b/tools/cpp/BUILD.tpl index 730224f..37986a9 100644 --- a/tools/cpp/BUILD.tpl +++ b/tools/cpp/BUILD.tpl
@@ -67,6 +67,7 @@ objcopy_files = ":empty", strip_files = ":empty", supports_param_files = %{supports_param_files}, + module_map = %{modulemap}, ) cc_toolchain_config(
diff --git a/tools/cpp/generate_system_module_map.sh b/tools/cpp/generate_system_module_map.sh new file mode 100755 index 0000000..a232c2b --- /dev/null +++ b/tools/cpp/generate_system_module_map.sh
@@ -0,0 +1,26 @@ +#!/bin/sh +# Copyright 2020 The Bazel Authors. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +set -eu + +echo 'module "crosstool" [system] {' + +for dir in $@; do + find -L "${dir}" -type f 2>/dev/null | sort | uniq | while read header; do + echo " textual header \"${header}\"" + done +done + +echo "}"
diff --git a/tools/cpp/unix_cc_configure.bzl b/tools/cpp/unix_cc_configure.bzl index 84f5479..d48485b 100644 --- a/tools/cpp/unix_cc_configure.bzl +++ b/tools/cpp/unix_cc_configure.bzl
@@ -20,6 +20,7 @@ "auto_configure_warning", "auto_configure_warning_maybe", "escape_string", + "execute", "get_env_var", "get_starlark_list", "resolve_labels", @@ -28,32 +29,25 @@ "write_builtin_include_directory_paths", ) -def _field(name, value): - """Returns properly indented top level crosstool field.""" - if type(value) == "list": - return "\n".join([" " + name + ": '" + v + "'" for v in value]) - elif type(value) == "string": - return " " + name + ": '" + value + "'" - else: - auto_configure_fail("Unexpected field type: " + type(value)) - return "" - def _uniq(iterable): """Remove duplicates from a list.""" unique_elements = {element: None for element in iterable} return unique_elements.keys() +def _generate_system_module_map(repository_ctx, dirs, script_path): + return execute(repository_ctx, [script_path] + dirs) + def _prepare_include_path(repo_ctx, path): - """Resolve and sanitize include path before outputting it into the crosstool. + """Resolve include path before outputting it into the crosstool. Args: repo_ctx: repository_ctx object. - path: an include path to be sanitized. + path: an include path to be resolved. Returns: - Sanitized include path that can be written to the crosstoot. Resulting path - is absolute if it is outside the repository and relative otherwise. + Resolved include path. Resulting path is absolute if it is outside the + repository and relative otherwise. """ repo_root = str(repo_ctx.path(".")) @@ -62,8 +56,8 @@ repo_root += "/" path = str(repo_ctx.path(path)) if path.startswith(repo_root): - return escape_string(path[len(repo_root):]) - return escape_string(path) + return path[len(repo_root):] + return path def _get_value(it): """Convert `it` in serialized protobuf format.""" @@ -123,7 +117,20 @@ return path def get_escaped_cxx_inc_directories(repository_ctx, cc, lang_flag, additional_flags = []): - """Compute the list of default %-escaped C++ include directories.""" + """Deprecated. Compute the list of %-escaped C++ include directories. + + This function is no longer needed by cc_configure and is left there only for backwards + compatibility reasons. + """ + return [escape_string(s) for s in _get_cxx_include_directories( + repository_ctx, + cc, + lang_flag, + additional_flags, + )] + +def _get_cxx_include_directories(repository_ctx, cc, lang_flag, additional_flags = []): + """Compute the list of C++ include directories.""" result = repository_ctx.execute([cc, "-E", lang_flag, "-", "-v"] + additional_flags) index1 = result.stderr.find(_INC_DIR_MARKER_BEGIN) if index1 == -1: @@ -284,6 +291,9 @@ link_flags = '"--coverage"' return compile_flags, link_flags +def _is_clang(repository_ctx, cc): + return "clang" in repository_ctx.execute([cc, "-v"]).stderr + def _find_generic(repository_ctx, name, env_name, overriden_tools, warn = False, silent = False): """Find a generic C++ toolchain tool. Doesn't %-escape the result.""" @@ -319,6 +329,7 @@ """Configure C++ toolchain on Unix platforms.""" paths = resolve_labels(repository_ctx, [ "@bazel_tools//tools/cpp:BUILD.tpl", + "@bazel_tools//tools/cpp:generate_system_module_map.sh", "@bazel_tools//tools/cpp:armeabi_cc_toolchain_config.bzl", "@bazel_tools//tools/cpp:unix_cc_toolchain_config.bzl", "@bazel_tools//tools/cpp:linux_cc_wrapper.sh.tpl", @@ -339,6 +350,7 @@ darwin = cpu_value == "darwin" cc = _find_generic(repository_ctx, "gcc", "CC", overriden_tools) + is_clang = _is_clang(repository_ctx, cc) overriden_tools = dict(overriden_tools) overriden_tools["gcc"] = cc overriden_tools["gcov"] = _find_generic( @@ -408,15 +420,15 @@ coverage_compile_flags, coverage_link_flags = _coverage_flags(repository_ctx, darwin) builtin_include_directories = _uniq( - get_escaped_cxx_inc_directories(repository_ctx, cc, "-xc") + - get_escaped_cxx_inc_directories(repository_ctx, cc, "-xc++", cxx_opts) + - get_escaped_cxx_inc_directories( + _get_cxx_include_directories(repository_ctx, cc, "-xc") + + _get_cxx_include_directories(repository_ctx, cc, "-xc++", cxx_opts) + + _get_cxx_include_directories( repository_ctx, cc, "-xc", _get_no_canonical_prefixes_opt(repository_ctx, cc), ) + - get_escaped_cxx_inc_directories( + _get_cxx_include_directories( repository_ctx, cc, "-xc++", @@ -424,6 +436,13 @@ ), ) + if is_clang: + repository_ctx.file("module.modulemap", _generate_system_module_map( + repository_ctx, + builtin_include_directories, + paths["@bazel_tools//tools/cpp:generate_system_module_map.sh"], + )) + write_builtin_include_directory_paths(repository_ctx, cc, builtin_include_directories) repository_ctx.template( "BUILD", @@ -431,6 +450,7 @@ { "%{cc_toolchain_identifier}": cc_toolchain_identifier, "%{name}": cpu_value, + "%{modulemap}": ("\":module.modulemap\"" if is_clang else "None"), "%{supports_param_files}": "0" if darwin else "1", "%{cc_compiler_deps}": get_starlark_list([":builtin_include_directory_paths"] + ( [":cc_wrapper"] if darwin else [] @@ -438,7 +458,7 @@ "%{compiler}": escape_string(get_env_var( repository_ctx, "BAZEL_COMPILER", - "compiler", + "clang" if is_clang else "compiler", False, )), "%{abi_version}": escape_string(get_env_var(
diff --git a/tools/cpp/unix_cc_toolchain_config.bzl b/tools/cpp/unix_cc_toolchain_config.bzl index afcf651..6fca87a 100644 --- a/tools/cpp/unix_cc_toolchain_config.bzl +++ b/tools/cpp/unix_cc_toolchain_config.bzl
@@ -26,6 +26,66 @@ ) load("@bazel_tools//tools/build_defs/cc:action_names.bzl", "ACTION_NAMES") +def layering_check_features(compiler): + if compiler != "clang": + return [] + return [ + feature( + name = "use_module_maps", + requires = [feature_set(features = ["module_maps"])], + flag_sets = [ + flag_set( + actions = [ + ACTION_NAMES.c_compile, + ACTION_NAMES.cpp_compile, + ACTION_NAMES.cpp_header_parsing, + ACTION_NAMES.cpp_module_compile, + ], + flag_groups = [ + flag_group( + flags = [ + "-fmodule-name=%{module_name}", + "-fmodule-map-file=%{module_map_file}", + ], + ), + ], + ), + ], + ), + + # Tell blaze we support module maps in general, so they will be generated + # for all c/c++ rules. + # Note: not all C++ rules support module maps; thus, do not imply this + # feature from other features - instead, require it. + feature(name = "module_maps", enabled = True), + feature( + name = "layering_check", + implies = ["use_module_maps"], + flag_sets = [ + flag_set( + actions = [ + ACTION_NAMES.c_compile, + ACTION_NAMES.cpp_compile, + ACTION_NAMES.cpp_header_parsing, + ACTION_NAMES.cpp_module_compile, + ], + flag_groups = [ + flag_group(flags = [ + "-fmodules-strict-decluse", + "-Wprivate-header", + ]), + flag_group( + iterate_over = "dependent_module_map_files", + flags = [ + "-fmodule-map-file=%{dependent_module_map_files}", + ], + ), + ], + ), + ], + ), + ] + all_compile_actions = [ ACTION_NAMES.c_compile, ACTION_NAMES.cpp_compile, @@ -1131,7 +1191,7 @@ user_compile_flags_feature, sysroot_feature, unfiltered_compile_flags_feature, - ] + ] + layering_check_features(ctx.attr.compiler) else: features = [ supports_pic_feature, @@ -1150,7 +1210,7 @@ user_compile_flags_feature, sysroot_feature, unfiltered_compile_flags_feature, - ] + ] + layering_check_features(ctx.attr.compiler) return cc_common.create_cc_toolchain_config_info( ctx = ctx,