From 47b63cd508f993d9fab2acfbf0dcf86cdc8c5335 Mon Sep 17 00:00:00 2001 From: Daniel Bertalan Date: Thu, 18 Jul 2024 16:26:32 +0200 Subject: [PATCH] [lld-macho] Save all thin archive members in repro tarball (#97169) Previously, we only saved those members of thin archives into a repro file that were actually used during linking. However, -ObjC handling requires us to inspect all members, even those that don't end up being loaded. We weren't handling missing members correctly and crashed with an "unhandled `Error`" failure in LLVM_ENABLE_ABI_BREAKING_CHECKS builds. To fix this, we now eagerly load all object files and warn when encountering missing members (in the instances where it wasn't a hard error before). To avoid having to patch out the checks when dealing with older repro files, the `--no-warn-thin-archive-missing-members` flag is added as an escape hatch. --- lld/MachO/Config.h | 1 + lld/MachO/Driver.cpp | 43 ++++++++++++++++++-- lld/MachO/InputFiles.cpp | 4 -- lld/MachO/Options.td | 3 ++ lld/test/MachO/reproduce-thin-archive-objc.s | 25 ++++++++++++ lld/test/MachO/reproduce-thin-archives.s | 19 +++++++-- 6 files changed, 83 insertions(+), 12 deletions(-) create mode 100644 lld/test/MachO/reproduce-thin-archive-objc.s diff --git a/lld/MachO/Config.h b/lld/MachO/Config.h index 5c354e0fe882..e79812b16ec1 100644 --- a/lld/MachO/Config.h +++ b/lld/MachO/Config.h @@ -212,6 +212,7 @@ struct Configuration { bool csProfileGenerate = false; llvm::StringRef csProfilePath; bool pgoWarnMismatch; + bool warnThinArchiveMissingMembers; bool callGraphProfileSort = false; llvm::StringRef printSymbolOrder; diff --git a/lld/MachO/Driver.cpp b/lld/MachO/Driver.cpp index ffb3feae25ca..dc9d635b48ec 100644 --- a/lld/MachO/Driver.cpp +++ b/lld/MachO/Driver.cpp @@ -270,6 +270,20 @@ struct ArchiveFileInfo { static DenseMap loadedArchives; +static void saveThinArchiveToRepro(ArchiveFile const *file) { + assert(tar && file->getArchive().isThin()); + + Error e = Error::success(); + for (const object::Archive::Child &c : file->getArchive().children(e)) { + MemoryBufferRef mb = CHECK(c.getMemoryBufferRef(), + toString(file) + ": failed to get buffer"); + tar->append(relativeToRoot(CHECK(c.getFullName(), file)), mb.getBuffer()); + } + if (e) + error(toString(file) + + ": Archive::children failed: " + toString(std::move(e))); +} + static InputFile *addFile(StringRef path, LoadType loadType, bool isLazy = false, bool isExplicit = true, bool isBundleLoader = false, @@ -301,6 +315,9 @@ static InputFile *addFile(StringRef path, LoadType loadType, if (!archive->isEmpty() && !archive->hasSymbolTable()) error(path + ": archive has no index; run ranlib to add one"); file = make(std::move(archive), isForceHidden); + + if (tar && file->getArchive().isThin()) + saveThinArchiveToRepro(file); } else { file = entry->second.file; // Command-line loads take precedence. If file is previously loaded via @@ -330,9 +347,13 @@ static InputFile *addFile(StringRef path, LoadType loadType, reason = "-all_load"; break; } - if (Error e = file->fetch(c, reason)) - error(toString(file) + ": " + reason + - " failed to load archive member: " + toString(std::move(e))); + if (Error e = file->fetch(c, reason)) { + if (config->warnThinArchiveMissingMembers) + warn(toString(file) + ": " + reason + + " failed to load archive member: " + toString(std::move(e))); + else + llvm::consumeError(std::move(e)); + } } if (e) error(toString(file) + @@ -349,7 +370,18 @@ static InputFile *addFile(StringRef path, LoadType loadType, Error e = Error::success(); for (const object::Archive::Child &c : file->getArchive().children(e)) { Expected mb = c.getMemoryBufferRef(); - if (!mb || !hasObjCSection(*mb)) + if (!mb) { + // We used to create broken repro tarballs that only included those + // object files from thin archives that ended up being used. + if (config->warnThinArchiveMissingMembers) + warn(toString(file) + ": -ObjC failed to open archive member: " + + toString(mb.takeError())); + else + llvm::consumeError(mb.takeError()); + continue; + } + + if (!hasObjCSection(*mb)) continue; if (Error e = file->fetch(c, "-ObjC")) error(toString(file) + ": -ObjC failed to load archive member: " + @@ -1699,6 +1731,9 @@ bool link(ArrayRef argsArr, llvm::raw_ostream &stdoutOS, config->csProfilePath = args.getLastArgValue(OPT_cs_profile_path); config->pgoWarnMismatch = args.hasFlag(OPT_pgo_warn_mismatch, OPT_no_pgo_warn_mismatch, true); + config->warnThinArchiveMissingMembers = + args.hasFlag(OPT_warn_thin_archive_missing_members, + OPT_no_warn_thin_archive_missing_members, true); config->generateUuid = !args.hasArg(OPT_no_uuid); for (const Arg *arg : args.filtered(OPT_alias)) { diff --git a/lld/MachO/InputFiles.cpp b/lld/MachO/InputFiles.cpp index b40a812f30bd..3086c9cc4729 100644 --- a/lld/MachO/InputFiles.cpp +++ b/lld/MachO/InputFiles.cpp @@ -2200,10 +2200,6 @@ Error ArchiveFile::fetch(const object::Archive::Child &c, StringRef reason) { if (!mb) return mb.takeError(); - // Thin archives refer to .o files, so --reproduce needs the .o files too. - if (tar && c.getParent()->isThin()) - tar->append(relativeToRoot(CHECK(c.getFullName(), this)), mb->getBuffer()); - Expected> modTime = c.getLastModified(); if (!modTime) return modTime.takeError(); diff --git a/lld/MachO/Options.td b/lld/MachO/Options.td index dc2212399222..bbd8bf70c3a0 100644 --- a/lld/MachO/Options.td +++ b/lld/MachO/Options.td @@ -153,6 +153,9 @@ def cs_profile_path: Joined<["--"], "cs-profile-path=">, defm pgo_warn_mismatch: BB<"pgo-warn-mismatch", "turn on warnings about profile cfg mismatch (default)", "turn off warnings about profile cfg mismatch">, Group; +defm warn_thin_archive_missing_members : BB<"warn-thin-archive-missing-members", + "Warn on missing object files referenced by thin archives (default)", + "Do not warn on missing object files referenced by thin archives">, Group; // This is a complete Options.td compiled from Apple's ld(1) manpage // dated 2018-03-07 and cross checked with ld64 source code in repo diff --git a/lld/test/MachO/reproduce-thin-archive-objc.s b/lld/test/MachO/reproduce-thin-archive-objc.s new file mode 100644 index 000000000000..c5fe42f13052 --- /dev/null +++ b/lld/test/MachO/reproduce-thin-archive-objc.s @@ -0,0 +1,25 @@ +# REQUIRES: x86 + +## For a long time, LLD only included those members from thin archives that were actually used +## during linking. However, we need to iterate over all members for -ObjC, check that we don't +## crash when we encounter a missing member. + +# RUN: rm -rf %t; mkdir %t +# RUN: sed s/SYM/_main/ %s | llvm-mc -filetype=obj -triple=x86_64-apple-macos -o %t/main.o +# RUN: sed s/SYM/_unused/ %s | llvm-mc -filetype=obj -triple=x86_64-apple-macos -o %t/unused.o + +# RUN: cd %t; llvm-ar rcsT unused.a unused.o; rm unused.o +## FIXME: Absolute paths don't end up relativized in the repro file. + +# RUN: %no-fatal-warnings-lld %t/main.o %t/unused.a -ObjC -o /dev/null 2>&1 \ +# RUN: | FileCheck %s --check-prefix=WARN + +# RUN: %lld %t/main.o %t/unused.a -ObjC --no-warn-thin-archive-missing-members -o /dev/null \ +# RUN: | FileCheck %s --implicit-check-not 'warning' --allow-empty + +# WARN: ld64.lld: warning: {{.*}}unused.a: -ObjC failed to open archive member: 'unused.o' + +.text +.globl SYM +SYM: + ret diff --git a/lld/test/MachO/reproduce-thin-archives.s b/lld/test/MachO/reproduce-thin-archives.s index 9dee3f400e06..33eeaede7aa4 100644 --- a/lld/test/MachO/reproduce-thin-archives.s +++ b/lld/test/MachO/reproduce-thin-archives.s @@ -1,10 +1,11 @@ # REQUIRES: x86 -# RUN: rm -rf %t.dir -# RUN: mkdir -p %t.dir -# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos %s -o %t.dir/foo.o +# RUN: rm -rf %t.dir; split-file %s %t.dir + +# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos %t.dir/foo.s -o %t.dir/foo.o +# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos %t.dir/unused.s -o %t.dir/unused.o # RUN: cd %t.dir -# RUN: llvm-ar rcsT foo.a foo.o +# RUN: llvm-ar rcsT foo.a foo.o unused.o # RUN: %lld foo.a -o /dev/null --reproduce repro.tar # RUN: tar tf repro.tar | FileCheck -DPATH='repro/%:t.dir' %s @@ -12,9 +13,19 @@ # RUN: %lld -all_load foo.a -o /dev/null --reproduce repro2.tar # RUN: tar tf repro2.tar | FileCheck -DPATH='repro2/%:t.dir' %s +# RUN: %lld -ObjC foo.a -o /dev/null --reproduce repro3.tar +# RUN: tar tf repro3.tar | FileCheck -DPATH='repro3/%:t.dir' %s + # CHECK-DAG: [[PATH]]/foo.a # CHECK-DAG: [[PATH]]/foo.o +# CHECK-DAG: [[PATH]]/unused.o +#--- foo.s .globl _main _main: nop + +#--- unused.s +.globl _unused +_unused: + nop