[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

688

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.

737

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

751

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

688

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.

737

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

751

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/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.
+# ERR: error: '{{.*}}': 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/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'
+# CHECK: error: '{{.*}}': 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/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

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())) {
+  auto PhdrsOrErr = In.program_headers();
+  if (auto Err = PhdrsOrErr.takeError())
+    return 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);
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 createFileError(File, std::move(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(In);
+    if (auto E = BuildIdBytesOrErr.takeError())
+      return createFileError(Config.InputFilename, std::move(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;
+      return createFileError(Config.InputFilename, std::move(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;
+    return createFileError(Config.OutputFilename, std::move(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