From c23230140d8a29d4db9a7da62ee2de7ae524a8ff Mon Sep 17 00:00:00 2001 From: Chandler Carruth Date: Thu, 15 Jan 2026 17:13:38 -0800 Subject: [PATCH] Fix OS config features and use them to move OS-specific flags (#6608) This PR merges the OS-specific Clang flags into the main Clang flags features using feature-based constraints instead of separate features conditionally added. Similarly for libc++. This also move flags to more correctly live in the Clang flag set vs. the libc++ flag set as some of these flags were specific to using libc++. To make this change, the libc++ feature needs to be computed rather than being fixed, as we need to add search paths based on the installed location of LLVM and Clang. All of this only works when the OS-config flags work. The earlier PR adding these had a bug -- _none_ of the OS features would ever be enabled. This didn't result in a problem as the initial use was only to _disable_ flags on the wrong OS. Now that we're enabling flags, we have to get it right by marking all of these as `enabled`. --- .../cc_toolchain_config_features.bzl | 8 +- .../cc_toolchain_cpp_features.bzl | 148 +++++++++++++----- .../clang_cc_toolchain_config.bzl | 101 ++---------- 3 files changed, 128 insertions(+), 129 deletions(-) diff --git a/bazel/cc_toolchains/cc_toolchain_config_features.bzl b/bazel/cc_toolchains/cc_toolchain_config_features.bzl index 1341b31d58df..990190d66a8d 100644 --- a/bazel/cc_toolchains/cc_toolchain_config_features.bzl +++ b/bazel/cc_toolchains/cc_toolchain_config_features.bzl @@ -15,10 +15,10 @@ load( "feature", ) -freebsd_target_feature = feature(name = "freebsd_target") -linux_target_feature = feature(name = "linux_target") -macos_target_feature = feature(name = "macos_target") -windows_target_feature = feature(name = "windows_target") +freebsd_target_feature = feature(name = "freebsd_target", enabled = True) +linux_target_feature = feature(name = "linux_target", enabled = True) +macos_target_feature = feature(name = "macos_target", enabled = True) +windows_target_feature = feature(name = "windows_target", enabled = True) os_target_features = { "freebsd": [freebsd_target_feature], diff --git a/bazel/cc_toolchains/cc_toolchain_cpp_features.bzl b/bazel/cc_toolchains/cc_toolchain_cpp_features.bzl index 6f5e3ef84fe2..218090437026 100644 --- a/bazel/cc_toolchains/cc_toolchain_cpp_features.bzl +++ b/bazel/cc_toolchains/cc_toolchain_cpp_features.bzl @@ -150,6 +150,48 @@ clang_feature = feature( ), ], ), + flag_set( + actions = all_link_actions, + flag_groups = [ + flag_group( + flags = [ + "-fuse-ld=lld", + # Force the C++ standard library and runtime libraries + # to be statically linked. This works even with libc++ + # and libunwind despite the names, provided libc++ is + # built with the CMake option: + # - `-DCMAKE_POSITION_INDEPENDENT_CODE=ON` + "-static-libstdc++", + "-static-libgcc", + # Link with Clang's runtime library. This is always + # linked statically. + "-rtlib=compiler-rt", + # Link with pthread. + "-lpthread", + ], + ), + ], + with_features = [with_feature_set(["linux_target"])], + ), + flag_set( + actions = [ACTION_NAMES.cpp_link_executable], + flag_groups = [flag_group( + expand_if_available = "force_pic", + flags = ["-pie"], + )], + with_features = [with_feature_set([ + "linux_target", + "freebsd_target", + ])], + ), + flag_set( + actions = [ACTION_NAMES.cpp_link_executable], + flag_groups = [flag_group( + expand_if_available = "force_pic", + flags = ["-fpie"], + )], + with_features = [with_feature_set(["macos_target"])], + ), ], ) @@ -192,39 +234,73 @@ _libcpp_release_flags = [ "-D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_FAST", ] -libcxx_feature = feature( - name = "libcxx", - enabled = True, - flag_sets = [ - flag_set( - actions = all_cpp_compile_actions + all_link_actions, - flag_groups = [flag_group(flags = [ - "-stdlib=libc++", - ])], - with_features = [ - # libc++ is only used on non-Windows platforms. - with_feature_set(not_features = ["windows_target"]), - ], - ), - flag_set( - actions = all_cpp_compile_actions, - flag_groups = [flag_group(flags = _libcpp_debug_flags)], - with_features = [with_feature_set(not_features = ["opt"])], - ), - flag_set( - actions = all_cpp_compile_actions, - flag_groups = [flag_group(flags = _libcpp_release_flags)], - with_features = [with_feature_set(features = ["opt"])], - ), - flag_set( - actions = all_link_actions, - flag_groups = [flag_group(flags = [ - "-unwindlib=libunwind", - ])], - with_features = [ - # libc++ is only used on non-Windows platforms. - with_feature_set(not_features = ["windows_target"]), - ], - ), - ], -) +def libcxx_feature(llvm_bindir = None, clang_bindir = None): + """Builds a libc++ feature. + + Returns: + The feature for use with `cc_toolchain_config`. + + Args: + llvm_bindir: Optional LLVM installation `bin` directory, causes the + feature to look for adjacent installed libraries. + clang_bindir: Optional Clang installation `bin` directory, causes the + feature to look for adjacent installed libraries if different from + `llvm_bindir`. + """ + + # Explicitly add LLVM libs to the search path to preempt the + # detected GCC installation's library paths. Those might have a + # system installed libc++ and we want to find the one next to + # our Clang. + extra_link_flags = [] + if llvm_bindir: + extra_link_flags.append("-L" + llvm_bindir + "/../lib") + if clang_bindir and clang_bindir != llvm_bindir: + extra_link_flags.append("-L" + clang_bindir + "/../lib") + + return feature( + name = "libcxx", + enabled = True, + flag_sets = [ + flag_set( + actions = all_cpp_compile_actions + all_link_actions, + flag_groups = [flag_group(flags = [ + "-stdlib=libc++", + ])], + with_features = [ + # libc++ is only used on non-Windows platforms. + with_feature_set(not_features = ["windows_target"]), + ], + ), + flag_set( + actions = all_cpp_compile_actions, + flag_groups = [flag_group(flags = _libcpp_debug_flags)], + with_features = [with_feature_set(not_features = ["opt"])], + ), + flag_set( + actions = all_cpp_compile_actions, + flag_groups = [flag_group(flags = _libcpp_release_flags)], + with_features = [with_feature_set(features = ["opt"])], + ), + flag_set( + actions = all_link_actions, + flag_groups = [flag_group(flags = [ + "-unwindlib=libunwind", + ])], + with_features = [ + # libc++ is only used on non-Windows platforms. + with_feature_set(not_features = ["windows_target"]), + ], + ), + flag_set( + actions = all_link_actions, + flag_groups = [flag_group(flags = extra_link_flags + [ + # Force linking the static libc++abi archive here. This + # *should* be linked automatically, but not every release of + # LLVM correctly sets the CMake flags to do so. + "-l:libc++abi.a", + ])], + with_features = [with_feature_set(["linux_target"])], + ), + ], + ) diff --git a/bazel/cc_toolchains/clang_cc_toolchain_config.bzl b/bazel/cc_toolchains/clang_cc_toolchain_config.bzl index 084deb5e7749..dd4dc051a1e7 100644 --- a/bazel/cc_toolchains/clang_cc_toolchain_config.bzl +++ b/bazel/cc_toolchains/clang_cc_toolchain_config.bzl @@ -11,13 +11,13 @@ load( "feature_set", "flag_group", "flag_set", + "with_feature_set", ) load("@rules_cc//cc:defs.bzl", "cc_toolchain") load("@rules_cc//cc/common:cc_common.bzl", "cc_common") load( ":cc_toolchain_actions.bzl", "all_compile_actions", - "all_link_actions", "preprocessor_compile_actions", ) load( @@ -101,6 +101,16 @@ def _build_features(ctx): "-DCLANG_VERSION_FOR_CACHE=\"%s\"" % clang_version_for_cache, ])], ), + flag_set( + actions = [ + ACTION_NAMES.c_compile, + ACTION_NAMES.cpp_compile, + ACTION_NAMES.cpp_header_parsing, + ACTION_NAMES.cpp_module_compile, + ], + flag_groups = [flag_group(flags = ["-DHAVE_MALLCTL"])], + with_features = [with_feature_set(["freebsd_target"])], + ), ], ) @@ -119,90 +129,6 @@ def _build_features(ctx): ], ) - # TODO: Refactor this into a reusable form in its own file. - linux_flags_feature = feature( - name = "linux_flags", - enabled = True, - flag_sets = [ - flag_set( - actions = all_link_actions, - flag_groups = [flag_group( - flags = [ - "-fuse-ld=lld", - # Force the C++ standard library and runtime libraries - # to be statically linked. This works even with libc++ - # and libunwind despite the names, provided libc++ is - # built with the CMake option: - # - `-DCMAKE_POSITION_INDEPENDENT_CODE=ON` - "-static-libstdc++", - "-static-libgcc", - # Link with Clang's runtime library. This is always - # linked statically. - "-rtlib=compiler-rt", - # Explicitly add LLVM libs to the search path to preempt - # the detected GCC installation's library paths. Those - # might have a system installed libc++ and we want to - # find the one next to our Clang. - "-L" + llvm_bindir + "/../lib", - # Link with pthread. - "-lpthread", - # Force linking the static libc++abi archive here. This - # *should* be linked automatically, but not every - # release of LLVM correctly sets the CMake flags to do - # so. - "-l:libc++abi.a", - ], - )], - ), - flag_set( - actions = [ACTION_NAMES.cpp_link_executable], - flag_groups = [flag_group( - expand_if_available = "force_pic", - flags = ["-pie"], - )], - ), - ], - ) - - # TODO: Refactor this into a reusable form in its own file. - macos_flags_feature = feature( - name = "macos_flags", - enabled = True, - flag_sets = [ - flag_set( - actions = [ACTION_NAMES.cpp_link_executable], - flag_groups = [flag_group( - expand_if_available = "force_pic", - flags = ["-fpie"], - )], - ), - ], - ) - - # TODO: Refactor this into a reusable form in its own file. - freebsd_flags_feature = feature( - name = "freebsd_flags", - enabled = True, - flag_sets = [ - flag_set( - actions = [ - ACTION_NAMES.c_compile, - ACTION_NAMES.cpp_compile, - ACTION_NAMES.cpp_header_parsing, - ACTION_NAMES.cpp_module_compile, - ], - flag_groups = [flag_group(flags = ["-DHAVE_MALLCTL"])], - ), - flag_set( - actions = [ACTION_NAMES.cpp_link_executable], - flag_groups = [flag_group( - expand_if_available = "force_pic", - flags = ["-pie"], - )], - ), - ], - ) - # The order of the features determines the relative order of flags used. features = [] features += base_features @@ -212,7 +138,7 @@ def _build_features(ctx): clang_feature, clang_warnings_feature, # Enable libc++ where supported. - libcxx_feature, + libcxx_feature(llvm_bindir, clang_bindir), # Include any project-specific flags, importantly after the Clang and # warnings features so we can override as necessary here. @@ -236,7 +162,6 @@ def _build_features(ctx): # features are order sensitive. if ctx.attr.target_os == "linux": features.append(sanitizer_static_lib_flags) - features.append(linux_flags_feature) sysroot = None elif ctx.attr.target_os == "windows": # TODO: Need to figure out if we need to add windows specific features @@ -245,11 +170,9 @@ def _build_features(ctx): sysroot = None elif ctx.attr.target_os == "macos": features.append(macos_asan_workarounds) - features.append(macos_flags_feature) sysroot = sysroot_dir elif ctx.attr.target_os == "freebsd": features.append(sanitizer_static_lib_flags) - features.append(freebsd_flags_feature) sysroot = sysroot_dir else: fail("Unsupported target OS!")