Skip to content

Commit

Permalink
Implement raw_headers_as_headers_mode
Browse files Browse the repository at this point in the history
Summary:
There has been a tension, a disconnect, between `raw_headers`
and `headers` for years. `raw_headers` + `include_directories` were a necessity
to support msvc and third-party code which does 'bad things' with includes,
like, `#include "../some_header.h"`, or which would have problems with buck
generated symlink trees and include the same header via different paths.
This results in buck targets either using `headers` (and fixing or working
around the above issues) or code using `raw_headers` (and not having to worry
about it). Reconciling this tension was never a priority because it felt
like a simple harmless convention.

There is a harm, though. The Windows filesystem is slow. Clang on Windows,
when searching for headers, has to wade through potentially hundreds or
thousands of directories for each one. In the process clang also performs
a filename canonicalization operation that can contend on a lock deep in
the filesystem, when run with large local concurrency.
This was profiled as taking up to 7 seconds to resolve `<string>` in a .cpp file
in a certain build, compared to potentially just 100ms without having all of
the extraneous user deps. This is because clang always searches system
header paths last, after searching user include paths, because that
is how the standard library header paths are specified.

There is a mitigation, however. Clang supports header maps, which are
essentially 'pre-indexed' header lookup tables. Instead of searching
the filesystem, clang just does an in memory map lookup to resolve
an include. No lock contention, no slowdowns in the filesystem layers.

While we can operate buck in a 'mixed' mode where code using `headers`
can take advantage of header maps, targets using `raw_headers` were out
of luck. Until now. Paralleling the similarly named (but presently unused)
`headers_as_raw_headers_mode`, this introduces a conversion mechanism
which can convert `raw_headers` and `include_directories` into an internal
representation that can treat them as `headers`. Thus we can emit header
maps for all targets, and then reap the performance benefits for clang
users on Windows.

A per target setting is also supported for targets we can't or don't
want to fix. Or, for incremental adoptions by using a default disabled
mode but enabling it on specific targets.

The behavior is disabled entirely unless the toolchain explicitly
opts into the feature through a non-default value on `raw_headers_as_headers_mode`
on the `cxx_toolchain`.

Reviewed By: jtbraun

Differential Revision: D65517235

fbshipit-source-id: 778d79cfc5ede5c114b342c67b4f90cd6a7b9742
  • Loading branch information
Andrew Krieger authored and facebook-github-bot committed Nov 16, 2024
1 parent a66b931 commit 9f90f62
Show file tree
Hide file tree
Showing 8 changed files with 119 additions and 9 deletions.
3 changes: 2 additions & 1 deletion cxx/cxx_toolchain.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ load(
)
load("@prelude//cxx:cxx_utility.bzl", "cxx_toolchain_allow_cache_upload_args")
load("@prelude//cxx:debug.bzl", "SplitDebugMode")
load("@prelude//cxx:headers.bzl", "HeaderMode", "HeadersAsRawHeadersMode")
load("@prelude//cxx:headers.bzl", "HeaderMode", "HeadersAsRawHeadersMode", "RawHeadersAsHeadersMode")
load("@prelude//cxx:linker.bzl", "LINKERS", "is_pdb_generated")
load("@prelude//cxx:target_sdk_version.bzl", "get_toolchain_target_sdk_version")
load("@prelude//linking:link_info.bzl", "LinkOrdering", "LinkStyle")
Expand Down Expand Up @@ -186,6 +186,7 @@ def cxx_toolchain_impl(ctx):
llvm_link = ctx.attrs.llvm_link[RunInfo] if ctx.attrs.llvm_link else None,
object_format = CxxObjectFormat(object_format),
headers_as_raw_headers_mode = HeadersAsRawHeadersMode(ctx.attrs.headers_as_raw_headers_mode) if ctx.attrs.headers_as_raw_headers_mode != None else None,
raw_headers_as_headers_mode = RawHeadersAsHeadersMode(ctx.attrs.raw_headers_as_headers_mode) if ctx.attrs.raw_headers_as_headers_mode != None else None,
conflicting_header_basename_allowlist = ctx.attrs.conflicting_header_basename_exemptions,
pic_behavior = PicBehavior(ctx.attrs.pic_behavior),
split_debug_mode = SplitDebugMode(ctx.attrs.split_debug_mode),
Expand Down
3 changes: 3 additions & 0 deletions cxx/cxx_toolchain_types.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,7 @@ CxxToolchainInfo = provider(
"use_distributed_thinlto": provider_field(typing.Any, default = None),
"header_mode": provider_field(typing.Any, default = None),
"headers_as_raw_headers_mode": provider_field(typing.Any, default = None),
"raw_headers_as_headers_mode": provider_field(typing.Any, default = None),
"linker_info": provider_field(typing.Any, default = None),
"object_format": provider_field(typing.Any, default = None),
"binary_utilities_info": provider_field(typing.Any, default = None),
Expand Down Expand Up @@ -251,6 +252,7 @@ def cxx_toolchain_infos(
header_mode,
internal_tools: CxxInternalTools,
headers_as_raw_headers_mode = None,
raw_headers_as_headers_mode = None,
conflicting_header_basename_allowlist = [],
asm_compiler_info = None,
as_compiler_info = None,
Expand Down Expand Up @@ -293,6 +295,7 @@ def cxx_toolchain_infos(
conflicting_header_basename_allowlist = conflicting_header_basename_allowlist,
header_mode = header_mode,
headers_as_raw_headers_mode = headers_as_raw_headers_mode,
raw_headers_as_headers_mode = raw_headers_as_headers_mode,
linker_info = linker_info,
llvm_link = llvm_link,
binary_utilities_info = binary_utilities_info,
Expand Down
42 changes: 42 additions & 0 deletions cxx/headers.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,13 @@ load(":platform.bzl", "cxx_by_platform")
# #include "namespace/wfh/baz.h"
CxxHeadersNaming = enum("apple", "regular")

# Modes supporting implementing the `raw_headers` parameter of C++ rules using
# symlink trees and/or header maps through `headers`.
RawHeadersAsHeadersMode = enum(
"enabled",
"disabled",
)

# Modes supporting implementing the `headers` parameter of C++ rules using raw
# headers instead of e.g. symlink trees.
HeadersAsRawHeadersMode = enum(
Expand Down Expand Up @@ -130,6 +137,41 @@ def cxx_get_regular_cxx_headers_layout(ctx: AnalysisContext) -> CxxHeadersLayout
def cxx_attr_exported_header_style(ctx: AnalysisContext) -> HeaderStyle:
return HeaderStyle(ctx.attrs.exported_header_style)

def _concat_inc_dir_with_raw_header(namespace, inc_dir, header) -> list[str] | None:
namespace_parts = namespace.split("/")
inc_dir_parts = inc_dir.split("/")
header_parts = header.short_path.split("/")

for part in inc_dir_parts:
if part == ".":
continue
if part == "..":
if not namespace_parts:
# Too many .., would set include root out of cell
return None
header_parts = [namespace_parts.pop()] + header_parts
elif part == header_parts[0]:
header_parts = header_parts[1:]
else:
# Header not accessible under this folder
return None
return header_parts

def as_headers(
ctx: AnalysisContext,
raw_headers: list[Artifact],
include_directories: list[str]) -> list[CHeader]:
headers = []
base_namespace = ctx.label.package
for header in raw_headers:
for inc_dir in include_directories:
inc_dir = paths.normalize(inc_dir)
mapped_header = _concat_inc_dir_with_raw_header(base_namespace, inc_dir, header)
if mapped_header:
headers.append(CHeader(artifact = header, name = "/".join(mapped_header), namespace = "", named = True))

return headers

def _get_attr_headers(xs: typing.Any, namespace: str, naming: CxxHeadersNaming) -> list[CHeader]:
if type(xs) == type([]):
return [CHeader(artifact = x, name = _get_list_header_name(x, naming), namespace = namespace, named = False) for x in xs]
Expand Down
52 changes: 49 additions & 3 deletions cxx/preprocessor.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ load(
"CxxHeadersNaming",
"HeaderStyle",
"HeadersAsRawHeadersMode",
"RawHeadersAsHeadersMode",
"as_headers",
"as_raw_headers",
"cxx_attr_exported_header_style",
"cxx_attr_exported_headers",
Expand Down Expand Up @@ -232,8 +234,22 @@ def cxx_exported_preprocessor_info(ctx: AnalysisContext, headers_layout: CxxHead

# Add in raw headers and include dirs from attrs.
raw_headers.extend(value_or(ctx.attrs.raw_headers, []))
include_dirs.extend([ctx.label.path.add(x) for x in ctx.attrs.public_include_directories])
system_include_dirs.extend([ctx.label.path.add(x) for x in ctx.attrs.public_system_include_directories])

# if raw-headers-as-headers is enabled, convert raw headers to headers
if raw_headers and _attr_raw_headers_as_headers_mode(ctx) != RawHeadersAsHeadersMode("disabled"):
exported_headers = as_headers(ctx, raw_headers, ctx.attrs.public_include_directories + ctx.attrs.public_system_include_directories)
exported_header_map = {
paths.join(h.namespace, h.name): h.artifact
for h in exported_headers
}
raw_headers.clear()

# Force system exported header style if any system include directories are set.
if ctx.attrs.public_system_include_directories:
style = HeaderStyle("system")
else:
include_dirs.extend([ctx.label.path.add(x) for x in ctx.attrs.public_include_directories])
system_include_dirs.extend([ctx.label.path.add(x) for x in ctx.attrs.public_system_include_directories])

args = _get_exported_preprocessor_args(ctx, exported_header_map, style, compiler_type, raw_headers, extra_preprocessors)

Expand Down Expand Up @@ -349,7 +365,19 @@ def _cxx_private_preprocessor_info(

# Add in raw headers and include dirs from attrs.
all_raw_headers.extend(raw_headers)
include_dirs.extend([ctx.label.path.add(x) for x in ctx.attrs.include_directories])

# if raw-headers-as-headers is enabled, convert raw headers to headers
if all_raw_headers and _attr_raw_headers_as_headers_mode(ctx) != RawHeadersAsHeadersMode("disabled"):
# private headers are also accessible via public_include_directories
# same as exported_preprocessor_flags apply to the target itself.
headers = as_headers(ctx, all_raw_headers, ctx.attrs.include_directories + getattr(ctx.attrs, "public_include_directories", []) + getattr(ctx.attrs, "public_system_include_directories", []))
header_map = {
paths.join(h.namespace, h.name): h.artifact
for h in headers
}
all_raw_headers.clear()
else:
include_dirs.extend([ctx.label.path.add(x) for x in ctx.attrs.include_directories])

args = _get_private_preprocessor_args(ctx, header_map, compiler_type, all_raw_headers)

Expand Down Expand Up @@ -390,6 +418,24 @@ def _header_style_args(style: HeaderStyle, path: cmd_args, compiler_type: str) -
return format_system_include_arg(path, compiler_type)
fail("unsupported header style: {}".format(style))

def _attr_raw_headers_as_headers_mode(ctx: AnalysisContext) -> RawHeadersAsHeadersMode:
"""
Return the `RawHeadersAsHeadersMode` setting to use for this rule.
"""

mode = get_cxx_toolchain_info(ctx).raw_headers_as_headers_mode

# If the platform hasn't set a raw headers translation mode, we don't do anything.
if mode == None:
return RawHeadersAsHeadersMode("disabled")

# Otherwise use the rule-specific setting, if provided (not available on prebuilt_cxx_library).
if getattr(ctx.attrs, "raw_headers_as_headers_mode", None) != None:
return RawHeadersAsHeadersMode(ctx.attrs.raw_headers_as_headers_mode)

# Fallback to platform default.
return mode

def _attr_headers_as_raw_headers_mode(ctx: AnalysisContext) -> HeadersAsRawHeadersMode:
"""
Return the `HeadersAsRawHeadersMode` setting to use for this rule.
Expand Down
9 changes: 5 additions & 4 deletions decls/apple_rules.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -456,6 +456,11 @@ apple_library = prelude_rule(
native_common.link_whole(link_whole_type = attrs.option(attrs.bool(), default = None)) |
cxx_common.reexport_all_header_dependencies_arg() |
cxx_common.exported_deps_arg() |
cxx_common.raw_headers_arg() |
cxx_common.include_directories_arg() |
cxx_common.public_include_directories_arg() |
cxx_common.public_system_include_directories_arg() |
cxx_common.raw_headers_as_headers_mode_arg() |
apple_common.extra_xcode_sources() |
apple_common.extra_xcode_files() |
apple_common.serialize_debugging_options_arg() |
Expand Down Expand Up @@ -488,7 +493,6 @@ apple_library = prelude_rule(
"force_static": attrs.option(attrs.bool(), default = None),
"headers_as_raw_headers_mode": attrs.option(attrs.enum(HeadersAsRawHeadersMode), default = None),
"import_obj_c_forward_declarations": attrs.bool(default = True),
"include_directories": attrs.set(attrs.string(), sorted = True, default = []),
"info_plist": attrs.option(attrs.source(), default = None),
"info_plist_substitutions": attrs.dict(key = attrs.string(), value = attrs.string(), sorted = False, default = {}),
"labels": attrs.list(attrs.string(), default = []),
Expand All @@ -512,9 +516,6 @@ apple_library = prelude_rule(
"precompiled_header": attrs.option(attrs.source(), default = None),
"prefix_header": attrs.option(attrs.source(), default = None),
"public_framework_headers": attrs.named_set(attrs.source(), sorted = True, default = []),
"public_include_directories": attrs.set(attrs.string(), sorted = True, default = []),
"public_system_include_directories": attrs.set(attrs.string(), sorted = True, default = []),
"raw_headers": attrs.set(attrs.source(), sorted = True, default = []),
"sdk_modules": attrs.list(attrs.string(), default = []),
"soname": attrs.option(attrs.string(), default = None),
"static_library_basename": attrs.option(attrs.string(), default = None),
Expand Down
2 changes: 2 additions & 0 deletions decls/common.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ LogLevel = ["off", "severe", "warning", "info", "config", "fine", "finer", "fine

OnDuplicateEntry = ["fail", "overwrite", "append"]

RawHeadersAsHeadersMode = ["enabled", "disabled"]

SourceAbiVerificationMode = ["off", "log", "fail"]

TestType = ["junit", "junit5", "testng"]
Expand Down
13 changes: 12 additions & 1 deletion decls/cxx_common.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
# the generated docs, and so those should be verified to be accurate and
# well-formatted (and then delete this TODO)

load(":common.bzl", "CxxSourceType", "IncludeType")
load(":common.bzl", "CxxSourceType", "IncludeType", "RawHeadersAsHeadersMode")

def _srcs_arg():
return {
Expand Down Expand Up @@ -398,6 +398,16 @@ def _raw_headers_arg():
"""),
}

def _raw_headers_as_headers_mode_arg():
return {
"raw_headers_as_headers_mode": attrs.option(attrs.enum(RawHeadersAsHeadersMode), default = None, doc = """
Controls whether raw_headers and *include_directories attributes should be automatically
converted to headers and symlink trees and/or header maps via headers. Only has an effect if
the cxx_toolchain has explicitly opted into supporting this behavior via a non-default value,
even if the value is disabled.
"""),
}

def _include_directories_arg():
return {
"include_directories": attrs.set(attrs.string(), sorted = True, default = [], doc = """
Expand Down Expand Up @@ -462,6 +472,7 @@ cxx_common = struct(
exported_platform_deps_arg = _exported_platform_deps_arg,
supports_merged_linking = _supports_merged_linking,
raw_headers_arg = _raw_headers_arg,
raw_headers_as_headers_mode_arg = _raw_headers_as_headers_mode_arg,
include_directories_arg = _include_directories_arg,
public_include_directories_arg = _public_include_directories_arg,
public_system_include_directories_arg = _public_system_include_directories_arg,
Expand Down
4 changes: 4 additions & 0 deletions decls/cxx_rules.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ cxx_binary = prelude_rule(
buck.deps_query_arg() |
cxx_common.raw_headers_arg() |
cxx_common.include_directories_arg() |
cxx_common.raw_headers_as_headers_mode_arg() |
{
"contacts": attrs.list(attrs.string(), default = []),
"cxx_runtime_type": attrs.option(attrs.enum(CxxRuntimeType), default = None),
Expand Down Expand Up @@ -526,6 +527,7 @@ cxx_library = prelude_rule(
native_common.link_whole(link_whole_type = attrs.option(attrs.bool(), default = None)) |
native_common.soname() |
cxx_common.raw_headers_arg() |
cxx_common.raw_headers_as_headers_mode_arg() |
cxx_common.include_directories_arg() |
cxx_common.public_include_directories_arg() |
cxx_common.public_system_include_directories_arg() |
Expand Down Expand Up @@ -858,6 +860,7 @@ cxx_test = prelude_rule(
"""),
} |
cxx_common.raw_headers_arg() |
cxx_common.raw_headers_as_headers_mode_arg() |
cxx_common.include_directories_arg() |
{
"framework": attrs.option(attrs.enum(CxxTestType), default = None, doc = """
Expand Down Expand Up @@ -949,6 +952,7 @@ cxx_toolchain = prelude_rule(
examples = None,
further = None,
attrs = (
cxx_common.raw_headers_as_headers_mode_arg() |
{
"archive_contents": attrs.enum(ArchiveContents, default = "normal"),
"archiver": attrs.source(),
Expand Down

0 comments on commit 9f90f62

Please sign in to comment.