From a3d3cec19fec410e20be1e649e5035ae889448a1 Mon Sep 17 00:00:00 2001 From: Nuri Amari Date: Thu, 13 Feb 2025 12:56:29 -0800 Subject: [PATCH] Stop using a filelist for linker invocations (attempt 2) Summary: This is a modified version of D68593087. We still remove the filelist, but keep the linker order exactly the same. We have too many tests implicitly relying on link order that break if we modify the link order, so although it is somewhat arbitrary keep it the same. Reviewed By: rmaz Differential Revision: D69531936 fbshipit-source-id: a6fbfa52054cd302bf4ef4c5b076b62790b16c90 --- prelude/cxx/cxx_executable.bzl | 3 - prelude/cxx/cxx_library.bzl | 3 - prelude/cxx/cxx_link_utility.bzl | 55 +++------ prelude/cxx/dist_lto/darwin_dist_lto.bzl | 1 - prelude/cxx/dist_lto/dist_lto.bzl | 1 - prelude/cxx/link.bzl | 10 -- prelude/linking/link_info.bzl | 136 ++++++----------------- prelude/rust/build.bzl | 2 - 8 files changed, 49 insertions(+), 162 deletions(-) diff --git a/prelude/cxx/cxx_executable.bzl b/prelude/cxx/cxx_executable.bzl index e5ecc45932e41..66ac2049f4b06 100644 --- a/prelude/cxx/cxx_executable.bzl +++ b/prelude/cxx/cxx_executable.bzl @@ -784,9 +784,6 @@ def cxx_executable(ctx: AnalysisContext, impl_params: CxxRuleConstructorParams, sub_targets["linker.argsfile"] = [DefaultInfo( default_output = binary.linker_argsfile, )] - sub_targets["linker.filelist"] = [DefaultInfo( - default_outputs = filter(None, [binary.linker_filelist]), - )] link_cmd_debug_output = make_link_command_debug_output(binary) if link_cmd_debug_output != None: diff --git a/prelude/cxx/cxx_library.bzl b/prelude/cxx/cxx_library.bzl index ed5bb9ca0b9ea..29ac418ee9c0b 100644 --- a/prelude/cxx/cxx_library.bzl +++ b/prelude/cxx/cxx_library.bzl @@ -1336,9 +1336,6 @@ def _form_library_outputs( "linker.command": [DefaultInfo( default_outputs = filter(None, [link_cmd_debug_output_file]), )], - "linker.filelist": [DefaultInfo( - default_outputs = filter(None, [shlib.linker_filelist]), - )], "unstripped": [DefaultInfo( default_output = unstripped, )], diff --git a/prelude/cxx/cxx_link_utility.bzl b/prelude/cxx/cxx_link_utility.bzl index 569a4c491b8f7..5820cf632ecc5 100644 --- a/prelude/cxx/cxx_link_utility.bzl +++ b/prelude/cxx/cxx_link_utility.bzl @@ -20,8 +20,8 @@ load( "LinkArgs", "LinkOrdering", # @unused Used as a type "unpack_link_args", - "unpack_link_args_excluding_filelist", - "unpack_link_args_filelist", + "unpack_link_args_excluding_object_files_and_lazy_archives", + "unpack_link_args_object_files_and_lazy_archives_only", ) load("@prelude//linking:lto.bzl", "LtoMode") load( @@ -69,12 +69,6 @@ LinkArgsOutput = record( link_args = ArgLike, hidden = list[typing.Any], pdb_artifact = Artifact | None, - # The filelist artifact which contains the list of all object files. - # Only present for Darwin linkers. Note that object files referenced - # _inside_ the filelist are _not_ part of the `hidden` field above. - # That's by design - we do not want to materialise _all_ object files - # to inspect the filelist. Intended to be used for debugging. - filelist = Artifact | None, ) def get_extra_darwin_linker_flags() -> cmd_args: @@ -88,7 +82,6 @@ def make_link_args( actions: AnalysisActions, cxx_toolchain_info: CxxToolchainInfo, links: list[LinkArgs], - suffix = None, output_short_path: [str, None] = None, link_ordering: [LinkOrdering, None] = None) -> LinkArgsOutput: """ @@ -96,7 +89,6 @@ def make_link_args( args to work when passed to a linker, and optionally an artifact where DWO outputs will be written to. """ - suffix = "" if suffix == None else "-" + suffix args = cmd_args() hidden = [] @@ -136,44 +128,23 @@ def make_link_args( pdb_artifact = actions.declare_output(pdb_filename) hidden.append(pdb_artifact.as_output()) - filelists = None if linker_type == LinkerType("darwin"): - filelists = filter(None, [unpack_link_args_filelist(link) for link in links]) - hidden.extend(filelists) - - for link in links: - if filelists: - # If we are using a filelist, only add argument that aren't already in the - # filelist. This is to avoid duplicate inputs in the link command. - args.add( - unpack_link_args_excluding_filelist( - link, - link_ordering = link_ordering, - link_metadata_flag = linker_info.link_metadata_flag, - ), - ) - else: - args.add( - unpack_link_args( - link, - link_ordering = link_ordering, - link_metadata_flag = linker_info.link_metadata_flag, - ), - ) - - # On Darwin, filelist args _must_ come last as the order can affect symbol - # resolution and result in binary size increases. - filelist_file = None - if filelists: - path = actions.write("filelist%s.txt" % suffix, filelists) - args.add(cmd_args(["-Xlinker", "-filelist", "-Xlinker", path])) - filelist_file = path + # We order inputs partially for binary size, and partially for historical + # reasons. It is important that lazy object file (non link-whole archives) + # be listed last on the command line so they are not unnecessarily loaded. + # Some applications implicitly rely upon this ordering, so it is difficult + # to change. Note link_ordering configuration is not respected. The + # link_metadata_flag is also ignored as that is a flag to the GNU linker wrapper, + # which does not apply to Darwin. + args.add(filter(None, [unpack_link_args_excluding_object_files_and_lazy_archives(link) for link in links])) + args.add(filter(None, [unpack_link_args_object_files_and_lazy_archives_only(link) for link in links])) + else: + args.add(filter(None, [unpack_link_args(link, link_ordering = link_ordering, link_metadata_flag = linker_info.link_metadata_flag) for link in links])) return LinkArgsOutput( link_args = args, hidden = [args] + hidden, pdb_artifact = pdb_artifact, - filelist = filelist_file, ) def shared_libs_symlink_tree_name(output: Artifact) -> str: diff --git a/prelude/cxx/dist_lto/darwin_dist_lto.bzl b/prelude/cxx/dist_lto/darwin_dist_lto.bzl index 7f59c20ca5b4f..762ec3dcfdce8 100644 --- a/prelude/cxx/dist_lto/darwin_dist_lto.bzl +++ b/prelude/cxx/dist_lto/darwin_dist_lto.bzl @@ -699,7 +699,6 @@ def cxx_darwin_dist_link( dwp = None, external_debug_info = external_debug_info, linker_argsfile = linker_argsfile_out, - linker_filelist = None, # DistLTO doesn't use filelists linker_command = None, # There is no notion of a single linker command for DistLTO index_argsfile = index_argsfile_out, dist_thin_lto_codegen_argsfile = opt_flags_for_debugging_argsfile, diff --git a/prelude/cxx/dist_lto/dist_lto.bzl b/prelude/cxx/dist_lto/dist_lto.bzl index 4c053c333d37f..e891170051315 100644 --- a/prelude/cxx/dist_lto/dist_lto.bzl +++ b/prelude/cxx/dist_lto/dist_lto.bzl @@ -679,7 +679,6 @@ def cxx_gnu_dist_link( dwp = dwp_output, external_debug_info = external_debug_info, linker_argsfile = linker_argsfile_out, - linker_filelist = None, # DistLTO doesn't use filelists linker_command = None, # There is no notion of a single linker command for DistLTO index_argsfile = index_argsfile_out, dist_thin_lto_codegen_argsfile = None, # Only Darwin builds provide is argsfile diff --git a/prelude/cxx/link.bzl b/prelude/cxx/link.bzl index d6f789642ec85..1da4222b7fd25 100644 --- a/prelude/cxx/link.bzl +++ b/prelude/cxx/link.bzl @@ -212,20 +212,11 @@ def cxx_link_into( split_debug_output = split_debug_lto_info.output expect(not generates_split_debug(cxx_toolchain_info) or split_debug_output != None) - link_args_suffix = None - if opts.identifier: - link_args_suffix = opts.identifier - if opts.category_suffix: - if link_args_suffix: - link_args_suffix += "-" + opts.category_suffix - else: - link_args_suffix = opts.category_suffix link_args_output = make_link_args( ctx, ctx.actions, cxx_toolchain_info, links_with_linker_map, - suffix = link_args_suffix, output_short_path = output.short_path, link_ordering = value_or( opts.link_ordering, @@ -370,7 +361,6 @@ def cxx_link_into( dwp = dwp_artifact, external_debug_info = external_debug_info, linker_argsfile = argfile, - linker_filelist = link_args_output.filelist, linker_command = command, import_library = opts.import_library, pdb = link_args_output.pdb_artifact, diff --git a/prelude/linking/link_info.bzl b/prelude/linking/link_info.bzl index 08c8c533cf869..4c286cab7a59f 100644 --- a/prelude/linking/link_info.bzl +++ b/prelude/linking/link_info.bzl @@ -241,24 +241,8 @@ def wrap_link_info( metadata = inner.metadata, ) -# Returns true if the command line argument representation of this linkable, -# could be passed within a filelist. -def _is_linkable_included_in_filelist(linkable: LinkableTypes) -> bool: - if isinstance(linkable, ArchiveLinkable): - # Link whole archives don't appear in the filelist, but are passed directly to the linker - # with a -force-load (MachO) or -whole-archive (ELF) flag. Regular archives do appear in the filelist. - return not linkable.link_whole - elif isinstance(linkable, SharedLibLinkable) or \ - isinstance(linkable, FrameworksLinkable) or \ - isinstance(linkable, SwiftRuntimeLinkable) or \ - isinstance(linkable, SwiftmoduleLinkable): - # These are all passed directly via various command line flags, not via a filelist. - return False - elif isinstance(linkable, ObjectsLinkable): - # Object files always appear in the filelist. - return True - else: - fail("Encountered unhandled filelist-like linkable {}".format(str(linkable))) +def _is_linkable_comprised_of_object_files_or_a_lazy_archive(linkable: LinkableTypes) -> bool: + return isinstance(linkable, ObjectsLinkable) or (isinstance(linkable, ArchiveLinkable) and not linkable.link_whole) # Adds appropriate args representing `linkable` to `args` def append_linkable_args(args: cmd_args, linkable: LinkableTypes): @@ -300,23 +284,25 @@ def append_linkable_args(args: cmd_args, linkable: LinkableTypes): LinkInfoArgumentFilter = enum( "all", - "filelist_only", - "excluding_filelist", + "object_files_and_lazy_archives_only", + "exclude_object_files_and_lazy_archives", ) def link_info_to_args(value: LinkInfo, argument_type_filter: LinkInfoArgumentFilter = LinkInfoArgumentFilter("all")) -> cmd_args: result = cmd_args() - do_pre_post_flags = argument_type_filter == LinkInfoArgumentFilter("all") or argument_type_filter == LinkInfoArgumentFilter("excluding_filelist") + do_pre_post_flags = argument_type_filter == LinkInfoArgumentFilter("all") or argument_type_filter == LinkInfoArgumentFilter("exclude_object_files_and_lazy_archives") if do_pre_post_flags: result.add(value.pre_flags) for linkable in value.linkables: - if (argument_type_filter == LinkInfoArgumentFilter("all")) or ( - argument_type_filter == LinkInfoArgumentFilter("filelist_only") and _is_linkable_included_in_filelist(linkable) - ) or ( - argument_type_filter == LinkInfoArgumentFilter("excluding_filelist") and not _is_linkable_included_in_filelist(linkable) - ): + if argument_type_filter == LinkInfoArgumentFilter("all"): + append_linkable_args(result, linkable) + + elif argument_type_filter == LinkInfoArgumentFilter("object_files_and_lazy_archives_only") and _is_linkable_comprised_of_object_files_or_a_lazy_archive(linkable): + append_linkable_args(result, linkable) + + elif argument_type_filter == LinkInfoArgumentFilter("exclude_object_files_and_lazy_archives") and not _is_linkable_comprised_of_object_files_or_a_lazy_archive(linkable): append_linkable_args(result, linkable) if do_pre_post_flags: @@ -346,21 +332,11 @@ def _link_info_stripped_link_args(infos: LinkInfos): info = infos.stripped or infos.default return link_info_to_args(info, argument_type_filter = LinkInfoArgumentFilter("all")) -def _link_info_default_filelist(infos: LinkInfos): - info = infos.default - return link_info_to_args(info, argument_type_filter = LinkInfoArgumentFilter("filelist_only")) +def _link_info_object_files_and_lazy_archives_only_args(infos: LinkInfos): + return link_info_to_args(infos.default, argument_type_filter = LinkInfoArgumentFilter("object_files_and_lazy_archives_only")) -def _link_info_stripped_filelist(infos: LinkInfos): - info = infos.stripped or infos.default - return link_info_to_args(info, argument_type_filter = LinkInfoArgumentFilter("filelist_only")) - -def _link_info_default_excluding_filelist_args(infos: LinkInfos): - info = infos.default - return link_info_to_args(info, argument_type_filter = LinkInfoArgumentFilter("excluding_filelist")) - -def _link_info_stripped_excluding_filelist_args(infos: LinkInfos): - info = infos.stripped or infos.default - return link_info_to_args(info, argument_type_filter = LinkInfoArgumentFilter("excluding_filelist")) +def _link_info_excluding_object_files_and_lazy_archives_args(infos: LinkInfos): + return link_info_to_args(infos.default, argument_type_filter = LinkInfoArgumentFilter("exclude_object_files_and_lazy_archives")) def link_info_to_metadata_args(info: LinkInfo, args: cmd_args | None = None) -> ArgLike: if args == None: @@ -373,34 +349,14 @@ def _link_info_metadata_args(infos: LinkInfos): info = infos.stripped or infos.default return link_info_to_metadata_args(info) -def _link_info_has_default_filelist(children: list[bool], infos: [LinkInfos, None]) -> bool: - if infos: - info = infos.default - if len(link_info_to_args(info, argument_type_filter = LinkInfoArgumentFilter("filelist_only")).inputs): - return True - return any(children) - -def _link_info_has_stripped_filelist(children: list[bool], infos: [LinkInfos, None]) -> bool: - if infos: - info = infos.stripped or infos.default - if len(link_info_to_args(info, argument_type_filter = LinkInfoArgumentFilter("filelist_only")).inputs): - return True - return any(children) - # TransitiveSet of LinkInfos. LinkInfosTSet = transitive_set( args_projections = { "default": _link_info_default_args, - "default_excluding_filelist": _link_info_default_excluding_filelist_args, - "default_filelist": _link_info_default_filelist, + "exclude_object_files_and_lazy_archives": _link_info_excluding_object_files_and_lazy_archives_args, "metadata": _link_info_metadata_args, + "object_files_and_lazy_archives_only": _link_info_object_files_and_lazy_archives_only_args, "stripped": _link_info_stripped_link_args, - "stripped_excluding_filelist": _link_info_stripped_excluding_filelist_args, - "stripped_filelist": _link_info_stripped_filelist, - }, - reductions = { - "has_default_filelist": _link_info_has_default_filelist, - "has_stripped_filelist": _link_info_has_stripped_filelist, }, ) @@ -447,11 +403,6 @@ LinkedObject = record( # This argsfile is generated in the `cxx_link` step and contains a list of arguments # passed to the linker. It is being exposed as a sub-target for debugging purposes. linker_argsfile = field(Artifact | None, None), - # The filelist is generated in the `cxx_link` step and contains a list of - # object files (static libs or plain object files) passed to the linker. - # It is being exposed for debugging purposes. Only present when a Darwin - # linker is used. - linker_filelist = field(Artifact | None, None), # The linker command as generated by `cxx_link`. Exposed for debugging purposes only. # Not present for DistLTO scenarios. linker_command = field([cmd_args, None], None), @@ -732,51 +683,39 @@ def unpack_link_args( fail("Unpacked invalid empty link args") -def unpack_link_args_filelist(args: LinkArgs) -> [ArgLike, None]: +def unpack_link_args_excluding_object_files_and_lazy_archives(args: LinkArgs) -> [ArgLike, None]: if args.tset != None: - tset = args.tset.infos - stripped = args.tset.prefer_stripped - if not tset.reduce("has_stripped_filelist" if stripped else "has_default_filelist"): - return None - return tset.project_as_args("stripped_filelist" if stripped else "default_filelist") + if args.tset.prefer_stripped: + fail("Preferring stripped link infos is not supported by this function.") + + return args.tset.infos.project_as_args("exclude_object_files_and_lazy_archives") if args.infos != None: result_args = cmd_args() for info in args.infos: - result_args.add(link_info_to_args(info, argument_type_filter = LinkInfoArgumentFilter("filelist_only"))) - - if not len(result_args.inputs): - return None - + result_args.add(link_info_to_args(info, argument_type_filter = LinkInfoArgumentFilter("exclude_object_files_and_lazy_archives"))) return result_args if args.flags != None: - return None + return args.flags fail("Unpacked invalid empty link args") -def unpack_link_args_excluding_filelist( - args: LinkArgs, - link_ordering: [LinkOrdering, None] = None, - link_metadata_flag: str | None = None) -> ArgLike: - cmd = link_args_metadata_with_flag(args, link_metadata_flag) +def unpack_link_args_object_files_and_lazy_archives_only(args: LinkArgs) -> [ArgLike, None]: if args.tset != None: - ordering = link_ordering.value if link_ordering else "preorder" - - tset = args.tset.infos if args.tset.prefer_stripped: - cmd.add(tset.project_as_args("stripped_excluding_filelist", ordering = ordering)) - else: - cmd.add(tset.project_as_args("default_excluding_filelist", ordering = ordering)) - return cmd + fail("Preferring stripped link infos is not supported by this function.") + + return args.tset.infos.project_as_args("object_files_and_lazy_archives_only") if args.infos != None: - cmd.add([link_info_to_args(info, LinkInfoArgumentFilter("excluding_filelist")) for info in args.infos]) - return cmd + result_args = cmd_args() + for info in args.infos: + result_args.add(link_info_to_args(info, argument_type_filter = LinkInfoArgumentFilter("object_files_and_lazy_archives_only"))) + return result_args if args.flags != None: - cmd.add(args.flags) - return cmd + return None fail("Unpacked invalid empty link args") @@ -1015,7 +954,6 @@ LinkCommandDebugOutput = record( filename = str, command = ArgLike, argsfile = Artifact, - filelist = Artifact | None, dist_thin_lto_codegen_argsfile = Artifact | None, dist_thin_lto_index_argsfile = Artifact | None, ) @@ -1040,7 +978,6 @@ def make_link_command_debug_output(linked_object: LinkedObject) -> [LinkCommandD filename = linked_object.output.short_path, command = linked_object.linker_command, argsfile = linked_object.linker_argsfile, - filelist = linked_object.linker_filelist, dist_thin_lto_index_argsfile = linked_object.dist_thin_lto_index_argsfile, dist_thin_lto_codegen_argsfile = linked_object.dist_thin_lto_codegen_argsfile, ) @@ -1051,7 +988,6 @@ def make_link_command_debug_output(linked_object: LinkedObject) -> [LinkCommandD # # For local thin-LTO: # - linker argfile -# - linker filelist (if present - only applicable to Darwin linkers) # # For distributed thin-LTO: # - thin-link argsfile (without inputs just flags) @@ -1076,8 +1012,8 @@ def make_link_command_debug_output_json_info(ctx: AnalysisContext, debug_outputs "filename": debug_output.filename, }) - # Ensure all argsfile and filelists get materialized, as those are needed for debugging - associated_artifacts.extend(filter(None, [debug_output.argsfile, debug_output.filelist])) + # Ensure all argsfile get materialized, as those are needed for debugging + associated_artifacts.extend(filter(None, [debug_output.argsfile])) # Explicitly drop all inputs by using `with_inputs = False`, we don't want # to materialize all inputs to the link actions (which includes all object files diff --git a/prelude/rust/build.bzl b/prelude/rust/build.bzl index 5b5e0550ec04d..12a9f7b6d2e2f 100644 --- a/prelude/rust/build.bzl +++ b/prelude/rust/build.bzl @@ -367,7 +367,6 @@ def generate_rustdoc_test( params.dep_link_strategy, ), ], - "{}-{}".format(common_args.subdir, common_args.tempfile), ) link_args_output.link_args.add(ctx.attrs.doc_linker_flags or []) @@ -604,7 +603,6 @@ def rust_compile( LinkArgs(flags = extra_link_args), inherited_link_args, ], - "{}-{}".format(subdir, tempfile), output_short_path = emit_op.output.short_path, ) linker_argsfile, _ = ctx.actions.write(