Skip to content

Commit

Permalink
Fix linker inputs for cc_shared_library linking
Browse files Browse the repository at this point in the history
This CL makes cc_shared_library and cc_binary account for the possibility that
a single rule (i.e. a single owner) may place more than one linker_input in a
linking context. There is no rule saying that this cannot be the case but until
now the implementation of cc_library did place just a single linker input per
owner.

The Starlark implementation of cc_library is slightly different and has made
this bug surface. Rather than change Starlark cc_library we fix
cc_shared_library to have the right behavior. We won't control every C++ rule
people will write using the Starlark API and therefore can't guarantee that
some other rule won't place more than one linker_input per owner.

The existing tests already broke with the Starlark cc_library implementation.

RELNOTES:none
PiperOrigin-RevId: 430195224
  • Loading branch information
oquenchil authored and copybara-github committed Feb 22, 2022
1 parent 8bd989e commit c046f96
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 14 deletions.
9 changes: 5 additions & 4 deletions src/main/starlark/builtins_bzl/common/cc/cc_binary.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -452,12 +452,13 @@ def _filter_libraries_that_are_linked_dynamically(ctx, cc_linking_context, cpp_c

(link_statically_labels, link_dynamically_labels) = _separate_static_and_dynamic_link_libraries(graph_structure_aspect_nodes, can_be_linked_dynamically)

owners_seen = {}
linker_inputs_seen = {}
for linker_input in linker_inputs:
owner = str(linker_input.owner)
if owner in owners_seen:
stringified_linker_input = cc_helper.stringify_linker_input(linker_input)
if stringified_linker_input in linker_inputs_seen:
continue
owners_seen[owner] = True
linker_inputs_seen[stringified_linker_input] = True
owner = str(linker_input.owner)
if owner not in link_dynamically_labels and (owner in link_statically_labels or _get_canonical_form(ctx.label) == owner):
if owner in link_once_static_libs_map:
fail(owner + " is already linked statically in " + link_once_static_libs_map[owner] + " but not exported.")
Expand Down
22 changes: 22 additions & 0 deletions src/main/starlark/builtins_bzl/common/cc/cc_helper.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -435,6 +435,27 @@ def _additional_inputs_from_linking_context(linking_context):
inputs.extend(linker_input.additional_inputs)
return depset(inputs, order = "topological")

def _stringify_linker_input(linker_input):
parts = []
parts.append(str(linker_input.owner))
for library in linker_input.libraries:
if library.static_library != None:
parts.append(library.static_library.path)
if library.pic_static_library != None:
parts.append(library.pic_static_library.path)
if library.dynamic_library != None:
parts.append(library.dynamic_library.path)
if library.interface_library != None:
parts.append(library.interface_library.path)

for additional_input in linker_input.additional_inputs:
parts.append(additional_input.path)

for linkstamp in linker_input.linkstamps:
parts.append(linkstamp.file.path)

return "".join(parts)

cc_helper = struct(
merge_cc_debug_contexts = _merge_cc_debug_contexts,
is_code_coverage_enabled = _is_code_coverage_enabled,
Expand Down Expand Up @@ -462,4 +483,5 @@ cc_helper = struct(
dll_hash_suffix = _dll_hash_suffix,
get_windows_def_file_for_linking = _get_windows_def_file_for_linking,
generate_def_file = _generate_def_file,
stringify_linker_input = _stringify_linker_input,
)
Original file line number Diff line number Diff line change
Expand Up @@ -263,12 +263,14 @@ def _filter_inputs(
precompiled_only_dynamic_libraries = []
unaccounted_for_libs = []
exports = {}
owners_seen = {}
linker_inputs_seen = {}
dynamic_only_roots = {}
for linker_input in dependency_linker_inputs:
owner = str(linker_input.owner)
if owner in owners_seen:
stringified_linker_input = cc_helper.stringify_linker_input(linker_input)
if stringified_linker_input in linker_inputs_seen:
continue
owners_seen[owner] = True
linker_inputs_seen[stringified_linker_input] = True
owner = str(linker_input.owner)
if owner in link_dynamically_labels:
dynamic_linker_input = transitive_exports[owner]
linker_inputs.append(dynamic_linker_input)
Expand All @@ -278,7 +280,6 @@ def _filter_inputs(
link_once_static_libs_map[owner] + " but not exported")

is_direct_export = owner in direct_exports

dynamic_only_libraries = []
static_libraries = []
for library in linker_input.libraries:
Expand All @@ -288,15 +289,14 @@ def _filter_inputs(
static_libraries.append(library)

if len(dynamic_only_libraries):
if not len(static_libraries):
if is_direct_export:
fail("Do not place libraries which only contain a precompiled dynamic library in roots.")

precompiled_only_dynamic_libraries.extend(dynamic_only_libraries)

if not len(static_libraries):
if is_direct_export:
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)

if is_direct_export:
wrapped_library = _wrap_static_library_with_alwayslink(
Expand Down Expand Up @@ -334,6 +334,14 @@ def _filter_inputs(
else:
unaccounted_for_libs.append(linker_input.owner)

if dynamic_only_roots:
message = ("Do not place libraries which only contain a " +
"precompiled dynamic library in roots. The following " +
"libraries only have precompiled dynamic libraries:\n")
for dynamic_only_root in dynamic_only_roots:
message += dynamic_only_root + "\n"
fail(message)

_throw_error_if_unaccounted_libs(unaccounted_for_libs)
return (exports, linker_inputs, link_once_static_libs, precompiled_only_dynamic_libraries)

Expand Down

0 comments on commit c046f96

Please sign in to comment.