Add aspect hints to cc_shared_library

src/main/java/com/google/devtools/build/lib/bazel/rules/CcRules.java

@@ -65,6 +65,7 @@ public void init(ConfiguredRuleClassProvider.Builder builder) {
     BazelCcModule bazelCcModule = new BazelCcModule();
     builder.addConfigurationFragment(CppConfiguration.class);
     builder.addStarlarkAccessibleTopLevels("CcSharedLibraryInfo", Starlark.NONE);
+    builder.addStarlarkAccessibleTopLevels("CcSharedLibraryHintInfo", Starlark.NONE);
     builder.addBuildInfoFactory(new CppBuildInfo());
 
     builder.addNativeAspectClass(graphNodeAspect);

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/experimental_cc_shared_library.bzl", "GraphNodeInfo", "build_exports_map_from_only_dynamic_deps", "build_link_once_static_libs_map", "merge_cc_shared_library_infos", "throw_linked_but_not_exported_errors")
+load(":common/cc/experimental_cc_shared_library.bzl", "GraphNodeInfo", "build_exports_map_from_only_dynamic_deps", "build_link_once_static_libs_map", "merge_cc_shared_library_infos", "throw_linked_but_not_exported_errors", "separate_static_and_dynamic_link_libraries")
 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")
@@ -329,29 +329,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 _separate_static_and_dynamic_link_libraries(direct_children, can_be_linked_dynamically):
-    link_statically_labels = {}
-    link_dynamically_labels = {}
-    all_children = list(direct_children)
-    seen_labels = {}
-
-    # Some of the logic here is a duplicate from cc_shared_library.
-    # But some parts are different hence rewriting.
-    for i in range(2147483647):
-        if i == len(all_children):
-            break
-        node = all_children[i]
-        node_label = str(node.label)
-        if node_label in seen_labels:
-            continue
-        seen_labels[node_label] = True
-        if node_label in can_be_linked_dynamically:
-            link_dynamically_labels[node_label] = True
-        else:
-            link_statically_labels[node_label] = True
-            all_children.extend(node.children)
-    return (link_statically_labels, link_dynamically_labels)
-
 def _filter_libraries_that_are_linked_dynamically(ctx, cc_linking_context, cpp_config):
     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)
@@ -368,7 +345,7 @@ def _filter_libraries_that_are_linked_dynamically(ctx, cc_linking_context, cpp_c
         if owner in exports_map:
             can_be_linked_dynamically[owner] = True
 
-    (link_statically_labels, link_dynamically_labels) = _separate_static_and_dynamic_link_libraries(graph_structure_aspect_nodes, can_be_linked_dynamically)
+    (link_statically_labels, link_dynamically_labels) = separate_static_and_dynamic_link_libraries(graph_structure_aspect_nodes, can_be_linked_dynamically)
 
     linker_inputs_seen = {}
     linked_statically_but_not_exported = {}

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

@@ -37,7 +37,7 @@ GraphNodeInfo = provider(
     "Nodes in the graph of shared libraries.",
     fields = {
         "children": "Other GraphNodeInfo from dependencies of this target",
-        "label": "Label of the target visited",
+        "owners": "Owners of the linker inputs in the targets visited",
         "linkable_more_than_once": "Linkable into more than a single cc_shared_library",
     },
 )
@@ -54,6 +54,42 @@ CcSharedLibraryInfo = provider(
     },
 )
 
+CcSharedLibraryHintInfo = provider(
+    doc = """
+    This provider should be used by rules that provide C++ linker inputs and
+    want to guide what the cc_shared_library uses. The reason for this may be
+    for example because the rule is not providing a standard provider like
+    CcInfo or ProtoInfo or because the rule does not want certain attributes
+    to be used for linking into shared libraries. It may also be needed if the
+    rule is using non-standard linker_input.owner names.
+
+    Propagation of the cc_shared_library aspect will always happen via all
+    attributes that provide either CcInfo, ProtoInfo or
+    CcSharedLibraryHintInfo, the hints control whether the result of that
+    propagation actually gets used.
+    """,
+    fields = {
+        "attributes": ("[String] - If not set, the aspect will use the result of every " +
+                       "dependency that provides CcInfo, ProtoInfo or CcSharedLibraryHintInfo. " +
+                       "If empty list, the aspect will not use the result of any dependency. If " +
+                       "the list contains a list of attribute names, the aspect will only use the " +
+                       "dependencies corresponding to those attributes as long as they provide CcInfo, " +
+                       "ProtoInfo or CcSharedLibraryHintInfo"),
+        "owners": ("[Label] - cc_shared_library will know which linker_inputs to link based on the owners "+
+                   "field of each linker_input. Most rules will simply use the ctx.label but certain " +
+                   "APIs like cc_common.create_linker_input(owner=) accept any label. " +
+                   "cc_common.create_linking_context_from_compilation_outputs() accepts a `name` which " +
+                   "will then be used to create the owner of the linker_input together with ctx.package." +
+                   "For these cases, since the cc_shared_library cannot guess, the rule author should "+
+                   "provide a hint with the owners of the linker inputs. If the value of owners is not set, then " +
+                   "ctx.label will be used. If the rule author passes a list and they want ctx.label plus some other " +
+                   "label then they will have to add ctx.label explicitly. If you want to use custom owners from C++ " +
+                   "rules keep as close to the original ctx.label as possible, to avoid conflicts with linker_inputs " +
+                   "created by other targets keep the original repository name, the original package name and re-use " +
+                   "the original name as part of your new name, limiting your custom addition to a prefix or suffix."),
+    },
+)
+
 # For each target, find out whether it should be linked statically or
 # dynamically.
 def _separate_static_and_dynamic_link_libraries(
@@ -72,16 +108,45 @@ def _separate_static_and_dynamic_link_libraries(
             break
 
         node = all_children[i]
-        node_label = str(node.label)
 
-        if node_label in seen_labels:
-            continue
-        seen_labels[node_label] = True
-
-        if node_label in can_be_linked_dynamically:
-            targets_to_be_linked_dynamically_set[node_label] = True
-        else:
-            targets_to_be_linked_statically_map[node_label] = node.linkable_more_than_once
+        must_add_children = False
+        # The *_seen variables are used to track a programmatic error and fail
+        # if it happens.  Every value in node.owners presumably corresponds to
+        # a linker_input in the same exact target. Therefore if we have seen
+        # any of the owners already, then we must have also seen all the other
+        # owners in the same node. Viceversa when we haven't seen them yet. If
+        # both of these values are non-zero after the loop, the most likely
+        # reason would be a bug in the implementation. It could potentially be
+        # triggered by users if they use owner labels that do not keep most of
+        # the ctx.label.package and ctx.label.name which then clash with other
+        # target's owners (unlikely). For now though if the error is
+        # triggered, it's reasonable to require manual revision by
+        # the cc_shared_library implementation owners.
+        has_owners_seen = False
+        has_owners_not_seen = False
+        for owner in node.owners:
+            # TODO(bazel-team): Do not convert Labels to string to save on
+            # garbage string allocations.
+            owner_str = str(owner)
+
+            if owner_str in seen_labels:
+                has_owners_seen = True
+                continue
+
+            has_owners_not_seen = True
+            seen_labels[owner_str] = True
+
+            if owner_str in can_be_linked_dynamically:
+                targets_to_be_linked_dynamically_set[owner_str] = True
+            else:
+                targets_to_be_linked_statically_map[owner_str] = node.linkable_more_than_once
+                must_add_children = True
+
+        if has_owners_seen and has_owners_not_seen:
+            fail("Your build has triggered a programmatic error in the cc_shared_library rule. "
+                 + "Please file an issue in https://github.com/bazelbuild/bazel")
+
+        if must_add_children:
             all_children.extend(node.children)
 
     return (targets_to_be_linked_statically_map, targets_to_be_linked_dynamically_set)
@@ -209,20 +274,22 @@ def _find_top_level_linker_input_labels(
             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.
+        must_add_children = False
+        for owner in node.owners:
+            owner_str = str(owner)
+            if owner_str in linker_inputs_to_be_linked_statically_map:
+                must_add_children = True
+                for linker_input in linker_inputs_to_be_linked_statically_map[owner_str]:
+                    if _contains_code_to_link(linker_input):
+                        top_level_linker_input_labels_set[owner_str] = True
+                        must_add_children = False
+                        break
+            elif owner_str 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.
+                must_add_children = True
+
+        if must_add_children:
             nodes_to_check.extend(node.children)
 
     return top_level_linker_input_labels_set
@@ -590,10 +657,16 @@ def _cc_shared_library_impl(ctx):
 def _graph_structure_aspect_impl(target, ctx):
     children = []
 
+    attributes = dir(ctx.rule.attr)
+    owners = [ctx.label]
+    if CcSharedLibraryHintInfo in target:
+        attributes = getattr(target[CcSharedLibraryHintInfo], "attributes", dir(ctx.rule.attr))
+        owners = getattr(target[CcSharedLibraryHintInfo], "owners", [ctx.label])
+
     # Collect graph structure info from any possible deplike attribute. The aspect
     # itself applies across every deplike attribute (attr_aspects is *), so enumerate
     # over all attributes and consume GraphNodeInfo if available.
-    for fieldname in dir(ctx.rule.attr):
+    for fieldname in attributes:
         deps = getattr(ctx.rule.attr, fieldname, None)
         if type(deps) == "list":
             for dep in deps:
@@ -609,17 +682,16 @@ def _graph_structure_aspect_impl(target, ctx):
         for tag in ctx.rule.attr.tags:
             if tag == LINKABLE_MORE_THAN_ONCE:
                 linkable_more_than_once = True
-
     return [GraphNodeInfo(
-        label = ctx.label,
+        owners = owners,
         children = children,
         linkable_more_than_once = linkable_more_than_once,
     )]
 
 graph_structure_aspect = aspect(
     attr_aspects = ["*"],
-    required_providers = [[CcInfo], [ProtoInfo]],
-    required_aspect_providers = [[CcInfo]],
+    required_providers = [[CcInfo], [ProtoInfo], [CcSharedLibraryHintInfo]],
+    required_aspect_providers = [[CcInfo], [CcSharedLibraryHintInfo]],
     implementation = _graph_structure_aspect_impl,
 )
 
@@ -654,3 +726,4 @@ merge_cc_shared_library_infos = _merge_cc_shared_library_infos
 build_link_once_static_libs_map = _build_link_once_static_libs_map
 build_exports_map_from_only_dynamic_deps = _build_exports_map_from_only_dynamic_deps
 throw_linked_but_not_exported_errors = _throw_linked_but_not_exported_errors
+separate_static_and_dynamic_link_libraries = _separate_static_and_dynamic_link_libraries

src/main/starlark/builtins_bzl/common/exports.bzl

@@ -17,7 +17,7 @@
 load("@_builtins//:common/cc/cc_import.bzl", "cc_import")
 load("@_builtins//:common/cc/cc_binary_wrapper.bzl", "cc_binary")
 load("@_builtins//:common/cc/cc_test_wrapper.bzl", cc_test = "cc_test_wrapper")
-load("@_builtins//:common/cc/experimental_cc_shared_library.bzl", "CcSharedLibraryInfo", "cc_shared_library")
+load("@_builtins//:common/cc/experimental_cc_shared_library.bzl", "CcSharedLibraryInfo", "cc_shared_library", "CcSharedLibraryHintInfo")
 load("@_builtins//:common/objc/objc_import.bzl", "objc_import")
 load("@_builtins//:common/objc/objc_library.bzl", "objc_library")
 load("@_builtins//:common/objc/compilation_support.bzl", "compilation_support")
@@ -40,6 +40,7 @@ exported_toplevels = {
     # "original value".
     "_builtins_dummy": "overridden value",
     "CcSharedLibraryInfo": CcSharedLibraryInfo,
+    "CcSharedLibraryHintInfo": CcSharedLibraryHintInfo,
     "proto_common_do_not_use": proto_common_do_not_use,
     "PyRuntimeInfo": PyRuntimeInfo,
     "PyInfo": PyInfo,

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

@@ -10,6 +10,7 @@ load(
     "check_linking_action_lib_parameters_test",
     "forwarding_cc_lib",
     "nocode_cc_lib",
+    "wrapped_cc_lib",
 )
 
 LINKABLE_MORE_THAN_ONCE = "LINKABLE_MORE_THAN_ONCE"
@@ -121,6 +122,7 @@ cc_shared_library(
         "foo",
         "cc_lib_with_no_srcs",
         "nocode_cc_lib",
+        "should_not_be_linked_cc_lib",
         "a_suffix",
     ],
     user_link_flags = select({
@@ -160,10 +162,31 @@ cc_library(
     }),
 )
 
+wrapped_cc_lib(
+    name = "wrapped_cc_lib",
+    deps = [
+        "indirect_dep",
+    ],
+)
+
 forwarding_cc_lib(
     name = "cc_lib_with_no_srcs",
     deps = [
-        "indirect_dep",
+        "wrapped_cc_lib",
+    ],
+)
+
+wrapped_cc_lib(
+    name = "should_not_be_linked_wrapped",
+    deps = [
+        "indirect_dep3",
+    ],
+)
+
+forwarding_cc_lib(
+    name = "should_not_be_linked_cc_lib",
+    do_not_follow_deps = [
+        "should_not_be_linked_wrapped",
     ],
 )
 
@@ -187,6 +210,11 @@ cc_library(
     srcs = ["indirect_dep2.cc"],
 )
 
+cc_library(
+    name = "indirect_dep3",
+    srcs = ["indirect_dep3.cc"],
+)
+
 cc_library(
     name = "a_suffix",
     srcs = ["a_suffix.cc"],

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

@@ -46,6 +46,7 @@ function test_shared_library_symbols() {
   check_symbol_present "$symbols" "t _Z3quxv"
   check_symbol_present "$symbols" "t _Z12indirect_depv"
   check_symbol_present "$symbols" "t _Z13indirect_dep2v"
+  check_symbol_absent "$symbols" "_Z13indirect_dep3v"
   check_symbol_absent "$symbols" "_Z4bar3v"
 }
 

src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/indirect_dep3.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_dep3() { return 0; }