[llvm-objcopy] Changed command line parsing errors

[abrachet]

Summary

Tidied up errors during command line parsing to be more consistent with the rest of llvm-objcopy errors.

[abrachet]

Added a test case for --preserve-dates

[abrachet]

Fixed invalid-preserve-dates test case

[rupprecht]

llvm/test/tools/llvm-objcopy/ELF/preserve-dates.test

1 ↗

(On Diff #203422)

This test is redundant w/ strip-preserve-atime.test and strip-preserve-mtime.test, so you can remove it.

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

210–211

Flag validation errors should go in CopyConfig.cpp whenever possible so we can do simple validation as early as possible.

[abrachet]

Moved the check to preserve dates in parseXXOptions(). Fixed error messages to be lower case start, no period at the end.

[abrachet]

removed redundant test

[abrachet]

Ran clang-format

[abrachet]

Updated test cases and changed poorly worded error

[rupprecht]

llvm/tools/llvm-objcopy/CopyConfig.cpp

666–667

These can be combined, i.e. if (Config.PreserveDates && (Config.OutputFilename == "-" || Config.InputFilename == "-"))

805–806

strip is a little more exotic in the way it accepts args; objcopy only supports:

$ llvm-objcopy foo  # "copies" in place
$ llvm-objcopy foo bar
$ llvm-objcopy foo bar baz # not valid

Whereas strip supports a couple variants, including N args:

$ llvm-strip foo  # strips in place
$ llvm-strip foo bar baz ... # also strips N objects in place
$ llvm-strip foo -o bar # strips foo, saving it as bar

and the way it's handled above (with DriverConfig) is that we create one CopyConfig with all the non-filename args, and then modify it in-place when we copy it into DriverConfig. So, Config.OutputFilename here is really just the *last* filename. (e.g. try this: llvm-strip -p - foo).

An alternative for strip (and maybe we could use the same thing for objcopy, I don't have a preference) would be to just check directly against the positional args:

if (Config.PreserveDates && (llvm::is_contained(Positional, "-") || InputArgs.getLastArgValue(STRIP_output) == "-"))

(see include/llvm/ADT/STLExtras.h)

[abrachet]

llvm/tools/llvm-objcopy/CopyConfig.cpp

483

No quotes around the format because it can only be zlib or zlib-gnu neither of which have spaces. Can be easily added though.

[abrachet]

Account for strip handling of input files

[abrachet]

Small change in executeObjcopyOnBinary()

[rupprecht]

I'm good with this after these two comments:

llvm/test/tools/llvm-objcopy/ELF/invalid-preserve-dates.test

3

This actually isn't being run because there is no FileCheck command above. The commands should be like:

# RUN: not llvm-objcopy --preserve-dates - %t < %p/Inputs/alloc-symtab.o 2>&1 | FileCheck %s

5

Can you add some cases for strip?

# Testing N args
not llvm-strip --preserve-dates -
not llvm-strip --preserve-dates %p/Inputs/alloc-symtab.o -
not llvm-strip --preserve-dates - %p/Inputs/alloc-symtab.o
not llvm-strip --preserve-dates %p/Inputs/alloc-symtab.o - %p/Inputs/alloc-symtab.o

# Testing -o
not llvm-strip --preserve-dates - -o %p/Inputs/alloc-symtab.o
not llvm-strip --preserve-dates %p/Inputs/alloc-symtab.o -o -

(and others if you think of any)

[jhenderson]

Tidied up errors during command line parsing to be more consistent with the rest of llvm-objcopy errors. Also, ensure --preserve-dates is only ever used when both input and output files are regular files.

I haven't looked in depth, and I expect there's nothing wrong with these changes, but really they should be two separate commits. One for "tidying up error messages" and the other for the --preserve-dates check. Could you split them up before committing, please?

Also a quick tip for when writing tests for bug fixes: make sure the test fails prior to your change in the way you'd expect it to. This would have picked up the missing FileCheck, above I believe.

[abrachet]

Moved --preserve-dates error handling to a new patch

[rupprecht]

Thanks for splitting up the patch! I should have requested that earlier.

[jhenderson]

LGTM too.

llvm/test/tools/llvm-objcopy/ELF/invalid-preserve-dates.test

+# RUN: not llvm-objcopy --preserve-dates - %t < %p/Inputs/alloc-symtab.o
+# CHECK: error: --preserve-dates requires a file
+
+# RUN: not llvm-objcopy --preserve-dates %p/Inputs/alloc-symtab.o - 
+# CHECK: error: --preserve-dates requires a file

llvm/tools/llvm-objcopy/CopyConfig.cpp

 static Expected<SectionFlag>
 parseSectionFlagSet(ArrayRef<StringRef> SectionFlags) {
   SectionFlag ParsedFlags = SectionFlag::SecNone;
   for (StringRef Flag : SectionFlags) {
     SectionFlag ParsedFlag = parseSectionRenameFlag(Flag);
     if (ParsedFlag == SectionFlag::SecNone)
       return createStringError(
           errc::invalid_argument,
-          "Unrecognized section flag '%s'. Flags supported for GNU "
+          "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;
   }
 
   return ParsedFlags;
 }
 
 static Expected<SectionRename> parseRenameSectionValue(StringRef FlagValue) {
   if (!FlagValue.contains('='))
     return createStringError(errc::invalid_argument,
-                             "Bad format for --rename-section: missing '='");
+                             "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;
static Expected<SectionRename> parseRenameSectionValue(StringRef FlagValue) {
 
   return SR;
 }
 
 static Expected<SectionFlagsUpdate>
 parseSetSectionFlagValue(StringRef FlagValue) {
   if (!StringRef(FlagValue).contains('='))
     return createStringError(errc::invalid_argument,
-                             "Bad format for --set-section-flags: missing '='");
+                             "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;
static const StringMap<MachineInfo> ArchMap{
     {"sparc", {ELF::EM_SPARC, false, true}},
     {"x86-64", {ELF::EM_X86_64, true, true}},
 };
 
 static Expected<const MachineInfo &> getMachineInfo(StringRef Arch) {
   auto Iter = ArchMap.find(Arch);
   if (Iter == std::end(ArchMap))
     return createStringError(errc::invalid_argument,
-                             "Invalid architecture: '%s'", Arch.str().c_str());
+                             "invalid architecture: '%s'", Arch.str().c_str());
   return Iter->getValue();
 }
 
 // FIXME: consolidate with the bfd parsing used by lld.
 static const StringMap<MachineInfo> OutputFormatMap{
     // Name, {EMachine, 64bit, LittleEndian}
     // x86
     {"elf32-i386", {ELF::EM_386, false, true}},

 };
 
 static Expected<MachineInfo> getOutputFormatMachineInfo(StringRef Format) {
   StringRef OriginalFormat = Format;
   bool IsFreeBSD = Format.consume_back("-freebsd");
   auto Iter = OutputFormatMap.find(Format);
   if (Iter == std::end(OutputFormatMap))
     return createStringError(errc::invalid_argument,
-                             "Invalid output format: '%s'",
+                             "invalid output format: '%s'",
                              OriginalFormat.str().c_str());
   MachineInfo MI = Iter->getValue();
   if (IsFreeBSD)
     MI.OSABI = ELF::ELFOSABI_FREEBSD;
   return {MI};
 }
 
 static Error addSymbolsFromFile(std::vector<NameOrRegex> &Symbols,
Expected<DriverConfig> parseObjcopyOptions(ArrayRef<const char *> ArgsArr) {
   for (auto Arg : InputArgs.filtered(OBJCOPY_UNKNOWN))
     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())
-    return createStringError(errc::invalid_argument, "No input file specified");
+    return createStringError(errc::invalid_argument, "no input file specified");
 
   if (Positional.size() > 2)
     return createStringError(errc::invalid_argument,
-                             "Too many positional arguments");
+                             "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)))
     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())
       return createStringError(
           errc::invalid_argument,
-          "Specified binary input without specifiying an architecture");
+          "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" &&
       Config.OutputFormat != "ihex") {
     Expected<MachineInfo> MI = getOutputFormatMachineInfo(Config.OutputFormat);
if (Arg->getOption().getID() == OBJCOPY_compress_debug_sections_eq) {
           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)
         return createStringError(
             errc::invalid_argument,
-            "Invalid or unsupported --compress-debug-sections format: %s",
+            "invalid or unsupported --compress-debug-sections format: %s",
             InputArgs.getLastArgValue(OBJCOPY_compress_debug_sections_eq)
                 .str()
                 .c_str());
     }
     if (!zlib::isAvailable())
       return createStringError(
           errc::invalid_argument,
           "LLVM was not compiled with LLVM_ENABLE_ZLIB: can not compress");
Expected<DriverConfig> parseObjcopyOptions(ArrayRef<const char *> ArgsArr) {
   Config.SplitDWO = InputArgs.getLastArgValue(OBJCOPY_split_dwo);
   Config.SymbolsPrefix = InputArgs.getLastArgValue(OBJCOPY_prefix_symbols);
   Config.AllocSectionsPrefix =
       InputArgs.getLastArgValue(OBJCOPY_prefix_alloc_sections);
 
   for (auto Arg : InputArgs.filtered(OBJCOPY_redefine_symbol)) {
     if (!StringRef(Arg->getValue()).contains('='))
       return createStringError(errc::invalid_argument,
-                               "Bad format for --redefine-sym");
+                               "bad format for --redefine-sym");
     auto Old2New = StringRef(Arg->getValue()).split('=');
     if (!Config.SymbolsToRename.insert(Old2New).second)
       return createStringError(errc::invalid_argument,
-                               "Multiple redefinition of symbol %s",
+                               "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()))
       return std::move(E);
 
   for (auto Arg : InputArgs.filtered(OBJCOPY_rename_section)) {
     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",
+                               "multiple renames of section '%s'",
                                SR->OriginalName.str().c_str());
   }
   for (auto Arg : InputArgs.filtered(OBJCOPY_set_section_flags)) {
     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",
+          "--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))
       return createStringError(
Expected<DriverConfig> parseObjcopyOptions(ArrayRef<const char *> ArgsArr) {
   Config.AllowBrokenLinks = InputArgs.hasArg(OBJCOPY_allow_broken_links);
 
   Config.DeterministicArchives = InputArgs.hasFlag(
       OBJCOPY_enable_deterministic_archives,
       OBJCOPY_disable_deterministic_archives, /*default=*/true);
 
   Config.PreserveDates = InputArgs.hasArg(OBJCOPY_preserve_dates);
 
+  if (Config.OutputFilename == "-" || Config.InputFilename == "-")
+    if (Config.PreserveDates)
+      return createStringError(errc::invalid_argument, "--preserve-dates requires a file");
+
   for (auto Arg : InputArgs)
     if (Arg->getOption().matches(OBJCOPY_set_start)) {
       auto EAddr = getAsInteger<uint64_t>(Arg->getValue());
       if (!EAddr)
         return createStringError(
             EAddr.getError(), "bad entry point address: '%s'", Arg->getValue());
 
       Config.EntryExpr = [EAddr](uint64_t) { return *EAddr; };
if (Arg->getOption().matches(OBJCOPY_set_start)) {
         return Expr(EAddr) + *EIncr;
       };
     }
 
   if (Config.DecompressDebugSections &&
       Config.CompressionType != DebugCompressionType::None) {
     return createStringError(
         errc::invalid_argument,
-        "Cannot specify --compress-debug-sections at the same time as "
+        "cannot specify --compress-debug-sections at the same time as "
         "--decompress-debug-sections at the same time");
   }
 
   if (Config.DecompressDebugSections && !zlib::isAvailable())
     return createStringError(
         errc::invalid_argument,
         "LLVM was not compiled with LLVM_ENABLE_ZLIB: cannot decompress");
 
Expected<DriverConfig> parseStripOptions(ArrayRef<const char *> ArgsArr) {
   SmallVector<const char *, 2> Positional;
   for (auto Arg : InputArgs.filtered(STRIP_UNKNOWN))
     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())
-    return createStringError(errc::invalid_argument, "No input file specified");
+    return createStringError(errc::invalid_argument, "no input file specified");
 
   if (Positional.size() > 1 && InputArgs.hasArg(STRIP_output))
     return createStringError(
         errc::invalid_argument,
-        "Multiple input files cannot be used in combination with -o");
+        "multiple input files cannot be used in combination with -o");
 
   CopyConfig Config;
   bool UseRegexp = InputArgs.hasArg(STRIP_regex);
   Config.AllowBrokenLinks = InputArgs.hasArg(STRIP_allow_broken_links);
   Config.StripDebug = InputArgs.hasArg(STRIP_strip_debug);
 
   if (InputArgs.hasArg(STRIP_discard_all, STRIP_discard_locals))
     Config.DiscardMode =
Expected<DriverConfig> parseStripOptions(ArrayRef<const char *> ArgsArr) {
   } else {
     for (const char *Filename : Positional) {
       Config.InputFilename = Filename;
       Config.OutputFilename = Filename;
       DC.CopyConfigs.push_back(Config);
     }
   }
 
+  if (Config.OutputFilename == "-" || Config.InputFilename == "-")
+    if (Config.PreserveDates)
+      return createStringError(errc::invalid_argument, "--preserve-dates requires a file");
+
   return std::move(DC);
 }
 
 } // namespace objcopy
 } // namespace llvm

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

 #include "llvm/Object/ELFObjectFile.h"
 #include "llvm/Object/ELFTypes.h"
 #include "llvm/Object/Error.h"
 #include "llvm/Object/MachO.h"
 #include "llvm/Option/Arg.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Option/Option.h"
 #include "llvm/Support/Casting.h"
+#include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/Memory.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/WithColor.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 #include <cassert>
 #include <cstdlib>
 #include <memory>
 #include <string>
 #include <system_error>
 #include <utility>
 
 namespace llvm {
 namespace objcopy {
 
 // The name this program was invoked as.
 StringRef ToolName;
 
 LLVM_ATTRIBUTE_NORETURN void error(Twine Message) {
-  WithColor::error(errs(), ToolName) << Message << ".\n";
+  WithColor::error(errs(), ToolName) << Message << "\n";
   errs().flush();
   exit(1);
 }
 
 LLVM_ATTRIBUTE_NORETURN void error(Error E) {
   assert(E);
   std::string Buf;
   raw_string_ostream OS(Buf);
static Error restoreDateOnFile(StringRef Filename,
   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 Error executeObjcopy(const CopyConfig &Config) {
   sys::fs::file_status Stat;
   if (Config.PreserveDates)
     if (auto EC = sys::fs::status(Config.InputFilename, Stat))
       return createFileError(Config.InputFilename, EC);
   
   if (Config.InputFormat == "binary") {
     auto BufOrErr = MemoryBuffer::getFile(Config.InputFilename);
     if (!BufOrErr)
       return createFileError(Config.InputFilename, BufOrErr.getError());
     FileBuffer FB(Config.OutputFilename);
     if (Error E = executeObjcopyOnRawBinary(Config, *BufOrErr->get(), FB))
       return E;
   } else {