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`.

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],

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"])],
+            ),
+        ],
+    )

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!")