Skip to content

Commit 231dfc2

Browse files
fmeumcopybara-github
fmeum
authored and
copybara-github
committed
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
1 parent 6306240 commit 231dfc2

File tree

8 files changed

+217
-11
lines changed

8 files changed

+217
-11
lines changed

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

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,3 +82,22 @@ Follow these guidelines for include paths:
8282
use the [`include_prefix`](/reference/be/c-cpp#cc_library.include_prefix) and
8383
[`strip_include_prefix`](/reference/be/c-cpp#cc_library.strip_include_prefix)
8484
arguments on the `cc_library` rule target.
85+
86+
### Toolchain features {:#toolchain-features}
87+
88+
The following optional [features](/docs/cc-toolchain-config-reference#features)
89+
can improve the hygiene of a C++ project. They can be enabled using the
90+
`--features` command-line flag or the `features` attribute of
91+
[`repo`](/external/overview#repo.bazel),
92+
[`package`](/reference/be/functions#package) or `cc_*` rules:
93+
94+
* The `parse_headers` feature makes it so that the C++ compiler is used to parse
95+
(but not compile) all header files in the built targets and their dependencies
96+
when using the
97+
[`--process_headers_in_dependencies`](/reference/command-line-reference#flag--process_headers_in_dependencies)
98+
flag. This can help catch issues in header-only libraries and ensure that
99+
headers are self-contained and independent of the order in which they are
100+
included.
101+
* The `layering_check` feature enforces that targets only include headers
102+
provided by their direct dependencies. The default toolchain supports this
103+
feature on Linux with `clang` as the compiler.

‎src/test/shell/bazel/bazel_layering_check_test.sh‎

Lines changed: 73 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,12 @@ cc_binary(
3030
deps = [":hello_lib"],
3131
)
3232
33+
cc_library(
34+
name = 'hello_header_only',
35+
hdrs = ['hello_header_only.h'],
36+
deps = [":hello_lib"],
37+
)
38+
3339
cc_library(
3440
name = "hello_lib",
3541
srcs = ["hello_private.h", "hellolib.cc"],
@@ -115,11 +121,28 @@ int main() {
115121
return hello() - 42;
116122
}
117123
#endif
124+
EOF
125+
126+
cat >hello/hello_header_only.h <<EOF
127+
#ifdef private_header
128+
#include "hello_private.h"
129+
int func() {
130+
return helloPrivate() - 42;
131+
}
132+
#elif defined layering_violation
133+
#include "base.h"
134+
int func() {
135+
return base() - 42;
136+
}
137+
#else
138+
#include "hello.h"
139+
int func() {
140+
return hello() - 42;
141+
}
142+
#endif
118143
EOF
119144
}
120145

121-
# TODO(hlopko): Add a test for a "toplevel" header-only library
122-
# once we have parse_headers support in cc_configure.
123146
function test_bazel_layering_check() {
124147
if is_darwin; then
125148
echo "This test doesn't run on Darwin. Skipping."
@@ -135,7 +158,7 @@ function test_bazel_layering_check() {
135158
write_files
136159

137160
CC="${clang_tool}" bazel build \
138-
//hello:hello --linkopt=-fuse-ld=gold --features=layering_check \
161+
//hello:hello --features=layering_check \
139162
&> "${TEST_log}" || fail "Build with layering_check failed"
140163

141164
bazel-bin/hello/hello || fail "the built binary failed to run"
@@ -148,23 +171,65 @@ function test_bazel_layering_check() {
148171
fail "module map files were not generated"
149172
fi
150173

151-
# Specifying -fuse-ld=gold explicitly to override -fuse-ld=/usr/bin/ld.gold
152-
# passed in by cc_configure because Ubuntu-16.04 ships with an old
153-
# clang version that doesn't accept that.
154174
CC="${clang_tool}" bazel build \
155175
--copt=-D=private_header \
156-
//hello:hello --linkopt=-fuse-ld=gold --features=layering_check \
176+
//hello:hello --features=layering_check \
157177
&> "${TEST_log}" && fail "Build of private header violation with "\
158178
"layering_check should have failed"
159179
expect_log "use of private header from outside its module: 'hello_private.h'"
160180

161181
CC="${clang_tool}" bazel build \
162182
--copt=-D=layering_violation \
163-
//hello:hello --linkopt=-fuse-ld=gold --features=layering_check \
183+
//hello:hello --features=layering_check \
164184
&> "${TEST_log}" && fail "Build of private header violation with "\
165185
"layering_check should have failed"
166186
expect_log "module //hello:hello does not depend on a module exporting "\
167187
"'base.h'"
168188
}
169189

190+
function test_bazel_layering_check_header_only() {
191+
if is_darwin; then
192+
echo "This test doesn't run on Darwin. Skipping."
193+
return
194+
fi
195+
196+
local -r clang_tool=$(which clang)
197+
if [[ ! -x ${clang_tool:-/usr/bin/clang_tool} ]]; then
198+
echo "clang not installed. Skipping test."
199+
return
200+
fi
201+
202+
write_files
203+
204+
CC="${clang_tool}" bazel build \
205+
//hello:hello_header_only --features=layering_check --features=parse_headers \
206+
-s --process_headers_in_dependencies \
207+
&> "${TEST_log}" || fail "Build with layering_check + parse_headers failed"
208+
209+
if [[ ! -e bazel-bin/hello/hello_header_only.cppmap ]]; then
210+
fail "module map file for hello_header_only was not generated"
211+
fi
212+
213+
if [[ ! -e bazel-bin/hello/hello_lib.cppmap ]]; then
214+
fail "module map file for hello_lib was not generated"
215+
fi
216+
217+
CC="${clang_tool}" bazel build \
218+
--copt=-D=private_header \
219+
//hello:hello_header_only --features=layering_check --features=parse_headers \
220+
--process_headers_in_dependencies \
221+
&> "${TEST_log}" && fail "Build of private header violation with "\
222+
"layering_check + parse_headers should have failed"
223+
expect_log "use of private header from outside its module: 'hello_private.h'"
224+
225+
CC="${clang_tool}" bazel build \
226+
--copt=-D=layering_violation \
227+
//hello:hello_header_only --features=layering_check --features=parse_headers \
228+
--process_headers_in_dependencies \
229+
&> "${TEST_log}" && fail "Build of private header violation with "\
230+
"layering_check + parse_headers should have failed"
231+
expect_log "module //hello:hello_header_only does not depend on a module exporting "\
232+
"'base.h'"
233+
}
234+
170235
run_suite "test layering_check"

‎src/test/shell/bazel/cc_integration_test.sh‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1909,4 +1909,39 @@ EOF
19091909
fi
19101910
}
19111911

1912+
function test_parse_headers_unclean() {
1913+
mkdir pkg
1914+
cat > pkg/BUILD <<'EOF'
1915+
cc_library(name = "lib", hdrs = ["lib.h"])
1916+
EOF
1917+
cat > pkg/lib.h <<'EOF'
1918+
// Missing include of cstdint, which defines uint8_t.
1919+
uint8_t foo();
1920+
EOF
1921+
1922+
bazel build -s --process_headers_in_dependencies --features parse_headers \
1923+
//pkg:lib &> "$TEST_log" && fail "Build should have failed due to unclean headers"
1924+
expect_log "Compiling pkg/lib.h"
1925+
expect_log "error:.*'uint8_t'"
1926+
1927+
bazel build -s --process_headers_in_dependencies \
1928+
//pkg:lib &> "$TEST_log" || fail "Build should have passed"
1929+
}
1930+
1931+
function test_parse_headers_clean() {
1932+
mkdir pkg
1933+
cat > pkg/BUILD <<'EOF'
1934+
package(features = ["parse_headers"])
1935+
cc_library(name = "lib", hdrs = ["lib.h"])
1936+
EOF
1937+
cat > pkg/lib.h <<'EOF'
1938+
#include <cstdint>
1939+
uint8_t foo();
1940+
EOF
1941+
1942+
bazel build -s --process_headers_in_dependencies \
1943+
//pkg:lib &> "$TEST_log" || fail "Build should have passed"
1944+
expect_log "Compiling pkg/lib.h"
1945+
}
1946+
19121947
run_suite "cc_integration_test"

‎tools/cpp/BUILD.tpl‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,7 @@ cc_toolchain(
8484
linker_files = ":compiler_deps",
8585
objcopy_files = ":empty",
8686
strip_files = ":empty",
87+
supports_header_parsing = 1,
8788
supports_param_files = 1,
8889
module_map = %{modulemap},
8990
)

‎tools/cpp/linux_cc_wrapper.sh.tpl‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,37 @@
1818
#
1919
set -eu
2020

21+
OUTPUT=
22+
23+
function parse_option() {
24+
local -r opt="$1"
25+
if [[ "${OUTPUT}" = "1" ]]; then
26+
OUTPUT=$opt
27+
elif [[ "$opt" = "-o" ]]; then
28+
# output is coming
29+
OUTPUT=1
30+
fi
31+
}
32+
33+
# let parse the option list
34+
for i in "$@"; do
35+
if [[ "$i" = @* && -r "${i:1}" ]]; then
36+
while IFS= read -r opt
37+
do
38+
parse_option "$opt"
39+
done < "${i:1}" || exit 1
40+
else
41+
parse_option "$i"
42+
fi
43+
done
44+
2145
# Set-up the environment
2246
%{env}
2347

2448
# Call the C++ compiler
2549
%{cc} "$@"
50+
51+
# Generate an empty file if header processing succeeded.
52+
if [[ "${OUTPUT}" == *.h.processed ]]; then
53+
echo -n > "${OUTPUT}"
54+
fi

‎tools/cpp/osx_cc_wrapper.sh.tpl‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,11 @@ done
7171
# Call the C++ compiler
7272
%{cc} "$@"
7373

74+
# Generate an empty file if header processing succeeded.
75+
if [[ "${OUTPUT}" == *.h.processed ]]; then
76+
echo -n > "${OUTPUT}"
77+
fi
78+
7479
function get_library_path() {
7580
for libdir in ${LIB_DIRS}; do
7681
if [ -f ${libdir}/lib$1.so ]; then

‎tools/cpp/unix_cc_configure.bzl‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -387,6 +387,10 @@ def configure_unix_toolchain(repository_ctx, cpu_value, overriden_tools):
387387
overriden_tools["ar"] = _find_generic(repository_ctx, "libtool", "LIBTOOL", overriden_tools)
388388
auto_configure_warning_maybe(repository_ctx, "CC used: " + str(cc))
389389
tool_paths = _get_tool_paths(repository_ctx, overriden_tools)
390+
391+
# The parse_header tool needs to be a wrapper around the compiler as it has
392+
# to touch the output file.
393+
tool_paths["parse_headers"] = "cc_wrapper.sh"
390394
cc_toolchain_identifier = escape_string(get_env_var(
391395
repository_ctx,
392396
"CC_TOOLCHAIN_NAME",
@@ -546,9 +550,10 @@ def configure_unix_toolchain(repository_ctx, cpu_value, overriden_tools):
546550
"%{cc_toolchain_identifier}": cc_toolchain_identifier,
547551
"%{name}": cpu_value,
548552
"%{modulemap}": ("\":module.modulemap\"" if generate_modulemap else "None"),
549-
"%{cc_compiler_deps}": get_starlark_list([":builtin_include_directory_paths"] + (
550-
[":cc_wrapper"] if darwin else []
551-
)),
553+
"%{cc_compiler_deps}": get_starlark_list([
554+
":builtin_include_directory_paths",
555+
":cc_wrapper",
556+
]),
552557
"%{compiler}": escape_string(get_env_var(
553558
repository_ctx,
554559
"BAZEL_COMPILER",

‎tools/cpp/unix_cc_toolchain_config.bzl‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,47 @@ def layering_check_features(compiler):
9696
),
9797
]
9898

99+
def parse_headers_support(parse_headers_tool_path):
100+
if not parse_headers_tool_path:
101+
return [], []
102+
action_configs = [
103+
action_config(
104+
action_name = ACTION_NAMES.cpp_header_parsing,
105+
tools = [
106+
tool(path = parse_headers_tool_path),
107+
],
108+
flag_sets = [
109+
flag_set(
110+
flag_groups = [
111+
flag_group(
112+
flags = [
113+
# Note: This treats all headers as C++ headers, which may lead to
114+
# parsing failures for C headers that are not valid C++.
115+
# For such headers, use features = ["-parse_headers"] to selectively
116+
# disable parsing.
117+
"-xc++-header",
118+
"-fsyntax-only",
119+
],
120+
),
121+
],
122+
),
123+
],
124+
implies = [
125+
# Copied from the legacy feature definition in CppActionConfigs.java.
126+
"legacy_compile_flags",
127+
"user_compile_flags",
128+
"sysroot",
129+
"unfiltered_compile_flags",
130+
"compiler_input_flags",
131+
"compiler_output_flags",
132+
],
133+
),
134+
]
135+
features = [
136+
feature(name = "parse_headers"),
137+
]
138+
return action_configs, features
139+
99140
all_compile_actions = [
100141
ACTION_NAMES.c_compile,
101142
ACTION_NAMES.cpp_compile,
@@ -1488,6 +1529,12 @@ def _impl(ctx):
14881529
generate_linkmap_feature,
14891530
]
14901531

1532+
parse_headers_action_configs, parse_headers_features = parse_headers_support(
1533+
parse_headers_tool_path = ctx.attr.tool_paths.get("parse_headers"),
1534+
)
1535+
action_configs += parse_headers_action_configs
1536+
features += parse_headers_features
1537+
14911538
return cc_common.create_cc_toolchain_config_info(
14921539
ctx = ctx,
14931540
features = features,

0 commit comments

Comments
 (0)