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

[seiya]

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.

[compnerd]

llvm/tools/llvm-objcopy/CopyConfig.cpp

285–286

I think it would be nicer to use a tuple with an enumeration to give the values names rather than "first" and "second"

[seiya]

llvm/tools/llvm-objcopy/CopyConfig.cpp

285–286

What is an elegant way to get an element from tuple using enum? We need to cast the enum values so it should be std::get<static_cast<std::size_t>(Foo::Bar)>(Tuple). It looks verbose a bit to me.

[jhenderson]

Could you explain a bit more the motivation behind this change, please? If I'm not mistaken, it's to allow using input and output format with non-ELF (e.g. COFF and Mach-O) targets?

llvm/tools/llvm-objcopy/CopyConfig.cpp

285–286

I think it would be nicer to use a tuple with an enumeration to give the values names rather than "first" and "second"

Shouldn't it should just be a struct then?

struct {
  FileFormat Format;
  MachineInfo Machine;
};

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

32

Why has this been added?

142

Refer -> See

[seiya]

In D63239#1541208, @jhenderson wrote:

Could you explain a bit more the motivation behind this change, please? If I'm not mistaken, it's to allow using input and output format with non-ELF (e.g. COFF and Mach-O) targets?

Yes, that's correct. I'd like to use FileFormat instead of a target string for Config.OutputFormat to allow calling non-ELF implementations in executeObjcopyOnRawBinary.

[seiya]

llvm/tools/llvm-objcopy/CopyConfig.cpp

285–286

struct sounds good to me. I'll use it here.

[seiya]

• Replaced std::pair with a struct.
• Refer -> See

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

32

Oh I forgot to remove this include. Thanks.

[jhenderson]

llvm/tools/llvm-objcopy/CopyConfig.h

53

As this is only used in the .cpp, it can be moved in there.

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

142

Missing trailing full stop.

[seiya]

• Addressed review comments.
• Resolved conflicts with origin/master.

[seiya]

• Removed a temprary file accidentally included in the patch.

[jhenderson]

LGTM, with one nit. Also, if I understand it correctly, this change should have no functional change, right? In that case, please tag the summary with [NFC] (i.e. [llvm-objcopy][NFC] Refactor output target parsing) when you commit.

llvm/tools/llvm-objcopy/CopyConfig.cpp

324

I don't think you need to use auto here.

[seiya]

• Explicate the type instead of auto.

[seiya]

In D63239#1543266, @jhenderson wrote:

LGTM, with one nit. Also, if I understand it correctly, this change should have no functional change, right? In that case, please tag the summary with [NFC] (i.e. [llvm-objcopy][NFC] Refactor output target parsing) when you commit.

Yes it is NFC. Thanks.

[rupprecht]

llvm/tools/llvm-objcopy/CopyConfig.cpp

288

I haven't looked at the full list of bfd targets (for macho/coff in particular), but do we need the extra field, or can we infer it from starting with "elf"?

457–462

This should use a StringSwitch

(I suppose adding an Unrecognized file format type to handle the 'else' part?)

473–475

I'm not sure this is really an error. Behold:

$ echo 'int x=0;' | ~/dev/clang -x c -c - -o /tmp/bss.o
$ objcopy -Ielf32-x86-64 /tmp/bss.o /tmp/bss2.o
$ file /tmp/bss.o /tmp/bss2.o
/tmp/bss.o:  ELF 64-bit LSB relocatable, x86-64, version 1 (SYSV), not stripped
/tmp/bss2.o: ELF 32-bit LSB relocatable, x86-64, version 1 (SYSV), not stripped

[seiya]

llvm/tools/llvm-objcopy/CopyConfig.cpp

288

I'm not quite sure but according to my GNU objcopy build (with --enable-targets=all) you're right: we can infer the file type from the prefix.

473–475

Nice catch! Will fix this.

[seiya]

• Addressed some review comments.

[seiya]

• Refactored a bit.

llvm/tools/llvm-objcopy/CopyConfig.cpp

457–462

I think we don't need Unrecognized for now because currently we simply ignore unrecognized input formats and guess the file format by createBinary.

[jhenderson]

llvm/tools/llvm-objcopy/CopyConfig.cpp

458

ignores -> ignore

I'm not really sure what this is trying to point out though. Is it an existing bug in llvm-objcopy or one you are introducing?

[rupprecht]

llvm/tools/llvm-objcopy/CopyConfig.cpp

458

It's an existing bug; we handle "-I binary" and "-I ihex", but specifying "-Ielf32..." is the same as not specifying it at all.

Maybe a clearer message would be "ignore the target for non-binary/ihex formats"

[seiya]

• Updated a comment.

[jhenderson]

LGTM, with one comment fix.

llvm/tools/llvm-objcopy/CopyConfig.cpp

458

Nit: We -> we

[seiya]

• We -> we

[jakehehrlich]

General idea looks good to me!

llvm/tools/llvm-objcopy/CopyConfig.h

 #include "llvm/Support/Regex.h"
 // Necessary for llvm::DebugCompressionType::None
 #include "llvm/Target/TargetOptions.h"
 #include <vector>
 
 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.
 struct MachineInfo {
   MachineInfo(uint16_t EM, uint8_t ABI, bool Is64, bool IsLittle)
       : EMachine(EM), OSABI(ABI), Is64Bit(Is64), IsLittleEndian(IsLittle) {}
   // Alternative constructor that defaults to NONE for OSABI.
   MachineInfo(uint16_t EM, bool Is64, bool IsLittle)
       : MachineInfo(EM, ELF::ELFOSABI_NONE, Is64, IsLittle) {}
   // Default constructor for unset fields.
   MachineInfo() : MachineInfo(0, 0, false, false) {}
   uint16_t EMachine;
   uint8_t OSABI;
   bool Is64Bit;
   bool IsLittleEndian;
 };
 
 // Flags set by --set-section-flags or --rename-section. Interpretation of these
 // is format-specific and not all flags are meaningful for all object file
 // formats. This is a bitmask; many section flags may be set.
 enum SectionFlag {
   SecNone = 0,
   SecAlloc = 1 << 0,
   SecLoad = 1 << 1,
   SecNoload = 1 << 2,
   SecReadonly = 1 << 3,
struct NewSymbolInfo {
   uint8_t Bind = ELF::STB_GLOBAL;
   uint8_t Visibility = ELF::STV_DEFAULT;
 };
 
 // Configuration for copying/stripping a single file.
 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;
   // Only applicable when --output-format!=binary (e.g. elf64-x86-64).
   Optional<MachineInfo> OutputArch;
 
   // Advanced options
   StringRef AddGnuDebugLink;

llvm/tools/llvm-objcopy/CopyConfig.cpp

 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());
   return Iter->getValue();
 }
 
+struct TargetInfo {
+  FileFormat Format;
+  MachineInfo Machine;
+};
+
 // FIXME: consolidate with the bfd parsing used by lld.
-static const StringMap<MachineInfo> OutputFormatMap{
-    // Name, {EMachine, 64bit, LittleEndian}
+static const StringMap<TargetInfo> TargetMap{
+    // Name, {FileFormat, {EMachine, 64bit, LittleEndian}}
     // x86
-    {"elf32-i386", {ELF::EM_386, false, true}},
-    {"elf32-x86-64", {ELF::EM_X86_64, false, true}},
-    {"elf64-x86-64", {ELF::EM_X86_64, true, true}},
+    {"elf32-i386", {FileFormat::ELF, {ELF::EM_386, false, true}}},
+    {"elf32-x86-64", {FileFormat::ELF, {ELF::EM_X86_64, false, true}}},
+    {"elf64-x86-64", {FileFormat::ELF, {ELF::EM_X86_64, true, true}}},
     // Intel MCU
-    {"elf32-iamcu", {ELF::EM_IAMCU, false, true}},
+    {"elf32-iamcu", {FileFormat::ELF, {ELF::EM_IAMCU, false, true}}},
     // ARM
-    {"elf32-littlearm", {ELF::EM_ARM, false, true}},
+    {"elf32-littlearm", {FileFormat::ELF, {ELF::EM_ARM, false, true}}},
     // ARM AArch64
-    {"elf64-aarch64", {ELF::EM_AARCH64, true, true}},
-    {"elf64-littleaarch64", {ELF::EM_AARCH64, true, true}},
+    {"elf64-aarch64", {FileFormat::ELF, {ELF::EM_AARCH64, true, true}}},
+    {"elf64-littleaarch64", {FileFormat::ELF, {ELF::EM_AARCH64, true, true}}},
     // RISC-V
-    {"elf32-littleriscv", {ELF::EM_RISCV, false, true}},
-    {"elf64-littleriscv", {ELF::EM_RISCV, true, true}},
+    {"elf32-littleriscv", {FileFormat::ELF, {ELF::EM_RISCV, false, true}}},
+    {"elf64-littleriscv", {FileFormat::ELF, {ELF::EM_RISCV, true, true}}},
     // PowerPC
-    {"elf32-powerpc", {ELF::EM_PPC, false, false}},
-    {"elf32-powerpcle", {ELF::EM_PPC, false, true}},
-    {"elf64-powerpc", {ELF::EM_PPC64, true, false}},
-    {"elf64-powerpcle", {ELF::EM_PPC64, true, true}},
+    {"elf32-powerpc", {FileFormat::ELF, {ELF::EM_PPC, false, false}}},
+    {"elf32-powerpcle", {FileFormat::ELF, {ELF::EM_PPC, false, true}}},
+    {"elf64-powerpc", {FileFormat::ELF, {ELF::EM_PPC64, true, false}}},
+    {"elf64-powerpcle", {FileFormat::ELF, {ELF::EM_PPC64, true, true}}},
     // MIPS
-    {"elf32-bigmips", {ELF::EM_MIPS, false, false}},
-    {"elf32-ntradbigmips", {ELF::EM_MIPS, false, false}},
-    {"elf32-ntradlittlemips", {ELF::EM_MIPS, false, true}},
-    {"elf32-tradbigmips", {ELF::EM_MIPS, false, false}},
-    {"elf32-tradlittlemips", {ELF::EM_MIPS, false, true}},
-    {"elf64-tradbigmips", {ELF::EM_MIPS, true, false}},
-    {"elf64-tradlittlemips", {ELF::EM_MIPS, true, true}},
+    {"elf32-bigmips", {FileFormat::ELF, {ELF::EM_MIPS, false, false}}},
+    {"elf32-ntradbigmips", {FileFormat::ELF, {ELF::EM_MIPS, false, false}}},
+    {"elf32-ntradlittlemips", {FileFormat::ELF, {ELF::EM_MIPS, false, true}}},
+    {"elf32-tradbigmips", {FileFormat::ELF, {ELF::EM_MIPS, false, false}}},
+    {"elf32-tradlittlemips", {FileFormat::ELF, {ELF::EM_MIPS, false, true}}},
+    {"elf64-tradbigmips", {FileFormat::ELF, {ELF::EM_MIPS, true, false}}},
+    {"elf64-tradlittlemips", {FileFormat::ELF, {ELF::EM_MIPS, true, true}}},
 };
 
-static Expected<MachineInfo> getOutputFormatMachineInfo(StringRef Format) {
+static Expected<TargetInfo> getTargetInfoByTargetName(StringRef Format) {
   StringRef OriginalFormat = Format;
   bool IsFreeBSD = Format.consume_back("-freebsd");
-  auto Iter = OutputFormatMap.find(Format);
-  if (Iter == std::end(OutputFormatMap))
+  auto Iter = TargetMap.find(Format);
+  if (Iter == std::end(TargetMap))
     return createStringError(errc::invalid_argument,
                              "Invalid output format: '%s'",
                              OriginalFormat.str().c_str());
-  MachineInfo MI = Iter->getValue();
+  TargetInfo Target = Iter->getValue();
   if (IsFreeBSD)
-    MI.OSABI = ELF::ELFOSABI_FREEBSD;
-  return {MI};
+    Target.Machine.OSABI = ELF::ELFOSABI_FREEBSD;
+  return {Target};
 }
 
 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);
Expected<DriverConfig> parseObjcopyOptions(ArrayRef<const char *> ArgsArr) {
   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);
+  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") {
+  if (InputFormat.empty())
+    Config.InputFormat = FileFormat::Unspecified;
+  else if (InputFormat == "ihex")
+    Config.InputFormat = FileFormat::IHex;
+  else if (InputFormat == "binary") {
+    Config.InputFormat = FileFormat::Binary;
     auto BinaryArch = InputArgs.getLastArgValue(OBJCOPY_binary_architecture);
     if (BinaryArch.empty())
       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" &&
-      Config.OutputFormat != "ihex") {
-    Expected<MachineInfo> MI = getOutputFormatMachineInfo(Config.OutputFormat);
-    if (!MI)
-      return MI.takeError();
-    Config.OutputArch = *MI;
+  } else
+    return createStringError(errc::invalid_argument,
+                             "unsupported input format: '%s'",
+                             InputFormat.str().c_str());
+
+  if (OutputFormat.empty())
+    Config.OutputFormat = FileFormat::Unspecified;
+  else if (OutputFormat == "binary")
+    Config.OutputFormat = FileFormat::Binary;
+  else if (OutputFormat == "ihex")
+    Config.OutputFormat = FileFormat::IHex;
+  else {
+    Expected<TargetInfo> Target = getTargetInfoByTargetName(OutputFormat);
+    if (!Target)
+      return Target.takeError();
+    Config.OutputFormat = Target->Format;
+    Config.OutputArch = Target->Machine;
   }
 
   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 =

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

return llvm::make_unique<ELFWriter<ELF64BE>>(Obj, Buf,
                                                  !Config.StripSections);
   }
   llvm_unreachable("Invalid output format");
 }
 
 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>
 static Expected<ArrayRef<uint8_t>>
 findBuildID(const CopyConfig &Config, const object::ELFFile<ELFT> &In) {
   auto PhdrsOrErr = In.program_headers();
   if (auto Err = PhdrsOrErr.takeError())
     return createFileError(Config.InputFilename, std::move(Err));

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/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"
static Error executeObjcopyOnIHex(const CopyConfig &Config, MemoryBuffer &In,
   // TODO: support output formats other than ELF.
   return elf::executeObjcopyOnIHex(Config, In, Out);
 }
 
 /// The function executeObjcopyOnRawBinary does the dispatch based on the format
 /// 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.
+  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);
   }
+}
 
 /// The function executeObjcopyOnBinary does the dispatch based on the format
 /// of the input binary (ELF, MachO or COFF).
 static Error executeObjcopyOnBinary(const CopyConfig &Config,
                                     object::Binary &In, Buffer &Out) {
   if (auto *ELFBinary = dyn_cast<object::ELFObjectFileBase>(&In))
     return elf::executeObjcopyOnBinary(Config, *ELFBinary, Out);
   else if (auto *COFFBinary = dyn_cast<object::COFFObjectFile>(&In))

 /// 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);
 
   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);
     if (!BufOrErr)
       return createFileError(Config.InputFilename, BufOrErr.getError());
     FileBuffer FB(Config.OutputFilename);
     if (Error E = ProcessRaw(Config, *BufOrErr->get(), FB))
       return E;