Link unused targets listed in cc_shared_library.dynamic_deps

It used to be an invariant in the cc_shared_library design that the rule would only link the dynamic_deps (other cc_shared_libraries) that were exporting cc_library targets coming from the cc_library graph (in other words targets reachable from cc_shared_library.deps, formerly cc_shared_library.roots). However, these were not being linked silently without giving an error. It also turns out that it's a valid use case not to require the library in the cc_library graph when for example owners of the cc_shared_library target want users to only depend on the dynamic library and make their cc_library private so that it's never linked statically. cc_binary already allowed this and linked the unused direct dynamic_deps. RELNOTES:none PiperOrigin-RevId: 544076791 Change-Id: I78668c6cc26676922cd1478e290019ca4fccd675
aiuto · Jun 28, 2023 · ba5c740 · ba5c740
1 parent d14a56f
commit ba5c740
Show file tree

Hide file tree

Showing 5 changed files with 83 additions and 59 deletions.
diff --git a/src/main/starlark/builtins_bzl/common/cc/cc_binary.bzl b/src/main/starlark/builtins_bzl/common/cc/cc_binary.bzl
@@ -15,7 +15,7 @@
 """cc_binary Starlark implementation replacing native"""
 
 load(":common/cc/semantics.bzl", "semantics")
-load(":common/cc/cc_shared_library.bzl", "CcSharedLibraryInfo", "GraphNodeInfo", "build_exports_map_from_only_dynamic_deps", "build_link_once_static_libs_map", "merge_cc_shared_library_infos", "separate_static_and_dynamic_link_libraries", "sort_linker_inputs", "throw_linked_but_not_exported_errors")
+load(":common/cc/cc_shared_library.bzl", "GraphNodeInfo", "add_unused_dynamic_deps", "build_exports_map_from_only_dynamic_deps", "build_link_once_static_libs_map", "merge_cc_shared_library_infos", "separate_static_and_dynamic_link_libraries", "sort_linker_inputs", "throw_linked_but_not_exported_errors")
 load(":common/cc/cc_helper.bzl", "cc_helper", "linker_mode")
 load(":common/cc/cc_info.bzl", "CcInfo")
 load(":common/cc/cc_common.bzl", "cc_common")
@@ -331,22 +331,6 @@ def _collect_transitive_dwo_artifacts(cc_compilation_outputs, cc_debug_context,
             transitive_dwo_files = cc_debug_context.files
     return depset(dwo_files, transitive = [transitive_dwo_files])
 
-def _build_map_direct_dynamic_dep_to_transitive_dynamic_deps(ctx):
-    all_dynamic_dep_linker_inputs = {}
-    direct_dynamic_dep_to_transitive_dynamic_deps = {}
-    for dep in ctx.attr.dynamic_deps:
-        owner = dep[CcSharedLibraryInfo].linker_input.owner
-        all_dynamic_dep_linker_inputs[owner] = dep[CcSharedLibraryInfo].linker_input
-        transitive_dynamic_dep_labels = []
-        for dynamic_dep in dep[CcSharedLibraryInfo].dynamic_deps.to_list():
-            all_dynamic_dep_linker_inputs[dynamic_dep[1].owner] = dynamic_dep[1]
-            transitive_dynamic_dep_labels.append(dynamic_dep[1].owner)
-        transitive_dynamic_dep_labels_set = depset(transitive_dynamic_dep_labels, order = "topological")
-        for export in dep[CcSharedLibraryInfo].exports:
-            direct_dynamic_dep_to_transitive_dynamic_deps[export] = transitive_dynamic_dep_labels_set
-
-    return direct_dynamic_dep_to_transitive_dynamic_deps, all_dynamic_dep_linker_inputs
-
 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)
@@ -365,19 +349,15 @@ def _filter_libraries_that_are_linked_dynamically(ctx, feature_configuration, cc
 
     # Entries in unused_dynamic_linker_inputs will be marked None if they are
     # used
-    (
-        transitive_dynamic_dep_labels,
-        unused_dynamic_linker_inputs,
-    ) = _build_map_direct_dynamic_dep_to_transitive_dynamic_deps(ctx)
-
     (
         targets_to_be_linked_statically_map,
         targets_to_be_linked_dynamically_set,
         topologically_sorted_labels,
+        unused_dynamic_linker_inputs,
     ) = separate_static_and_dynamic_link_libraries(
+        ctx,
         graph_structure_aspect_nodes,
         can_be_linked_dynamically,
-        transitive_dynamic_dep_labels,
     )
 
     topologically_sorted_labels = [ctx.label] + topologically_sorted_labels
@@ -411,19 +391,7 @@ def _filter_libraries_that_are_linked_dynamically(ctx, feature_configuration, cc
     # main binary, even indirect ones that are dependencies of direct
     # dynamic dependencies of this binary.
     link_indirect_deps = cc_common.is_enabled(feature_configuration = feature_configuration, feature_name = "targets_windows")
-    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():
-        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],
-            )
-            linker_inputs_count += 1
-            if dynamic_linker_input_owner not in topologically_sorted_labels_set:
-                topologically_sorted_labels.append(dynamic_linker_input_owner)
+    linker_inputs_count += add_unused_dynamic_deps(ctx, unused_dynamic_linker_inputs, _add_linker_input_to_dict, topologically_sorted_labels, link_indirect_deps)
 
     throw_linked_but_not_exported_errors(linked_statically_but_not_exported)
 

diff --git a/src/main/starlark/builtins_bzl/common/cc/cc_shared_library.bzl b/src/main/starlark/builtins_bzl/common/cc/cc_shared_library.bzl
@@ -83,9 +83,14 @@ def _sort_linker_inputs(topologically_sorted_labels, label_to_linker_inputs, lin
 # dynamically. The transitive_dynamic_dep_labels parameter is only needed for
 # binaries because they link all dynamic_deps (cc_binary|cc_test).
 def _separate_static_and_dynamic_link_libraries(
+        ctx,
         direct_children,
-        can_be_linked_dynamically,
-        transitive_dynamic_dep_labels = {}):
+        can_be_linked_dynamically):
+    (
+        transitive_dynamic_dep_labels,
+        all_dynamic_dep_linker_inputs,
+    ) = _build_map_direct_dynamic_dep_to_transitive_dynamic_deps(ctx)
+
     node = None
     all_children = reversed(direct_children)
     targets_to_be_linked_statically_map = {}
@@ -209,7 +214,7 @@ def _separate_static_and_dynamic_link_libraries(
             transitive.append(first_owner_to_depset[child.owners[0]])
         topologically_sorted_labels = depset(transitive = transitive, order = "topological").to_list()
 
-    return (targets_to_be_linked_statically_map, targets_to_be_linked_dynamically_set, topologically_sorted_labels)
+    return (targets_to_be_linked_statically_map, targets_to_be_linked_dynamically_set, topologically_sorted_labels, all_dynamic_dep_linker_inputs)
 
 def _create_linker_context(ctx, linker_inputs):
     return cc_common.create_linking_context(
@@ -389,11 +394,15 @@ def _filter_inputs(
 
     # The targets_to_be_linked_statically_map points to whether the target to
     # be linked statically can be linked more than once.
+    # Entries in unused_dynamic_linker_inputs will be marked None if they are
+    # used
     (
         targets_to_be_linked_statically_map,
         targets_to_be_linked_dynamically_set,
         topologically_sorted_labels,
+        unused_dynamic_linker_inputs,
     ) = _separate_static_and_dynamic_link_libraries(
+        ctx,
         graph_structure_aspect_nodes,
         can_be_linked_dynamically,
     )
@@ -437,6 +446,8 @@ def _filter_inputs(
         linker_inputs_seen[stringified_linker_input] = True
         owner = str(linker_input.owner)
         if owner in targets_to_be_linked_dynamically_set:
+            unused_dynamic_linker_inputs[transitive_exports[owner].owner] = None
+
             # Link the library in this iteration dynamically,
             # transitive_exports contains the artifacts produced by a
             # cc_shared_library
@@ -502,6 +513,8 @@ def _filter_inputs(
             message += dynamic_only_root + "\n"
         fail(message)
 
+    linker_inputs_count += _add_unused_dynamic_deps(ctx, unused_dynamic_linker_inputs, _add_linker_input_to_dict, topologically_sorted_labels, link_indirect_deps = False)
+
     if ctx.attr.experimental_disable_topo_sort_do_not_use_remove_before_7_0:
         linker_inputs = experimental_remove_before_7_0_linker_inputs
     else:
@@ -567,6 +580,39 @@ def _get_deps(ctx):
 
     return deps
 
+def _build_map_direct_dynamic_dep_to_transitive_dynamic_deps(ctx):
+    all_dynamic_dep_linker_inputs = {}
+    direct_dynamic_dep_to_transitive_dynamic_deps = {}
+    for dep in ctx.attr.dynamic_deps:
+        owner = dep[CcSharedLibraryInfo].linker_input.owner
+        all_dynamic_dep_linker_inputs[owner] = dep[CcSharedLibraryInfo].linker_input
+        transitive_dynamic_dep_labels = []
+        for dynamic_dep in dep[CcSharedLibraryInfo].dynamic_deps.to_list():
+            all_dynamic_dep_linker_inputs[dynamic_dep[1].owner] = dynamic_dep[1]
+            transitive_dynamic_dep_labels.append(dynamic_dep[1].owner)
+        transitive_dynamic_dep_labels_set = depset(transitive_dynamic_dep_labels, order = "topological")
+        for export in dep[CcSharedLibraryInfo].exports:
+            direct_dynamic_dep_to_transitive_dynamic_deps[export] = transitive_dynamic_dep_labels_set
+
+    return direct_dynamic_dep_to_transitive_dynamic_deps, all_dynamic_dep_linker_inputs
+
+def _add_unused_dynamic_deps(ctx, unused_dynamic_linker_inputs, add_linker_inputs_lambda, topologically_sorted_labels, link_indirect_deps):
+    linker_inputs_count = 0
+    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():
+        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_inputs_lambda(
+                dynamic_linker_input_owner,
+                unused_dynamic_linker_inputs[dynamic_linker_input_owner],
+            )
+            linker_inputs_count += 1
+            if dynamic_linker_input_owner not in topologically_sorted_labels_set:
+                topologically_sorted_labels.append(dynamic_linker_input_owner)
+    return linker_inputs_count
+
 def _cc_shared_library_impl(ctx):
     if not cc_common.check_experimental_cc_shared_library():
         if len(ctx.attr.static_deps):
@@ -806,3 +852,4 @@ build_exports_map_from_only_dynamic_deps = _build_exports_map_from_only_dynamic_
 throw_linked_but_not_exported_errors = _throw_linked_but_not_exported_errors
 separate_static_and_dynamic_link_libraries = _separate_static_and_dynamic_link_libraries
 sort_linker_inputs = _sort_linker_inputs
+add_unused_dynamic_deps = _add_unused_dynamic_deps
diff --git a/...tarlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/BUILD.builtin_test b/...tarlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/BUILD.builtin_test
@@ -61,7 +61,7 @@ cc_binary(
         "foo_so",
         "bar_so",
     ],
-    deps = ["foo", "bar"],
+    deps = ["foo"],
 )
 
 cc_shared_library(
@@ -77,32 +77,18 @@ cc_shared_library(
     deps = [":a_suffix"],
 )
 
-cc_library(
-    name = "diamond_lib1",
-    deps = [
-        ":a_suffix",
-    ],
-)
-
-cc_library(
-    name = "diamond_lib2",
-    deps = [
-         ":a_suffix",
-    ],
-)
-
 cc_shared_library(
     name = "diamond_so",
     dynamic_deps = [":a_so"],
     features = ["windows_export_all_symbols"],
-    deps = [":qux", "diamond_lib1"],
+    deps = [":qux"],
 )
 
 cc_shared_library(
     name = "diamond2_so",
     dynamic_deps = [":a_so"],
     features = ["windows_export_all_symbols"],
-    deps = [":bar",  "diamond_lib2"],
+    deps = [":bar"],
 )
 
 cc_binary(
@@ -129,7 +115,8 @@ cc_shared_library(
     }),
     dynamic_deps = [
         "bar_so",
-        "//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library3:diff_pkg_so"
+        "//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library3:diff_pkg_so",
+        "private_lib_so",
     ],
     features = ["windows_export_all_symbols"],
     exports_filter = [
@@ -445,6 +432,24 @@ cc_library(
     ],
 )
 
+cc_shared_library(
+    name = "private_lib_so",
+    deps = [
+        ":private_lib",
+    ],
+)
+
+genrule(
+    name = "private_cc_lib_source",
+    outs = ["private_cc_library.cc"],
+    cmd = "touch $@",
+)
+
+cc_library(
+    name = "private_lib",
+    srcs = [":private_cc_library.cc"]
+)
+
 build_failure_test(
     name = "two_dynamic_deps_same_export_in_so_test",
     message = "Two shared libraries in dependencies export the same symbols",

diff --git a/...iltins_bzl/cc/cc_shared_library/test_cc_shared_library/failing_targets/BUILD.builtin_test b/...iltins_bzl/cc/cc_shared_library/test_cc_shared_library/failing_targets/BUILD.builtin_test
@@ -12,7 +12,6 @@ cc_binary(
     dynamic_deps = ["//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library:bar_so"],
     tags = TAGS,
     deps = [
-        "//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library:bar",
         "//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library:barX",
     ],
 )
@@ -29,7 +28,6 @@ cc_shared_library(
 cc_library(
     name = "intermediate",
     deps = [
-        "//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library:bar",
         "//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library:barX",
     ],
 )

diff --git a/...tarlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/starlark_tests.bzl b/...tarlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/starlark_tests.bzl
@@ -58,6 +58,11 @@ def _linking_order_test_impl(env, target):
             matching.contains("foo.pic.o"),
             matching.contains("baz.pic.o"),
         ]).in_order()
+
+        env.expect.that_collection(args).contains_at_least([
+            "-lprivate_lib_so",
+        ])
+
         env.expect.where(
             detail = "liba_suffix.pic.o should be the last user library linked",
         ).that_str(user_libs[-1]).equals("a_suffix.pic.o")
@@ -181,6 +186,7 @@ def _runfiles_test_impl(env, target):
         "libfoo_so.so",
         "libbar_so.so",
         "libdiff_pkg_so.so",
+        "libprivate_lib_so.so",
         "Smain_Sstarlark_Stests_Sbuiltins_Ubzl_Scc_Scc_Ushared_Ulibrary_Stest_Ucc_Ushared_Ulibrary_Slibfoo_Uso.so",
         "Smain_Sstarlark_Stests_Sbuiltins_Ubzl_Scc_Scc_Ushared_Ulibrary_Stest_Ucc_Ushared_Ulibrary_Slibbar_Uso.so",
         "Smain_Sstarlark_Stests_Sbuiltins_Ubzl_Scc_Scc_Ushared_Ulibrary_Stest_Ucc_Ushared_Ulibrary3_Slibdiff_Upkg_Uso.so",