From 9f90f624ad4415141f57e1039f4ca88c31cc2d71 Mon Sep 17 00:00:00 2001 From: Andrew Krieger Date: Fri, 15 Nov 2024 22:24:06 -0800 Subject: [PATCH] Implement `raw_headers_as_headers_mode` 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 `` 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 --- cxx/cxx_toolchain.bzl | 3 ++- cxx/cxx_toolchain_types.bzl | 3 +++ cxx/headers.bzl | 42 ++++++++++++++++++++++++++++++ cxx/preprocessor.bzl | 52 ++++++++++++++++++++++++++++++++++--- decls/apple_rules.bzl | 9 ++++--- decls/common.bzl | 2 ++ decls/cxx_common.bzl | 13 +++++++++- decls/cxx_rules.bzl | 4 +++ 8 files changed, 119 insertions(+), 9 deletions(-) diff --git a/cxx/cxx_toolchain.bzl b/cxx/cxx_toolchain.bzl index 308e9f753..5ced0c2d4 100644 --- a/cxx/cxx_toolchain.bzl +++ b/cxx/cxx_toolchain.bzl @@ -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") @@ -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), diff --git a/cxx/cxx_toolchain_types.bzl b/cxx/cxx_toolchain_types.bzl index 944b01ea0..52aba8faf 100644 --- a/cxx/cxx_toolchain_types.bzl +++ b/cxx/cxx_toolchain_types.bzl @@ -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), @@ -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, @@ -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, diff --git a/cxx/headers.bzl b/cxx/headers.bzl index c78c16ecb..c141f9fdc 100644 --- a/cxx/headers.bzl +++ b/cxx/headers.bzl @@ -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( @@ -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] diff --git a/cxx/preprocessor.bzl b/cxx/preprocessor.bzl index 570119a6d..7eac346dc 100644 --- a/cxx/preprocessor.bzl +++ b/cxx/preprocessor.bzl @@ -21,6 +21,8 @@ load( "CxxHeadersNaming", "HeaderStyle", "HeadersAsRawHeadersMode", + "RawHeadersAsHeadersMode", + "as_headers", "as_raw_headers", "cxx_attr_exported_header_style", "cxx_attr_exported_headers", @@ -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) @@ -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) @@ -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. diff --git a/decls/apple_rules.bzl b/decls/apple_rules.bzl index 2f1463cbd..9fd27fae3 100644 --- a/decls/apple_rules.bzl +++ b/decls/apple_rules.bzl @@ -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() | @@ -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 = []), @@ -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), diff --git a/decls/common.bzl b/decls/common.bzl index b598801c5..efd87bc95 100644 --- a/decls/common.bzl +++ b/decls/common.bzl @@ -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"] diff --git a/decls/cxx_common.bzl b/decls/cxx_common.bzl index 75fde91f8..eefb2d67d 100644 --- a/decls/cxx_common.bzl +++ b/decls/cxx_common.bzl @@ -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 { @@ -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 = """ @@ -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, diff --git a/decls/cxx_rules.bzl b/decls/cxx_rules.bzl index fc3ba296f..83a693496 100644 --- a/decls/cxx_rules.bzl +++ b/decls/cxx_rules.bzl @@ -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), @@ -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() | @@ -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 = """ @@ -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(),