Fix cc_shared_library to take into account indirect top level deps

The cc_shared_library implementation wraps its top level direct dependencies in
whole archive because otherwise since there aren't any symbols in the shared
library depending on them they would be dropped by the linker.

There are instances where the target placed in the direct dep of a
cc_shared_library doesn't have a linker_input or if it has it, the linker_input
may not have any code to be linked statically into the shared library.

When that happens, the actual code to be linked is depended on indirectly, e.g.
cc_shared_library -> cc_proto_library -> proto_library. Here it is the code
generated by the aspect applied on proto_library that should be linked
statically. Since this dependency was indirect, that code wasn't
whole-archived. Before this fix, we only tried to whole-archive
cc_proto_library but the cc_proto_library target itself did not produce any
linker_inputs, for this reason the code from the proto_library was dropped by
the linker.

This CL also adds more test coverage for the dynamic_only_roots failure
triggered explicitly by the cc_shared_library implementation.

RELNOTES:none
PiperOrigin-RevId: 515017649
Change-Id: I022e1cb4f70fa4584893d3c34172236fdf2e5f73

Author: oquenchil

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

@@ -193,6 +193,47 @@ def _check_if_target_should_be_exported_with_filter(target, current_label, expor
 
     return False
 
+# Checks if the linker_input has code to link statically, i.e. either
+# archives or object files, ignores library.dynamic_library.
+def _contains_code_to_link(linker_input):
+    for library in linker_input.libraries:
+        if (library.static_library != None or
+            library.pic_static_library != None or
+            len(library.objects) or len(library.pic_objects)):
+            return True
+
+    return False
+
+def _find_top_level_linker_input_labels(
+        nodes,
+        linker_inputs_to_be_linked_statically_map,
+        targets_to_be_linked_dynamically_set):
+    top_level_linker_input_labels_set = {}
+    nodes_to_check = list(nodes)
+
+    for i in range(2147483647):
+        if i == len(nodes_to_check):
+            break
+
+        node = nodes_to_check[i]
+        node_label = str(node.label)
+        if node_label in linker_inputs_to_be_linked_statically_map:
+            has_code_to_link = False
+            for linker_input in linker_inputs_to_be_linked_statically_map[node_label]:
+                if _contains_code_to_link(linker_input):
+                    top_level_linker_input_labels_set[node_label] = True
+                    has_code_to_link = True
+                    break
+
+            if not has_code_to_link:
+                nodes_to_check.extend(node.children)
+        elif node_label not in targets_to_be_linked_dynamically_set:
+            # This can happen when there was a target in the graph that exported other libraries'
+            # linker_inputs but didn't contribute any linker_input of its own.
+            nodes_to_check.extend(node.children)
+
+    return top_level_linker_input_labels_set
+
 def _filter_inputs(
         ctx,
         feature_configuration,
@@ -205,11 +246,11 @@ def _filter_inputs(
 
     graph_structure_aspect_nodes = []
     dependency_linker_inputs = []
-    direct_exports = {}
-    for export in deps:
-        direct_exports[str(export.label)] = True
-        dependency_linker_inputs.extend(export[CcInfo].linking_context.linker_inputs.to_list())
-        graph_structure_aspect_nodes.append(export[GraphNodeInfo])
+    direct_deps_set = {}
+    for dep in deps:
+        direct_deps_set[str(dep.label)] = True
+        dependency_linker_inputs.extend(dep[CcInfo].linking_context.linker_inputs.to_list())
+        graph_structure_aspect_nodes.append(dep[GraphNodeInfo])
 
     can_be_linked_dynamically = {}
     for linker_input in dependency_linker_inputs:
@@ -224,6 +265,18 @@ def _filter_inputs(
         can_be_linked_dynamically,
     )
 
+    linker_inputs_to_be_linked_statically_map = {}
+    for linker_input in dependency_linker_inputs:
+        owner = str(linker_input.owner)
+        if owner in targets_to_be_linked_statically_map:
+            linker_inputs_to_be_linked_statically_map.setdefault(owner, []).append(linker_input)
+
+    top_level_linker_input_labels_set = _find_top_level_linker_input_labels(
+        graph_structure_aspect_nodes,
+        linker_inputs_to_be_linked_statically_map,
+        targets_to_be_linked_dynamically_set,
+    )
+
     # We keep track of precompiled_only_dynamic_libraries, so that we can add
     # them to runfiles.
     precompiled_only_dynamic_libraries = []
@@ -260,7 +313,6 @@ def _filter_inputs(
                 # link_once_static_libs_map[owner] but is not being exported
                 linked_statically_but_not_exported.setdefault(link_once_static_libs_map[owner], []).append(owner)
 
-            is_direct_export = owner in direct_exports
             dynamic_only_libraries = []
             static_libraries = []
             for library in linker_input.libraries:
@@ -272,15 +324,15 @@ def _filter_inputs(
             if len(dynamic_only_libraries):
                 precompiled_only_dynamic_libraries.extend(dynamic_only_libraries)
                 if not len(static_libraries):
-                    if is_direct_export:
+                    if owner in direct_deps_set:
                         dynamic_only_roots[owner] = True
                     linker_inputs.append(linker_input)
                     continue
             if len(static_libraries) and owner in dynamic_only_roots:
                 dynamic_only_roots.pop(owner)
 
             linker_input_to_be_linked_statically = linker_input
-            if is_direct_export:
+            if owner in top_level_linker_input_labels_set:
                 linker_input_to_be_linked_statically = _wrap_static_library_with_alwayslink(
                     ctx,
                     feature_configuration,

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

@@ -7,7 +7,9 @@ load(
     "linking_suffix_test",
     "paths_test",
     "runfiles_test",
-    "check_already_linked_inputs_are_not_passed_to_linking_action_test",
+    "check_linking_action_lib_parameters_test",
+    "forwarding_cc_lib",
+    "nocode_cc_lib",
 )
 
 LINKABLE_MORE_THAN_ONCE = "LINKABLE_MORE_THAN_ONCE"
@@ -117,6 +119,8 @@ cc_shared_library(
     deps = [
         "baz",
         "foo",
+        "cc_lib_with_no_srcs",
+        "nocode_cc_lib",
         "a_suffix",
     ],
     user_link_flags = select({
@@ -156,6 +160,33 @@ cc_library(
     }),
 )
 
+forwarding_cc_lib(
+    name = "cc_lib_with_no_srcs",
+    deps = [
+        "indirect_dep",
+    ],
+)
+
+nocode_cc_lib(
+    name = "nocode_cc_lib",
+    additional_inputs = [
+        ":additional_script.txt",
+    ],
+    deps = [
+        "indirect_dep2",
+    ],
+)
+
+cc_library(
+    name = "indirect_dep",
+    srcs = ["indirect_dep.cc"],
+)
+
+cc_library(
+    name = "indirect_dep2",
+    srcs = ["indirect_dep2.cc"],
+)
+
 cc_library(
     name = "a_suffix",
     srcs = ["a_suffix.cc"],
@@ -420,13 +451,13 @@ runfiles_test(
     target_under_test = ":python_test",
 )
 
-check_already_linked_inputs_are_not_passed_to_linking_action_test(
+check_linking_action_lib_parameters_test(
     name = "check_binary_doesnt_take_already_linked_in_libs",
     target_under_test = ":binary",
     libs_that_shouldnt_be_present = ["foo", "bar"],
 )
 
-check_already_linked_inputs_are_not_passed_to_linking_action_test(
+check_linking_action_lib_parameters_test(
     name = "check_shared_lib_doesnt_take_already_linked_in_libs",
     target_under_test = ":foo_so",
     libs_that_shouldnt_be_present = ["bar"],
@@ -437,3 +468,9 @@ build_failure_test(
     message = "'cc_shared_library' must have at least one dependency in 'deps' (or 'roots')",
     target_under_test = "//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/failing_targets:failing_with_no_deps_so",
 )
+
+build_failure_test(
+    name = "direct_dep_with_only_shared_lib_file",
+    message = "Do not place libraries which only contain a precompiled dynamic library",
+    target_under_test = "//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/failing_targets:failing_only_dynamic_lib",
+)

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

@@ -38,14 +38,15 @@ function check_symbol_absent() {
 
 function test_shared_library_symbols() {
   foo_so=$(find . -name libfoo_so.so)
-  symbols=$(nm -D $foo_so)
+  symbols=$(nm $foo_so)
   check_symbol_present "$symbols" "U _Z3barv"
   check_symbol_present "$symbols" "T _Z3bazv"
   check_symbol_present "$symbols" "T _Z3foov"
-
-  check_symbol_absent "$symbols" "_Z3quxv"
+  check_symbol_present "$symbols" "T _Z3foov"
+  check_symbol_present "$symbols" "t _Z3quxv"
+  check_symbol_present "$symbols" "t _Z12indirect_depv"
+  check_symbol_present "$symbols" "t _Z13indirect_dep2v"
   check_symbol_absent "$symbols" "_Z4bar3v"
-  check_symbol_absent "$symbols" "_Z4bar4v"
 }
 
 function test_shared_library_user_link_flags() {

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

@@ -89,3 +89,27 @@ cc_shared_library(
     name = "failing_with_no_deps_so",
     tags = TAGS,
 )
+
+
+cc_shared_library(
+    name = "failing_only_dynamic_lib",
+    deps = [
+        "dynamic_only",
+    ],
+    tags = TAGS,
+)
+
+cc_library(
+    name = "dynamic_only",
+    srcs = [
+        "libabc.so",
+    ],
+)
+
+genrule(
+    name = "abc_lib",
+    srcs = [],
+    outs = ["libabc.so"],
+    cmd = "touch \"$@\"",
+)
+

src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/indirect_dep.cc

@@ -0,0 +1,15 @@
+// Copyright 2023 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+int indirect_dep() { return 0; }

src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/indirect_dep2.cc

@@ -0,0 +1,15 @@
+// Copyright 2023 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+int indirect_dep2() { return 0; }

src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/starlark_tests.bzl

@@ -223,7 +223,7 @@ interface_library_output_group_test = analysistest.make(
     },
 )
 
-def _check_already_linked_inputs_are_not_passed_to_linking_action_test_impl(ctx):
+def _check_linking_action_lib_parameters_test_impl(ctx):
     env = analysistest.begin(ctx)
 
     actions = analysistest.target_actions(env)
@@ -240,9 +240,37 @@ def _check_already_linked_inputs_are_not_passed_to_linking_action_test_impl(ctx)
 
     return analysistest.end(env)
 
-check_already_linked_inputs_are_not_passed_to_linking_action_test = analysistest.make(
-    _check_already_linked_inputs_are_not_passed_to_linking_action_test_impl,
+check_linking_action_lib_parameters_test = analysistest.make(
+    _check_linking_action_lib_parameters_test_impl,
     attrs = {
         "libs_that_shouldnt_be_present": attr.string_list(),
     },
 )
+
+def _forwarding_cc_lib_impl(ctx):
+    return [ctx.attr.deps[0][CcInfo]]
+
+forwarding_cc_lib = rule(
+    implementation = _forwarding_cc_lib_impl,
+    attrs = {
+        "deps": attr.label_list(),
+    },
+    provides = [CcInfo],
+)
+
+def _nocode_cc_lib_impl(ctx):
+    linker_input = cc_common.create_linker_input(
+        owner = ctx.label,
+        additional_inputs = depset([ctx.files.additional_inputs[0]]),
+    )
+    cc_info = CcInfo(linking_context = cc_common.create_linking_context(linker_inputs = depset([linker_input])))
+    return [cc_common.merge_cc_infos(cc_infos = [cc_info, ctx.attr.deps[0][CcInfo]])]
+
+nocode_cc_lib = rule(
+    implementation = _nocode_cc_lib_impl,
+    attrs = {
+        "additional_inputs": attr.label_list(allow_files = True),
+        "deps": attr.label_list(),
+    },
+    provides = [CcInfo],
+)