Add aspect hints to cc_shared_library

One reason why cc_shared_library wasn't working for upb_proto_library was because indirect cc_shared_library.deps (i.e.  linker_inputs not provided by the immediate dependency) were being dropped, that was fixed in f9008f6.

A second reason why it wasn't working was because until now cc_shared_library has relied on transitive deps providing CcInfo or ProtoInfo while the upb_proto_library aspect provides a custom wrapped CcInfo.  This change fixes that via aspect hints. (It would still require a change in the upb rule implementation).

A third reason why it didn't work for upb was because to avoid a collision with the other aspects applied on proto_libraries, the upb_proto_library implementation added a suffix to the name when calling cc_common.create_linking_context(). This in turn caused the `owner` used in the `linker_inputs` to not match the labels of the targets seen by the cc_shared_library. To fix this, the hints provider accepts a list of owners.

Apart from those features, the hints provider also allows specifying a list of attributes for the cases where we don't want the result of every dependency to be used (this hasn't come up yet and it doesn't affect upb).

This idea was more or less described [here](#16296 (comment)).

Note: I am using the words "aspect hints" because conceptually they are. However, none of this is using the aspect_hints mechanism supported by Bazel rules. Here we are simply using providers.

Fixes #17676.

Closes #17725.

PiperOrigin-RevId: 516488095
Change-Id: I603ed8df90fef0636525d60707692930cd23fa5a

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", "separate_static_and_dynamic_link_libraries", "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")
@@ -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,46 @@ 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 +275,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 +658,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 +683,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 +727,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", "CcSharedLibraryHintInfo", "CcSharedLibraryInfo", "cc_shared_library")
 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; }