[llvm-objcopy] Add file names to error messages

[seiya]

Summary

This patch adds the file names to llvm-objcopy error messages. It makes easy to identify which file causes an error.

Bugzilla: https://bugs.llvm.org/show_bug.cgi?id=41798

[seiya]

We need to remove unwrapOrError in the file parser [1] as well to print the file name in parsing errors. Removing unwrapOrError and return Error instead sounds straightforward to me but it could make the code messy. I'd like to hear your comments.

[1]: https://github.com/llvm/llvm-project/blob/4669cf27508ba00274367d62b295e1a7ca20c7ba/llvm/tools/llvm-objcopy/ELF/Object.cpp#L1119

[jhenderson]

With a lot of the tests, it might be nice to make sure the file name matches exactly. This can be achieved using FileCheck patterns and command-line options like so:

# RUN: yaml2obj %s > %t.o
# RUN: not llvm-objcopy %t.o %t2.o <options> | FileCheck %s -DFILE=%t.o
# CHECK: error: [[FILE]]: error message...

This makes sure we use the right filename in the error, either the input or output filename.

[jhenderson]

Another thing is that you should make sure that all the code paths you modified are tested where possible. The tests probably already exist, and just need updating to check that the filename appears.

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

691

I don't have a problem with this change, but why did you make it? Also, it should probably be a separate patch, since it's not related to the bug you are trying to fix.

740–744

Rather than create the file error here, it might make more sense to call createFileError in findBuildId.

754

linkToBuildIdDir already calls createFileError in some situations. Rather than repeat it here, it would be better to change any necessary return sites in that function.

[seiya]

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

691

I changed this to print the output file name specified in --dump-section because, for instance, creating the output file named File may fail with an EISDIR error.

I'll remove this line and submit another patch to update this.

740–744

Indeed. I'll change findBuildId to take Config and return a FileError from it.

754

I thought it would be helpful to print the input file name but now realized that it is not so helpful. I'll remove this change and return the error from this function as it is because it already includes the file name which caused the error.

[seiya]

• Applied feedbacks from James.
• Update tests to check if the error message contains the file name.

[seiya]

I noticed that this patch does not include the updates to dump-section.test. I'll update soon.

[seiya]

• Updated dump-section.test.

[jhenderson]

LGTM, but I'd like a second pair of eyes on it from someone like @MaskRay, @rupprecht, or @jakehehrlich before this goes in. I assume you'll need somebody to commit it for you?

[jakehehrlich]

Awesome. LGTM. Tell me if you want me or someone else to land it. You can also email Chris Latner to get access yourself.

[seiya]

In D61993#1508403, @jhenderson wrote:

LGTM, but I'd like a second pair of eyes on it from someone like @MaskRay, @rupprecht, or @jakehehrlich before this goes in. I assume you'll need somebody to commit it for you?

Yes, I would like you to commit this.

[seiya]

In D61993#1509025, @jakehehrlich wrote:

Awesome. LGTM. Tell me if you want me or someone else to land it. You can also email Chris Latner to get access yourself.

You mean a write access to SVN as described here? https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

[jhenderson]

In D61993#1509320, @seiya wrote:

In D61993#1509025, @jakehehrlich wrote:

Awesome. LGTM. Tell me if you want me or someone else to land it. You can also email Chris Latner to get access yourself.

You mean a write access to SVN as described here? https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

Yes, since you're now making regular patches.

I'll try to get around to committing this later today, but I've got some critical things to do today, so it might need to wait until @jakehehrlich gets in later.

[seiya]

@jhenderson I see. I'll ask Chris for a commit access.

[rupprecht]

I'm guessing we have more tests that could use the -DINPUT method here to verify the file is there in the error. This patch LGTM though. Thanks!

[seiya]

Now I have a commit access. I'll commit this by myself. @jhenderson

[seiya]

Changes in 5316a0d200f8 conflicts with this patch. Should I upload this patch again here for review or should I simply resolve conflicts and push to the repository?

[jhenderson]

In D61993#1511949, @seiya wrote:

Changes in 5316a0d200f8 conflicts with this patch. Should I upload this patch again here for review or should I simply resolve conflicts and push to the repository?

Use your own judgement. If the changes require significant effort to fix, then feel free to provide an updated diff, but if not, you don't need to.

[seiya]

In D61993#1511964, @jhenderson wrote:

In D61993#1511949, @seiya wrote:

Changes in 5316a0d200f8 conflicts with this patch. Should I upload this patch again here for review or should I simply resolve conflicts and push to the repository?

Use your own judgement. If the changes require significant effort to fix, then feel free to provide an updated diff, but if not, you don't need to.

I see. I'll resolve conflicts and commit this patch without uploading here since it only changes the error messages.

llvm/test/tools/llvm-objcopy/ELF/dump-section.test

 # RUN: yaml2obj %s > %t
 # RUN: llvm-objcopy -O binary -j .text %t %t2
 # RUN: llvm-objcopy -O binary --only-section .text %t %t3
 # RUN: llvm-objcopy --dump-section .text=%t4 %t %t5
 # RUN: llvm-objcopy --dump-section .foo=%t6 %t %t7
-# RUN: not llvm-objcopy --dump-section .bar=%t8 %t %t9 2>&1 | FileCheck %s --check-prefix=NOBITS
+# RUN: not llvm-objcopy --dump-section .bar=%t8 %t %t9 2>&1 | FileCheck %s --check-prefix=NOBITS -DINPUT=%t
 # RUN: od -t x1 %t2 | FileCheck %s
 # RUN: od -t x1 %t6 | FileCheck %s --check-prefix=NON-ALLOC
 # RUN: wc -c %t2 | FileCheck %s --check-prefix=SIZE
 # RUN: diff %t2 %t3
 # RUN: diff %t4 %t3
 
 !ELF
 FileHeader:
Sections:
     - Section: .text
 
 #CHECK: 0000000 de ad be ef
 
 #NON-ALLOC: 0000000 ca fe
 
 #SIZE: 4
 
-#NOBITS: Can't dump section ".bar": it has no contents
+#NOBITS: error: '[[INPUT]]': Can't dump section ".bar": it has no contents

llvm/test/tools/llvm-objcopy/ELF/dynrelocsec-remove-shlink-reference.test

 # RUN: yaml2obj %s > %t
 
 ## Check we cannot remove the .dynsym symbol table because dynamic
 ## relocation section .rela.dyn still references it via sh_link field.
-# RUN: not llvm-objcopy -R .dynsym %t %t2 2>&1 >/dev/null | FileCheck %s --check-prefix=ERR
-# ERR: error: Symbol table .dynsym cannot be removed because it is referenced by the relocation section .rela.dyn.
+# RUN: not llvm-objcopy -R .dynsym %t %t2 2>&1 >/dev/null | FileCheck %s --check-prefix=ERR -DINPUT=%t
+# ERR: error: '[[INPUT]]': Symbol table .dynsym cannot be removed because it is referenced by the relocation section .rela.dyn.
 
 ## Check we can remove .dynsym after removing the reference.
 # RUN: llvm-objcopy -R .dynsym -R .rela.dyn %t %t2
 # RUN: llvm-readelf --sections %t2 | FileCheck %s --implicit-check-not=".dynsym"
 
 ## Check we zero out sh_link field and allow producing output with the --allow-broken-links switch.
 # RUN: llvm-objcopy -R .dynsym --allow-broken-links %t %t2
 # RUN: llvm-readelf --sections %t2 | FileCheck %s --check-prefix=DROP-LINK

llvm/test/tools/llvm-objcopy/ELF/dynsym-error-remove-strtab.test

-# RUN: not llvm-objcopy -R .dynstr %p/Inputs/dynsym.so %t 2>&1 >/dev/null | FileCheck %s --check-prefix=ERR
+# RUN: not llvm-objcopy -R .dynstr %p/Inputs/dynsym.so %t 2>&1 >/dev/null | FileCheck %s --check-prefix=ERR1 -DINPUT=%p/Inputs/dynsym.so
 # RUN: cp  %p/Inputs/dynsym.so %t2
-# RUN: not llvm-strip --no-strip-all -R .dynstr %t2 2>&1 >/dev/null | FileCheck %s --check-prefix=ERR
+# RUN: not llvm-strip --no-strip-all -R .dynstr %t2 2>&1 >/dev/null | FileCheck %s --check-prefix=ERR2 -DINPUT=%t2
 
-# ERR: Section .dynstr cannot be removed because it is referenced by the section .dynsym
+# ERR1: error: '[[INPUT]]': Section .dynstr cannot be removed because it is referenced by the section .dynsym
+# ERR2: error: '[[INPUT]]': Section .dynstr cannot be removed because it is referenced by the section .dynsym
 
 # RUN: llvm-objcopy --allow-broken-links -R .dynstr %p/Inputs/dynsym.so %t3
 # RUN: llvm-objdump --section-headers %t3 | FileCheck %s --check-prefix=SECTIONS --implicit-check-not=.dynstr
 # RUN: cp  %p/Inputs/dynsym.so %t4
 # RUN: llvm-strip --no-strip-all --allow-broken-links -R .dynstr %t4
 # RUN: llvm-objdump --section-headers %t4 | FileCheck %s --check-prefix=SECTIONS --implicit-check-not=.dynstr
 
 # SECTIONS: .dynsym
 # SECTIONS: .dynamic

llvm/test/tools/llvm-objcopy/ELF/no-build-id.test

 # RUN: yaml2obj %s > %t
-# RUN: not llvm-objcopy --build-id-link-dir=%t-dir --build-id-link-input=.debug %t 2>&1 >/dev/null | FileCheck %s
+# RUN: not llvm-objcopy --build-id-link-dir=%t-dir --build-id-link-input=.debug %t 2>&1 >/dev/null | FileCheck %s -DINPUT=%t
 
-# CHECK: Could not find build ID.
+# CHECK: error: '[[INPUT]]': Could not find build ID.
 
 --- !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_EXEC
   Machine:         EM_X86_64
 Sections:

llvm/test/tools/llvm-objcopy/ELF/reloc-error-remove-symtab.test

 # RUN: yaml2obj %s > %t
-# RUN: not llvm-objcopy -R .symtab %t %t2 2>&1 >/dev/null | FileCheck %s --check-prefix=ERR
+# RUN: not llvm-objcopy -R .symtab %t %t2 2>&1 >/dev/null | FileCheck %s --check-prefix=ERR1 -DINPUT=%t
 # RUN: cp %t %t3
-# RUN: not llvm-strip --no-strip-all -R .symtab %t3 2>&1 >/dev/null | FileCheck %s --check-prefix=ERR
+# RUN: not llvm-strip --no-strip-all -R .symtab %t3 2>&1 >/dev/null | FileCheck %s --check-prefix=ERR2 -DINPUT=%t3
 
 !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_REL
   Machine:         EM_X86_64
 Sections:
Relocations:
         Type:   R_X86_64_PC32
 
 Symbols:
   - Name:     foo
     Type:     STT_FUNC
     Size:     4
     Binding:  STB_GLOBAL
 
-# ERR: Symbol table .symtab cannot be removed because it is referenced by the relocation section .rel.text.
+# ERR1: error: '[[INPUT]]': Symbol table .symtab cannot be removed because it is referenced by the relocation section .rel.text.
+# ERR2: error: '[[INPUT]]': Symbol table .symtab cannot be removed because it is referenced by the relocation section .rel.text.
 
 # RUN: llvm-objcopy --allow-broken-links -R .symtab %t %t4
 # RUN: llvm-readobj --sections %t4 | FileCheck %s --check-prefix=SECTIONS --implicit-check-not=.symtab
 # RUN: cp %t %t5
 # RUN: llvm-strip --no-strip-all --allow-broken-links -R .symtab %t5
 # RUN: llvm-readobj --sections %t5 | FileCheck %s --check-prefix=SECTIONS --implicit-check-not=.symtab
 
 # SECTIONS:        Name: .rel.text
 # SECTIONS:        Link
 # SECTIONS-SAME: : 0

llvm/test/tools/llvm-objcopy/ELF/remove-linked-section.test

 # RUN: yaml2obj %s -o %t.o
-# RUN: not llvm-objcopy -R .foo %t.o %t1 2>&1 >/dev/null | FileCheck %s --check-prefix=ERR
+# RUN: not llvm-objcopy -R .foo %t.o %t1 2>&1 >/dev/null | FileCheck %s --check-prefix=ERR1 -DINPUT=%t.o
 # RUN: cp %t.o %t2
-# RUN: not llvm-strip --no-strip-all -R .foo %t2 2>&1 >/dev/null | FileCheck %s --check-prefix=ERR
+# RUN: not llvm-strip --no-strip-all -R .foo %t2 2>&1 >/dev/null | FileCheck %s --check-prefix=ERR2 -DINPUT=%t2
 
 --- !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_REL
   Machine:         EM_X86_64
 Sections:
   - Name:            .foo
     Type:            SHT_PROGBITS
   - Name:            .bar
     Type:            SHT_PROGBITS
     Link:            .foo
 
-# ERR: Section .foo cannot be removed because it is referenced by the section .bar
+# ERR1: error: '[[INPUT]]': Section .foo cannot be removed because it is referenced by the section .bar
+# ERR2: error: '[[INPUT]]': Section .foo cannot be removed because it is referenced by the section .bar
 
 # RUN: llvm-objcopy --allow-broken-links -R .foo %t.o %t3
 # RUN: llvm-readobj --sections %t3 | FileCheck %s --check-prefix=SECTIONS --implicit-check-not=.foo
 # RUN: cp %t.o %t4
 # RUN: llvm-strip --no-strip-all --allow-broken-links -R .foo %t4
 # RUN: llvm-readobj --sections %t4 | FileCheck %s --check-prefix=SECTIONS --implicit-check-not=.foo
 
 # SECTIONS:      Name: .bar
 # SECTIONS:      Link
 # SECTIONS-SAME: : 0

llvm/test/tools/llvm-objcopy/ELF/remove-shstrtab-error.test

 # RUN: yaml2obj %s > %t
-# RUN: not llvm-objcopy -R .shstrtab %t %t2 2>&1 >/dev/null | FileCheck %s
+# RUN: not llvm-objcopy -R .shstrtab %t %t2 2>&1 >/dev/null | FileCheck %s -DINPUT=%t
 
 !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_REL
   Machine:         EM_X86_64
 
-# CHECK: Cannot write section header table because section header string table was removed.
+# CHECK: error: '[[INPUT]]': Cannot write section header table because section header string table was removed.

llvm/test/tools/llvm-objcopy/ELF/strip-section-err.test

 ## Check we cannot remove a section containing symbols
 ## referenced by relocations contained in the object.
 
 # RUN: yaml2obj %s > %t1
-# RUN: not llvm-objcopy -R .data %t1 2>&1 | FileCheck %s
-# CHECK: error: Section .data can't be removed: (.text+0x1) has relocation against symbol 'foo'
+# RUN: not llvm-objcopy -R .data %t1 2>&1 | FileCheck %s -DINPUT=%t1
+# CHECK: error: '[[INPUT]]': Section .data can't be removed: (.text+0x1) has relocation against symbol 'foo'
 
 ## Check the behavior when we also remove the relocation section.
 ## We have no reference in this case and hence no error should be emitted.
 
 # RUN: yaml2obj %s > %t2
 # RUN: llvm-objcopy -R .data -R .rela.text %t2 %t3
 # RUN: llvm-objdump --section-headers %t3 | FileCheck %s --check-prefix=NOSEC
 # NOSEC-NOT: .data

llvm/test/tools/llvm-objcopy/ELF/strip-symbol-and-relocation.test

 ## strip the corresponding relocation section at the same time.
 
 # RUN: llvm-objcopy --strip-symbol=bar -R .rela.text %t %t2
 # RUN: llvm-readelf -s -S %t2 | FileCheck %s --check-prefix=STRIPSYM
 
 # STRIPSYM-NOT: bar
 # STRIPSYM-NOT: .rela.text
 
-# RUN: not llvm-objcopy --strip-symbol=bar %t %t2 2>&1 | FileCheck %s --check-prefix=ERR
-# ERR: not stripping symbol 'bar' because it is named in a relocation
+# RUN: not llvm-objcopy --strip-symbol=bar %t %t2 2>&1 | FileCheck %s --check-prefix=ERR -DINPUT=%t
+# ERR: error: '[[INPUT]]': not stripping symbol 'bar' because it is named in a relocation
 
 --- !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_REL
   Machine:         EM_X86_64
 Sections:

llvm/test/tools/llvm-objcopy/ELF/symtab-error-on-remove-strtab.test

 # RUN: yaml2obj %s > %t
-# RUN: not llvm-objcopy -R .strtab %t %t2 2>&1 >/dev/null | FileCheck %s --check-prefix=ERR
+# RUN: not llvm-objcopy -R .strtab %t %t2 2>&1 >/dev/null | FileCheck %s --check-prefix=ERR1 -DINPUT=%t
 # RUN: cp %t %t3
-# RUN: not llvm-strip --no-strip-all -R .strtab %t3 2>&1 >/dev/null | FileCheck %s --check-prefix=ERR
+# RUN: not llvm-strip --no-strip-all -R .strtab %t3 2>&1 >/dev/null | FileCheck %s --check-prefix=ERR2 -DINPUT=%t3
 
 !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_REL
   Machine:         EM_X86_64
 
-# ERR: String table .strtab cannot be removed because it is referenced by the symbol table .symtab
+# ERR1: error: '[[INPUT]]': String table .strtab cannot be removed because it is referenced by the symbol table .symtab
+# ERR2: error: '[[INPUT]]': String table .strtab cannot be removed because it is referenced by the symbol table .symtab
 
 # RUN: llvm-objcopy --allow-broken-links -R .strtab %t %t4
 # RUN: llvm-objdump --section-headers %t4 | FileCheck %s --check-prefix=SECTIONS --implicit-check-not=.strtab
 # RUN: cp %t %t5
 # RUN: llvm-strip --no-strip-all --allow-broken-links -R .strtab %t %t5
 # RUN: llvm-objdump --section-headers %t5 | FileCheck %s --check-prefix=SECTIONS --implicit-check-not=.strtab
 
 # SECTIONS: .symtab

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

case ELFT_ELF64BE:
     return llvm::make_unique<ELFWriter<ELF64BE>>(Obj, Buf,
                                                  !Config.StripSections);
   }
   llvm_unreachable("Invalid output format");
 }
 
 template <class ELFT>
 static Expected<ArrayRef<uint8_t>>
-findBuildID(const object::ELFFile<ELFT> &In) {
-  for (const auto &Phdr : unwrapOrError(In.program_headers())) {
+findBuildID(const CopyConfig &Config, const object::ELFFile<ELFT> &In) {
+  auto PhdrsOrErr = In.program_headers();
+  if (auto Err = PhdrsOrErr.takeError())
+    return createFileError(Config.InputFilename, std::move(Err));
+
+  for (const auto &Phdr : *PhdrsOrErr) {
     if (Phdr.p_type != PT_NOTE)
       continue;
     Error Err = Error::success();
     for (const auto &Note : In.notes(Phdr, Err))
       if (Note.getType() == NT_GNU_BUILD_ID && Note.getName() == ELF_NOTE_GNU)
         return Note.getDesc();
     if (Err)
-      return std::move(Err);
+      return createFileError(Config.InputFilename, std::move(Err));
   }
-  return createStringError(llvm::errc::invalid_argument,
-                           "Could not find build ID.");
+
+  return createFileError(
+      Config.InputFilename,
+      createStringError(llvm::errc::invalid_argument,
+                        "Could not find build ID."));
 }
 
 static Expected<ArrayRef<uint8_t>>
-findBuildID(const object::ELFObjectFileBase &In) {
+findBuildID(const CopyConfig &Config, const object::ELFObjectFileBase &In) {
   if (auto *O = dyn_cast<ELFObjectFile<ELF32LE>>(&In))
-    return findBuildID(*O->getELFFile());
+    return findBuildID(Config, *O->getELFFile());
   else if (auto *O = dyn_cast<ELFObjectFile<ELF64LE>>(&In))
-    return findBuildID(*O->getELFFile());
+    return findBuildID(Config, *O->getELFFile());
   else if (auto *O = dyn_cast<ELFObjectFile<ELF32BE>>(&In))
-    return findBuildID(*O->getELFFile());
+    return findBuildID(Config, *O->getELFFile());
   else if (auto *O = dyn_cast<ELFObjectFile<ELF64BE>>(&In))
-    return findBuildID(*O->getELFFile());
+    return findBuildID(Config, *O->getELFFile());
 
   llvm_unreachable("Bad file format");
 }
 
 template <class... Ts>
 static Error makeStringError(std::error_code EC, const Twine &Msg, Ts&&... Args) {
   std::string FullMsg = (EC.message() + ": " + Msg).str();
   return createStringError(EC, FullMsg.c_str(), std::forward<Ts>(Args)...);
if (SecName.startswith(".note") && SecName != ".note.GNU-stack")
       NewSection.Type = SHT_NOTE;
   }
 
   for (const auto &Flag : Config.DumpSection) {
     std::pair<StringRef, StringRef> SecPair = Flag.split("=");
     StringRef SecName = SecPair.first;
     StringRef File = SecPair.second;
     if (Error E = dumpSectionToFile(SecName, File, Obj))
-      return createFileError(Config.InputFilename, std::move(E));
+      return E;
   }
 
   if (!Config.AddGnuDebugLink.empty())
     Obj.addSection<GnuDebugLinkSection>(Config.AddGnuDebugLink,
                                         Config.GnuDebugLinkCRC32);
 
   for (const NewSymbolInfo &SI : Config.SymbolsToAdd) {
     SectionBase *Sec = Obj.findSection(SI.SectionName);
Error executeObjcopyOnBinary(const CopyConfig &Config,
   std::unique_ptr<Object> Obj = Reader.create();
   // Prefer OutputArch (-O<format>) if set, otherwise infer it from the input.
   const ElfType OutputElfType =
       Config.OutputArch ? getOutputElfType(Config.OutputArch.getValue())
                         : getOutputElfType(In);
   ArrayRef<uint8_t> BuildIdBytes;
 
   if (!Config.BuildIdLinkDir.empty()) {
-    BuildIdBytes = unwrapOrError(findBuildID(In));
+    auto BuildIdBytesOrErr = findBuildID(Config, In);
+    if (auto E = BuildIdBytesOrErr.takeError())
+      return E;
+    BuildIdBytes = *BuildIdBytesOrErr;
+
     if (BuildIdBytes.size() < 2)
       return createFileError(
           Config.InputFilename,
           createStringError(object_error::parse_failed,
                             "build ID is smaller than two bytes."));
   }
 
   if (!Config.BuildIdLinkDir.empty() && Config.BuildIdLinkInput)
     if (Error E =
             linkToBuildIdDir(Config, Config.InputFilename,
                              Config.BuildIdLinkInput.getValue(), BuildIdBytes))
       return E;
 
   if (Error E = handleArgs(Config, *Obj, Reader, OutputElfType))
-    return E;
+    return createFileError(Config.InputFilename, std::move(E));
+
   std::unique_ptr<Writer> Writer =
       createWriter(Config, *Obj, Out, OutputElfType);
   if (Error E = Writer->finalize())
-    return E;
+    return createFileError(Config.InputFilename, std::move(E));
   if (Error E = Writer->write())
     return E;
   if (!Config.BuildIdLinkDir.empty() && Config.BuildIdLinkOutput)
     if (Error E =
             linkToBuildIdDir(Config, Config.OutputFilename,
                              Config.BuildIdLinkOutput.getValue(), BuildIdBytes))
-      return E;
+      return createFileError(Config.OutputFilename, std::move(E));
 
   return Error::success();
 }
 
 } // end namespace elf
 } // end namespace objcopy
 } // end namespace llvm