[llvm] r365173 - [llvm-objcopy][NFC] Refactor output target parsing v2

Seiya Nuta via llvm-commits llvm-commits at lists.llvm.org
Thu Jul 4 22:28:38 PDT 2019


Author: seiya
Date: Thu Jul  4 22:28:38 2019
New Revision: 365173

URL: http://llvm.org/viewvc/llvm-project?rev=365173&view=rev
Log:
[llvm-objcopy][NFC] Refactor output target parsing v2

Summary:
Use an enum instead of string to hold the output file format in Config.InputFormat and Config.OutputFormat. It's essential to support other output file formats other than ELF.

This patch originally has been submitted as D63239. However, there was an use-of-uninitialized-value bug and reverted in r364379 (git commit 4ee933c).

This patch includes the fix for the bug by setting Config.InputFormat/Config.OutputFormat in parseStripOptions.

Reviewers: espindola, alexshap, rupprecht, jhenderson

Reviewed By: jhenderson

Subscribers: emaste, arichardson, jakehehrlich, MaskRay, llvm-commits

Tags: #llvm

Differential Revision: https://reviews.llvm.org/D64170

Modified:
    llvm/trunk/tools/llvm-objcopy/CopyConfig.cpp
    llvm/trunk/tools/llvm-objcopy/CopyConfig.h
    llvm/trunk/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
    llvm/trunk/tools/llvm-objcopy/llvm-objcopy.cpp

Modified: llvm/trunk/tools/llvm-objcopy/CopyConfig.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-objcopy/CopyConfig.cpp?rev=365173&r1=365172&r2=365173&view=diff
==============================================================================
--- llvm/trunk/tools/llvm-objcopy/CopyConfig.cpp (original)
+++ llvm/trunk/tools/llvm-objcopy/CopyConfig.cpp Thu Jul  4 22:28:38 2019
@@ -277,8 +277,13 @@ static Expected<const MachineInfo &> get
   return Iter->getValue();
 }
 
+struct TargetInfo {
+  FileFormat Format;
+  MachineInfo Machine;
+};
+
 // FIXME: consolidate with the bfd parsing used by lld.
-static const StringMap<MachineInfo> OutputFormatMap{
+static const StringMap<MachineInfo> TargetMap{
     // Name, {EMachine, 64bit, LittleEndian}
     // x86
     {"elf32-i386", {ELF::EM_386, false, true}},
@@ -312,18 +317,28 @@ static const StringMap<MachineInfo> Outp
     {"elf32-sparcel", {ELF::EM_SPARC, 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))
+static Expected<TargetInfo>
+getOutputTargetInfoByTargetName(StringRef TargetName) {
+  StringRef OriginalTargetName = TargetName;
+  bool IsFreeBSD = TargetName.consume_back("-freebsd");
+  auto Iter = TargetMap.find(TargetName);
+  if (Iter == std::end(TargetMap))
     return createStringError(errc::invalid_argument,
                              "invalid output format: '%s'",
-                             OriginalFormat.str().c_str());
+                             OriginalTargetName.str().c_str());
   MachineInfo MI = Iter->getValue();
   if (IsFreeBSD)
     MI.OSABI = ELF::ELFOSABI_FREEBSD;
-  return {MI};
+
+  FileFormat Format;
+  if (TargetName.startswith("elf"))
+    Format = FileFormat::ELF;
+  else
+    // This should never happen because `TargetName` is valid (it certainly
+    // exists in the TargetMap).
+    llvm_unreachable("unknown target prefix");
+
+  return {TargetInfo{Format, MI}};
 }
 
 static Error addSymbolsFromFile(std::vector<NameOrRegex> &Symbols,
@@ -445,14 +460,23 @@ Expected<DriverConfig> parseObjcopyOptio
         "--target cannot be used with --input-target or --output-target");
 
   bool UseRegex = InputArgs.hasArg(OBJCOPY_regex);
+  StringRef InputFormat, OutputFormat;
   if (InputArgs.hasArg(OBJCOPY_target)) {
-    Config.InputFormat = InputArgs.getLastArgValue(OBJCOPY_target);
-    Config.OutputFormat = InputArgs.getLastArgValue(OBJCOPY_target);
+    InputFormat = InputArgs.getLastArgValue(OBJCOPY_target);
+    OutputFormat = InputArgs.getLastArgValue(OBJCOPY_target);
   } else {
-    Config.InputFormat = InputArgs.getLastArgValue(OBJCOPY_input_target);
-    Config.OutputFormat = InputArgs.getLastArgValue(OBJCOPY_output_target);
+    InputFormat = InputArgs.getLastArgValue(OBJCOPY_input_target);
+    OutputFormat = InputArgs.getLastArgValue(OBJCOPY_output_target);
   }
-  if (Config.InputFormat == "binary") {
+
+  // FIXME:  Currently, we ignore the target for non-binary/ihex formats
+  // explicitly specified by -I option (e.g. -Ielf32-x86-64) and guess the
+  // format by llvm::object::createBinary regardless of the option value.
+  Config.InputFormat = StringSwitch<FileFormat>(InputFormat)
+                           .Case("binary", FileFormat::Binary)
+                           .Case("ihex", FileFormat::IHex)
+                           .Default(FileFormat::Unspecified);
+  if (Config.InputFormat == FileFormat::Binary) {
     auto BinaryArch = InputArgs.getLastArgValue(OBJCOPY_binary_architecture);
     if (BinaryArch.empty())
       return createStringError(
@@ -463,12 +487,17 @@ Expected<DriverConfig> parseObjcopyOptio
       return MI.takeError();
     Config.BinaryArch = *MI;
   }
-  if (!Config.OutputFormat.empty() && Config.OutputFormat != "binary" &&
-      Config.OutputFormat != "ihex") {
-    Expected<MachineInfo> MI = getOutputFormatMachineInfo(Config.OutputFormat);
-    if (!MI)
-      return MI.takeError();
-    Config.OutputArch = *MI;
+
+  Config.OutputFormat = StringSwitch<FileFormat>(OutputFormat)
+                            .Case("binary", FileFormat::Binary)
+                            .Case("ihex", FileFormat::IHex)
+                            .Default(FileFormat::Unspecified);
+  if (Config.OutputFormat == FileFormat::Unspecified && !OutputFormat.empty()) {
+    Expected<TargetInfo> Target = getOutputTargetInfoByTargetName(OutputFormat);
+    if (!Target)
+      return Target.takeError();
+    Config.OutputFormat = Target->Format;
+    Config.OutputArch = Target->Machine;
   }
 
   if (auto Arg = InputArgs.getLastArg(OBJCOPY_compress_debug_sections,
@@ -804,6 +833,8 @@ parseStripOptions(ArrayRef<const char *>
                         STRIP_disable_deterministic_archives, /*default=*/true);
 
   Config.PreserveDates = InputArgs.hasArg(STRIP_preserve_dates);
+  Config.InputFormat = FileFormat::Unspecified;
+  Config.OutputFormat = FileFormat::Unspecified;
 
   DriverConfig DC;
   if (Positional.size() == 1) {

Modified: llvm/trunk/tools/llvm-objcopy/CopyConfig.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-objcopy/CopyConfig.h?rev=365173&r1=365172&r2=365173&view=diff
==============================================================================
--- llvm/trunk/tools/llvm-objcopy/CopyConfig.h (original)
+++ llvm/trunk/tools/llvm-objcopy/CopyConfig.h Thu Jul  4 22:28:38 2019
@@ -26,6 +26,13 @@
 namespace llvm {
 namespace objcopy {
 
+enum class FileFormat {
+  Unspecified,
+  ELF,
+  Binary,
+  IHex,
+};
+
 // This type keeps track of the machine info for various architectures. This
 // lets us map architecture names to ELF types and the e_machine value of the
 // ELF file.
@@ -104,9 +111,9 @@ struct NewSymbolInfo {
 struct CopyConfig {
   // Main input/output options
   StringRef InputFilename;
-  StringRef InputFormat;
+  FileFormat InputFormat;
   StringRef OutputFilename;
-  StringRef OutputFormat;
+  FileFormat OutputFormat;
 
   // Only applicable for --input-format=binary
   MachineInfo BinaryArch;

Modified: llvm/trunk/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-objcopy/ELF/ELFObjcopy.cpp?rev=365173&r1=365172&r2=365173&view=diff
==============================================================================
--- llvm/trunk/tools/llvm-objcopy/ELF/ELFObjcopy.cpp (original)
+++ llvm/trunk/tools/llvm-objcopy/ELF/ELFObjcopy.cpp Thu Jul  4 22:28:38 2019
@@ -154,12 +154,14 @@ static std::unique_ptr<Writer> createELF
 static std::unique_ptr<Writer> createWriter(const CopyConfig &Config,
                                             Object &Obj, Buffer &Buf,
                                             ElfType OutputElfType) {
-  using Functor = std::function<std::unique_ptr<Writer>()>;
-  return StringSwitch<Functor>(Config.OutputFormat)
-      .Case("binary", [&] { return llvm::make_unique<BinaryWriter>(Obj, Buf); })
-      .Case("ihex", [&] { return llvm::make_unique<IHexWriter>(Obj, Buf); })
-      .Default(
-          [&] { return createELFWriter(Config, Obj, Buf, OutputElfType); })();
+  switch (Config.OutputFormat) {
+  case FileFormat::Binary:
+    return llvm::make_unique<BinaryWriter>(Obj, Buf);
+  case FileFormat::IHex:
+    return llvm::make_unique<IHexWriter>(Obj, Buf);
+  default:
+    return createELFWriter(Config, Obj, Buf, OutputElfType);
+  }
 }
 
 template <class ELFT>

Modified: llvm/trunk/tools/llvm-objcopy/llvm-objcopy.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-objcopy/llvm-objcopy.cpp?rev=365173&r1=365172&r2=365173&view=diff
==============================================================================
--- llvm/trunk/tools/llvm-objcopy/llvm-objcopy.cpp (original)
+++ llvm/trunk/tools/llvm-objcopy/llvm-objcopy.cpp Thu Jul  4 22:28:38 2019
@@ -140,11 +140,18 @@ static Error executeObjcopyOnIHex(const
 /// of the output specified by the command line options.
 static Error executeObjcopyOnRawBinary(const CopyConfig &Config,
                                        MemoryBuffer &In, Buffer &Out) {
-  // TODO: llvm-objcopy should parse CopyConfig.OutputFormat to recognize
-  // formats other than ELF / "binary" and invoke
-  // elf::executeObjcopyOnRawBinary, macho::executeObjcopyOnRawBinary or
-  // coff::executeObjcopyOnRawBinary accordingly.
-  return elf::executeObjcopyOnRawBinary(Config, In, Out);
+  switch (Config.OutputFormat) {
+  case FileFormat::ELF:
+  // FIXME: Currently, we call elf::executeObjcopyOnRawBinary even if the
+  // output format is binary/ihex or it's not given. This behavior differs from
+  // GNU objcopy. See https://bugs.llvm.org/show_bug.cgi?id=42171 for details.
+  case FileFormat::Binary:
+  case FileFormat::IHex:
+  case FileFormat::Unspecified:
+    return elf::executeObjcopyOnRawBinary(Config, In, Out);
+  }
+
+  llvm_unreachable("unsupported output format");
 }
 
 /// The function executeObjcopyOnBinary does the dispatch based on the format
@@ -238,10 +245,17 @@ static Error executeObjcopy(const CopyCo
   }
 
   typedef Error (*ProcessRawFn)(const CopyConfig &, MemoryBuffer &, Buffer &);
-  auto ProcessRaw = StringSwitch<ProcessRawFn>(Config.InputFormat)
-                        .Case("binary", executeObjcopyOnRawBinary)
-                        .Case("ihex", executeObjcopyOnIHex)
-                        .Default(nullptr);
+  ProcessRawFn ProcessRaw;
+  switch (Config.InputFormat) {
+  case FileFormat::Binary:
+    ProcessRaw = executeObjcopyOnRawBinary;
+    break;
+  case FileFormat::IHex:
+    ProcessRaw = executeObjcopyOnIHex;
+    break;
+  default:
+    ProcessRaw = nullptr;
+  }
 
   if (ProcessRaw) {
     auto BufOrErr = MemoryBuffer::getFileOrSTDIN(Config.InputFilename);




More information about the llvm-commits mailing list