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

BertalanD · 2024-06-29T16:59:22Z

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.

The repro file in llvm#94716 contains a few thin archives without their member object files present. I couldn't figure out why this happened (issue in repro tarball generation?), but this caused an "unhandled Error" crash on `LLVM_ENABLE_ABI_BREAKING_CHECKS` builds.

llvmbot · 2024-06-29T16:59:53Z

@llvm/pr-subscribers-lld-macho

Author: Daniel Bertalan (BertalanD)

Changes

The repro file in #94716 contains a few thin archives without their member object files present. I couldn't figure out why this happened (issue in repro tarball generation?), but this caused an "unhandled Error" crash on LLVM_ENABLE_ABI_BREAKING_CHECKS builds.

Full diff: https://github.com/llvm/llvm-project/pull/97169.diff

1 Files Affected:

(modified) lld/MachO/Driver.cpp (+6-1)

diff --git a/lld/MachO/Driver.cpp b/lld/MachO/Driver.cpp
index 9dddabcf3680c..eb64e764100b2 100644
--- a/lld/MachO/Driver.cpp
+++ b/lld/MachO/Driver.cpp
@@ -349,7 +349,12 @@ 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) {
+            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: " +

llvmbot · 2024-06-29T16:59:53Z

@llvm/pr-subscribers-lld

Author: Daniel Bertalan (BertalanD)

Changes

The repro file in #94716 contains a few thin archives without their member object files present. I couldn't figure out why this happened (issue in repro tarball generation?), but this caused an "unhandled Error" crash on LLVM_ENABLE_ABI_BREAKING_CHECKS builds.

Full diff: https://github.com/llvm/llvm-project/pull/97169.diff

1 Files Affected:

(modified) lld/MachO/Driver.cpp (+6-1)

diff --git a/lld/MachO/Driver.cpp b/lld/MachO/Driver.cpp
index 9dddabcf3680c..eb64e764100b2 100644
--- a/lld/MachO/Driver.cpp
+++ b/lld/MachO/Driver.cpp
@@ -349,7 +349,12 @@ 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) {
+            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: " +

BertalanD · 2024-07-06T12:12:15Z

lld/test/MachO/reproduce-thin-archive-objc.s

+# 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
+## FIXME: Absolute paths don't end up relativized in the repro file.


This is #97845

nico · 2024-07-06T12:19:18Z

lld/MachO/Driver.cpp

-          if (!mb || !hasObjCSection(*mb))
+          if (!mb) {
+            // Thin archives from repro tarballs can contain missing members
+            // if the member was not loaded later during the initial link.


Should we add a if (c.getParent()->isThin()) here to not drop valid errors?

But outside or repro files, .o files referenced from an archive being missing seems like something we probably want to diag on, hmm.

Maybe we should eagerly put all .o files referenced from thin archives into the repro file always, not just when they're referenced?

Not crashing on this seems nice too though, so maybe in addition to adding all .o files from a thin archive to repro files, it'd be nice to also consumeError() here if isThin(), but emit a diag instead of doing it silently.

That seems like a good approach to me.

I ended up basically going for the approach outlined here:

also include unused object files in the repro tarball

warn on missing member

make warning toggleable so we don't get spam on stderr when processing old repro files

My last question is what should happen if we don't find members when force-loading, i.e.:

llvm-project/lld/MachO/Driver.cpp

Lines 333 to 335 in 2ad7b4a

if (Error e = file->fetch(c, reason))

error(toString(file) + ": " + reason +

" failed to load archive member: " + toString(std::move(e)));

Downgrade this to a warning too? This was not affected by the issue we're fixing (since fetch() was called unconditionally here), but if we add a user-facing option, we should be consistent about our behavior.

That makes sense to me, yes.

nico · 2024-07-15T18:52:33Z

lld/MachO/Driver.cpp

@@ -349,7 +366,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) {
+            // For a long time, only referenced object files were included in


nit "included in from"

nico

Other than that, looks great, thanks!

MaskRay · 2024-07-20T03:13:36Z

lld/test/MachO/reproduce-thin-archive-objc.s

+# 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'


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

Summary: 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. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60250793

llvmbot added lld lld:MachO labels Jun 29, 2024

BertalanD mentioned this pull request Jul 5, 2024

lld crash with Wl,-fixup_chains when build ios #94716

Closed

BertalanD added 3 commits July 5, 2024 18:15

Merge branch 'main' into thin-archive-missing

637e66f

Add test

dc53f82

Add explanation

8e29cd6

BertalanD requested review from nico and int3 July 5, 2024 17:47

Fix Windows CI

2216582

BertalanD commented Jul 6, 2024

View reviewed changes

nico reviewed Jul 6, 2024

View reviewed changes

BertalanD added 4 commits July 6, 2024 22:02

Merge branch 'main' into thin-archive-missing

6f6610f

Merge branch 'main' into thin-archive-missing

91e56d8

Add warning and mechanism to suppress it

02b3fae

Ensure that unreferenced members get included

3ecb35c

BertalanD changed the title ~~[lld-macho] Fix unchecked Error crash for thin archive missing child~~ [lld-macho] Save all thin archive members in repro tarball Jul 15, 2024

BertalanD requested a review from nico July 15, 2024 17:53

nico reviewed Jul 15, 2024

View reviewed changes

nico approved these changes Jul 15, 2024

View reviewed changes

BertalanD added 2 commits July 18, 2024 10:55

Merge branch 'main' into thin-archive-missing

e51d1dc

fix comment + consistently warn instead of erroring

851013e

This comment was marked as outdated.

Sign in to view

fix formatting

c17513d

BertalanD merged commit 47b63cd into llvm:main Jul 18, 2024
5 of 6 checks passed

BertalanD deleted the thin-archive-missing branch July 18, 2024 14:26

MaskRay reviewed Jul 20, 2024

View reviewed changes

nico mentioned this pull request Dec 17, 2024

[lld/COFF] Handle -start-lib / -end-lib better in /reproduce: output #119752

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

BertalanD commented Jun 29, 2024 •

edited

Loading

llvmbot commented Jun 29, 2024

llvmbot commented Jun 29, 2024

BertalanD Jul 6, 2024

nico Jul 6, 2024

nico Jul 6, 2024

BertalanD Jul 12, 2024

nico Jul 15, 2024

nico Jul 15, 2024

nico left a comment

This comment was marked as outdated.

MaskRay Jul 20, 2024

	if (Error e = file->fetch(c, reason))
	error(toString(file) + ": " + reason +
	" failed to load archive member: " + toString(std::move(e)));

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

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

Conversation

BertalanD commented Jun 29, 2024 • edited Loading

llvmbot commented Jun 29, 2024

llvmbot commented Jun 29, 2024

BertalanD Jul 6, 2024

Choose a reason for hiding this comment

nico Jul 6, 2024

Choose a reason for hiding this comment

nico Jul 6, 2024

Choose a reason for hiding this comment

BertalanD Jul 12, 2024

Choose a reason for hiding this comment

nico Jul 15, 2024

Choose a reason for hiding this comment

nico Jul 15, 2024

Choose a reason for hiding this comment

nico left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

MaskRay Jul 20, 2024

Choose a reason for hiding this comment

BertalanD commented Jun 29, 2024 •

edited

Loading