[libc][bazel] Allow configure options to alter all targets #89251
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The previous state was leading to inconsistencies. Some targets would get the options and some wouldn't. As an example, the `MEMORY_COPTS` definitions would only apply to the `:string_memory_utils` target but not to the `:memcpy` target. This patch makes sure definitions are applied throughout the LLVM libc targets as `local_defines`. This ensures that the preprocessor definitions don't propagate to depending targets outside of LLVM libc, and that all libc targets have consistent preprocessor definitions.
|
@llvm/pr-subscribers-libc Author: Guillaume Chatelet (gchatelet) ChangesThe previous state was leading to inconsistencies. Some targets would get the options and some wouldn't. As an example, the Full diff: https://github.com/llvm/llvm-project/pull/89251.diff 3 Files Affected:
diff --git a/utils/bazel/llvm-project-overlay/libc/BUILD.bazel b/utils/bazel/llvm-project-overlay/libc/BUILD.bazel
index be02c227043218..61e6e6eabc492d 100644
--- a/utils/bazel/llvm-project-overlay/libc/BUILD.bazel
+++ b/utils/bazel/llvm-project-overlay/libc/BUILD.bazel
@@ -23,17 +23,6 @@ package(
licenses(["notice"])
-PRINTF_COPTS = [
- "LIBC_COPT_STDIO_USE_SYSTEM_FILE",
- "LIBC_COPT_PRINTF_DISABLE_WRITE_INT",
-]
-
-MEMORY_COPTS = [
- # "LIBC_COPT_MEMCPY_X86_USE_REPMOVSB_FROM_SIZE=0",
- # "LIBC_COPT_MEMCPY_X86_USE_SOFTWARE_PREFETCHING",
- # "LIBC_COPT_MEMSET_X86_USE_SOFTWARE_PREFETCHING",
-]
-
# A flag to pick which `mpfr` to use for math tests.
# Usage: `--@llvm-project//libc:mpfr=<disable|external|system>`.
# Flag documentation: https://bazel.build/extending/config
@@ -2420,7 +2409,6 @@ libc_support_library(
"src/string/memory_utils/op_x86.h",
"src/string/memory_utils/utils.h",
],
- defines = MEMORY_COPTS,
textual_hdrs = [
"src/string/memory_utils/aarch64/inline_bcmp.h",
"src/string/memory_utils/aarch64/inline_memcmp.h",
@@ -3188,7 +3176,6 @@ libc_function(
libc_support_library(
name = "printf_config",
hdrs = ["src/stdio/printf_core/printf_config.h"],
- defines = PRINTF_COPTS,
deps = [
],
)
@@ -3196,7 +3183,6 @@ libc_support_library(
libc_support_library(
name = "printf_core_structs",
hdrs = ["src/stdio/printf_core/core_structs.h"],
- defines = PRINTF_COPTS,
deps = [
":__support_cpp_string_view",
":__support_fputil_fp_bits",
@@ -3207,7 +3193,6 @@ libc_support_library(
libc_support_library(
name = "printf_parser",
hdrs = ["src/stdio/printf_core/parser.h"],
- defines = PRINTF_COPTS,
deps = [
":__support_arg_list",
":__support_common",
@@ -3228,7 +3213,7 @@ libc_support_library(
libc_support_library(
name = "printf_mock_parser",
hdrs = ["src/stdio/printf_core/parser.h"],
- defines = PRINTF_COPTS + ["LIBC_COPT_MOCK_ARG_LIST"],
+ defines = ["LIBC_COPT_MOCK_ARG_LIST"],
deps = [
":__support_arg_list",
":__support_common",
@@ -3248,7 +3233,6 @@ libc_support_library(
name = "printf_writer",
srcs = ["src/stdio/printf_core/writer.cpp"],
hdrs = ["src/stdio/printf_core/writer.h"],
- defines = PRINTF_COPTS,
deps = [
":__support_cpp_string_view",
":__support_macros_optimization",
@@ -3273,7 +3257,6 @@ libc_support_library(
"src/stdio/printf_core/string_converter.h",
"src/stdio/printf_core/write_int_converter.h",
],
- defines = PRINTF_COPTS,
deps = [
":__support_big_int",
":__support_common",
@@ -3297,7 +3280,6 @@ libc_support_library(
name = "printf_main",
srcs = ["src/stdio/printf_core/printf_main.cpp"],
hdrs = ["src/stdio/printf_core/printf_main.h"],
- defines = PRINTF_COPTS,
deps = [
":__support_arg_list",
":printf_converter",
@@ -3310,7 +3292,6 @@ libc_support_library(
libc_support_library(
name = "vfprintf_internal",
hdrs = ["src/stdio/printf_core/vfprintf_internal.h"],
- defines = PRINTF_COPTS,
deps = [
":__support_arg_list",
":__support_file_file",
@@ -3324,7 +3305,6 @@ libc_function(
name = "sprintf",
srcs = ["src/stdio/sprintf.cpp"],
hdrs = ["src/stdio/sprintf.h"],
- defines = PRINTF_COPTS,
deps = [
":__support_arg_list",
":__support_cpp_limits",
@@ -3338,7 +3318,6 @@ libc_function(
name = "snprintf",
srcs = ["src/stdio/snprintf.cpp"],
hdrs = ["src/stdio/snprintf.h"],
- defines = PRINTF_COPTS,
deps = [
":__support_arg_list",
":errno",
@@ -3351,7 +3330,6 @@ libc_function(
name = "printf",
srcs = ["src/stdio/printf.cpp"],
hdrs = ["src/stdio/printf.h"],
- defines = PRINTF_COPTS,
deps = [
":__support_arg_list",
":__support_file_file",
@@ -3364,7 +3342,6 @@ libc_function(
name = "fprintf",
srcs = ["src/stdio/fprintf.cpp"],
hdrs = ["src/stdio/fprintf.h"],
- defines = PRINTF_COPTS,
deps = [
":__support_arg_list",
":__support_file_file",
@@ -3377,7 +3354,6 @@ libc_function(
name = "vsprintf",
srcs = ["src/stdio/vsprintf.cpp"],
hdrs = ["src/stdio/vsprintf.h"],
- defines = PRINTF_COPTS,
deps = [
":__support_arg_list",
":__support_cpp_limits",
@@ -3391,7 +3367,6 @@ libc_function(
name = "vsnprintf",
srcs = ["src/stdio/vsnprintf.cpp"],
hdrs = ["src/stdio/vsnprintf.h"],
- defines = PRINTF_COPTS,
deps = [
":__support_arg_list",
":errno",
@@ -3404,7 +3379,6 @@ libc_function(
name = "vprintf",
srcs = ["src/stdio/vprintf.cpp"],
hdrs = ["src/stdio/vprintf.h"],
- defines = PRINTF_COPTS,
deps = [
":__support_arg_list",
":__support_file_file",
@@ -3417,7 +3391,6 @@ libc_function(
name = "vfprintf",
srcs = ["src/stdio/vfprintf.cpp"],
hdrs = ["src/stdio/vfprintf.h"],
- defines = PRINTF_COPTS,
deps = [
":__support_arg_list",
":__support_file_file",
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 be59e18ffd89a9..ec3714407cb914 100644
--- a/utils/bazel/llvm-project-overlay/libc/libc_build_rules.bzl
+++ b/utils/bazel/llvm-project-overlay/libc/libc_build_rules.bzl
@@ -6,6 +6,7 @@
load("@bazel_skylib//lib:paths.bzl", "paths")
load("@bazel_skylib//lib:selects.bzl", "selects")
+load(":libc_configure_options.bzl", "LIBC_CONFIGURE_OPTIONS")
load(":libc_namespace.bzl", "LIBC_NAMESPACE")
load(":platforms.bzl", "PLATFORM_CPU_ARM64", "PLATFORM_CPU_X86_64")
@@ -21,13 +22,14 @@ def libc_common_copts():
"-DLIBC_NAMESPACE=" + LIBC_NAMESPACE,
]
-def _libc_library(name, hidden, copts = [], deps = [], **kwargs):
+def _libc_library(name, hidden, copts = [], deps = [], local_defines = [], **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.
hidden: Whether the symbols should be explicitly hidden or not.
**kwargs: All other attributes relevant for the cc_library rule.
"""
@@ -40,6 +42,7 @@ def _libc_library(name, hidden, copts = [], deps = [], **kwargs):
native.cc_library(
name = name,
copts = copts + libc_common_copts(),
+ local_defines = local_defines + LIBC_CONFIGURE_OPTIONS,
deps = deps,
linkstatic = 1,
**kwargs
diff --git a/utils/bazel/llvm-project-overlay/libc/libc_configure_options.bzl b/utils/bazel/llvm-project-overlay/libc/libc_configure_options.bzl
new file mode 100644
index 00000000000000..f780c323d9a996
--- /dev/null
+++ b/utils/bazel/llvm-project-overlay/libc/libc_configure_options.bzl
@@ -0,0 +1,49 @@
+# This file is licensed under the Apache License v2.0 with LLVM Exceptions.
+# See https://llvm.org/LICENSE.txt for license information.
+# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+"""LLVM libc configuration options.
+The canonical list of user options is in 'libc/config/config.json'.
+These options are then processed by CMake and turned into preprocessor
+definitions. We don't have this logic in Bazel yet but the list of definitions
+is discoverable with the following command:
+
+> git grep -hoE '\bLIBC_COPT_\\w*' -- '*.h' '*.cpp' | sort -u
+"""
+
+# This list of definitions is used to customize LLVM libc.
+LIBC_CONFIGURE_OPTIONS = [
+ # Documentation in libc/docs/dev/printf_behavior.rst
+ # "LIBC_COPT_FLOAT_TO_STR_NO_SPECIALIZE_LD",
+ # "LIBC_COPT_FLOAT_TO_STR_NO_TABLE",
+ # "LIBC_COPT_FLOAT_TO_STR_USE_DYADIC_FLOAT",
+ # "LIBC_COPT_FLOAT_TO_STR_USE_DYADIC_FLOAT_LD",
+ # "LIBC_COPT_FLOAT_TO_STR_USE_INT_CALC",
+ # "LIBC_COPT_FLOAT_TO_STR_USE_MEGA_LONG_DOUBLE_TABLE",
+
+ # Documentation in libc/src/string/memory_utils/...
+ # "LIBC_COPT_MEMCPY_USE_EMBEDDED_TINY",
+ # "LIBC_COPT_MEMCPY_X86_USE_REPMOVSB_FROM_SIZE",
+ # "LIBC_COPT_MEMCPY_X86_USE_SOFTWARE_PREFETCHING",
+ # "LIBC_COPT_MEMSET_X86_USE_SOFTWARE_PREFETCHING",
+
+ # Documentation in libc/docs/dev/printf_behavior.rst
+ # "LIBC_COPT_PRINTF_CONV_ATLAS",
+ # "LIBC_COPT_PRINTF_DISABLE_FIXED_POINT",
+ # "LIBC_COPT_PRINTF_DISABLE_FLOAT",
+ # "LIBC_COPT_PRINTF_DISABLE_INDEX_MODE",
+ "LIBC_COPT_PRINTF_DISABLE_WRITE_INT",
+ # "LIBC_COPT_PRINTF_HEX_LONG_DOUBLE",
+ # "LIBC_COPT_PRINTF_INDEX_ARR_LEN",
+ # "LIBC_COPT_PRINTF_NO_NULLPTR_CHECKS",
+ # "LIBC_COPT_SCANF_DISABLE_FLOAT",
+ # "LIBC_COPT_SCANF_DISABLE_INDEX_MODE",
+ "LIBC_COPT_STDIO_USE_SYSTEM_FILE",
+ # "LIBC_COPT_STRING_UNSAFE_WIDE_READ",
+ # "LIBC_COPT_STRTOFLOAT_DISABLE_CLINGER_FAST_PATH",
+ # "LIBC_COPT_STRTOFLOAT_DISABLE_EISEL_LEMIRE",
+ # "LIBC_COPT_STRTOFLOAT_DISABLE_SIMPLE_DECIMAL_CONVERSION",
+
+ # Documentation in libc/src/__support/libc_assert.h
+ # "LIBC_COPT_USE_C_ASSERT",
+]
|
gchatelet
commented
Apr 18, 2024
| @@ -3228,7 +3213,7 @@ libc_support_library( | |||
| libc_support_library( | |||
| name = "printf_mock_parser", | |||
| hdrs = ["src/stdio/printf_core/parser.h"], | |||
| defines = PRINTF_COPTS + ["LIBC_COPT_MOCK_ARG_LIST"], | |||
| defines = ["LIBC_COPT_MOCK_ARG_LIST"], | |||
There was a problem hiding this comment.
Note to libc maintainers: we should probably not use defines at all in our BUILD files as they will propagate up outside of the libc project /me think. Upon changes this potentially triggers a rebuild of all targets depending on libc. @michaelrj-google @nickdesaulniers @lntue
edit: It's not really the rebuild that is the issue (the change in preprocessor definition is likely to change the libc code and hence trigger a rebuild) but the fact that bazel has to propagate them to all targets which might affect performance for large build graphs.
There was a problem hiding this comment.
So after discussing this internally a bit I think we want to remove all defines from LLVM libc.
We should really use local_define to make the configuration internal and not leak preprocessor definitions to the outside world.
The documentation is quite clear as well:
Be very careful, since this may have far-reaching effects. When in doubt, add define values to "local_defines" instead.
legrosbuffle
approved these changes
Apr 23, 2024
|
@michaelrj-google can you have a look as well? |
michaelrj-google
approved these changes
Apr 23, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The previous state was leading to inconsistencies. Some targets would get the options and some wouldn't. As an example, the
MEMORY_COPTSdefinitions would only apply to the:string_memory_utilstarget but not to the:memcpytarget. This patch makes sure definitions are applied throughout the LLVM libc targets aslocal_defines. This ensures that the preprocessor definitions don't propagate to depending targets outside of LLVM libc, and that all libc targets have consistent preprocessor definitions.