Stop linking indirect dynamic_deps in binaries

RELNOTES[INC]:cc_binary targets with dynamic_deps attributes no longer link indirect dynamic_deps on Unix. This might be an incompatible change if you are using RUNPATHs (instead of RPATHs) in your cc_shared_libraries. Enable the feature "exclude_bazel_rpaths_in_transitive_libs" or "use_rpath_instead_of_runpath" for those cc_shared_libraries.

PiperOrigin-RevId: 542842891
Change-Id: I261e27d96760f0a68a85c7e28961067085f9c400

Author: oquenchil

src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java

@@ -831,6 +831,13 @@ public CppLinkAction build() throws InterruptedException, RuleErrorException {
         userLinkFlags.addAll(cppConfiguration.getLtoIndexOptions());
       }
 
+      NestedSet<String> runtimeLibrarySearchDirectories =
+          collectedLibrariesToLink.getRuntimeLibrarySearchDirectories();
+      if (linkType.getActionName().equals(CppActionNames.CPP_LINK_DYNAMIC_LIBRARY)
+          && featureConfiguration.isEnabled(
+              CppRuleClasses.EXCLUDE_BAZEL_RPATHS_IN_TRANSITIVE_LIBS_FEATURE_NAME)) {
+        runtimeLibrarySearchDirectories = null;
+      }
       variables =
           LinkBuildVariables.setupVariables(
               ((RuleContext) actionConstructionContext).getStarlarkThread(),
@@ -858,7 +865,7 @@ public CppLinkAction build() throws InterruptedException, RuleErrorException {
               ltoOutputRootPrefix,
               defFile != null ? defFile.getExecPathString() : null,
               fdoContext,
-              collectedLibrariesToLink.getRuntimeLibrarySearchDirectories(),
+              runtimeLibrarySearchDirectories,
               collectedLibrariesToLink.getLibrariesToLink(),
               collectedLibrariesToLink.getLibrarySearchDirectories(),
               /* addIfsoRelatedVariables= */ true);

src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java

@@ -517,6 +517,37 @@ public static ToolchainTypeRequirement ccToolchainTypeRequirement(RuleDefinition
    */
   public static final String LEGACY_IS_CC_TEST_FEATURE_NAME = "legacy_is_cc_test";
 
+  /**
+   * By default Bazel will be embed runtime search directories (RPATHS) in transitive shared
+   * libraries, however, for Linux they are wrong in most cases since the runfiles directory where
+   * the transitive shared library will live will not be known at the time of linking. The runfiles
+   * directory is decided by dependent rules, we don't know where those dependents will live. For
+   * Mac (where it works differently by searching from the library's path instead of the main
+   * binary's) the loader paths (not rpaths) are correct so the paths will work.
+   *
+   * <p>This feature controls whether Bazel will embed those rpaths into the transitive shared
+   * library.
+   */
+  public static final String EXCLUDE_BAZEL_RPATHS_IN_TRANSITIVE_LIBS_FEATURE_NAME =
+      "exclude_bazel_rpaths_in_transitive_libs";
+
+  /**
+   * With this feature enabled cc_binary will link all its dynamic_deps, even the ones it depends on
+   * transitively, linking indirect deps might be necessary because if the RPATHs haven't been set
+   * up properly in those dynamic_deps then the loader won't be able to find those libraries unless
+   * they are also linked. For a production binary this is probably not the desired behavior and you
+   * can switch it off by disabling this feature, for the binary to work you have to make sure that
+   * the RPATHs in all shared libraries are set up properly though. The default toolchains have this
+   * behavior switched off for cc_binaries by default. The behavior for cc_tests with dynamic_deps
+   * on all platforms and Windows cc_binaries is hardcoded to always link every transitive library.
+   *
+   * <p>This feature controls the behavior for shared libraries depended on via dynamic_deps and
+   * doesn't control the behavior of the dynamic dependencies created by cc_libraries and used for
+   * cc_tests or cc_binaries(linkstatic=0).
+   */
+  public static final String LINK_INDIRECT_DYNAMIC_DEPS_IN_BINARY_FEATURE_NAME =
+      "link_indirect_dynamic_deps_in_binary";
+
   /** Ancestor for all rules that do include scanning. */
   public static final class CcIncludeScanningRule implements RuleDefinition {
     private final boolean addGrepIncludes;

src/main/starlark/builtins_bzl/common/cc/cc_binary.bzl

@@ -347,7 +347,7 @@ def _build_map_direct_dynamic_dep_to_transitive_dynamic_deps(ctx):
 
     return direct_dynamic_dep_to_transitive_dynamic_deps, all_dynamic_dep_linker_inputs
 
-def _filter_libraries_that_are_linked_dynamically(ctx, cc_linking_context):
+def _filter_libraries_that_are_linked_dynamically(ctx, feature_configuration, cc_linking_context):
     merged_cc_shared_library_infos = merge_cc_shared_library_infos(ctx)
     link_once_static_libs_map = build_link_once_static_libs_map(merged_cc_shared_library_infos)
     transitive_exports = build_exports_map_from_only_dynamic_deps(merged_cc_shared_library_infos)
@@ -363,8 +363,6 @@ def _filter_libraries_that_are_linked_dynamically(ctx, cc_linking_context):
         if owner in transitive_exports:
             can_be_linked_dynamically[owner] = True
 
-    transitive_dynamic_dep_labels = {}
-
     # Entries in unused_dynamic_linker_inputs will be marked None if they are
     # used
     (
@@ -412,20 +410,27 @@ def _filter_libraries_that_are_linked_dynamically(ctx, cc_linking_context):
     # Unlike Unix on Windows every dynamic dependency must be linked to the
     # main binary, even indirect ones that are dependencies of direct
     # dynamic dependencies of this binary. So even though linking indirect
-    # dynamic dependencies is not needed for Unix, we link them here too
+    # dynamic dependencies is not needed for Unix, we link them here for tests too
     # because we cannot know whether the shared libraries were linked with
     # RUNPATH or RPATH. If they were linked with the former, then the loader
     # won't search in the runfiles directory of this binary for the library, it
     # will only search in the RUNPATH set at the time of linking the shared
     # library and we cannot possibly know at that point the runfiles directory
     # of all of its dependents.
-    #
-    # TODO(bazel-team):Add a feature so that indirect dynamic dependencies are
-    # not linked. This should be opt-in for those people that are creating a
-    # production binary and have manually set the RPATH/RUNPATHS correctly.
+    link_indirect_deps = (
+        ctx.attr._is_test or
+        cc_common.is_enabled(feature_configuration = feature_configuration, feature_name = "targets_windows") or
+        cc_common.is_enabled(
+            feature_configuration = feature_configuration,
+            feature_name = "link_indirect_dynamic_deps_in_binary",
+        )
+    )
+    direct_dynamic_dep_labels = {dep[CcSharedLibraryInfo].linker_input.owner: True for dep in ctx.attr.dynamic_deps}
     topologically_sorted_labels_set = {label: True for label in topologically_sorted_labels}
     for dynamic_linker_input_owner, unused_linker_input in unused_dynamic_linker_inputs.items():
-        if unused_linker_input:
+        should_link_input = (unused_linker_input and
+                             (link_indirect_deps or dynamic_linker_input_owner in direct_dynamic_dep_labels))
+        if should_link_input:
             _add_linker_input_to_dict(
                 dynamic_linker_input_owner,
                 unused_dynamic_linker_inputs[dynamic_linker_input_owner],
@@ -523,7 +528,7 @@ def _create_transitive_linking_actions(
     cc_linking_context = cc_info.linking_context
 
     if len(ctx.attr.dynamic_deps) > 0:
-        cc_linking_context = _filter_libraries_that_are_linked_dynamically(ctx, cc_linking_context)
+        cc_linking_context = _filter_libraries_that_are_linked_dynamically(ctx, feature_configuration, cc_linking_context)
     link_deps_statically = True
     if linking_mode == linker_mode.LINKING_DYNAMIC:
         link_deps_statically = False
@@ -659,6 +664,7 @@ def cc_binary_impl(ctx, additional_linkopts):
     if ctx.attr._is_test and cpp_config.incompatible_enable_cc_test_feature:
         features.append("is_cc_test")
         disabled_features.append("legacy_is_cc_test")
+
     feature_configuration = cc_common.configure_features(
         ctx = ctx,
         cc_toolchain = cc_toolchain,

src/main/starlark/builtins_bzl/common/cc/cc_shared_library.bzl

@@ -417,8 +417,10 @@ def _filter_inputs(
     linker_inputs_seen = {}
     linker_inputs_count = 0
     label_to_linker_inputs = {}
+    experimental_remove_before_7_0_linker_inputs = []
 
     def _add_linker_input_to_dict(owner, linker_input):
+        experimental_remove_before_7_0_linker_inputs.append(linker_input)
         label_to_linker_inputs.setdefault(owner, []).append(linker_input)
 
     # We use this dictionary to give an error if a target containing only
@@ -500,14 +502,17 @@ def _filter_inputs(
             message += dynamic_only_root + "\n"
         fail(message)
 
-    sorted_linker_inputs = _sort_linker_inputs(
-        topologically_sorted_labels,
-        label_to_linker_inputs,
-        linker_inputs_count,
-    )
+    if ctx.attr.experimental_disable_topo_sort_do_not_use_remove_before_7_0:
+        linker_inputs = experimental_remove_before_7_0_linker_inputs
+    else:
+        linker_inputs = _sort_linker_inputs(
+            topologically_sorted_labels,
+            label_to_linker_inputs,
+            linker_inputs_count,
+        )
 
     _throw_linked_but_not_exported_errors(linked_statically_but_not_exported)
-    return (exports, sorted_linker_inputs, curr_link_once_static_libs_set.keys(), precompiled_only_dynamic_libraries)
+    return (exports, linker_inputs, curr_link_once_static_libs_set.keys(), precompiled_only_dynamic_libraries)
 
 def _throw_linked_but_not_exported_errors(error_libs_dict):
     if not error_libs_dict:

src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/BUILD.builtin_test

@@ -25,6 +25,7 @@ licenses(["notice"])
 
 package(
     default_visibility = ["//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library:__subpackages__"],
+    features = ["exclude_bazel_rpaths_in_transitive_libs"],
 )
 
 py_test(
@@ -45,6 +46,7 @@ cc_binary(
     name = "binary",
     srcs = ["main.cc"],
     dynamic_deps = ["foo_so"],
+    features = ["use_rpath_instead_of_runpath"],
     deps = [
         ":foo",
     ],
@@ -131,7 +133,7 @@ cc_shared_library(
         "bar_so",
         "//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library3:diff_pkg_so"
     ],
-    features = ["windows_export_all_symbols"],
+    features = ["windows_export_all_symbols", "use_rpath_instead_of_runpath"],
     exports_filter = [
         ":indirect_dep2",
     ],

src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/cc_shared_library_integration_test.sh

@@ -52,7 +52,7 @@ function test_shared_library_symbols() {
 
 function test_shared_library_user_link_flags() {
   foo_so=$(find . -name libfoo_so.so)
-  objdump -x $foo_so | grep $RPATH | grep "kittens" > /dev/null \
+  objdump -x $foo_so | grep RPATH | grep "kittens" > /dev/null \
       || (echo "Expected to have RUNPATH contain 'kittens' (set by user_link_flags)" \
           && exit 1)
 }
@@ -73,7 +73,16 @@ function test_cc_test() {
   do_test_binary $cc_test
 }
 
+function test_number_of_linked_libs() {
+  binary=$(find . -name binary)
+  expected_num_libs="6"
+  num_libs=$(readelf -d  $binary | grep NEEDED | wc -l)
+  echo "$num_libs" | (grep -q  "$expected_num_libs" \
+    || (echo "Expected no more than "$expected_num_libs" linked libraries but was $num_libs" && exit 1))
+}
+
 test_shared_library_user_link_flags
 test_shared_library_symbols
 test_binary
 test_cc_test
+test_number_of_linked_libs

src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/testenv.sh

@@ -14,4 +14,3 @@
 # limitations under the License.
 
 LDD_BINARY="ldd"
-RPATH="RUNPATH"

src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library3/BUILD.builtin_test

@@ -16,7 +16,7 @@ cc_library(
 
 cc_shared_library(
     name = "diff_pkg_so",
-    features = ["windows_export_all_symbols"],
+    features = ["windows_export_all_symbols", "exclude_bazel_rpaths_in_transitive_libs"],
     deps = [
         ":diff_pkg",
     ],

tools/cpp/unix_cc_toolchain_config.bzl

@@ -609,6 +609,10 @@ def _impl(ctx):
         ],
     )
 
+    link_indirect_dynamic_deps_in_binary_feature = feature(
+        name = "link_indirect_dynamic_deps_in_binary",
+    )
+
     fission_support_feature = feature(
         name = "fission_support",
         flag_sets = [
@@ -1359,6 +1363,23 @@ def _impl(ctx):
 
     # TODO(#8303): Mac crosstool should also declare every feature.
     if is_linux:
+        use_rpath_instead_of_runpath_feature = feature(
+            name = "use_rpath_instead_of_runpath",
+            flag_sets = [
+                flag_set(
+                    actions = all_link_actions + lto_index_actions,
+                    flag_groups = [
+                        flag_group(
+                            flags = ["-Wl,--disable-new-dtags"],
+                        ),
+                    ],
+                ),
+            ],
+        )
+        exclude_bazel_rpaths_in_transitive_libs_feature = feature(
+            name = "exclude_bazel_rpaths_in_transitive_libs",
+        )
+
         # Linux artifact name patterns are the default.
         artifact_name_patterns = []
         features = [
@@ -1383,6 +1404,9 @@ def _impl(ctx):
             linkstamps_feature,
             output_execpath_flags_feature,
             runtime_library_search_directories_feature,
+            exclude_bazel_rpaths_in_transitive_libs_feature,
+            use_rpath_instead_of_runpath_feature,
+            link_indirect_dynamic_deps_in_binary_feature,
             library_search_directories_feature,
             libtool_feature,
             archiver_flags_feature,
@@ -1445,6 +1469,7 @@ def _impl(ctx):
             default_link_flags_feature,
             user_link_flags_feature,
             default_link_libs_feature,
+            link_indirect_dynamic_deps_in_binary_feature,
             fdo_optimize_feature,
             dbg_feature,
             opt_feature,