[lld-link] Do not assert when reporting error about non-thin archive

[nico]

Follow-up to https://reviews.llvm.org/D57974, which added calls to Archive::Child::getFullName() to produce strings in errors.

But getFullName() is only valid on thin archives, and should only be used to open the file the archive points to. For diagnostics, getName() is better: It works for both thin and non-thin files, and it doesn't make a very long string for thin files. And we already prepend the name of the parent archive file anyways.

[llvmbot]

@llvm/pr-subscribers-lld

@llvm/pr-subscribers-platform-windows

Author: Nico Weber (nico)

Changes

Follow-up to https://reviews.llvm.org/D57974, which added calls to Archive::Child::getFullName() to produce strings in errors.

But getFullName() is only valid on thin archives, and should only be used to open the file the archive points to. For diagnostics, getName() is better: It works for both thin and non-thin files, and it doesn't make a very long string for thin files. And we already prepend the name of the parent archive file anyways.

---

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

1 Files Affected:

• (modified) lld/COFF/Driver.cpp (+4-4)

diff --git a/lld/COFF/Driver.cpp b/lld/COFF/Driver.cpp
index acba156ce341d..fc6073dc3ec4e 100644
--- a/lld/COFF/Driver.cpp
+++ b/lld/COFF/Driver.cpp
@@ -404,9 +404,9 @@ void LinkerDriver::enqueueArchiveMember(const Archive::Child &c,
                                         const Archive::Symbol &sym,
                                         StringRef parentName) {
 
-  auto reportBufferError = [=](Error &&e, StringRef childName) {
+  auto reportBufferError = [=](Error &&e) {
     Fatal(ctx) << "could not get the buffer for the member defining symbol "
-               << &sym << ": " << parentName << "(" << childName
+               << &sym << ": " << parentName << "(" << check(c.getName())
                << "): " << std::move(e);
   };
 
@@ -414,7 +414,7 @@ void LinkerDriver::enqueueArchiveMember(const Archive::Child &c,
     uint64_t offsetInArchive = c.getChildOffset();
     Expected<MemoryBufferRef> mbOrErr = c.getMemoryBufferRef();
     if (!mbOrErr)
-      reportBufferError(mbOrErr.takeError(), check(c.getFullName()));
+      reportBufferError(mbOrErr.takeError());
     MemoryBufferRef mb = mbOrErr.get();
     enqueueTask([=]() {
       llvm::TimeTraceScope timeScope("Archive: ", mb.getBufferIdentifier());
@@ -433,7 +433,7 @@ void LinkerDriver::enqueueArchiveMember(const Archive::Child &c,
   enqueueTask([=]() {
     auto mbOrErr = future->get();
     if (mbOrErr.second)
-      reportBufferError(errorCodeToError(mbOrErr.second), childName);
+      reportBufferError(errorCodeToError(mbOrErr.second));
     llvm::TimeTraceScope timeScope("Archive: ",
                                    mbOrErr.first->getBufferIdentifier());
     ctx.driver.addThinArchiveBuffer(takeBuffer(std::move(mbOrErr.first)),

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff origin/main HEAD --extensions cpp -- lld/COFF/Driver.cpp

⚠️
The reproduction instructions above might return results for more than one PR
in a stack if you are using a stacked PR workflow. You can limit the results by
changing origin/main to the base branch/commit you want to compare against.
⚠️

View the diff from clang-format here.

diff --git a/lld/COFF/Driver.cpp b/lld/COFF/Driver.cpp
index a515b39dc..ceb5ceaca 100644
--- a/lld/COFF/Driver.cpp
+++ b/lld/COFF/Driver.cpp
@@ -405,10 +405,9 @@ void LinkerDriver::enqueueArchiveMember(const Archive::Child &c,
                                         StringRef parentName) {
 
   auto reportBufferError = [=](Error &&e) {
-    StringRef childName =
-      CHECK(c.getName(),
-            "could not get child name for archive " + parentName +
-            " while loading symbol " + toCOFFString(ctx, sym));
+    StringRef childName = CHECK(
+        c.getName(), "could not get child name for archive " + parentName +
+                         " while loading symbol " + toCOFFString(ctx, sym));
     Fatal(ctx) << "could not get the buffer for the member defining symbol "
                << &sym << ": " << parentName << "(" << childName
                << "): " << std::move(e);

[mstorsjo]

Is it possible to make a test for this change?

[nico]

Is it possible to make a test for this change?

Not easily, I think. You need a corrupt archive as input, which we don't have an easy way of creating as far as I know.