Skip to content

Commit dbb09c9

Browse files
oquenchilcopybara-github
authored andcommitted
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
1 parent a50cca5 commit dbb09c9

File tree

8 files changed

+206
-59
lines changed

8 files changed

+206
-59
lines changed

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ public void init(ConfiguredRuleClassProvider.Builder builder) {
6565
BazelCcModule bazelCcModule = new BazelCcModule();
6666
builder.addConfigurationFragment(CppConfiguration.class);
6767
builder.addStarlarkAccessibleTopLevels("CcSharedLibraryInfo", Starlark.NONE);
68+
builder.addStarlarkAccessibleTopLevels("CcSharedLibraryHintInfo", Starlark.NONE);
6869
builder.addBuildInfoFactory(new CppBuildInfo());
6970

7071
builder.addNativeAspectClass(graphNodeAspect);

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

Lines changed: 2 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
"""cc_binary Starlark implementation replacing native"""
1616

1717
load(":common/cc/semantics.bzl", "semantics")
18-
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")
18+
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")
1919
load(":common/cc/cc_helper.bzl", "cc_helper", "linker_mode")
2020
load(":common/cc/cc_info.bzl", "CcInfo")
2121
load(":common/cc/cc_common.bzl", "cc_common")
@@ -329,29 +329,6 @@ def _collect_transitive_dwo_artifacts(cc_compilation_outputs, cc_debug_context,
329329
transitive_dwo_files = cc_debug_context.files
330330
return depset(dwo_files, transitive = [transitive_dwo_files])
331331

332-
def _separate_static_and_dynamic_link_libraries(direct_children, can_be_linked_dynamically):
333-
link_statically_labels = {}
334-
link_dynamically_labels = {}
335-
all_children = list(direct_children)
336-
seen_labels = {}
337-
338-
# Some of the logic here is a duplicate from cc_shared_library.
339-
# But some parts are different hence rewriting.
340-
for i in range(2147483647):
341-
if i == len(all_children):
342-
break
343-
node = all_children[i]
344-
node_label = str(node.label)
345-
if node_label in seen_labels:
346-
continue
347-
seen_labels[node_label] = True
348-
if node_label in can_be_linked_dynamically:
349-
link_dynamically_labels[node_label] = True
350-
else:
351-
link_statically_labels[node_label] = True
352-
all_children.extend(node.children)
353-
return (link_statically_labels, link_dynamically_labels)
354-
355332
def _filter_libraries_that_are_linked_dynamically(ctx, cc_linking_context, cpp_config):
356333
merged_cc_shared_library_infos = merge_cc_shared_library_infos(ctx)
357334
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
368345
if owner in exports_map:
369346
can_be_linked_dynamically[owner] = True
370347

371-
(link_statically_labels, link_dynamically_labels) = _separate_static_and_dynamic_link_libraries(graph_structure_aspect_nodes, can_be_linked_dynamically)
348+
(link_statically_labels, link_dynamically_labels) = separate_static_and_dynamic_link_libraries(graph_structure_aspect_nodes, can_be_linked_dynamically)
372349

373350
linker_inputs_seen = {}
374351
linked_statically_but_not_exported = {}

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

Lines changed: 103 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ GraphNodeInfo = provider(
3737
"Nodes in the graph of shared libraries.",
3838
fields = {
3939
"children": "Other GraphNodeInfo from dependencies of this target",
40-
"label": "Label of the target visited",
40+
"owners": "Owners of the linker inputs in the targets visited",
4141
"linkable_more_than_once": "Linkable into more than a single cc_shared_library",
4242
},
4343
)
@@ -54,6 +54,42 @@ CcSharedLibraryInfo = provider(
5454
},
5555
)
5656

57+
CcSharedLibraryHintInfo = provider(
58+
doc = """
59+
This provider should be used by rules that provide C++ linker inputs and
60+
want to guide what the cc_shared_library uses. The reason for this may be
61+
for example because the rule is not providing a standard provider like
62+
CcInfo or ProtoInfo or because the rule does not want certain attributes
63+
to be used for linking into shared libraries. It may also be needed if the
64+
rule is using non-standard linker_input.owner names.
65+
66+
Propagation of the cc_shared_library aspect will always happen via all
67+
attributes that provide either CcInfo, ProtoInfo or
68+
CcSharedLibraryHintInfo, the hints control whether the result of that
69+
propagation actually gets used.
70+
""",
71+
fields = {
72+
"attributes": ("[String] - If not set, the aspect will use the result of every " +
73+
"dependency that provides CcInfo, ProtoInfo or CcSharedLibraryHintInfo. " +
74+
"If empty list, the aspect will not use the result of any dependency. If " +
75+
"the list contains a list of attribute names, the aspect will only use the " +
76+
"dependencies corresponding to those attributes as long as they provide CcInfo, " +
77+
"ProtoInfo or CcSharedLibraryHintInfo"),
78+
"owners": ("[Label] - cc_shared_library will know which linker_inputs to link based on the owners " +
79+
"field of each linker_input. Most rules will simply use the ctx.label but certain " +
80+
"APIs like cc_common.create_linker_input(owner=) accept any label. " +
81+
"cc_common.create_linking_context_from_compilation_outputs() accepts a `name` which " +
82+
"will then be used to create the owner of the linker_input together with ctx.package." +
83+
"For these cases, since the cc_shared_library cannot guess, the rule author should " +
84+
"provide a hint with the owners of the linker inputs. If the value of owners is not set, then " +
85+
"ctx.label will be used. If the rule author passes a list and they want ctx.label plus some other " +
86+
"label then they will have to add ctx.label explicitly. If you want to use custom owners from C++ " +
87+
"rules keep as close to the original ctx.label as possible, to avoid conflicts with linker_inputs " +
88+
"created by other targets keep the original repository name, the original package name and re-use " +
89+
"the original name as part of your new name, limiting your custom addition to a prefix or suffix."),
90+
},
91+
)
92+
5793
# For each target, find out whether it should be linked statically or
5894
# dynamically.
5995
def _separate_static_and_dynamic_link_libraries(
@@ -72,16 +108,46 @@ def _separate_static_and_dynamic_link_libraries(
72108
break
73109

74110
node = all_children[i]
75-
node_label = str(node.label)
76111

77-
if node_label in seen_labels:
78-
continue
79-
seen_labels[node_label] = True
80-
81-
if node_label in can_be_linked_dynamically:
82-
targets_to_be_linked_dynamically_set[node_label] = True
83-
else:
84-
targets_to_be_linked_statically_map[node_label] = node.linkable_more_than_once
112+
must_add_children = False
113+
114+
# The *_seen variables are used to track a programmatic error and fail
115+
# if it happens. Every value in node.owners presumably corresponds to
116+
# a linker_input in the same exact target. Therefore if we have seen
117+
# any of the owners already, then we must have also seen all the other
118+
# owners in the same node. Viceversa when we haven't seen them yet. If
119+
# both of these values are non-zero after the loop, the most likely
120+
# reason would be a bug in the implementation. It could potentially be
121+
# triggered by users if they use owner labels that do not keep most of
122+
# the ctx.label.package and ctx.label.name which then clash with other
123+
# target's owners (unlikely). For now though if the error is
124+
# triggered, it's reasonable to require manual revision by
125+
# the cc_shared_library implementation owners.
126+
has_owners_seen = False
127+
has_owners_not_seen = False
128+
for owner in node.owners:
129+
# TODO(bazel-team): Do not convert Labels to string to save on
130+
# garbage string allocations.
131+
owner_str = str(owner)
132+
133+
if owner_str in seen_labels:
134+
has_owners_seen = True
135+
continue
136+
137+
has_owners_not_seen = True
138+
seen_labels[owner_str] = True
139+
140+
if owner_str in can_be_linked_dynamically:
141+
targets_to_be_linked_dynamically_set[owner_str] = True
142+
else:
143+
targets_to_be_linked_statically_map[owner_str] = node.linkable_more_than_once
144+
must_add_children = True
145+
146+
if has_owners_seen and has_owners_not_seen:
147+
fail("Your build has triggered a programmatic error in the cc_shared_library rule. " +
148+
"Please file an issue in https://github.com/bazelbuild/bazel")
149+
150+
if must_add_children:
85151
all_children.extend(node.children)
86152

87153
return (targets_to_be_linked_statically_map, targets_to_be_linked_dynamically_set)
@@ -209,20 +275,22 @@ def _find_top_level_linker_input_labels(
209275
break
210276

211277
node = nodes_to_check[i]
212-
node_label = str(node.label)
213-
if node_label in linker_inputs_to_be_linked_statically_map:
214-
has_code_to_link = False
215-
for linker_input in linker_inputs_to_be_linked_statically_map[node_label]:
216-
if _contains_code_to_link(linker_input):
217-
top_level_linker_input_labels_set[node_label] = True
218-
has_code_to_link = True
219-
break
220-
221-
if not has_code_to_link:
222-
nodes_to_check.extend(node.children)
223-
elif node_label not in targets_to_be_linked_dynamically_set:
224-
# This can happen when there was a target in the graph that exported other libraries'
225-
# linker_inputs but didn't contribute any linker_input of its own.
278+
must_add_children = False
279+
for owner in node.owners:
280+
owner_str = str(owner)
281+
if owner_str in linker_inputs_to_be_linked_statically_map:
282+
must_add_children = True
283+
for linker_input in linker_inputs_to_be_linked_statically_map[owner_str]:
284+
if _contains_code_to_link(linker_input):
285+
top_level_linker_input_labels_set[owner_str] = True
286+
must_add_children = False
287+
break
288+
elif owner_str not in targets_to_be_linked_dynamically_set:
289+
# This can happen when there was a target in the graph that exported other libraries'
290+
# linker_inputs but didn't contribute any linker_input of its own.
291+
must_add_children = True
292+
293+
if must_add_children:
226294
nodes_to_check.extend(node.children)
227295

228296
return top_level_linker_input_labels_set
@@ -590,10 +658,16 @@ def _cc_shared_library_impl(ctx):
590658
def _graph_structure_aspect_impl(target, ctx):
591659
children = []
592660

661+
attributes = dir(ctx.rule.attr)
662+
owners = [ctx.label]
663+
if CcSharedLibraryHintInfo in target:
664+
attributes = getattr(target[CcSharedLibraryHintInfo], "attributes", dir(ctx.rule.attr))
665+
owners = getattr(target[CcSharedLibraryHintInfo], "owners", [ctx.label])
666+
593667
# Collect graph structure info from any possible deplike attribute. The aspect
594668
# itself applies across every deplike attribute (attr_aspects is *), so enumerate
595669
# over all attributes and consume GraphNodeInfo if available.
596-
for fieldname in dir(ctx.rule.attr):
670+
for fieldname in attributes:
597671
deps = getattr(ctx.rule.attr, fieldname, None)
598672
if type(deps) == "list":
599673
for dep in deps:
@@ -609,17 +683,16 @@ def _graph_structure_aspect_impl(target, ctx):
609683
for tag in ctx.rule.attr.tags:
610684
if tag == LINKABLE_MORE_THAN_ONCE:
611685
linkable_more_than_once = True
612-
613686
return [GraphNodeInfo(
614-
label = ctx.label,
687+
owners = owners,
615688
children = children,
616689
linkable_more_than_once = linkable_more_than_once,
617690
)]
618691

619692
graph_structure_aspect = aspect(
620693
attr_aspects = ["*"],
621-
required_providers = [[CcInfo], [ProtoInfo]],
622-
required_aspect_providers = [[CcInfo]],
694+
required_providers = [[CcInfo], [ProtoInfo], [CcSharedLibraryHintInfo]],
695+
required_aspect_providers = [[CcInfo], [CcSharedLibraryHintInfo]],
623696
implementation = _graph_structure_aspect_impl,
624697
)
625698

@@ -654,3 +727,4 @@ merge_cc_shared_library_infos = _merge_cc_shared_library_infos
654727
build_link_once_static_libs_map = _build_link_once_static_libs_map
655728
build_exports_map_from_only_dynamic_deps = _build_exports_map_from_only_dynamic_deps
656729
throw_linked_but_not_exported_errors = _throw_linked_but_not_exported_errors
730+
separate_static_and_dynamic_link_libraries = _separate_static_and_dynamic_link_libraries

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
load("@_builtins//:common/cc/cc_import.bzl", "cc_import")
1818
load("@_builtins//:common/cc/cc_binary_wrapper.bzl", "cc_binary")
1919
load("@_builtins//:common/cc/cc_test_wrapper.bzl", cc_test = "cc_test_wrapper")
20-
load("@_builtins//:common/cc/experimental_cc_shared_library.bzl", "CcSharedLibraryInfo", "cc_shared_library")
20+
load("@_builtins//:common/cc/experimental_cc_shared_library.bzl", "CcSharedLibraryHintInfo", "CcSharedLibraryInfo", "cc_shared_library")
2121
load("@_builtins//:common/objc/objc_import.bzl", "objc_import")
2222
load("@_builtins//:common/objc/objc_library.bzl", "objc_library")
2323
load("@_builtins//:common/objc/compilation_support.bzl", "compilation_support")
@@ -40,6 +40,7 @@ exported_toplevels = {
4040
# "original value".
4141
"_builtins_dummy": "overridden value",
4242
"CcSharedLibraryInfo": CcSharedLibraryInfo,
43+
"CcSharedLibraryHintInfo": CcSharedLibraryHintInfo,
4344
"proto_common_do_not_use": proto_common_do_not_use,
4445
"PyRuntimeInfo": PyRuntimeInfo,
4546
"PyInfo": PyInfo,

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

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ load(
1010
"check_linking_action_lib_parameters_test",
1111
"forwarding_cc_lib",
1212
"nocode_cc_lib",
13+
"wrapped_cc_lib",
1314
)
1415

1516
LINKABLE_MORE_THAN_ONCE = "LINKABLE_MORE_THAN_ONCE"
@@ -121,6 +122,7 @@ cc_shared_library(
121122
"foo",
122123
"cc_lib_with_no_srcs",
123124
"nocode_cc_lib",
125+
"should_not_be_linked_cc_lib",
124126
"a_suffix",
125127
],
126128
user_link_flags = select({
@@ -160,10 +162,31 @@ cc_library(
160162
}),
161163
)
162164

165+
wrapped_cc_lib(
166+
name = "wrapped_cc_lib",
167+
deps = [
168+
"indirect_dep",
169+
],
170+
)
171+
163172
forwarding_cc_lib(
164173
name = "cc_lib_with_no_srcs",
165174
deps = [
166-
"indirect_dep",
175+
"wrapped_cc_lib",
176+
],
177+
)
178+
179+
wrapped_cc_lib(
180+
name = "should_not_be_linked_wrapped",
181+
deps = [
182+
"indirect_dep3",
183+
],
184+
)
185+
186+
forwarding_cc_lib(
187+
name = "should_not_be_linked_cc_lib",
188+
do_not_follow_deps = [
189+
"should_not_be_linked_wrapped",
167190
],
168191
)
169192

@@ -187,6 +210,11 @@ cc_library(
187210
srcs = ["indirect_dep2.cc"],
188211
)
189212

213+
cc_library(
214+
name = "indirect_dep3",
215+
srcs = ["indirect_dep3.cc"],
216+
)
217+
190218
cc_library(
191219
name = "a_suffix",
192220
srcs = ["a_suffix.cc"],

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ function test_shared_library_symbols() {
4646
check_symbol_present "$symbols" "t _Z3quxv"
4747
check_symbol_present "$symbols" "t _Z12indirect_depv"
4848
check_symbol_present "$symbols" "t _Z13indirect_dep2v"
49+
check_symbol_absent "$symbols" "_Z13indirect_dep3v"
4950
check_symbol_absent "$symbols" "_Z4bar3v"
5051
}
5152

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
// Copyright 2023 The Bazel Authors. All rights reserved.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
int indirect_dep3() { return 0; }

0 commit comments

Comments
 (0)