[llvm-objcopy] Improve error message for unrecognised archive member

[jhenderson]

Summary

Prior to this patch, llvm-objcopy's error messages for archives with unsupported members only mentioned the archive name, not the member name, making them unhelpful. This change improves it by approximately following GNU objcopy's error message syntax of "<archive name>(<member name>): <problem>".

[grimar]

test/tools/llvm-objcopy/ELF/archive-unknown-members.test

2

unrecognised -> unrecognized I think?

26

You have %t.thin1.a %t2.a for llvm-objcopy call and just %t.thin2.a for llvm-strip.
I think you placed %t2.a by mistake?

tools/llvm-objcopy/llvm-objcopy.cpp

165

nit: while you are here: I would perhaps remove this variable since it is used only once.

[jhenderson]

Address review comments.

[jhenderson]

test/tools/llvm-objcopy/ELF/archive-unknown-members.test

2

Thanks. Stupid American spelling ;)

26

Oops, thanks.

[grimar]

LGTM

[jhenderson]

Thanks @grimar!

[grimar]

test/tools/llvm-objcopy/ELF/archive-unknown-members.test

32

recognised->recognized :]

[jhenderson]

Sorry, I didn't see your latest comment before committing. I've fixed it in a follow up change.

[MaskRay]

test/tools/llvm-objcopy/ELF/archive-unknown-members.test

32

recognised is not wrong..

[grimar]

test/tools/llvm-objcopy/ELF/archive-unknown-members.test

32

As far I understand there is a difference between British and American English.
Since our error messages spelling "recognized", i.e. in American way, I think it is good for
comments to be consistent. See rL360252

[jhenderson]

test/tools/llvm-objcopy/ELF/archive-unknown-members.test

32

I thought I read somewhere that the standard was American English in LLVM, but I now can't find it, so maybe I imagined it?

[grimar]

test/tools/llvm-objcopy/ELF/archive-unknown-members.test

32

Honestly saying I suggested to change unrecognised -> unrecognized initially because my spellchecker complained about this word. I realized that it is a British style only after your first reply :) I do not know what is the LLVM standard (I would not expect to see such a requirement probably.)

Having consistency in spelling is not that bad still I think. I.e. in this test error messages spelling wasn't consistent with the comments before and now it is. But perhaps it is really was too subtle to care about?

test/tools/llvm-objcopy/ELF/archive-unknown-members.test

+## Show that llvm-objcopy and llvm-strip emit errors when attempting to modify
+## archives containing unrecognised files, and do not modify anything. We use
+## --strip-debug so that the section should be removed, and a difference
+## detectable, if the file were modified.
+
+# RUN: yaml2obj %s -o %t.o
+# RUN: rm -f %t.a
+# RUN: llvm-ar rc %t.a %t.o %s
+# RUN: cp %t.a %t1.a
+# RUN: cp %t.a %t2.a
+
+# RUN: not llvm-objcopy --strip-debug %t1.a 2>&1 | FileCheck %s -DARCHIVE=%t1.a
+# RUN: not llvm-strip --strip-debug %t2.a 2>&1 | FileCheck %s -DARCHIVE=%t2.a
+## Verify that the archive was not modified, if any member couldn't be recognised.
+# RUN: cmp %t.a %t1.a
+# RUN: cmp %t.a %t2.a
+
+# CHECK: error: '[[ARCHIVE]](archive-unknown-members.test)': The file was not recognized as a valid object file
+
+# RUN: rm -f %t.thin1.a %t.thin2.a
+# RUN: cp %t.o %t1.o
+# RUN: cp %t.o %t2.o
+# RUN: llvm-ar rcT %t.thin1.a %t1.o %s
+# RUN: llvm-ar rcT %t.thin2.a %t2.o %s
+
+# RUN: not llvm-objcopy --strip-debug %t.thin1.a %t2.a 2>&1 \
+# RUN:   | FileCheck %s --check-prefix=THIN -DARCHIVE=%t.thin1.a -DMEMBER=%s
+# RUN: not llvm-strip --strip-debug %t.thin2.a 2>&1 \
+# RUN:   | FileCheck %s --check-prefix=THIN -DARCHIVE=%t.thin2.a -DMEMBER=%s
+## Verify that the first member was not modified, if a later member could not
+## be recognised.
+# RUN: cmp %t.o %t1.o
+# RUN: cmp %t.o %t2.o
+
+# THIN: error: '[[ARCHIVE]]([[MEMBER]])': The file was not recognized as a valid object file
+
+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_EXEC
+  Machine:         EM_X86_64
+Sections:
+  - Name:            .debug_foo
+    Type:            SHT_PROGBITS
+    Flags:           [ ]

tools/llvm-objcopy/llvm-objcopy.cpp

return createStringError(object_error::invalid_file_type,
                              "Unsupported object file format");
 }
 
 static Error executeObjcopyOnArchive(const CopyConfig &Config,
                                      const Archive &Ar) {
   std::vector<NewArchiveMember> NewArchiveMembers;
   Error Err = Error::success();
   for (const Archive::Child &Child : Ar.children(Err)) {
-    Expected<std::unique_ptr<Binary>> ChildOrErr = Child.getAsBinary();
-    if (!ChildOrErr)
-      return createFileError(Ar.getFileName(), ChildOrErr.takeError());
-    Binary *Bin = ChildOrErr->get();
-
     Expected<StringRef> ChildNameOrErr = Child.getName();
     if (!ChildNameOrErr)
       return createFileError(Ar.getFileName(), ChildNameOrErr.takeError());
 
+    Expected<std::unique_ptr<Binary>> ChildOrErr = Child.getAsBinary();
+    if (!ChildOrErr)
+      return createFileError(Ar.getFileName() + "(" + *ChildNameOrErr + ")",
+                             ChildOrErr.takeError());
+    Binary *Bin = ChildOrErr->get();
+
     MemBuffer MB(ChildNameOrErr.get());
     if (Error E = executeObjcopyOnBinary(Config, *Bin, MB))
       return E;
 
     Expected<NewArchiveMember> Member =
         NewArchiveMember::getOldMember(Child, Config.DeterministicArchives);
     if (!Member)
       return createFileError(Ar.getFileName(), Member.takeError());