Fix force-static heuristic (take 2)

Summary:
Libraries will mark themselves as `preferred_linkage = "static"` to
ensure they always get linked into their dependents, which omnibus
honors in it's implementation: if a root node depends on a force-static
body node, that body node is statically linked into the root node; if
no other body node depends on that force-static node, then it will not
be added to the link line of the body.

Respect this heuristic in link groups, but always statically linking a force-
static node into it's dependent, even if it crosses a link group.

Reviewed By: christycylee

Differential Revision: D43278287

fbshipit-source-id: d59b7303bd5c26cc6b041db02d4ec4c725ec38e6

prelude/apple/apple_binary.bzl

@@ -45,6 +45,8 @@ def apple_binary_impl(ctx: "context") -> ["provider"]:
         link_postprocessor = get_apple_link_postprocessor(ctx),
         link_group_info = get_link_group_info(ctx),
         prefer_stripped_objects = ctx.attrs.prefer_stripped_objects,
+        # Some apple rules rely on `static` libs *not* following dependents.
+        link_groups_force_static_follows_dependents = False,
     )
     (cxx_output, _comp_db_info, xcode_data_info) = cxx_executable(ctx, constructor_params)
 

prelude/apple/apple_library.bzl

@@ -166,6 +166,8 @@ def apple_library_rule_constructor_params_and_swift_providers(ctx: "context", pa
         generate_sub_targets = params.generate_sub_targets,
         generate_providers = params.generate_providers,
         link_postprocessor = get_apple_link_postprocessor(ctx),
+        # Some apple rules rely on `static` libs *not* following dependents.
+        link_groups_force_static_follows_dependents = False,
     ), swift_providers, exported_pre
 
 def _filter_swift_srcs(ctx: "context") -> (["CxxSrcWithFlags"], ["CxxSrcWithFlags"]):

prelude/cxx/cxx_executable.bzl

@@ -95,6 +95,7 @@ load(
     ":link_groups.bzl",
     "LINK_GROUP_MAP_DATABASE_SUB_TARGET",
     "create_link_groups",
+    "find_relevant_roots",
     "get_filtered_labels_to_links_map",
     "get_filtered_links",
     "get_filtered_targets",
@@ -267,10 +268,18 @@ def cxx_executable(ctx: "context", impl_params: CxxRuleConstructorParams.type, i
                 for name, lib in link_group_libs.items()
             },
             link_style = link_style,
-            deps = [d.linkable_graph.nodes.value.label for d in link_deps],
-            other_roots = link_group_other_roots,
+            roots = (
+                [d.linkable_graph.nodes.value.label for d in link_deps] +
+                find_relevant_roots(
+                    link_group = link_group,
+                    linkable_graph_node_map = linkable_graph_node_map,
+                    link_group_mappings = link_group_mappings,
+                    roots = link_group_other_roots,
+                )
+            ),
             is_executable_link = True,
             prefer_stripped = impl_params.prefer_stripped_objects,
+            force_static_follows_dependents = impl_params.link_groups_force_static_follows_dependents,
         )
 
         if is_cxx_test and link_group != None:
@@ -282,7 +291,7 @@ def cxx_executable(ctx: "context", impl_params: CxxRuleConstructorParams.type, i
                 link_group_mappings,
                 link_group_preferred_linkage,
                 link_style,
-                deps = [d.linkable_graph.nodes.value.label for d in link_deps],
+                roots = [d.linkable_graph.nodes.value.label for d in link_deps],
                 is_executable_link = True,
                 prefer_stripped = impl_params.prefer_stripped_objects,
             )

prelude/cxx/cxx_library.bzl

@@ -338,6 +338,7 @@ def cxx_library_parameterized(ctx: "context", impl_params: "CxxRuleConstructorPa
         non_exported_deps,
         impl_params.force_link_group_linking,
         frameworks_linkable,
+        force_static_follows_dependents = impl_params.link_groups_force_static_follows_dependents,
     )
     if impl_params.generate_sub_targets.link_group_map and link_group_map:
         sub_targets[LINK_GROUP_MAP_DATABASE_SUB_TARGET] = [link_group_map]
@@ -815,7 +816,8 @@ def _get_shared_library_links(
         exported_deps: ["dependency"],
         non_exported_deps: ["dependency"],
         force_link_group_linking,
-        frameworks_linkable: ["FrameworksLinkable", None]) -> ("LinkArgs", [DefaultInfo.type, None]):
+        frameworks_linkable: ["FrameworksLinkable", None],
+        force_static_follows_dependents: bool.type = True) -> ("LinkArgs", [DefaultInfo.type, None]):
     """
     TODO(T110378116): Omnibus linking always creates shared libraries by linking
     against shared dependencies. This is not true for link groups and possibly
@@ -862,8 +864,9 @@ def _get_shared_library_links(
             for name, lib in link_group_libs.items()
         },
         link_style = link_style,
-        deps = linkable_deps(non_exported_deps + exported_deps),
+        roots = linkable_deps(non_exported_deps + exported_deps),
         prefer_stripped = prefer_stripped,
+        force_static_follows_dependents = force_static_follows_dependents,
     )
     filtered_links = get_filtered_links(filtered_labels_to_links_map)
     filtered_targets = get_filtered_targets(filtered_labels_to_links_map)

prelude/cxx/cxx_types.bzl

@@ -168,4 +168,7 @@ CxxRuleConstructorParams = record(
     prefer_stripped_objects = field(bool.type, False),
     # The category suffix to use for executables actions (e.g. linking).
     exe_category_suffix = field(str.type, "executable"),
+    # Whether link groups liking should make `preferred_linkage = "static"` libs
+    # "follow" their dependents across link group boundaries.
+    link_groups_force_static_follows_dependents = field(bool.type, True),
 )

prelude/cxx/link_groups.bzl

@@ -221,13 +221,11 @@ def get_filtered_labels_to_links_map(
         link_group_mappings: [{"label": str.type}, None],
         link_group_preferred_linkage: {"label": Linkage.type},
         link_style: LinkStyle.type,
-        deps: ["label"],
-        # Additional roots to search for nodes in the target link group (e.g.
-        # dlopen-enabled libs or outlined native Python extensions).
-        other_roots: ["label"] = [],
+        roots: ["label"],
         link_group_libs: {str.type: (["label", None], LinkInfos.type)} = {},
         prefer_stripped: bool.type = False,
-        is_executable_link: bool.type = False) -> {"label": LinkGroupLinkInfo.type}:
+        is_executable_link: bool.type = False,
+        force_static_follows_dependents: bool.type = True) -> {"label": LinkGroupLinkInfo.type}:
     """
     Given a linkable graph, link style and link group mappings, finds all links
     to consider for linking traversing the graph as necessary and then
@@ -254,28 +252,10 @@ def get_filtered_labels_to_links_map(
 
         return node_linkables
 
-    # Walk through roots looking for the first node which maps to the current
-    # link group.
-    def collect_and_traverse_roots(roots, node_target):
-        node_link_group = link_group_mappings.get(node_target)
-        if node_link_group == MATCH_ALL_LABEL:
-            roots.append(node_target)
-            return []
-        if node_link_group == link_group:
-            roots.append(node_target)
-            return []
-        return get_traversed_deps(node_target)
-
     # Get all potential linkable targets
-    other_deps = []
-    breadth_first_traversal_by(
-        linkable_graph_node_map,
-        other_roots,
-        partial(collect_and_traverse_roots, other_deps),
-    )
     linkables = breadth_first_traversal_by(
         linkable_graph_node_map,
-        deps + other_deps,
+        roots,
         get_traversed_deps,
     )
 
@@ -349,7 +329,10 @@ def get_filtered_labels_to_links_map(
         else:  # static or static_pic
             target_link_group = link_group_mappings.get(target)
 
-            if not target_link_group and not link_group:
+            # Always add force-static libs to the link.
+            if force_static_follows_dependents and node.preferred_linkage == Linkage("static"):
+                add_link(target, actual_link_style)
+            elif not target_link_group and not link_group:
                 # Ungrouped linkable targets belong to the unlabeled executable
                 add_link(target, actual_link_style)
             elif is_executable_link and target_link_group == NO_MATCH_LABEL:
@@ -467,6 +450,36 @@ def make_link_group_info(groups: [Group.type], mappings: {"label": str.type}) ->
         mappings = mappings,
     )
 
+def find_relevant_roots(
+        link_group: [str.type, None] = None,
+        linkable_graph_node_map: {"label": LinkableNode.type} = {},
+        link_group_mappings: {"label": str.type} = {},
+        roots: ["label"] = []):
+    # Walk through roots looking for the first node which maps to the current
+    # link group.
+    def collect_and_traverse_roots(roots, node_target):
+        node = linkable_graph_node_map.get(node_target)
+        if node.preferred_linkage == Linkage("static"):
+            return node.deps + node.exported_deps
+        node_link_group = link_group_mappings.get(node_target)
+        if node_link_group == MATCH_ALL_LABEL:
+            roots.append(node_target)
+            return []
+        if node_link_group == link_group:
+            roots.append(node_target)
+            return []
+        return node.deps + node.exported_deps
+
+    relevant_roots = []
+
+    breadth_first_traversal_by(
+        linkable_graph_node_map,
+        roots,
+        partial(collect_and_traverse_roots, relevant_roots),
+    )
+
+    return relevant_roots
+
 def _create_link_group(
         ctx: "context",
         spec: LinkGroupLibSpec.type,
@@ -497,7 +510,7 @@ def _create_link_group(
 
     # Get roots to begin the linkable search.
     # TODO(agallagher): We should use the groups "public" nodes as the roots.
-    deps = []
+    roots = []
     has_empty_root = False
     if spec.root != None:
         # If there's a linkable root attached to the spec, use that to guide
@@ -507,17 +520,28 @@ def _create_link_group(
             spec.root.link_infos,
             prefer_stripped = prefer_stripped_objects,
         ))
-        deps.extend(spec.root.deps)
+        roots.extend(spec.root.deps)
     else:
         for mapping in spec.group.mappings:
             # If there's no explicit root, this means we need to search the entire
             # graph to find candidate nodes.
             if mapping.root == None:
                 has_empty_root = True
             else:
-                deps.append(mapping.root.label)
+                roots.append(mapping.root.label)
+
+        # If this link group has an empty mapping, we need to search everything
+        # -- even the additional roots -- to find potential nodes in the link
+        # group.
         if has_empty_root:
-            deps.extend(executable_deps)
+            roots.extend(
+                find_relevant_roots(
+                    link_group = spec.group.name,
+                    linkable_graph_node_map = linkable_graph_node_map,
+                    link_group_mappings = link_group_mappings,
+                    roots = executable_deps + other_roots,
+                ),
+            )
 
     # Add roots...
     filtered_labels_to_links_map = get_filtered_labels_to_links_map(
@@ -527,11 +551,7 @@ def _create_link_group(
         link_group_preferred_linkage,
         link_group_libs = link_group_libs,
         link_style = link_style,
-        # If this link group has an empty mapping, we need to search everything
-        # -- even the additional roots -- to find potential nodes in the link
-        # group.
-        other_roots = other_roots if has_empty_root else [],
-        deps = deps,
+        roots = roots,
         is_executable_link = False,
         prefer_stripped = prefer_stripped_objects,
     )

prelude/python/cxx_python_extension.bzl

@@ -37,6 +37,8 @@ load(
 )
 load(
     "@prelude//linking:link_info.bzl",
+    "LinkInfo",
+    "LinkInfos",
     "LinkStyle",
     "Linkage",
     "create_merged_link_info",
@@ -163,6 +165,11 @@ def cxx_python_extension_impl(ctx: "context") -> ["provider"]:
 
         python_module_names[base_module.replace("/", ".") + module_name] = pyinit_symbol
 
+    # Add a dummy shared link info to avoid marking this node as preferred
+    # linkage being "static", which has a special meaning for various link
+    # strategies
+    link_infos[LinkStyle("shared")] = LinkInfos(default = LinkInfo())
+
     # Create linkable providers for the extension.
     link_deps = linkables(cxx_deps)
     linkable_providers = LinkableProviders(
@@ -174,7 +181,7 @@ def cxx_python_extension_impl(ctx: "context") -> ["provider"]:
                 linkable_node = create_linkable_node(
                     ctx = ctx,
                     deps = cxx_deps,
-                    preferred_linkage = Linkage("static"),
+                    preferred_linkage = Linkage("any"),
                     link_infos = link_infos,
                 ),
             ),