Skip to content

[libc][bazel] Create libc_release_library for release configurations. #130694

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 11, 2025

Conversation

vonosmas
Copy link
Contributor

See PR #130327 for background and motivation. This change expands the libc_support_library and libc_function rules to create filegroups that allow building a collection of llvm-libc functions together, from sources, as a part of a single cc_library that can then be used by the downstream clients.

This change also adds an example use of this macro under libc/test/BUILD.bazel to confirm that this macro works as expected.

See PR llvm#130327 for background and motivation. This change expands
the libc_support_library and libc_function rules to create filegroups
that allow building a collection of llvm-libc functions together,
from sources, as a part of a single cc_library that can then be used by
the downstream clients.

This change also adds an example use of this macro under
libc/test/BUILD.bazel to confirm that this macro works as expected.
@llvmbot llvmbot added libc bazel "Peripheral" support tier build system: utils/bazel labels Mar 11, 2025
@llvmbot
Copy link
Member

llvmbot commented Mar 11, 2025

@llvm/pr-subscribers-libc

Author: Alexey Samsonov (vonosmas)

Changes

See PR #130327 for background and motivation. This change expands the libc_support_library and libc_function rules to create filegroups that allow building a collection of llvm-libc functions together, from sources, as a part of a single cc_library that can then be used by the downstream clients.

This change also adds an example use of this macro under libc/test/BUILD.bazel to confirm that this macro works as expected.


Full diff: https://github.com/llvm/llvm-project/pull/130694.diff

2 Files Affected:

  • (modified) utils/bazel/llvm-project-overlay/libc/libc_build_rules.bzl (+83-21)
  • (modified) utils/bazel/llvm-project-overlay/libc/test/BUILD.bazel (+20)
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 09daa9ecb3675..0d60992bbd638 100644
--- a/utils/bazel/llvm-project-overlay/libc/libc_build_rules.bzl
+++ b/utils/bazel/llvm-project-overlay/libc/libc_build_rules.bzl
@@ -64,50 +64,72 @@ def _libc_library(name, copts = [], deps = [], local_defines = [], **kwargs):
         **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
+        weak = False):  # @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,
-        srcs,
         weak = False,
-        copts = [],
-        local_defines = [],
         **kwargs):
     """Add target for a libc function.
 
-    The libc function is eventually available as a cc_library target by name
-    "name". LLVM libc implementations of libc functions are in C++. So, this
-    rule internally generates a C wrapper for the C++ implementation and adds
-    it to the source list of the cc_library. This way, the C++ implementation
-    and the C wrapper are both available in the cc_library.
+    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.
 
     Args:
       name: Target name. It is normally the name of the function this target is
             for.
-      srcs: The .cpp files which contain the function implementation.
       weak: Make the symbol corresponding to the libc function "weak".
-      copts: The list of options to add to the C++ compilation command.
-      local_defines: The preprocessor defines which will be prepended with -D
-                     and passed to the compile command of this target but not
-                     its deps.
       **kwargs: Other attributes relevant for a cc_library. For example, deps.
     """
 
-    # We compile the code twice, the first target is suffixed with ".__internal__" and contains the
+    # 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
     # presence of another libc.
-    libc_support_library(
+    _libc_library(
         name = libc_internal_target(name),
-        srcs = srcs,
-        copts = copts,
-        local_defines = local_defines,
         **kwargs
     )
 
+    _libc_library_filegroups(name = name, is_function = True, **kwargs)
+
+
+    # TODO(PR #130327): Remove this after downstream uses are migrated to libc_release_library.
     # This second target is the llvm libc C function with default visibility.
     func_attrs = [
         "LLVM_LIBC_FUNCTION_ATTR_" + name + "='LLVM_LIBC_EMPTY, [[gnu::weak]]'",
@@ -115,9 +137,49 @@ def libc_function(
 
     _libc_library(
         name = name,
-        srcs = srcs,
-        copts = copts + libc_release_copts(),
-        local_defines = local_defines + func_attrs,
+        copts = libc_release_copts(),
+        local_defines = func_attrs,
+        **kwargs
+    )
+
+def libc_release_library(
+        name,
+        libc_functions,
+        weak_symbols = [],
+        **kwargs):
+    """Create the release version of a libc library.
+
+    Args:
+        name: Name of the cc_library target.
+        libc_functions: List of functions to include in the library. They should be
+            created by libc_function macro.
+        weak_symbols: List of function names that should be marked as weak symbols.
+        **kwargs: Other arguments relevant to cc_library.
+    """
+    # Combine all sources into a single filegroup to avoid repeated sources error.
+    native.filegroup(
+        name = name + "_srcs",
+        srcs = [function + "_fn_srcs" for function in libc_functions],
+    )
+
+    native.cc_library(
+        name = name + "_textual_hdr_library",
+        textual_hdrs = [function + "_fn_textual_hdrs" for function in libc_functions],
+    )
+
+    weak_attributes = [
+        "LLVM_LIBC_FUNCTION_ATTR_" + name + "='LLVM_LIBC_EMPTY, [[gnu::weak]]'"
+        for name in weak_symbols
+    ]
+
+    native.cc_library(
+        name = name,
+        srcs = [":" + name + "_srcs"],
+        copts = libc_common_copts() + libc_release_copts(),
+        local_defines = weak_attributes + LIBC_CONFIGURE_OPTIONS,
+        deps = [
+            ":" + name + "_textual_hdr_library",
+        ],
         **kwargs
     )
 
diff --git a/utils/bazel/llvm-project-overlay/libc/test/BUILD.bazel b/utils/bazel/llvm-project-overlay/libc/test/BUILD.bazel
index 70b0196469eca..ac9689713fea4 100644
--- a/utils/bazel/llvm-project-overlay/libc/test/BUILD.bazel
+++ b/utils/bazel/llvm-project-overlay/libc/test/BUILD.bazel
@@ -2,6 +2,26 @@
 # See https://llvm.org/LICENSE.txt for license information.
 # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 
+load("@bazel_skylib//rules:build_test.bzl", "build_test")
+load("//libc:libc_build_rules.bzl", "libc_release_library")
+
 package(default_visibility = ["//visibility:public"])
 
 exports_files(["libc_test_rules.bzl"])
+
+# Sanity check that libc_release_library macro works as intended.
+libc_release_library(
+    name = "libc_release_test",
+    libc_functions = [
+        "//libc:acosf",
+        "//libc:read",
+    ],
+    weak_symbols = [
+        "read",
+    ],
+)
+
+build_test(
+    name = "libc_release_test_build",
+    targets = [":libc_release_test"],
+)

Copy link
Contributor

@michaelrj-google michaelrj-google left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a nit

Copy link
Contributor

@jhuber6 jhuber6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this something that could also be provided by a CMake cache file?

@vonosmas
Copy link
Contributor Author

Is this something that could also be provided by a CMake cache file?

Joseph, could you elaborate? Do you suggest taking the list of sources/headers required to build a particular set of targets from the cache file CMake constructs? That probably wouldn't work in the Bazel world, where the CMake is never executed.

@jhuber6
Copy link
Contributor

jhuber6 commented Mar 11, 2025

Is this something that could also be provided by a CMake cache file?

Joseph, could you elaborate? Do you suggest taking the list of sources/headers required to build a particular set of targets from the cache file CMake constructs? That probably wouldn't work in the Bazel world, where the CMake is never executed.

I just mean if we have special release configurations if that's something we should export through CMake as well.

@vonosmas
Copy link
Contributor Author

Is this something that could also be provided by a CMake cache file?

Joseph, could you elaborate? Do you suggest taking the list of sources/headers required to build a particular set of targets from the cache file CMake constructs? That probably wouldn't work in the Bazel world, where the CMake is never executed.

I just mean if we have special release configurations if that's something we should export through CMake as well.

Ah, OK. I would defer to @michaelrj-google and @lntue here, but I believe that CMake is somewhat ahead in terms of "release configuration" support? E.g. it has a concept and implementation of OS/arch-specific entrypoints and sets of headers under /libc/config. Up to this point, Bazel has no way of assembling a list of standalone libc functions into something like "library", this CL is a first small step towards doing that.

@michaelrj-google
Copy link
Contributor

I don't think we currently have a way for CMake to build all of the library targets as one massive library, right now it's all individual targets that get combined into a libc.a. I'm not opposed to adding this functionality to cmake, but it'd need more than just a cmake cache file. Adding something like this would involve adding a bunch of new cmake to collect all the non-skipped targets' sources, and also creating a new target for the monobuild library.

@vonosmas vonosmas merged commit 10a6a34 into llvm:main Mar 11, 2025
9 checks passed
@vonosmas vonosmas deleted the pr130327 branch March 11, 2025 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bazel "Peripheral" support tier build system: utils/bazel libc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants