[llvm-objcopy][NFC] More error cleanup

[rupprecht]

Summary

This removes calls to error()/reportError() in the main driver (llvm-objcopy.cpp) as well as the associated argv-parsing (CopyConfig.cpp). logAllUnhandledErrors() is now the main way to print errors.

There are still a few uses from within the per-arch drivers, so we can't delete them yet... but almost!

[jhenderson]

I guess this is okay, but I'm wondering if it's gone too far? The majority of cases you've changed here are in the driver layer, so wouldn't likely ever be librarified. What's the motivation for this round?

llvm/tools/llvm-objcopy/CopyConfig.cpp

224

You've added an extra '.' at the end of this error message. I think our standard is (usually) to not have them. (We might want to tidy up the other messages that do have them already).

242

Ditto.

355

New full stop?

370

Ditto

398

Ditto.

464

Ditto.

470

Ditto.

554

Ditto.

604

Ditto.

[rupprecht]

• Fix error message punctuation

[rupprecht]

In D58316#1401185, @jhenderson wrote:

I guess this is okay, but I'm wondering if it's gone too far? The majority of cases you've changed here are in the driver layer, so wouldn't likely ever be librarified. What's the motivation for this round?

Nothing in particular -- I just don't see the need for several custom error() methods in this tool (or maybe any tool, for that matter), and I think it'd be simpler if there was just one way to do error propagation.

There is still a weak link though -- the per-arch drivers (e.g. ELF/ELFObjcopy.cpp) rely on CopyConfig, and CopyConfig.cpp currently relies on llvm-objcopy.h, but just for error handling. It's not a headers dependency (it's not CopyConfig.h that depends on llvm-objcopy.h), so it's not so bad, but I think it's simple enough to do and it makes it more straightforward that CopyConfig can exist completely w/o llvm-objcopy.h (which we may have to do if we librarify this).

llvm/tools/llvm-objcopy/CopyConfig.cpp

224

llvm-objcopy's error() automatically adds a "." when printing the error message, but printing w/ the default handler does not automatically add it, and we assert it ends with "." in some tests.

I don't have a preference, the added "." was to match that behavior. I've gone ahead and fixed all the tests that were checking for a trailing "." instead and reverted these bits. I can do other files in a separate change.

[jhenderson]

LGTM.

llvm/tools/llvm-objcopy/CopyConfig.cpp

224

I could have sworn I double-checked that! Either way, moving away from trailing full-stops is probably the right thing to do, as I think the majority of error messages from other tools do not use them (which I think is wrong, but we should be consistent). Perhaps @jakehehrlich has got something to say on this?

llvm/tools/llvm-objcopy/CopyConfig.h

 #define LLVM_TOOLS_LLVM_OBJCOPY_COPY_CONFIG_H
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Allocator.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/Regex.h"
 // Necessary for llvm::DebugCompressionType::None
 #include "llvm/Target/TargetOptions.h"
 #include <vector>
 
 namespace llvm {
 namespace objcopy {
 

 struct DriverConfig {
   SmallVector<CopyConfig, 1> CopyConfigs;
   BumpPtrAllocator Alloc;
 };
 
 // ParseObjcopyOptions returns the config and sets the input arguments. If a
 // help flag is set then ParseObjcopyOptions will print the help messege and
 // exit.
-DriverConfig parseObjcopyOptions(ArrayRef<const char *> ArgsArr);
+Expected<DriverConfig> parseObjcopyOptions(ArrayRef<const char *> ArgsArr);
 
 // ParseStripOptions returns the config and sets the input arguments. If a
 // help flag is set then ParseStripOptions will print the help messege and
 // exit.
-DriverConfig parseStripOptions(ArrayRef<const char *> ArgsArr);
+Expected<DriverConfig> parseStripOptions(ArrayRef<const char *> ArgsArr);
 
 } // namespace objcopy
 } // namespace llvm
 
 #endif

llvm/tools/llvm-objcopy/CopyConfig.cpp

return llvm::StringSwitch<SectionFlag>(SectionName)
       .Case("rom", SectionFlag::SecRom)
       .Case("merge", SectionFlag::SecMerge)
       .Case("strings", SectionFlag::SecStrings)
       .Case("contents", SectionFlag::SecContents)
       .Case("share", SectionFlag::SecShare)
       .Default(SectionFlag::SecNone);
 }
 
-static uint64_t parseSectionFlagSet(ArrayRef<StringRef> SectionFlags) {
+static Expected<uint64_t>
+parseSectionFlagSet(ArrayRef<StringRef> SectionFlags) {
   SectionFlag ParsedFlags = SectionFlag::SecNone;
   for (StringRef Flag : SectionFlags) {
     SectionFlag ParsedFlag = parseSectionRenameFlag(Flag);
     if (ParsedFlag == SectionFlag::SecNone)
-      error("Unrecognized section flag '" + Flag +
-            "'. Flags supported for GNU compatibility: alloc, load, noload, "
-            "readonly, debug, code, data, rom, share, contents, merge, "
-            "strings.");
+      return createStringError(
+          errc::invalid_argument,
+          "Unrecognized section flag '%s'. Flags supported for GNU "
+          "compatibility: alloc, load, noload, readonly, debug, code, data, "
+          "rom, share, contents, merge, strings.",
+          Flag.str().c_str());
     ParsedFlags |= ParsedFlag;
   }
 
   uint64_t NewFlags = 0;
   if (ParsedFlags & SectionFlag::SecAlloc)
     NewFlags |= ELF::SHF_ALLOC;
   if (!(ParsedFlags & SectionFlag::SecReadonly))
     NewFlags |= ELF::SHF_WRITE;
   if (ParsedFlags & SectionFlag::SecCode)
     NewFlags |= ELF::SHF_EXECINSTR;
   if (ParsedFlags & SectionFlag::SecMerge)
     NewFlags |= ELF::SHF_MERGE;
   if (ParsedFlags & SectionFlag::SecStrings)
     NewFlags |= ELF::SHF_STRINGS;
   return NewFlags;
 }
 
-static SectionRename parseRenameSectionValue(StringRef FlagValue) {
+static Expected<SectionRename> parseRenameSectionValue(StringRef FlagValue) {
   if (!FlagValue.contains('='))
-    error("Bad format for --rename-section: missing '='");
+    return createStringError(errc::invalid_argument,
+                             "Bad format for --rename-section: missing '='");
 
   // Initial split: ".foo" = ".bar,f1,f2,..."
   auto Old2New = FlagValue.split('=');
   SectionRename SR;
   SR.OriginalName = Old2New.first;
 
   // Flags split: ".bar" "f1" "f2" ...
   SmallVector<StringRef, 6> NameAndFlags;
   Old2New.second.split(NameAndFlags, ',');
   SR.NewName = NameAndFlags[0];
 
-  if (NameAndFlags.size() > 1)
-    SR.NewFlags = parseSectionFlagSet(makeArrayRef(NameAndFlags).drop_front());
+  if (NameAndFlags.size() > 1) {
+    Expected<uint64_t> ParsedFlagSet =
+        parseSectionFlagSet(makeArrayRef(NameAndFlags).drop_front());
+    if (!ParsedFlagSet)
+      return ParsedFlagSet.takeError();
+    SR.NewFlags = *ParsedFlagSet;
+  }
 
   return SR;
 }
 
-static SectionFlagsUpdate parseSetSectionFlagValue(StringRef FlagValue) {
+static Expected<SectionFlagsUpdate>
+parseSetSectionFlagValue(StringRef FlagValue) {
   if (!StringRef(FlagValue).contains('='))
-    error("Bad format for --set-section-flags: missing '='");
+    return createStringError(errc::invalid_argument,
+                             "Bad format for --set-section-flags: missing '='");
 
   // Initial split: ".foo" = "f1,f2,..."
   auto Section2Flags = StringRef(FlagValue).split('=');
   SectionFlagsUpdate SFU;
   SFU.Name = Section2Flags.first;
 
   // Flags split: "f1" "f2" ...
   SmallVector<StringRef, 6> SectionFlags;
   Section2Flags.second.split(SectionFlags, ',');
-  SFU.NewFlags = parseSectionFlagSet(SectionFlags);
+  Expected<uint64_t> ParsedFlagSet = parseSectionFlagSet(SectionFlags);
+  if (!ParsedFlagSet)
+    return ParsedFlagSet.takeError();
+  SFU.NewFlags = *ParsedFlagSet;
 
   return SFU;
 }
 
 static const StringMap<MachineInfo> ArchMap{
     // Name, {EMachine, 64bit, LittleEndian}
     {"aarch64", {ELF::EM_AARCH64, true, true}},
     {"arm", {ELF::EM_ARM, false, true}},
     {"i386", {ELF::EM_386, false, true}},
     {"i386:x86-64", {ELF::EM_X86_64, true, true}},
     {"powerpc:common64", {ELF::EM_PPC64, true, true}},
     {"sparc", {ELF::EM_SPARC, false, true}},
     {"x86-64", {ELF::EM_X86_64, true, true}},
 };
 
-static const MachineInfo &getMachineInfo(StringRef Arch) {
+static Expected<const MachineInfo &> getMachineInfo(StringRef Arch) {
   auto Iter = ArchMap.find(Arch);
   if (Iter == std::end(ArchMap))
-    error("Invalid architecture: '" + Arch + "'");
+    return createStringError(errc::invalid_argument,
+                             "Invalid architecture: '%s'.", Arch.str().c_str());
   return Iter->getValue();
 }
 
 static const StringMap<MachineInfo> OutputFormatMap{
     // Name, {EMachine, 64bit, LittleEndian}
     {"elf32-i386", {ELF::EM_386, false, true}},
     {"elf32-powerpcle", {ELF::EM_PPC, false, true}},
     {"elf32-x86-64", {ELF::EM_X86_64, false, true}},
     {"elf64-powerpcle", {ELF::EM_PPC64, true, true}},
     {"elf64-x86-64", {ELF::EM_X86_64, true, true}},
 };
 
-static const MachineInfo &getOutputFormatMachineInfo(StringRef Format) {
+static Expected<const MachineInfo &>
+getOutputFormatMachineInfo(StringRef Format) {
   auto Iter = OutputFormatMap.find(Format);
   if (Iter == std::end(OutputFormatMap))
-    error("Invalid output format: '" + Format + "'");
+    return createStringError(errc::invalid_argument,
+                             "Invalid output format: '%s'.",
+                             Format.str().c_str());
   return Iter->getValue();
 }
 
-static void addSymbolsFromFile(std::vector<NameOrRegex> &Symbols,
+static Error addSymbolsFromFile(std::vector<NameOrRegex> &Symbols,
                                 BumpPtrAllocator &Alloc, StringRef Filename,
                                 bool UseRegex) {
   StringSaver Saver(Alloc);
   SmallVector<StringRef, 16> Lines;
   auto BufOrErr = MemoryBuffer::getFile(Filename);
   if (!BufOrErr)
-    reportError(Filename, BufOrErr.getError());
+    return createFileError(Filename, BufOrErr.getError());
 
   BufOrErr.get()->getBuffer().split(Lines, '\n');
   for (StringRef Line : Lines) {
     // Ignore everything after '#', trim whitespace, and only add the symbol if
     // it's not empty.
     auto TrimmedLine = Line.split('#').first.trim();
     if (!TrimmedLine.empty())
       Symbols.emplace_back(Saver.save(TrimmedLine), UseRegex);
   }
+
+  return Error::success();
 }
 
 NameOrRegex::NameOrRegex(StringRef Pattern, bool IsRegex) {
   if (!IsRegex) {
     Name = Pattern;
     return;
   }
 
if (NewName.empty())
                                Filename.str().c_str(), LineNo + 1);
     SymbolsToRename.insert({Pair.first, NewName});
   }
   return Error::success();
 }
 // ParseObjcopyOptions returns the config and sets the input arguments. If a
 // help flag is set then ParseObjcopyOptions will print the help messege and
 // exit.
-DriverConfig parseObjcopyOptions(ArrayRef<const char *> ArgsArr) {
+Expected<DriverConfig> parseObjcopyOptions(ArrayRef<const char *> ArgsArr) {
   DriverConfig DC;
   ObjcopyOptTable T;
   unsigned MissingArgumentIndex, MissingArgumentCount;
   llvm::opt::InputArgList InputArgs =
       T.ParseArgs(ArgsArr, MissingArgumentIndex, MissingArgumentCount);
 
   if (InputArgs.size() == 0) {
     T.PrintHelp(errs(), "llvm-objcopy input [output]", "objcopy tool");
if (InputArgs.hasArg(OBJCOPY_version)) {
     outs() << "llvm-objcopy, compatible with GNU objcopy\n";
     cl::PrintVersionMessage();
     exit(0);
   }
 
   SmallVector<const char *, 2> Positional;
 
   for (auto Arg : InputArgs.filtered(OBJCOPY_UNKNOWN))
-    error("unknown argument '" + Arg->getAsString(InputArgs) + "'");
+    return createStringError(errc::invalid_argument, "unknown argument '%s'",
+                             Arg->getAsString(InputArgs).c_str());
 
   for (auto Arg : InputArgs.filtered(OBJCOPY_INPUT))
     Positional.push_back(Arg->getValue());
 
   if (Positional.empty())
-    error("No input file specified");
+    return createStringError(errc::invalid_argument, "No input file specified");
 
   if (Positional.size() > 2)
-    error("Too many positional arguments");
+    return createStringError(errc::invalid_argument,
+                             "Too many positional arguments");
 
   CopyConfig Config;
   Config.InputFilename = Positional[0];
   Config.OutputFilename = Positional[Positional.size() == 1 ? 0 : 1];
   if (InputArgs.hasArg(OBJCOPY_target) &&
       (InputArgs.hasArg(OBJCOPY_input_target) ||
        InputArgs.hasArg(OBJCOPY_output_target)))
-    error("--target cannot be used with --input-target or --output-target");
+    return createStringError(
+        errc::invalid_argument,
+        "--target cannot be used with --input-target or --output-target.");
 
   bool UseRegex = InputArgs.hasArg(OBJCOPY_regex);
   if (InputArgs.hasArg(OBJCOPY_target)) {
     Config.InputFormat = InputArgs.getLastArgValue(OBJCOPY_target);
     Config.OutputFormat = InputArgs.getLastArgValue(OBJCOPY_target);
   } else {
     Config.InputFormat = InputArgs.getLastArgValue(OBJCOPY_input_target);
     Config.OutputFormat = InputArgs.getLastArgValue(OBJCOPY_output_target);
   }
   if (Config.InputFormat == "binary") {
     auto BinaryArch = InputArgs.getLastArgValue(OBJCOPY_binary_architecture);
     if (BinaryArch.empty())
-      error("Specified binary input without specifiying an architecture");
-    Config.BinaryArch = getMachineInfo(BinaryArch);
+      return createStringError(
+          errc::invalid_argument,
+          "Specified binary input without specifiying an architecture.");
+    Expected<const MachineInfo &> MI = getMachineInfo(BinaryArch);
+    if (!MI)
+      return MI.takeError();
+    Config.BinaryArch = *MI;
+  }
+  if (!Config.OutputFormat.empty() && Config.OutputFormat != "binary") {
+    Expected<const MachineInfo &> MI =
+        getOutputFormatMachineInfo(Config.OutputFormat);
+    if (!MI)
+      return MI.takeError();
+    Config.OutputArch = *MI;
   }
-  if (!Config.OutputFormat.empty() && Config.OutputFormat != "binary")
-    Config.OutputArch = getOutputFormatMachineInfo(Config.OutputFormat);
 
   if (auto Arg = InputArgs.getLastArg(OBJCOPY_compress_debug_sections,
                                       OBJCOPY_compress_debug_sections_eq)) {
     Config.CompressionType = DebugCompressionType::Z;
 
     if (Arg->getOption().getID() == OBJCOPY_compress_debug_sections_eq) {
       Config.CompressionType =
           StringSwitch<DebugCompressionType>(
               InputArgs.getLastArgValue(OBJCOPY_compress_debug_sections_eq))
               .Case("zlib-gnu", DebugCompressionType::GNU)
               .Case("zlib", DebugCompressionType::Z)
               .Default(DebugCompressionType::None);
       if (Config.CompressionType == DebugCompressionType::None)
-        error("Invalid or unsupported --compress-debug-sections format: " +
-              InputArgs.getLastArgValue(OBJCOPY_compress_debug_sections_eq));
+        return createStringError(
+            errc::invalid_argument,
+            "Invalid or unsupported --compress-debug-sections format: %s.",
+            InputArgs.getLastArgValue(OBJCOPY_compress_debug_sections_eq)
+                .str()
+                .c_str());
       if (!zlib::isAvailable())
-        error("LLVM was not compiled with LLVM_ENABLE_ZLIB: can not compress.");
+        return createStringError(
+            errc::invalid_argument,
+            "LLVM was not compiled with LLVM_ENABLE_ZLIB: can not compress.");
     }
   }
 
   Config.AddGnuDebugLink = InputArgs.getLastArgValue(OBJCOPY_add_gnu_debuglink);
   Config.BuildIdLinkDir = InputArgs.getLastArgValue(OBJCOPY_build_id_link_dir);
   if (InputArgs.hasArg(OBJCOPY_build_id_link_input))
     Config.BuildIdLinkInput =
         InputArgs.getLastArgValue(OBJCOPY_build_id_link_input);
   if (InputArgs.hasArg(OBJCOPY_build_id_link_output))
     Config.BuildIdLinkOutput =
         InputArgs.getLastArgValue(OBJCOPY_build_id_link_output);
   Config.SplitDWO = InputArgs.getLastArgValue(OBJCOPY_split_dwo);
   Config.SymbolsPrefix = InputArgs.getLastArgValue(OBJCOPY_prefix_symbols);
 
   for (auto Arg : InputArgs.filtered(OBJCOPY_redefine_symbol)) {
     if (!StringRef(Arg->getValue()).contains('='))
-      error("Bad format for --redefine-sym");
+      return createStringError(errc::invalid_argument,
+                               "Bad format for --redefine-sym");
     auto Old2New = StringRef(Arg->getValue()).split('=');
     if (!Config.SymbolsToRename.insert(Old2New).second)
-      error("Multiple redefinition of symbol " + Old2New.first);
+      return createStringError(errc::invalid_argument,
+                               "Multiple redefinition of symbol %s",
+                               Old2New.first.str().c_str());
   }
 
   for (auto Arg : InputArgs.filtered(OBJCOPY_redefine_symbols))
     if (Error E = addSymbolsToRenameFromFile(Config.SymbolsToRename, DC.Alloc,
                                              Arg->getValue()))
-      error(std::move(E));
+      return std::move(E);
 
   for (auto Arg : InputArgs.filtered(OBJCOPY_rename_section)) {
-    SectionRename SR = parseRenameSectionValue(StringRef(Arg->getValue()));
-    if (!Config.SectionsToRename.try_emplace(SR.OriginalName, SR).second)
-      error("Multiple renames of section " + SR.OriginalName);
+    Expected<SectionRename> SR =
+        parseRenameSectionValue(StringRef(Arg->getValue()));
+    if (!SR)
+      return SR.takeError();
+    if (!Config.SectionsToRename.try_emplace(SR->OriginalName, *SR).second)
+      return createStringError(errc::invalid_argument,
+                               "Multiple renames of section %s",
+                               SR->OriginalName.str().c_str());
   }
   for (auto Arg : InputArgs.filtered(OBJCOPY_set_section_flags)) {
-    SectionFlagsUpdate SFU = parseSetSectionFlagValue(Arg->getValue());
-    if (!Config.SetSectionFlags.try_emplace(SFU.Name, SFU).second)
-      error("--set-section-flags set multiple times for section " + SFU.Name);
+    Expected<SectionFlagsUpdate> SFU =
+        parseSetSectionFlagValue(Arg->getValue());
+    if (!SFU)
+      return SFU.takeError();
+    if (!Config.SetSectionFlags.try_emplace(SFU->Name, *SFU).second)
+      return createStringError(
+          errc::invalid_argument,
+          "--set-section-flags set multiple times for section %s",
+          SFU->Name.str().c_str());
   }
   // Prohibit combinations of --set-section-flags when the section name is used
   // by --rename-section, either as a source or a destination.
   for (const auto &E : Config.SectionsToRename) {
     const SectionRename &SR = E.second;
     if (Config.SetSectionFlags.count(SR.OriginalName))
-      error("--set-section-flags=" + SR.OriginalName +
-            " conflicts with --rename-section=" + SR.OriginalName + "=" +
-            SR.NewName);
+      return createStringError(
+          errc::invalid_argument,
+          "--set-section-flags=%s conflicts with --rename-section=%s=%s.",
+          SR.OriginalName.str().c_str(), SR.OriginalName.str().c_str(),
+          SR.NewName.str().c_str());
     if (Config.SetSectionFlags.count(SR.NewName))
-      error("--set-section-flags=" + SR.NewName +
-            " conflicts with --rename-section=" + SR.OriginalName + "=" +
-            SR.NewName);
+      return createStringError(
+          errc::invalid_argument,
+          "--set-section-flags=%s conflicts with --rename-section=%s=%s.",
+          SR.NewName.str().c_str(), SR.OriginalName.str().c_str(),
+          SR.NewName.str().c_str());
   }
 
   for (auto Arg : InputArgs.filtered(OBJCOPY_remove_section))
     Config.ToRemove.emplace_back(Arg->getValue(), UseRegex);
   for (auto Arg : InputArgs.filtered(OBJCOPY_keep_section))
     Config.KeepSection.emplace_back(Arg->getValue(), UseRegex);
   for (auto Arg : InputArgs.filtered(OBJCOPY_only_section))
     Config.OnlySection.emplace_back(Arg->getValue(), UseRegex);
Config.DiscardMode =
             : DiscardType::Locals;
   Config.OnlyKeepDebug = InputArgs.hasArg(OBJCOPY_only_keep_debug);
   Config.KeepFileSymbols = InputArgs.hasArg(OBJCOPY_keep_file_symbols);
   Config.DecompressDebugSections =
       InputArgs.hasArg(OBJCOPY_decompress_debug_sections);
   for (auto Arg : InputArgs.filtered(OBJCOPY_localize_symbol))
     Config.SymbolsToLocalize.emplace_back(Arg->getValue(), UseRegex);
   for (auto Arg : InputArgs.filtered(OBJCOPY_localize_symbols))
-    addSymbolsFromFile(Config.SymbolsToLocalize, DC.Alloc, Arg->getValue(),
-                       UseRegex);
+    if (Error E = addSymbolsFromFile(Config.SymbolsToLocalize, DC.Alloc,
+                                     Arg->getValue(), UseRegex))
+      return std::move(E);
   for (auto Arg : InputArgs.filtered(OBJCOPY_keep_global_symbol))
     Config.SymbolsToKeepGlobal.emplace_back(Arg->getValue(), UseRegex);
   for (auto Arg : InputArgs.filtered(OBJCOPY_keep_global_symbols))
-    addSymbolsFromFile(Config.SymbolsToKeepGlobal, DC.Alloc, Arg->getValue(),
-                       UseRegex);
+    if (Error E = addSymbolsFromFile(Config.SymbolsToKeepGlobal, DC.Alloc,
+                                     Arg->getValue(), UseRegex))
+      return std::move(E);
   for (auto Arg : InputArgs.filtered(OBJCOPY_globalize_symbol))
     Config.SymbolsToGlobalize.emplace_back(Arg->getValue(), UseRegex);
   for (auto Arg : InputArgs.filtered(OBJCOPY_globalize_symbols))
-    addSymbolsFromFile(Config.SymbolsToGlobalize, DC.Alloc, Arg->getValue(),
-                       UseRegex);
+    if (Error E = addSymbolsFromFile(Config.SymbolsToGlobalize, DC.Alloc,
+                                     Arg->getValue(), UseRegex))
+      return std::move(E);
   for (auto Arg : InputArgs.filtered(OBJCOPY_weaken_symbol))
     Config.SymbolsToWeaken.emplace_back(Arg->getValue(), UseRegex);
   for (auto Arg : InputArgs.filtered(OBJCOPY_weaken_symbols))
-    addSymbolsFromFile(Config.SymbolsToWeaken, DC.Alloc, Arg->getValue(),
-                       UseRegex);
+    if (Error E = addSymbolsFromFile(Config.SymbolsToWeaken, DC.Alloc,
+                                     Arg->getValue(), UseRegex))
+      return std::move(E);
   for (auto Arg : InputArgs.filtered(OBJCOPY_strip_symbol))
     Config.SymbolsToRemove.emplace_back(Arg->getValue(), UseRegex);
   for (auto Arg : InputArgs.filtered(OBJCOPY_strip_symbols))
-    addSymbolsFromFile(Config.SymbolsToRemove, DC.Alloc, Arg->getValue(),
-                       UseRegex);
+    if (Error E = addSymbolsFromFile(Config.SymbolsToRemove, DC.Alloc,
+                                     Arg->getValue(), UseRegex))
+      return std::move(E);
   for (auto Arg : InputArgs.filtered(OBJCOPY_strip_unneeded_symbol))
     Config.UnneededSymbolsToRemove.emplace_back(Arg->getValue(), UseRegex);
   for (auto Arg : InputArgs.filtered(OBJCOPY_strip_unneeded_symbols))
-    addSymbolsFromFile(Config.UnneededSymbolsToRemove, DC.Alloc,
-                       Arg->getValue(), UseRegex);
+    if (Error E = addSymbolsFromFile(Config.UnneededSymbolsToRemove, DC.Alloc,
+                                     Arg->getValue(), UseRegex))
+      return std::move(E);
   for (auto Arg : InputArgs.filtered(OBJCOPY_keep_symbol))
     Config.SymbolsToKeep.emplace_back(Arg->getValue(), UseRegex);
 
   Config.DeterministicArchives = InputArgs.hasFlag(
       OBJCOPY_enable_deterministic_archives,
       OBJCOPY_disable_deterministic_archives, /*default=*/true);
 
   Config.PreserveDates = InputArgs.hasArg(OBJCOPY_preserve_dates);
 
   if (Config.DecompressDebugSections &&
       Config.CompressionType != DebugCompressionType::None) {
-    error("Cannot specify --compress-debug-sections at the same time as "
-          "--decompress-debug-sections at the same time");
+    return createStringError(
+        errc::invalid_argument,
+        "Cannot specify --compress-debug-sections at the same time as "
+        "--decompress-debug-sections at the same time.");
   }
 
   if (Config.DecompressDebugSections && !zlib::isAvailable())
-    error("LLVM was not compiled with LLVM_ENABLE_ZLIB: cannot decompress.");
+    return createStringError(
+        errc::invalid_argument,
+        "LLVM was not compiled with LLVM_ENABLE_ZLIB: cannot decompress.");
 
   DC.CopyConfigs.push_back(std::move(Config));
   return DC;
 }
 
 // ParseStripOptions returns the config and sets the input arguments. If a
 // help flag is set then ParseStripOptions will print the help messege and
 // exit.
-DriverConfig parseStripOptions(ArrayRef<const char *> ArgsArr) {
+Expected<DriverConfig> parseStripOptions(ArrayRef<const char *> ArgsArr) {
   StripOptTable T;
   unsigned MissingArgumentIndex, MissingArgumentCount;
   llvm::opt::InputArgList InputArgs =
       T.ParseArgs(ArgsArr, MissingArgumentIndex, MissingArgumentCount);
 
   if (InputArgs.size() == 0) {
     T.PrintHelp(errs(), "llvm-strip [options] file...", "strip tool");
     exit(1);
   }
 
   if (InputArgs.hasArg(STRIP_help)) {
     T.PrintHelp(outs(), "llvm-strip [options] file...", "strip tool");
     exit(0);
   }
 
   if (InputArgs.hasArg(STRIP_version)) {
     outs() << "llvm-strip, compatible with GNU strip\n";
     cl::PrintVersionMessage();
     exit(0);
   }
 
   SmallVector<const char *, 2> Positional;
   for (auto Arg : InputArgs.filtered(STRIP_UNKNOWN))
-    error("unknown argument '" + Arg->getAsString(InputArgs) + "'");
+    return createStringError(errc::invalid_argument, "unknown argument '%s'",
+                             Arg->getAsString(InputArgs).c_str());
   for (auto Arg : InputArgs.filtered(STRIP_INPUT))
     Positional.push_back(Arg->getValue());
 
   if (Positional.empty())
-    error("No input file specified");
+    return createStringError(errc::invalid_argument, "No input file specified");
 
   if (Positional.size() > 1 && InputArgs.hasArg(STRIP_output))
-    error("Multiple input files cannot be used in combination with -o");
+    return createStringError(
+        errc::invalid_argument,
+        "Multiple input files cannot be used in combination with -o.");
 
   CopyConfig Config;
   bool UseRegexp = InputArgs.hasArg(STRIP_regex);
   Config.StripDebug = InputArgs.hasArg(STRIP_strip_debug);
 
   if (InputArgs.hasArg(STRIP_discard_all, STRIP_discard_locals))
     Config.DiscardMode =
         InputArgs.hasFlag(STRIP_discard_all, STRIP_discard_locals)

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

static Error executeObjcopyOnArchive(const CopyConfig &Config,
   if (Err)
     return createFileError(Config.InputFilename, std::move(Err));
 
   return deepWriteArchive(Config.OutputFilename, NewArchiveMembers,
                           Ar.hasSymbolTable(), Ar.kind(),
                           Config.DeterministicArchives, Ar.isThin());
 }
 
-static void restoreDateOnFile(StringRef Filename,
+static Error restoreDateOnFile(StringRef Filename,
                                const sys::fs::file_status &Stat) {
   int FD;
 
   if (auto EC =
           sys::fs::openFileForWrite(Filename, FD, sys::fs::CD_OpenExisting))
-    reportError(Filename, EC);
+    return createFileError(Filename, EC);
 
   if (auto EC = sys::fs::setLastAccessAndModificationTime(
           FD, Stat.getLastAccessedTime(), Stat.getLastModificationTime()))
-    reportError(Filename, EC);
+    return createFileError(Filename, EC);
 
   if (auto EC = sys::Process::SafelyCloseFileDescriptor(FD))
-    reportError(Filename, EC);
+    return createFileError(Filename, EC);
+
+  return Error::success();
 }
 
 /// The function executeObjcopy does the higher level dispatch based on the type
 /// of input (raw binary, archive or single object file) and takes care of the
 /// format-agnostic modifications, i.e. preserving dates.
-static void executeObjcopy(const CopyConfig &Config) {
+static Error executeObjcopy(const CopyConfig &Config) {
   sys::fs::file_status Stat;
   if (Config.PreserveDates)
     if (auto EC = sys::fs::status(Config.InputFilename, Stat))
-      reportError(Config.InputFilename, EC);
+      return createFileError(Config.InputFilename, EC);
 
   if (Config.InputFormat == "binary") {
     auto BufOrErr = MemoryBuffer::getFile(Config.InputFilename);
     if (!BufOrErr)
-      reportError(Config.InputFilename, BufOrErr.getError());
+      return createFileError(Config.InputFilename, BufOrErr.getError());
     FileBuffer FB(Config.OutputFilename);
     if (Error E = executeObjcopyOnRawBinary(Config, *BufOrErr->get(), FB))
-      error(std::move(E));
+      return E;
   } else {
     Expected<OwningBinary<llvm::object::Binary>> BinaryOrErr =
         createBinary(Config.InputFilename);
     if (!BinaryOrErr)
-      reportError(Config.InputFilename, BinaryOrErr.takeError());
+      return createFileError(Config.InputFilename, BinaryOrErr.takeError());
 
     if (Archive *Ar = dyn_cast<Archive>(BinaryOrErr.get().getBinary())) {
       if (Error E = executeObjcopyOnArchive(Config, *Ar))
-        error(std::move(E));
+        return E;
     } else {
       FileBuffer FB(Config.OutputFilename);
       if (Error E = executeObjcopyOnBinary(Config,
                                            *BinaryOrErr.get().getBinary(), FB))
-        error(std::move(E));
+        return E;
     }
   }
 
   if (Config.PreserveDates) {
-    restoreDateOnFile(Config.OutputFilename, Stat);
+    if (Error E = restoreDateOnFile(Config.OutputFilename, Stat))
+      return E;
     if (!Config.SplitDWO.empty())
-      restoreDateOnFile(Config.SplitDWO, Stat);
+      if (Error E = restoreDateOnFile(Config.SplitDWO, Stat))
+        return E;
   }
+
+  return Error::success();
 }
 
 int main(int argc, char **argv) {
   InitLLVM X(argc, argv);
   ToolName = argv[0];
-  DriverConfig DriverConfig;
-  if (sys::path::stem(ToolName).contains("strip"))
-    DriverConfig = parseStripOptions(makeArrayRef(argv + 1, argc));
-  else
-    DriverConfig = parseObjcopyOptions(makeArrayRef(argv + 1, argc));
-  for (const CopyConfig &CopyConfig : DriverConfig.CopyConfigs)
-    executeObjcopy(CopyConfig);
+  bool IsStrip = sys::path::stem(ToolName).contains("strip");
+  Expected<DriverConfig> DriverConfig =
+      IsStrip ? parseStripOptions(makeArrayRef(argv + 1, argc))
+              : parseObjcopyOptions(makeArrayRef(argv + 1, argc));
+  if (!DriverConfig) {
+    logAllUnhandledErrors(DriverConfig.takeError(),
+                          WithColor::error(errs(), ToolName));
+    return 1;
+  }
+  for (const CopyConfig &CopyConfig : DriverConfig->CopyConfigs) {
+    if (Error E = executeObjcopy(CopyConfig)) {
+      logAllUnhandledErrors(std::move(E), WithColor::error(errs(), ToolName));
+      return 1;
+    }
+  }
+
+  return 0;
 }