Fix order of linker inputs in cc_[shared_library|binary]

In c73900b the order of the linker inputs in the linking command line was fixed for common archives between different direct deps. Before that fix the direct deps were merged sequentially without taking into account dependencies (no topological sort). After the change we were doing a topological sort but it was wrong in a way.

The cc_shared_library graph is a parallel graph to the cc_library graph. The former decides partitions of the latter. From the top level cc_library every other cc_library is reachable even if it was already linked to a cc_shared_library in the dynamic dependencies. The depset that we were using for linker inputs only took into account the cc_libraries for doing a topological sort, it didn't take into account any pruning we might have done from the graph due to the presence of a shared library. After the depset was sorted we then filtered the linker inputs of cc_libraries that were already present in cc_shared_libraries in dynamic_deps.

This was wrong. A cc_shared_library should be treated as a black box and its dependencies should not have any influence on the final topological order of the linker inputs. This CL addresses that by taking into account pruned inputs.

In a first attempt this change re-implemented topological sorting using Kahn's algorithm but this was different to what is implemented by depset(order="topological") since in Kahn's the nodes without incoming edges are always processed first. This had the undesired effect that even when it was possible to respect the order of the deps as listed on the BUILD file and still produce a valid topological sort, Kahn's ignored the order. A second attempt (the one in this CL) re-builds a topological depset by changing the pruning algorithm to traverse the graph depth-first, then lets Bazel handle the sorting by flattening the depset.

This CL also fixes the order for linker_inputs in cc_binary which had a wrong order by listing shared libraries first followed by static linker inputs without any consideration for all of the above.

RELNOTES:none
PiperOrigin-RevId: 542208041
Change-Id: Icaf3b488f3208259ff0bb0eef3318bef582533c5

Author: oquenchil

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/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_shared_library.bzl", "CcSharedLibraryInfo", "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", "sort_linker_inputs", "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")
@@ -331,10 +331,26 @@ 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 _filter_libraries_that_are_linked_dynamically(ctx, cc_linking_context, cpp_config):
+def _build_map_direct_dynamic_dep_to_transitive_dynamic_deps(ctx):
+    all_dynamic_dep_linker_inputs = {}
+    direct_dynamic_dep_to_transitive_dynamic_deps = {}
+    for dep in ctx.attr.dynamic_deps:
+        owner = dep[CcSharedLibraryInfo].linker_input.owner
+        all_dynamic_dep_linker_inputs[owner] = dep[CcSharedLibraryInfo].linker_input
+        transitive_dynamic_dep_labels = []
+        for dynamic_dep in dep[CcSharedLibraryInfo].dynamic_deps.to_list():
+            all_dynamic_dep_linker_inputs[dynamic_dep[1].owner] = dynamic_dep[1]
+            transitive_dynamic_dep_labels.append(dynamic_dep[1].owner)
+        transitive_dynamic_dep_labels_set = depset(transitive_dynamic_dep_labels, order = "topological")
+        for export in dep[CcSharedLibraryInfo].exports:
+            direct_dynamic_dep_to_transitive_dynamic_deps[export] = transitive_dynamic_dep_labels_set
+
+    return direct_dynamic_dep_to_transitive_dynamic_deps, all_dynamic_dep_linker_inputs
+
+def _filter_libraries_that_are_linked_dynamically(ctx, cc_linking_context):
     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)
-    exports_map = build_exports_map_from_only_dynamic_deps(merged_cc_shared_library_infos)
+    transitive_exports = build_exports_map_from_only_dynamic_deps(merged_cc_shared_library_infos)
     static_linker_inputs = []
     linker_inputs = cc_linking_context.linker_inputs.to_list()
 
@@ -344,28 +360,89 @@ def _filter_libraries_that_are_linked_dynamically(ctx, cc_linking_context, cpp_c
     can_be_linked_dynamically = {}
     for linker_input in linker_inputs:
         owner = str(linker_input.owner)
-        if owner in exports_map:
+        if owner in transitive_exports:
             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)
+    transitive_dynamic_dep_labels = {}
+
+    # Entries in unused_dynamic_linker_inputs will be marked None if they are
+    # used
+    (
+        transitive_dynamic_dep_labels,
+        unused_dynamic_linker_inputs,
+    ) = _build_map_direct_dynamic_dep_to_transitive_dynamic_deps(ctx)
+
+    (
+        targets_to_be_linked_statically_map,
+        targets_to_be_linked_dynamically_set,
+        topologically_sorted_labels,
+    ) = separate_static_and_dynamic_link_libraries(
+        graph_structure_aspect_nodes,
+        can_be_linked_dynamically,
+        transitive_dynamic_dep_labels,
+    )
+
+    topologically_sorted_labels = [ctx.label] + topologically_sorted_labels
 
     linker_inputs_seen = {}
     linked_statically_but_not_exported = {}
+    label_to_linker_inputs = {}
+
+    def _add_linker_input_to_dict(owner, linker_input):
+        label_to_linker_inputs.setdefault(owner, []).append(linker_input)
+
+    linker_inputs_count = 0
     for linker_input in linker_inputs:
         stringified_linker_input = cc_helper.stringify_linker_input(linker_input)
         if stringified_linker_input in linker_inputs_seen:
             continue
         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 str(ctx.label) == owner):
+        if owner in targets_to_be_linked_dynamically_set:
+            unused_dynamic_linker_inputs[transitive_exports[owner].owner] = None
+            _add_linker_input_to_dict(linker_input.owner, transitive_exports[owner])
+            linker_inputs_count += 1
+        elif owner in targets_to_be_linked_statically_map or str(ctx.label) == owner:
             if owner in link_once_static_libs_map:
-                linked_statically_but_not_exported.setdefault(link_once_static_libs_map[owner], []).append(owner)
+                linked_statically_but_not_exported.setdefault(targets_to_be_linked_statically_map[owner], []).append(owner)
             else:
-                static_linker_inputs.append(linker_input)
+                _add_linker_input_to_dict(linker_input.owner, linker_input)
+                linker_inputs_count += 1
+
+    # Unlike Unix on Windows every dynamic dependency must be linked to the
+    # main binary, even indirect ones that are dependencies of direct
+    # dynamic dependencies of this binary. So even though linking indirect
+    # dynamic dependencies is not needed for Unix, we link them here too
+    # because we cannot know whether the shared libraries were linked with
+    # RUNPATH or RPATH. If they were linked with the former, then the loader
+    # won't search in the runfiles directory of this binary for the library, it
+    # will only search in the RUNPATH set at the time of linking the shared
+    # library and we cannot possibly know at that point the runfiles directory
+    # of all of its dependents.
+    #
+    # TODO(bazel-team):Add a feature so that indirect dynamic dependencies are
+    # not linked. This should be opt-in for those people that are creating a
+    # production binary and have manually set the RPATH/RUNPATHS correctly.
+    topologically_sorted_labels_set = {label: True for label in topologically_sorted_labels}
+    for dynamic_linker_input_owner, unused_linker_input in unused_dynamic_linker_inputs.items():
+        if unused_linker_input:
+            _add_linker_input_to_dict(
+                dynamic_linker_input_owner,
+                unused_dynamic_linker_inputs[dynamic_linker_input_owner],
+            )
+            linker_inputs_count += 1
+            if dynamic_linker_input_owner not in topologically_sorted_labels_set:
+                topologically_sorted_labels.append(dynamic_linker_input_owner)
 
     throw_linked_but_not_exported_errors(linked_statically_but_not_exported)
 
-    return cc_common.create_linking_context(linker_inputs = depset(exports_map.values() + static_linker_inputs, order = "topological"))
+    sorted_linker_inputs = sort_linker_inputs(
+        topologically_sorted_labels,
+        label_to_linker_inputs,
+        linker_inputs_count,
+    )
+
+    return cc_common.create_linking_context(linker_inputs = depset(sorted_linker_inputs, order = "topological"))
 
 def _create_transitive_linking_actions(
         ctx,
@@ -446,7 +523,7 @@ def _create_transitive_linking_actions(
     cc_linking_context = cc_info.linking_context
 
     if len(ctx.attr.dynamic_deps) > 0:
-        cc_linking_context = _filter_libraries_that_are_linked_dynamically(ctx, cc_linking_context, cpp_config)
+        cc_linking_context = _filter_libraries_that_are_linked_dynamically(ctx, cc_linking_context)
     link_deps_statically = True
     if linking_mode == linker_mode.LINKING_DYNAMIC:
         link_deps_statically = False

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

@@ -50,24 +50,77 @@ CcSharedLibraryInfo = provider(
     },
 )
 
+def _programmatic_error(message = ""):
+    fail("Your build has triggered a programmatic error in the cc_shared_library rule. " +
+         "Please file an issue in https://github.com/bazelbuild/bazel : {}".format(message))
+
+def _sort_linker_inputs(topologically_sorted_labels, label_to_linker_inputs, linker_inputs_count):
+    # len(label_to_linker_inputs) might not match the topologically_sorted_labels
+    # size. The latter is looking at nodes in the graph but a node may not
+    # necessarily contribute any linker_inputs. For example a cc_library with
+    # no sources and only deps. Every linker_input.owner must be in
+    # topologically_sorted_labels otherwise there is an error in the rule
+    # implementation of the target providing that linker_input, e.g. it's
+    # missing a CcSharedLibraryHintInfo if it had custom owner names for linker
+    # inputs.
+    sorted_linker_inputs = []
+    for label in topologically_sorted_labels:
+        if label not in label_to_linker_inputs:
+            # This is ok. It can happen if no linker_inputs
+            # were added by a node in the graph.
+            continue
+        sorted_linker_inputs.extend(label_to_linker_inputs[label])
+
+    if len(sorted_linker_inputs) != linker_inputs_count:
+        owners = []
+        for sorted_linker_input in sorted_linker_inputs:
+            owners.append(str(sorted_linker_input.owner))
+        _programmatic_error("{} vs {}".format(",".join(owners), linker_inputs_count))
+
+    return sorted_linker_inputs
+
 # For each target, find out whether it should be linked statically or
-# dynamically.
+# dynamically. The transitive_dynamic_dep_labels parameter is only needed for
+# binaries because they link all dynamic_deps (cc_binary|cc_test).
 def _separate_static_and_dynamic_link_libraries(
         direct_children,
-        can_be_linked_dynamically):
+        can_be_linked_dynamically,
+        transitive_dynamic_dep_labels = {}):
     node = None
-    all_children = list(direct_children)
+    all_children = reversed(direct_children)
     targets_to_be_linked_statically_map = {}
     targets_to_be_linked_dynamically_set = {}
-
     seen_labels = {}
 
+    # The cc_shared_library graph is parallel to the cc_library graph.
+    # Propagation of linker inputs between cc_libraries happens via the CcInfo
+    # provider. Parallel to this we have cc_shared_libraries which may decide
+    # different partitions of the cc_library graph.
+    #
+    # In a previous implementation of cc_shared_library we relied on the
+    # topological sort given by flattening
+    # cc_info.linking_context.linker_inputs.to_list(), however this was wrong
+    # because the dependencies of a shared library (i.e. a pruned node here)
+    # influenced the final order.
+    #
+    # In order to fix this, the pruning below was changed from breadth-first
+    # traversal to depth-first traversal. While doing this we also recreate a
+    # depset with topological order that takes into account the pruned nodes
+    # and which will later be used to order the libraries in the linking
+    # command line. This will be in topological order and will respect the
+    # order of the deps as listed on the BUILD file as much as possible.
+    #
+    # Here we say "first_owner" because each node (see GraphNodeInfo) may have
+    # more than one linker_input (each potentially with a different owner) but
+    # using only the first owner as a key is enough.
+    first_owner_to_depset = {}
+
     # Horrible I know. Perhaps Starlark team gives me a way to prune a tree.
     for i in range(2147483647):
-        if i == len(all_children):
+        if not len(all_children):
             break
 
-        node = all_children[i]
+        node = all_children[-1]
 
         must_add_children = False
 
@@ -85,6 +138,8 @@ def _separate_static_and_dynamic_link_libraries(
         # the cc_shared_library implementation owners.
         has_owners_seen = False
         has_owners_not_seen = False
+        linked_dynamically = False
+        linked_statically = False
         for owner in node.owners:
             # TODO(bazel-team): Do not convert Labels to string to save on
             # garbage string allocations.
@@ -99,18 +154,62 @@ def _separate_static_and_dynamic_link_libraries(
 
             if owner_str in can_be_linked_dynamically:
                 targets_to_be_linked_dynamically_set[owner_str] = True
+                linked_dynamically = True
             else:
                 targets_to_be_linked_statically_map[owner_str] = node.linkable_more_than_once
                 must_add_children = True
+                linked_statically = 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")
+            _programmatic_error()
+
+        if linked_dynamically and linked_statically:
+            error_owners_list = [str(owner) for owner in node.owners]
+
+            # Our granularity is target level. Unless there is a different
+            # unsupported custom implementation of this rule it should be
+            # impossible for two linker_inputs from the same target to be
+            # linked differently, one statically and the other dynamically.
+            _programmatic_error(
+                message = "Nodes with linker_inputs linked statically and dynamically:" +
+                          "\n{}".format("\n".join(error_owners_list)),
+            )
 
         if must_add_children:
-            all_children.extend(node.children)
-
-    return (targets_to_be_linked_statically_map, targets_to_be_linked_dynamically_set)
+            # The order in which we process the children matter. all_children
+            # is being used as a stack, we will process first the nodes at the
+            # top of the stack (last in the list). The children are the
+            # dependencies of the current node, in order to respect the order
+            # in which dependencies were listed in the deps attribute in the
+            # BUILD file we must reverse the list so that the first one listed
+            # in the BUILD file is processed first.
+            all_children.extend(reversed(node.children))
+        else:
+            if node.owners[0] not in first_owner_to_depset:
+                # We have 3 cases in this branch:
+                #   1. Node has no children
+                #   2. The children have been pruned because the node is linked dynamically
+                #   3. Node has children that have been processed
+                # For case 3 we add the children's depsets. For case 2 we add the dynamic
+                # dep labels for transitive dynamic deps.
+                transitive = []
+                if str(node.owners[0]) in targets_to_be_linked_statically_map:
+                    for child in node.children:
+                        transitive.append(first_owner_to_depset[child.owners[0]])
+                elif str(node.owners[0]) in transitive_dynamic_dep_labels:
+                    transitive.append(transitive_dynamic_dep_labels[str(node.owners[0])])
+
+                first_owner_to_depset[node.owners[0]] = depset(direct = node.owners, transitive = transitive, order = "topological")
+            all_children.pop()
+
+    topologically_sorted_labels = []
+    if direct_children:
+        transitive = []
+        for child in direct_children:
+            transitive.append(first_owner_to_depset[child.owners[0]])
+        topologically_sorted_labels = depset(transitive = transitive, order = "topological").to_list()
+
+    return (targets_to_be_linked_statically_map, targets_to_be_linked_dynamically_set, topologically_sorted_labels)
 
 def _create_linker_context(ctx, linker_inputs):
     return cc_common.create_linking_context(
@@ -129,7 +228,7 @@ def _merge_cc_shared_library_infos(ctx):
         dynamic_deps.append(dynamic_dep_entry)
         transitive_dynamic_deps.append(dep[CcSharedLibraryInfo].dynamic_deps)
 
-    return depset(direct = dynamic_deps, transitive = transitive_dynamic_deps)
+    return depset(direct = dynamic_deps, transitive = transitive_dynamic_deps, order = "topological")
 
 def _build_exports_map_from_only_dynamic_deps(merged_shared_library_infos):
     exports_map = {}
@@ -267,7 +366,6 @@ def _filter_inputs(
         deps,
         transitive_exports,
         link_once_static_libs_map):
-    linker_inputs = []
     curr_link_once_static_libs_set = {}
 
     graph_structure_aspect_nodes = []
@@ -291,7 +389,11 @@ def _filter_inputs(
 
     # The targets_to_be_linked_statically_map points to whether the target to
     # be linked statically can be linked more than once.
-    (targets_to_be_linked_statically_map, targets_to_be_linked_dynamically_set) = _separate_static_and_dynamic_link_libraries(
+    (
+        targets_to_be_linked_statically_map,
+        targets_to_be_linked_dynamically_set,
+        topologically_sorted_labels,
+    ) = _separate_static_and_dynamic_link_libraries(
         graph_structure_aspect_nodes,
         can_be_linked_dynamically,
     )
@@ -313,6 +415,11 @@ def _filter_inputs(
     precompiled_only_dynamic_libraries = []
     exports = {}
     linker_inputs_seen = {}
+    linker_inputs_count = 0
+    label_to_linker_inputs = {}
+
+    def _add_linker_input_to_dict(owner, linker_input):
+        label_to_linker_inputs.setdefault(owner, []).append(linker_input)
 
     # We use this dictionary to give an error if a target containing only
     # precompiled dynamic libraries is placed directly in roots. If such a
@@ -331,7 +438,8 @@ def _filter_inputs(
             # Link the library in this iteration dynamically,
             # transitive_exports contains the artifacts produced by a
             # cc_shared_library
-            linker_inputs.append(transitive_exports[owner])
+            _add_linker_input_to_dict(linker_input.owner, transitive_exports[owner])
+            linker_inputs_count += 1
         elif owner in targets_to_be_linked_statically_map:
             if owner in link_once_static_libs_map:
                 # We are building a dictionary that will allow us to give
@@ -357,7 +465,8 @@ def _filter_inputs(
                 if not len(static_libraries):
                     if owner in direct_deps_set:
                         dynamic_only_roots[owner] = True
-                    linker_inputs.append(linker_input)
+                    _add_linker_input_to_dict(linker_input.owner, linker_input)
+                    linker_inputs_count += 1
                     continue
             if len(static_libraries) and owner in dynamic_only_roots:
                 dynamic_only_roots.pop(owner)
@@ -377,7 +486,8 @@ def _filter_inputs(
             ):
                 exports[owner] = True
 
-            linker_inputs.append(linker_input_to_be_linked_statically)
+            _add_linker_input_to_dict(linker_input.owner, linker_input_to_be_linked_statically)
+            linker_inputs_count += 1
 
             if not targets_to_be_linked_statically_map[owner]:
                 curr_link_once_static_libs_set[owner] = True
@@ -390,8 +500,14 @@ def _filter_inputs(
             message += dynamic_only_root + "\n"
         fail(message)
 
+    sorted_linker_inputs = _sort_linker_inputs(
+        topologically_sorted_labels,
+        label_to_linker_inputs,
+        linker_inputs_count,
+    )
+
     _throw_linked_but_not_exported_errors(linked_statically_but_not_exported)
-    return (exports, linker_inputs, curr_link_once_static_libs_set.keys(), precompiled_only_dynamic_libraries)
+    return (exports, sorted_linker_inputs, curr_link_once_static_libs_set.keys(), precompiled_only_dynamic_libraries)
 
 def _throw_linked_but_not_exported_errors(error_libs_dict):
     if not error_libs_dict:
@@ -684,3 +800,4 @@ 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
+sort_linker_inputs = _sort_linker_inputs