Skip to content

[lld-macho] Save all thin archive members in repro tarball #97169

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Jul 18, 2024
Merged
1 change: 1 addition & 0 deletions lld/MachO/Config.h
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,7 @@ struct Configuration {
bool csProfileGenerate = false;
llvm::StringRef csProfilePath;
bool pgoWarnMismatch;
bool warnThinArchiveMissingMembers;

bool callGraphProfileSort = false;
llvm::StringRef printSymbolOrder;
Expand Down
43 changes: 39 additions & 4 deletions lld/MachO/Driver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,20 @@ struct ArchiveFileInfo {

static DenseMap<StringRef, ArchiveFileInfo> 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,
Expand Down Expand Up @@ -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<ArchiveFile>(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
Expand Down Expand Up @@ -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) +
Expand All @@ -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<MemoryBufferRef> 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: " +
Expand Down Expand Up @@ -1699,6 +1731,9 @@ bool link(ArrayRef<const char *> 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)) {
Expand Down
4 changes: 0 additions & 4 deletions lld/MachO/InputFiles.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<TimePoint<std::chrono::seconds>> modTime = c.getLastModified();
if (!modTime)
return modTime.takeError();
Expand Down
3 changes: 3 additions & 0 deletions lld/MachO/Options.td
Original file line number Diff line number Diff line change
Expand Up @@ -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<grp_lld>;
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<grp_lld>;

// 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
Expand Down
25 changes: 25 additions & 0 deletions lld/test/MachO/reproduce-thin-archive-objc.s
Original file line number Diff line number Diff line change
@@ -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.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is #97845


# 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'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made some adjustment to hopefully make the test more conventional 2d756d9


.text
.globl SYM
SYM:
ret
19 changes: 15 additions & 4 deletions lld/test/MachO/reproduce-thin-archives.s
Original file line number Diff line number Diff line change
@@ -1,20 +1,31 @@
# 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

# 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