From ba5c74038ed3ae633a9bf8346c88b01e1931992e Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 28 Jun 2023 09:18:24 -0700 Subject: [PATCH] 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 --- .../builtins_bzl/common/cc/cc_binary.bzl | 40 ++------------ .../common/cc/cc_shared_library.bzl | 53 +++++++++++++++++-- .../test_cc_shared_library/BUILD.builtin_test | 41 +++++++------- .../failing_targets/BUILD.builtin_test | 2 - .../test_cc_shared_library/starlark_tests.bzl | 6 +++ 5 files changed, 83 insertions(+), 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 index b6724ecbf1f725..945a21f53bf548 100644 --- 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 index e82b34b2cb995b..690e6466d6506f 100644 --- 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/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/BUILD.builtin_test b/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/BUILD.builtin_test index 11203e74e90a03..c3399ee3140806 100644 --- a/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/BUILD.builtin_test +++ b/src/main/starlark/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/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/failing_targets/BUILD.builtin_test b/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/failing_targets/BUILD.builtin_test index 1e4a63657e4703..5b0b495c8ed70f 100644 --- a/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/failing_targets/BUILD.builtin_test +++ b/src/main/starlark/tests/builtins_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/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/starlark_tests.bzl b/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/starlark_tests.bzl index 27913fa891e73c..a74d65247cd9fd 100644 --- a/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/starlark_tests.bzl +++ b/src/main/starlark/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",