[llvm-objcopy] Consistently use createStringError instead of make_error<StringError>. NFC.

[mstorsjo]

Summary

This was requested in the review of D57006.

[jhenderson]

Make sure to mention in the commit comment that this also quotes certain symbol names.

I'm surprised that several of the error messages appear to be untested (only one test needed updating, but there are at least 3 different behaviour changes here). I take it testing them is not straightforward?

LGTM anyway.

[mstorsjo]

In D57014#1365204, @jhenderson wrote:

Make sure to mention in the commit comment that this also quotes certain symbol names.

Ah, right, yes - will do.

I'm surprised that several of the error messages appear to be untested (only one test needed updating, but there are at least 3 different behaviour changes here). I take it testing them is not straightforward?

Correct, the two cases where I add quotes but there's no test change for are currently not possible to trigger, I think - both the input as set by Reader, and updates by Object currently should make them impossible to trigger. I guess asserts would be an option then as well. (At an earlier point in the evolution of the removeSections patch, some of them were triggerable, but I realized it's easy to do the right thing instead of erroring out on dangling associative sections). There's also lots of error messages in Reader that aren't tested, mainly for broken inputs.

[jhenderson]

In D57014#1365220, @mstorsjo wrote:

In D57014#1365204, @jhenderson wrote:

Correct, the two cases where I add quotes but there's no test change for are currently not possible to trigger, I think - both the input as set by Reader, and updates by Object currently should make them impossible to trigger. I guess asserts would be an option then as well. (At an earlier point in the evolution of the removeSections patch, some of them were triggerable, but I realized it's easy to do the right thing instead of erroring out on dangling associative sections). There's also lots of error messages in Reader that aren't tested, mainly for broken inputs.

Okay, it might be nice to turn the unreachable ones into asserts (or simply delete the code), but that's not relevant to this change.

test/tools/llvm-objcopy/COFF/remove-section.test

 # SYMBOLS-REMOVE-TEXT-NEXT: AUX scnlen {{.*}} assoc 2 comdat 2
 # SYMBOLS-REMOVE-TEXT-NEXT: {{.*}}(sec 2){{.*}} foo
 # SYMBOLS-REMOVE-TEXT-EMPTY:
 #
 #
 # Removing the .comdat section fails, since the .text section has relocations
 # against it.
 #
-# ERROR-RELOC: Relocation target foo ({{.*}}) not found
+# ERROR-RELOC: Relocation target 'foo' ({{.*}}) not found
 #
 #
 # Removing the .comdat section and .text (with a relocation against .comdat)
 # works, as it also removes the .associative section transitively.
 #
 # SECTIONS-REMOVE-TEXT-COMDAT: Sections:
 # SECTIONS-REMOVE-TEXT-COMDAT-NEXT: Idx Name
 # SECTIONS-REMOVE-TEXT-COMDAT-NEXT: 0 .bss

tools/llvm-objcopy/COFF/COFFObjcopy.cpp

Obj.removeSymbols([&](const Symbol &Sym) {
     // symbols.
     if (Config.StripAll || Config.StripAllGNU)
       return true;
 
     if (is_contained(Config.SymbolsToRemove, Sym.Name)) {
       // Explicitly removing a referenced symbol is an error.
       if (Sym.Referenced)
         reportError(Config.OutputFilename,
-                    make_error<StringError>(
-                        "not stripping symbol '" + Sym.Name +
-                            "' because it is named in a relocation.",
-                        llvm::errc::invalid_argument));
+                    createStringError(llvm::errc::invalid_argument,
+                                      "not stripping symbol '%s' because it is "
+                                      "named in a relocation.",
+                                      Sym.Name.str().c_str()));
       return true;
     }
 
     if (!Sym.Referenced) {
       // With --strip-unneeded, GNU objcopy removes all unreferenced local
       // symbols, and any unreferenced undefined external.
       if (Config.StripUnneeded &&
           (Sym.Sym.StorageClass == IMAGE_SYM_CLASS_STATIC ||

tools/llvm-objcopy/COFF/Object.cpp

 
 Error Object::markSymbols() {
   for (Symbol &Sym : Symbols)
     Sym.Referenced = false;
   for (const Section &Sec : Sections) {
     for (const Relocation &R : Sec.Relocs) {
       auto It = SymbolMap.find(R.Target);
       if (It == SymbolMap.end())
-        return make_error<StringError>("Relocation target " + Twine(R.Target) +
-                                           " not found",
-                                       object_error::invalid_symbol_index);
+        return createStringError(object_error::invalid_symbol_index,
+                                 "Relocation target %zu not found", R.Target);
       It->second->Referenced = true;
     }
   }
   return Error::success();
 }
 
 void Object::addSections(ArrayRef<Section> NewSections) {
   for (Section S : NewSections) {

tools/llvm-objcopy/COFF/Reader.cpp

for (size_t I = 1, E = COFFObj.getNumberOfSections(); I <= E; I++) {
     if (auto EC = COFFObj.getSectionContents(Sec, S.Contents))
       return errorCodeToError(EC);
     ArrayRef<coff_relocation> Relocs = COFFObj.getRelocations(Sec);
     for (const coff_relocation &R : Relocs)
       S.Relocs.push_back(R);
     if (auto EC = COFFObj.getSectionName(Sec, S.Name))
       return errorCodeToError(EC);
     if (Sec->hasExtendedRelocations())
-      return make_error<StringError>("Extended relocations not supported yet",
-                                     object_error::parse_failed);
+      return createStringError(object_error::parse_failed,
+                               "Extended relocations not supported yet");
   }
   Obj.addSections(Sections);
   return Error::success();
 }
 
 Error COFFReader::readSymbols(Object &Obj, bool IsBigObj) const {
   std::vector<Symbol> Symbols;
   Symbols.reserve(COFFObj.getRawNumberOfSymbols());
for (uint32_t I = 0, E = COFFObj.getRawNumberOfSymbols(); I < E;) {
     // Find the unique id of the section
     if (SymRef.getSectionNumber() <=
         0) // Special symbol (undefined/absolute/debug)
       Sym.TargetSectionId = SymRef.getSectionNumber();
     else if (static_cast<uint32_t>(SymRef.getSectionNumber() - 1) <
              Sections.size())
       Sym.TargetSectionId = Sections[SymRef.getSectionNumber() - 1].UniqueId;
     else
-      return make_error<StringError>("Section number out of range",
-                                     object_error::parse_failed);
+      return createStringError(object_error::parse_failed,
+                               "Section number out of range");
     // For section definitions, check if it is comdat associative, and if
     // it is, find the target section unique id.
     const coff_aux_section_definition *SD = SymRef.getSectionDefinition();
     if (SD && SD->Selection == IMAGE_COMDAT_SELECT_ASSOCIATIVE) {
       int32_t Index = SD->getNumber(IsBigObj);
       if (Index <= 0 || static_cast<uint32_t>(Index - 1) >= Sections.size())
-        return make_error<StringError>("Unexpected associative section index",
-                                       object_error::parse_failed);
+        return createStringError(object_error::parse_failed,
+                                 "Unexpected associative section index");
       Sym.AssociativeComdatTargetSectionId = Sections[Index - 1].UniqueId;
     }
     I += 1 + SymRef.getNumberOfAuxSymbols();
   }
   Obj.addSymbols(Symbols);
   return Error::success();
 }
 
 Error COFFReader::setRelocTargets(Object &Obj) const {
   std::vector<const Symbol *> RawSymbolTable;
   for (const Symbol &Sym : Obj.getSymbols()) {
     RawSymbolTable.push_back(&Sym);
     for (size_t I = 0; I < Sym.Sym.NumberOfAuxSymbols; I++)
       RawSymbolTable.push_back(nullptr);
   }
   for (Section &Sec : Obj.getMutableSections()) {
     for (Relocation &R : Sec.Relocs) {
       if (R.Reloc.SymbolTableIndex >= RawSymbolTable.size())
-        return make_error<StringError>("SymbolTableIndex out of range",
-                                       object_error::parse_failed);
+        return createStringError(object_error::parse_failed,
+                                 "SymbolTableIndex out of range");
       const Symbol *Sym = RawSymbolTable[R.Reloc.SymbolTableIndex];
       if (Sym == nullptr)
-        return make_error<StringError>("Invalid SymbolTableIndex",
-                                       object_error::parse_failed);
+        return createStringError(object_error::parse_failed,
+                                 "Invalid SymbolTableIndex");
       R.Target = Sym->UniqueId;
       R.TargetName = Sym->Name;
     }
   }
   return Error::success();
 }
 
 Expected<std::unique_ptr<Object>> COFFReader::create() const {
   auto Obj = llvm::make_unique<Object>();
 
   const coff_file_header *CFH = nullptr;
   const coff_bigobj_file_header *CBFH = nullptr;
   COFFObj.getCOFFHeader(CFH);
   COFFObj.getCOFFBigObjHeader(CBFH);
   bool IsBigObj = false;
   if (CFH) {
     Obj->CoffFileHeader = *CFH;
   } else {
     if (!CBFH)
-      return make_error<StringError>("No COFF file header returned",
-                                     object_error::parse_failed);
+      return createStringError(object_error::parse_failed,
+                               "No COFF file header returned");
     // Only copying the few fields from the bigobj header that we need
     // and won't recreate in the end.
     Obj->CoffFileHeader.Machine = CBFH->Machine;
     Obj->CoffFileHeader.TimeDateStamp = CBFH->TimeDateStamp;
     IsBigObj = true;
   }
 
   if (Error E = readExecutableHeaders(*Obj))

tools/llvm-objcopy/COFF/Writer.cpp

 using namespace object;
 using namespace COFF;
 
 Error COFFWriter::finalizeRelocTargets() {
   for (Section &Sec : Obj.getMutableSections()) {
     for (Relocation &R : Sec.Relocs) {
       const Symbol *Sym = Obj.findSymbol(R.Target);
       if (Sym == nullptr)
-        return make_error<StringError>("Relocation target " + R.TargetName +
-                                           " (" + Twine(R.Target) +
-                                           ") not found",
-                                       object_error::invalid_symbol_index);
+        return createStringError(object_error::invalid_symbol_index,
+                                 "Relocation target '%s' (%zu) not found",
+                                 R.TargetName.str().c_str(), R.Target);
       R.Reloc.SymbolTableIndex = Sym->RawIndex;
     }
   }
   return Error::success();
 }
 
 Error COFFWriter::finalizeSectionNumbers() {
   for (Symbol &Sym : Obj.getMutableSymbols()) {
     if (Sym.TargetSectionId <= 0) {
       // Undefined, or a special kind of symbol. These negative values
       // are stored in the SectionNumber field which is unsigned.
       Sym.Sym.SectionNumber = static_cast<uint32_t>(Sym.TargetSectionId);
     } else {
       const Section *Sec = Obj.findSection(Sym.TargetSectionId);
       if (Sec == nullptr)
-        return make_error<StringError>("Symbol " + Sym.Name +
-                                           " points to a removed section",
-                                       object_error::invalid_symbol_index);
+        return createStringError(object_error::invalid_symbol_index,
+                                 "Symbol '%s' points to a removed section",
+                                 Sym.Name.str().c_str());
       Sym.Sym.SectionNumber = Sec->Index;
 
       if (Sym.Sym.NumberOfAuxSymbols == 1 &&
           Sym.Sym.StorageClass == IMAGE_SYM_CLASS_STATIC) {
         coff_aux_section_definition *SD =
             reinterpret_cast<coff_aux_section_definition *>(Sym.AuxData.data());
         uint32_t SDSectionNumber;
         if (Sym.AssociativeComdatTargetSectionId == 0) {
           // Not a comdat associative section; just set the Number field to
           // the number of the section itself.
           SDSectionNumber = Sec->Index;
         } else {
           Sec = Obj.findSection(Sym.AssociativeComdatTargetSectionId);
           if (Sec == nullptr)
-            return make_error<StringError>(
-                "Symbol " + Sym.Name + " is associative to a removed section",
-                object_error::invalid_symbol_index);
+            return createStringError(
+                object_error::invalid_symbol_index,
+                "Symbol '%s' is associative to a removed section",
+                Sym.Name.str().c_str());
           SDSectionNumber = Sec->Index;
         }
         // Update the section definition with the new section number.
         SD->NumberLowPart = static_cast<uint16_t>(SDSectionNumber);
         SD->NumberHighPart = static_cast<uint16_t>(SDSectionNumber >> 16);
       }
     }
   }
Error COFFWriter::patchDebugDirectory() {
   if (Dir->Size <= 0)
     return Error::success();
   for (const auto &S : Obj.getSections()) {
     if (Dir->RelativeVirtualAddress >= S.Header.VirtualAddress &&
         Dir->RelativeVirtualAddress <
             S.Header.VirtualAddress + S.Header.SizeOfRawData) {
       if (Dir->RelativeVirtualAddress + Dir->Size >
           S.Header.VirtualAddress + S.Header.SizeOfRawData)
-        return make_error<StringError>(
-            "Debug directory extends past end of section",
-            object_error::parse_failed);
+        return createStringError(object_error::parse_failed,
+                                 "Debug directory extends past end of section");
 
       size_t Offset = Dir->RelativeVirtualAddress - S.Header.VirtualAddress;
       uint8_t *Ptr = Buf.getBufferStart() + S.Header.PointerToRawData + Offset;
       uint8_t *End = Ptr + Dir->Size;
       while (Ptr < End) {
         debug_directory *Debug = reinterpret_cast<debug_directory *>(Ptr);
         Debug->PointerToRawData =
             S.Header.PointerToRawData + Offset + sizeof(debug_directory);
         Ptr += sizeof(debug_directory) + Debug->SizeOfData;
         Offset += sizeof(debug_directory) + Debug->SizeOfData;
       }
       // Debug directory found and patched, all done.
       return Error::success();
     }
   }
-  return make_error<StringError>("Debug directory not found",
-                                 object_error::parse_failed);
+  return createStringError(object_error::parse_failed,
+                           "Debug directory not found");
 }
 
 Error COFFWriter::write() {
   bool IsBigObj = Obj.getSections().size() > MaxNumberOfSections16;
   if (IsBigObj && Obj.IsPE)
-    return make_error<StringError>("Too many sections for executable",
-                                   object_error::parse_failed);
+    return createStringError(object_error::parse_failed,
+                             "Too many sections for executable");
   return write(IsBigObj);
 }
 
 } // end namespace coff
 } // end namespace objcopy
 } // end namespace llvm

tools/llvm-objcopy/ELF/ELFObjcopy.cpp

static void splitDWOToFile(const CopyConfig &Config, const Reader &Reader,
   Writer->write();
 }
 
 static Error dumpSectionToFile(StringRef SecName, StringRef Filename,
                                Object &Obj) {
   for (auto &Sec : Obj.sections()) {
     if (Sec.Name == SecName) {
       if (Sec.OriginalData.empty())
-        return make_error<StringError>("Can't dump section \"" + SecName +
-                                           "\": it has no contents",
-                                       object_error::parse_failed);
+        return createStringError(
+            object_error::parse_failed,
+            "Can't dump section \"%s\": it has no contents",
+            SecName.str().c_str());
       Expected<std::unique_ptr<FileOutputBuffer>> BufferOrErr =
           FileOutputBuffer::create(Filename, Sec.OriginalData.size());
       if (!BufferOrErr)
         return BufferOrErr.takeError();
       std::unique_ptr<FileOutputBuffer> Buf = std::move(*BufferOrErr);
       std::copy(Sec.OriginalData.begin(), Sec.OriginalData.end(),
                 Buf->getBufferStart());
       if (Error E = Buf->commit())
         return E;
       return Error::success();
     }
   }
-  return make_error<StringError>("Section not found",
-                                 object_error::parse_failed);
+  return createStringError(object_error::parse_failed, "Section not found");
 }
 
 static bool isCompressed(const SectionBase &Section) {
   const char *Magic = "ZLIB";
   return StringRef(Section.Name).startswith(".zdebug") ||
          (Section.OriginalData.size() > strlen(Magic) &&
           !strncmp(reinterpret_cast<const char *>(Section.OriginalData.data()),
                    Magic, strlen(Magic))) ||