From 25212567e74feeccfa42b2c80a4e8e22bf57afc0 Mon Sep 17 00:00:00 2001 From: Alexey Samsonov Date: Tue, 8 Apr 2025 16:16:01 -0700 Subject: [PATCH 1/2] [libc][bazel] Use Bazel aspects to implement libc_release_library. Instead of creating hundreds of implicit "filegroup" targets to keep track of sources and textual headers required to build each libc function or helper library, use Bazel aspects (see https://bazel.build/versions/8.0.0/extending/aspects), which enable transparent collection of transitive sources / textual headers while walking the dependency DAG, and minimizes the Starlark overhead. --- .../libc/libc_build_rules.bzl | 165 ++++++++++++------ 1 file changed, 112 insertions(+), 53 deletions(-) diff --git a/utils/bazel/llvm-project-overlay/libc/libc_build_rules.bzl b/utils/bazel/llvm-project-overlay/libc/libc_build_rules.bzl index 4e73170be1e81..5640bf02392fb 100644 --- a/utils/bazel/llvm-project-overlay/libc/libc_build_rules.bzl +++ b/utils/bazel/llvm-project-overlay/libc/libc_build_rules.bzl @@ -10,8 +10,9 @@ load(":libc_configure_options.bzl", "LIBC_CONFIGURE_OPTIONS") load(":libc_namespace.bzl", "LIBC_NAMESPACE") load(":platforms.bzl", "PLATFORM_CPU_X86_64") +# TODO: Remove this helper function once all donwstream users are migrated. def libc_internal_target(name): - return name + ".__internal__" + return name def libc_common_copts(): root_label = Label(":libc") @@ -44,84 +45,134 @@ def libc_release_copts(): }) return copts + platform_copts -def _libc_library(name, copts = [], deps = [], local_defines = [], **kwargs): +def _libc_library(name, **kwargs): """Internal macro to serve as a base for all other libc library rules. Args: name: Target name. - copts: The special compiler options for the target. - deps: The list of target dependencies if any. - local_defines: The list of target local_defines if any. **kwargs: All other attributes relevant for the cc_library rule. """ + for attr in ["copts", "local_defines"]: + if attr in kwargs: + fail("disallowed attribute: '{}' in rule: '{}'".format(attr, name)) native.cc_library( name = name, - copts = copts + libc_common_copts(), - local_defines = local_defines + LIBC_CONFIGURE_OPTIONS, - deps = deps, + copts = libc_common_copts(), + local_defines = LIBC_CONFIGURE_OPTIONS, linkstatic = 1, **kwargs ) -def _libc_library_filegroups( - name, - is_function, - srcs = [], - hdrs = [], - textual_hdrs = [], - deps = [], - # We're not using kwargs, but instead explicitly list all possible - # arguments that can be passed to libc_support_library or - # libc_function macros. This is done to limit the configurability - # and ensure the consistent and tightly controlled set of flags - # (see libc_common_copts and libc_release_copts above) is used to build - # libc code both for tests and for release configuration. - target_compatible_with = None): # @unused - """Internal macro to collect sources and headers required to build a library. - """ - - # filegroups created from "libc_function" macro has an extra "_fn" in their - # name to ensure that no other libc target can depend on libc_function. - prefix = name + ("_fn" if is_function else "") - native.filegroup( - name = prefix + "_srcs", - srcs = srcs + hdrs + [dep + "_srcs" for dep in deps], - ) - native.filegroup( - name = prefix + "_textual_hdrs", - srcs = textual_hdrs + [dep + "_textual_hdrs" for dep in deps], - ) - # A convenience function which should be used to list all libc support libraries. # Any library which does not define a public function should be listed with # libc_support_library. def libc_support_library(name, **kwargs): _libc_library(name = name, **kwargs) - _libc_library_filegroups(name = name, is_function = False, **kwargs) def libc_function(name, **kwargs): """Add target for a libc function. This macro creates an internal cc_library that can be used to test this - function, and creates filegroups required to include this function into - a release build of libc. + function. Args: - name: Target name. It is normally the name of the function this target is - for. + name: Target name. Typically the name of the function this target is for. **kwargs: Other attributes relevant for a cc_library. For example, deps. """ - # Build "internal" library with a function, the target has ".__internal__" suffix and contains - # C++ functions in the "LIBC_NAMESPACE" namespace. This allows us to test the function in the + # Builds "internal" library with a function, exposed as a C++ function in + # the "LIBC_NAMESPACE" namespace. This allows us to test the function in the # presence of another libc. _libc_library( name = libc_internal_target(name), **kwargs ) - _libc_library_filegroups(name = name, is_function = True, **kwargs) +# LibcLibraryInfo is used to collect all sources and textual headers required +# to build a particular libc_function or libc_support_library. +LibcLibraryInfo = provider( + fields = ["srcs", "textual_hdrs"], +) + +def _get_libc_info_aspect_impl(target, ctx): + maybe_srcs = getattr(ctx.rule.attr, "srcs", []) + maybe_hdrs = getattr(ctx.rule.attr, "hdrs", []) + maybe_textual_hdrs = getattr(ctx.rule.attr, "textual_hdrs", []) + maybe_deps = getattr(ctx.rule.attr, "deps", []) + return LibcLibraryInfo( + srcs = depset( + [ + f + for src in maybe_srcs + maybe_hdrs + for f in src.files.to_list() + if f.is_source + ], + transitive = [ + dep[LibcLibraryInfo].srcs + for dep in maybe_deps + if LibcLibraryInfo in dep + ], + ), + textual_hdrs = depset( + [ + f + for hdr in maybe_textual_hdrs + for f in hdr.files.to_list() + if f.is_source + ], + transitive = [ + dep[LibcLibraryInfo].textual_hdrs + for dep in maybe_deps + if LibcLibraryInfo in dep + ], + ), + ) + +_get_libc_info_aspect = aspect( + implementation = _get_libc_info_aspect_impl, + attr_aspects = ["deps"], +) + +def _get_libc_srcs_impl(ctx): + return DefaultInfo( + files = depset(transitive = [ + fn[LibcLibraryInfo].srcs + for fn in ctx.attr.libs + ]), + ) + +# get_libc_srcs returns the list of sources required to build all +# specified libraries. +get_libc_srcs = rule( + implementation = _get_libc_srcs_impl, + attrs = { + "libs": attr.label_list( + mandatory = True, + aspects = [_get_libc_info_aspect], + ), + }, +) + +def _get_libc_textual_hdrs_impl(ctx): + return DefaultInfo( + files = depset(transitive = [ + fn[LibcLibraryInfo].textual_hdrs + for fn in ctx.attr.libs + ]), + ) + +# get_libc_textual_hdrs returns the list of textual headers required to compile +# all specified libraries. +get_libc_textual_hdrs = rule( + implementation = _get_libc_textual_hdrs_impl, + attrs = { + "libs": attr.label_list( + mandatory = True, + aspects = [_get_libc_info_aspect], + ), + }, +) def libc_release_library( name, @@ -138,15 +189,18 @@ def libc_release_library( **kwargs: Other arguments relevant to cc_library. """ - # Combine all sources into a single filegroup to avoid repeated sources error. - native.filegroup( + get_libc_srcs( name = name + "_srcs", - srcs = [function + "_fn_srcs" for function in libc_functions], + libs = libc_functions, ) + get_libc_textual_hdrs( + name = name + "_textual_hdrs", + libs = libc_functions, + ) native.cc_library( name = name + "_textual_hdr_library", - textual_hdrs = [function + "_fn_textual_hdrs" for function in libc_functions], + textual_hdrs = [":" + name + "_textual_hdrs"], ) weak_attributes = [ @@ -175,15 +229,20 @@ def libc_header_library(name, hdrs, deps = [], **kwargs): **kwargs: All other attributes relevant for the cc_library rule. """ - # Combine sources from dependencies to create a single cc_library target. - native.filegroup( + get_libc_srcs( name = name + "_hdr_deps", - srcs = [dep + "_srcs" for dep in deps], + libs = deps, + ) + + get_libc_textual_hdrs( + name = name + "_textual_hdrs", + libs = deps, ) native.cc_library( name = name + "_textual_hdr_library", - textual_hdrs = [dep + "_textual_hdrs" for dep in deps], + textual_hdrs = [":" + name + "_textual_hdrs"], ) + native.cc_library( name = name, # Technically speaking, we should put _hdr_deps in srcs, as they are From dcd7e67b3b96772d0c2cb6c19950322ca80195c2 Mon Sep 17 00:00:00 2001 From: Alexey Samsonov Date: Wed, 9 Apr 2025 14:30:39 -0700 Subject: [PATCH 2/2] Address comments from rupprecht@ - better docs and names. --- .../libc/libc_build_rules.bzl | 51 ++++++++----------- 1 file changed, 22 insertions(+), 29 deletions(-) diff --git a/utils/bazel/llvm-project-overlay/libc/libc_build_rules.bzl b/utils/bazel/llvm-project-overlay/libc/libc_build_rules.bzl index 5640bf02392fb..86dfb53a86014 100644 --- a/utils/bazel/llvm-project-overlay/libc/libc_build_rules.bzl +++ b/utils/bazel/llvm-project-overlay/libc/libc_build_rules.bzl @@ -89,42 +89,37 @@ def libc_function(name, **kwargs): **kwargs ) -# LibcLibraryInfo is used to collect all sources and textual headers required -# to build a particular libc_function or libc_support_library. LibcLibraryInfo = provider( + "All source files and textual headers for building a particular library.", fields = ["srcs", "textual_hdrs"], ) -def _get_libc_info_aspect_impl(target, ctx): +def _get_libc_info_aspect_impl( + target, # @unused + ctx): maybe_srcs = getattr(ctx.rule.attr, "srcs", []) maybe_hdrs = getattr(ctx.rule.attr, "hdrs", []) maybe_textual_hdrs = getattr(ctx.rule.attr, "textual_hdrs", []) maybe_deps = getattr(ctx.rule.attr, "deps", []) return LibcLibraryInfo( srcs = depset( - [ - f - for src in maybe_srcs + maybe_hdrs - for f in src.files.to_list() - if f.is_source - ], transitive = [ dep[LibcLibraryInfo].srcs for dep in maybe_deps if LibcLibraryInfo in dep + ] + [ + src.files + for src in maybe_srcs + maybe_hdrs ], ), textual_hdrs = depset( - [ - f - for hdr in maybe_textual_hdrs - for f in hdr.files.to_list() - if f.is_source - ], transitive = [ dep[LibcLibraryInfo].textual_hdrs for dep in maybe_deps if LibcLibraryInfo in dep + ] + [ + hdr.files + for hdr in maybe_textual_hdrs ], ), ) @@ -134,7 +129,7 @@ _get_libc_info_aspect = aspect( attr_aspects = ["deps"], ) -def _get_libc_srcs_impl(ctx): +def _libc_srcs_filegroup_impl(ctx): return DefaultInfo( files = depset(transitive = [ fn[LibcLibraryInfo].srcs @@ -142,10 +137,9 @@ def _get_libc_srcs_impl(ctx): ]), ) -# get_libc_srcs returns the list of sources required to build all -# specified libraries. -get_libc_srcs = rule( - implementation = _get_libc_srcs_impl, +_libc_srcs_filegroup = rule( + doc = "Returns all sources for building the specified libraries.", + implementation = _libc_srcs_filegroup_impl, attrs = { "libs": attr.label_list( mandatory = True, @@ -154,7 +148,7 @@ get_libc_srcs = rule( }, ) -def _get_libc_textual_hdrs_impl(ctx): +def _libc_textual_hdrs_filegroup_impl(ctx): return DefaultInfo( files = depset(transitive = [ fn[LibcLibraryInfo].textual_hdrs @@ -162,10 +156,9 @@ def _get_libc_textual_hdrs_impl(ctx): ]), ) -# get_libc_textual_hdrs returns the list of textual headers required to compile -# all specified libraries. -get_libc_textual_hdrs = rule( - implementation = _get_libc_textual_hdrs_impl, +_libc_textual_hdrs_filegroup = rule( + doc = "Returns all textual headers for compiling the specified libraries.", + implementation = _libc_textual_hdrs_filegroup_impl, attrs = { "libs": attr.label_list( mandatory = True, @@ -189,12 +182,12 @@ def libc_release_library( **kwargs: Other arguments relevant to cc_library. """ - get_libc_srcs( + _libc_srcs_filegroup( name = name + "_srcs", libs = libc_functions, ) - get_libc_textual_hdrs( + _libc_textual_hdrs_filegroup( name = name + "_textual_hdrs", libs = libc_functions, ) @@ -229,12 +222,12 @@ def libc_header_library(name, hdrs, deps = [], **kwargs): **kwargs: All other attributes relevant for the cc_library rule. """ - get_libc_srcs( + _libc_srcs_filegroup( name = name + "_hdr_deps", libs = deps, ) - get_libc_textual_hdrs( + _libc_textual_hdrs_filegroup( name = name + "_textual_hdrs", libs = deps, )