[llvm-objcopy] Switch over to using TableGen for parsing arguments

[jakehehrlich]

Summary

So I'm starting back on the push to create llvm-strip which requires switching how arguments are being parsed. This change does the following

1. Deletes the old argument parsing code
2. Dactors out arguments into a Config class (will later be used repeatedly by llvm-strip)
3. Removes all uses of "-R=" and "-j=" because they're non-standard and hard to support in TableGen. I was wrong to use them in the first place
4. Adds in Opts.td (which will later be split into Common.td and Objcopy.td when Strip.td exists)
5. Adds in objcopy driver using Opts.td (later there will be a StripDriver and symlink)

[alexander-shaposhnikov]

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

136

for what it's worth, it seems to me this is kind of strange that a struct Config has these methods / logic (i mean CreateWriter, CreateReader, SplitDWOToFile). I've just looked at LLD which also has some kind of Config internally, but there the Config is not loaded with this business logic.

[jakehehrlich]

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

136

LLD uses a global Config object instead of warping it in a class like I have. I also don't like what I've done that much. The alternative that I see is to pass the Config in as an argument. For HandleArgs that would be pretty painful. I can do that for the others though, what do you think? Alternatively I can go though the burden of Maybe I should just use a different name? The idea is that you want to be able to set and configure all these different values and then make the result happen. Each driver should set one of these up for each copy operation and it should execute it. Any other ideas?

[compnerd]

llvm/tools/llvm-objcopy/Opts.td

13

LLVM style is to name this O, so that you can find it as OPT_O.

23

LLVM style would be to name this remove_section.

26

LLVM style would be to name this R.

34

LLVM style would be to name this j.

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

136

I agree with @alexshap here, I don't think that the Config structure here should have anything in it other than the options. It can be consumed by the tool to drive the tool as necessary. I can understand the desire to share code between the tool, but, I think that standalone functions or a different class would make for a better home for this.

[jhenderson]

Like the others who have commented, it seems to me like the execute() function should be a member of a driver class (or stand-alone, since we don't really need a class here), rather than a configuration class. A configuration describes how to do something, but I don't think doing it itself feels right to me (for a slightly abstract example, think configuration files for an application - the executable does the work, the config file just allows controlling what is done).

Is there a particular reason you don't want to follow LLD's approach of one global configuration variable? Alternatively making it a member of an "ObjcopyDriver" class?

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

10–12

These headers are in the wrong place in the list.

54

Is there a particular reason you've gone with "OBJCOPY_<OPTION_NAME>" here? I think it's more common to do OPT_<OPTION_NAME>.

136

The alternative that I see is to pass the Config in as an argument. For HandleArgs that would be pretty painful.

Why would it be painful? I don't see how it would be hard to extend this signature?

184–186

clang-format and elsewhere.

384

I think I'd prefer the argument parsing split into a separate function from the driver function, since it's a distinct block of work, and will make the different stages of the program somewhat clearer. Ideally, I'd also just hoist the body of Config::Execute into this function then.

386

Please don't abbreviate these two variables - I have no idea what "MAI" and "MAC" are.

466

I don't think return 0 is necessary in main?

[jakehehrlich]

In D44236#1031086, @jhenderson wrote:

Like the others who have commented, it seems to me like the execute() function should be a member of a driver class (or stand-alone, since we don't really need a class here), rather than a configuration class. A configuration describes how to do something, but I don't think doing it itself feels right to me (for a slightly abstract example, think configuration files for an application - the executable does the work, the config file just allows controlling what is done).

Is there a particular reason you don't want to follow LLD's approach of one global configuration variable? Alternatively making it a member of an "ObjcopyDriver" class?

Making an ObjcopyDriver class is probably fine. Eventually I want the Config to be copyable so that multiple different configs can be used at the same time.

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

54

Eventually these will clash with the llvm-strip symbols for the same thing.

136

It would just mean typing "Config." a ton of times. I'm fine making it a parameter though. It also means *everything* needs to take Config as a parameter.

384

After I put Config::Execute in its own function (per the discussion above) that code will (literally in the next diff) be shared between the StripDriver and the ObjcopyDriver

386

Yeah fair, that was cargo-culted from other tools. They stand for "missing argument count" and "missing argument index" which is 100% not clear. I'll fix that.

[jakehehrlich]

I split the "Config" up into "Config" and "CopyAction". The plan is for "CopyAction" to handle multiple formats and for "Config" to determine which sort of CopyAction is constructed. When COFF support rolls around we should be able to use CopyAction for those as well. Archives support is also something I plan on adding eventually and that will be a copy action composed of many copy actions.

[alexander-shaposhnikov]

so maybe the following approach can also be considered: (deliberately presented in the reverse order to see how different pieces would fit together):

int main(int argc, char **argv) {
    StringRef ToolName = path::filename(argv[0]);
    Config Conf;
    if (ToolName) == "objcopy")
       Conf = parseObjcopyArgs(argc, argv);
    else if (ToolName = "strip") 
       Conf = parseStripArgs(argc, argv);
    ExecuteObjcopy(Conf);
    return 0;
}

void ExecuteObjcopy(const Config &Conf) {
     auto Reader = createReader(Conf.InputName);
     if (Reader.isELF())
         return ExecuteElfObjcopy(Conf, Reader);
     if (Reader.isCoff())
         return ExecuteCoffObjcopy(Conf, Reader);
}

void ExecuteElfObjcopy(const Config &C, Reader &R) {
      Object O;  // intermediate representation which we discussed with Jake and James
      O.read(R);   
      /* modify the object according to the config, similarly to the existing handleArgs */
      .....
      .....  
      .....
      auto W = createWriter(R.elftype(), C.OutputFormat, C.OutputFilename);
      O.write(W);
 }

}

(seems to be a bit simpler, less inheritance, all the classes are "stateful")

[compnerd]

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

136

I'm not sure I understand why there is still so much "configuration" in the CopyAction here. The flag state should be hoisted into the Config as that is not part of the "copy" operation.

Could you please share what your plan is with the additional machinery here? It should be possible to do the work more simply (as @alexshap mentions).

[jakehehrlich]

This looks good to me. Thanks!

In D44236#1058119, @alexshap wrote:

so maybe the following approach can also be considered: (deliberately presented in the reverse order to see how different pieces would fit together):

int main(int argc, char **argv) {
    StringRef ToolName = path::filename(argv[0]);
    Config Conf;
    if (ToolName) == "objcopy")
       Conf = parseObjcopyArgs(argc, argv);
    else if (ToolName = "strip") 
       Conf = parseStripArgs(argc, argv);
    ExecuteObjcopy(Conf);
    return 0;
}

void ExecuteObjcopy(const Config &Conf) {
     auto Reader = createReader(Conf.InputName);
     if (Reader.isELF())
         return ExecuteElfObjcopy(Conf, Reader);
     if (Reader.isCoff())
         return ExecuteCoffObjcopy(Conf, Reader);
}

void ExecuteElfObjcopy(const Config &C, Reader &R) {
      Object O;  // intermediate representation which we discussed with Jake and James
      O.read(R);   
      /* modify the object according to the config, similarly to the existing handleArgs */
      .....
      .....  
      .....
      auto W = createWriter(R.elftype(), C.OutputFormat, C.OutputFilename);
      O.write(W);
 }

}

(seems to be a bit simpler, less inheritance, all the classes are "stateful")

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

136

These aren't necessarily in direct correspondence to flags; they are for objcopy but not for strip. For instance strip will set StripAll unless another strip option is used. The idea is that this is information shared by every action that could occur on any input type or via any driver (objcopy or strip). I like Alex's idea though so I'm going to implement that.

[alexander-shaposhnikov]

sorry about the disturbance, i'm wondering if i can help push this diff forward - switching to TableGen looks like the right thing to do.
I have (locally, have not sent them for code review yet) a couple of other patches to llvm-objcopy (with various features),
but I'd like to apply them on top of the refactored codebase (in particular, with this change), although it's not a blocker nor is it critical.

[jakehehrlich]

In D44236#1062012, @alexshap wrote:

sorry about the disturbance, i'm wondering if i can help push this diff forward - switching to TableGen looks like the right thing to do.
I have (locally, have not sent them for code review yet) a couple of other patches to llvm-objcopy (with various features),
but I'd like to apply them on top of the refactored codebase (in particular, with this change), although it's not a blocker nor is it critical.

I can have something up by the end of tomorrow for you to review. I'm also fine with you submitting something for me to review. You can copy pasta any part that happens to be useful.

[alexander-shaposhnikov]

great, thanks!

[jakehehrlich]

Updated to (in the future) match Alex's plan for how we can expand this.

[alexander-shaposhnikov]

great, many thanks,
I've added a few comments (some minor nits (like names of the variables + formatting)), if you don't mind - let's clang-format -style=llvm these files and update those names,
otherwise this looks good to me.

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

221–222

nit: const CopyConfig &Config

374

const CopyConfig &Config

388

nit: this blank line is probably unnecessary, maybe let's clang-format -style=llvm this diff or the entire file (if not yet).

391

nit: https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
I would use the explicit type InputArgList instead of auto because unlike some other case (i.e. auto *VarDecl = dyn_cast<VarDecl>(Decl)) here it's not mentioned anywhere else. (so, probably, being explicit would improve readability).

406

very minor nit: Positional.empty() (here and above where we check if size() equals 0)

412

out -> Out,
but, probably, Config would be a better name

[jakehehrlich]

ran format with style=llvm (git-clang-format seems to not have a way to do this?) and fixed a couple other nits

[alexander-shaposhnikov]

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

394

empty()

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

 # RUN: yaml2obj %s > %t
 # RUN: echo 0000 > %t.sec
-# RUN: llvm-objcopy -R=.test2 -add-section=.test2=%t.sec %t %t2
+# RUN: llvm-objcopy -R .test2 -add-section=.test2=%t.sec %t %t2
 # RUN: llvm-readobj -file-headers -sections -section-data %t2 | FileCheck %s
 
 !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_REL
   Machine:         EM_X86_64

llvm/test/tools/llvm-objcopy/add-section.test

 # RUN: yaml2obj %s > %t
 # RUN: llvm-objcopy -O binary -j .test2 %t %t.sec
-# RUN: llvm-objcopy -R=.test2 %t %t2
+# RUN: llvm-objcopy -R .test2 %t %t2
 # RUN: llvm-objcopy -add-section=.test2=%t.sec %t2 %t3
 # RUN: llvm-readobj -file-headers -sections -section-data %t3 | FileCheck %s
 
 !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_REL

llvm/test/tools/llvm-objcopy/basic-only-keep.test

 # RUN: yaml2obj %s > %t
 # RUN: llvm-objcopy -only-keep=.test %t %t2
-# RUN: llvm-objcopy -j=.test %t %t3
+# RUN: llvm-objcopy -j .test %t %t3
 # RUN: llvm-readobj -file-headers -sections %t2 | FileCheck %s
 # RUN: diff %t2 %t3
 
 !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_REL

llvm/test/tools/llvm-objcopy/binary-first-seg-offset-zero.test

 # RUN: yaml2obj %s -o %t
-# RUN: llvm-objcopy -R=.note -O binary %t %t2
+# RUN: llvm-objcopy -R .note -O binary %t %t2
 # RUN: od -Ax -t x1 %t2 | FileCheck %s
 
 !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_EXEC
   Machine:         EM_X86_64

llvm/test/tools/llvm-objcopy/keep-only-keep.test

 # RUN: yaml2obj %s > %t
 # RUN: llvm-objcopy -keep=.test2 -only-keep=.test %t %t2
-# RUN: llvm-objcopy -j=.test -keep=.test2 %t %t3
+# RUN: llvm-objcopy -j .test -keep=.test2 %t %t3
 # RUN: llvm-readobj -file-headers -sections %t2 | FileCheck %s
 # RUN: diff %t2 %t3
 
 !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_REL

llvm/test/tools/llvm-objcopy/only-keep-remove-strtab.test

 # RUN: yaml2obj %s > %t
 # RUN: llvm-objcopy -R .symtab -R .strtab -only-keep=.test %t %t2
-# RUN: llvm-objcopy -j=.test -R .strtab -R .symtab %t %t3
+# RUN: llvm-objcopy -j .test -R .strtab -R .symtab %t %t3
 # RUN: llvm-readobj -file-headers -sections %t2 | FileCheck %s
 # RUN: diff %t2 %t3
 
 !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_REL

llvm/test/tools/llvm-objcopy/parent-loop-check.test

 # This test has a subtle purpose. Because of the specifics of the layout
 # algorithm if we're not careful cycles can occur when we resolve the order
 # that we should layout segments in. In this test we're making sure that the
 # PT_LOAD segment won't set the PT_NOTE segment as the first to be laid out
 # while the PT_NOTE segment sets the PT_LOAD load to be the first to be laid
 # out. This problem becomes visible if the layout of the file changes due to
 # the removal of a section. We use -O binary here because removing a section
 # in this way won't cause the location of the PT_LOAD segment to change if we
 # don't.
 
 # RUN: yaml2obj %s -o %t
-# RUN: llvm-objcopy -R=.note -O binary %t %t2
+# RUN: llvm-objcopy -R .note -O binary %t %t2
 # RUN: od -Ax -t x1 %t2 | FileCheck %s
 
 !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_EXEC
   Machine:         EM_X86_64

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

 # RUN: yaml2obj %s > %t
-# RUN: llvm-objcopy -R=.test2 %t %t2
+# RUN: llvm-objcopy -R .test2 %t %t2
 # RUN: llvm-readobj -file-headers -sections %t2 | FileCheck %s
 
 !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_REL
   Machine:         EM_X86_64

llvm/tools/llvm-objcopy/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Object
+  Option
   Support
   MC
   )
+
+set(LLVM_TARGET_DEFINITIONS Opts.td)
+
+tablegen(LLVM Opts.inc -gen-opt-parser-defs)
+add_public_tablegen_target(ObjcopyTableGen)
+
 add_llvm_tool(llvm-objcopy
   llvm-objcopy.cpp
   Object.cpp
   )
 
 if(LLVM_INSTALL_BINUTILS_SYMLINKS)
   add_llvm_tool_symlink(objcopy llvm-objcopy)
 endif()

llvm/tools/llvm-objcopy/LLVMBuild.txt

 ;
 ;   http://llvm.org/docs/LLVMBuild.html
 ;
 ;===------------------------------------------------------------------------===;
 [component_0]
 type = Tool
 name = llvm-objcopy
 parent = Tools
-required_libraries = Object Support MC
+required_libraries = Object Option Support MC

llvm/tools/llvm-objcopy/Opts.td

+include "llvm/Option/OptParser.td"
+
+multiclass Eq<string name> {
+  def NAME: Separate<["--", "-"], name>;
+  def NAME # _eq: Joined<["--", "-"], name # "=">, Alias<!cast<Separate>(NAME)>;
+}
+
+def help : Flag<["-", "--"], "help">;
+defm binary_architecture : Eq<"binary-architecture">,
+                           HelpText<"Used when transforming an architecture-less format (such as binary) to another format">;
+def B : JoinedOrSeparate<["-"], "B">,
+        Alias<binary_architecture>;
+defm input_target : Eq<"input-target">,
+                    HelpText<"Format of the input file">,
+                    Values<"binary">;
+defm output_target : Eq<"output-target">,
+                     HelpText<"Format of the output file">,
+                     Values<"binary">;
+def O : JoinedOrSeparate<["-"], "O">,
+        Alias<output_target>;
+defm split_dwo : Eq<"split-dwo">,
+                 MetaVarName<"dwo-file">,
+                 HelpText<"Equivalent to extract-dwo on the input file to <dwo-file>, then strip-dwo on the input file">;
+defm add_gnu_debuglink : Eq<"add-gnu-debuglink">,
+                         MetaVarName<"debug-file">,
+                         HelpText<"adds a .gnu_debuglink for <debug-file>">;
+defm remove_section : Eq<"remove-section">,
+                      MetaVarName<"section">,
+                      HelpText<"Remove <section>">;
+def R : JoinedOrSeparate<["-"], "R">,
+        Alias<remove_section>;
+defm keep : Eq<"keep">,
+            MetaVarName<"section">,
+            HelpText<"Keep <section>">;
+defm only_keep : Eq<"only-keep">,
+                 MetaVarName<"section">,
+                 HelpText<"Remove all but <section>">;
+def j : JoinedOrSeparate<["-"], "j">,
+                      Alias<only_keep>;
+defm add_section : Eq<"add-section">,
+                   MetaVarName<"section=file">,
+                   HelpText<"Make a section named <section> with the contents of <file>.">;
+def strip_all : Flag<["-", "--"], "strip-all">,
+                HelpText<"Removes non-allocated sections other than .gnu.warning* sections">;
+def strip_all_gnu : Flag<["-", "--"], "strip-all-gnu">,
+                    HelpText<"Compaitable with GNU objcopy's --strip-all">;
+def strip_debug : Flag<["-", "--"], "strip-debug">,
+                  HelpText<"Removes all debug information">;
+def strip_dwo : Flag<["-", "--"], "strip-dwo">,
+                HelpText<"Remove all DWARF .dwo sections from file">;
+def strip_sections : Flag<["-", "--"], "strip-sections">,
+                     HelpText<"Remove all section headers">;
+def strip_non_alloc : Flag<["-", "--"], "strip-non-alloc">,
+                      HelpText<"Remove all non-allocated sections">;
+def extract_dwo : Flag<["-", "--"], "extract-dwo">,
+                  HelpText<"Remove all sections that are not DWARF .dwo sections from file">;
+def localize_hidden : Flag<["-", "--"], "localize-hidden">,
+                      HelpText<"Mark all symbols that have hidden or internal visibility as local">;

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

 //===- llvm-objcopy.cpp ---------------------------------------------------===//
 //
 //                      The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm-objcopy.h"
 #include "Object.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/BinaryFormat/ELF.h"
 #include "llvm/Object/Binary.h"
 #include "llvm/Object/ELFObjectFile.h"
 #include "llvm/Object/ELFTypes.h"
 #include "llvm/Object/Error.h"
+#include "llvm/Option/Arg.h"
+#include "llvm/Option/ArgList.h"
+#include "llvm/Option/Option.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/FileOutputBuffer.h"
 #include "llvm/Support/ManagedStatic.h"

 #include <string>
 #include <system_error>
 #include <utility>
 
 using namespace llvm;
 using namespace object;
 using namespace ELF;
 
+namespace {
+
+enum ID {
+  OBJCOPY_INVALID = 0, // This is not an option ID.
+#define OPTION(PREFIX, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,  \
+               HELPTEXT, METAVAR, VALUES)                                      \
+  OBJCOPY_##ID,
+#include "Opts.inc"
+#undef OPTION
+};
+
+#define PREFIX(NAME, VALUE) const char *const NAME[] = VALUE;
+#include "Opts.inc"
+#undef PREFIX
+
+static constexpr opt::OptTable::Info ObjcopyInfoTable[] = {
+#define OPTION(PREFIX, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,  \
+               HELPTEXT, METAVAR, VALUES)                                      \
+  {PREFIX,          NAME,         HELPTEXT,                                    \
+   METAVAR,         OBJCOPY_##ID, opt::Option::KIND##Class,                    \
+   PARAM,           FLAGS,        OBJCOPY_##GROUP,                             \
+   OBJCOPY_##ALIAS, ALIASARGS,    VALUES},
+#include "Opts.inc"
+#undef OPTION
+};
+
+class ObjcopyOptTable : public opt::OptTable {
+public:
+  ObjcopyOptTable() : OptTable(ObjcopyInfoTable, true) {}
+};
+
+} // namespace
+
 // The name this program was invoked as.
 static StringRef ToolName;
 
 namespace llvm {
 
 LLVM_ATTRIBUTE_NORETURN void error(Twine Message) {
   errs() << ToolName << ": " << Message << ".\n";
   errs().flush();
LLVM_ATTRIBUTE_NORETURN void reportError(StringRef File, Error E) {
   logAllUnhandledErrors(std::move(E), OS, "");
   OS.flush();
   errs() << ToolName << ": '" << File << "': " << Buf;
   exit(1);
 }
 
 } // end namespace llvm
 
-static cl::opt<std::string> InputFilename(cl::Positional, cl::desc("<input>"));
-static cl::opt<std::string> OutputFilename(cl::Positional, cl::desc("[ <output> ]"));
+// Holds information related to the input and output of files in different
+// formats.
+struct Config {
+  StringRef OutputFormat;
+  StringRef InputFormat;
+  StringRef BinaryArch;
+  StringRef InputFilename;
+  StringRef OutputFilename;
+};
+
+// The type of all kinds of copies. So ELF, COFF, Archive and MachO should
+// all be handled by sub classes of this.
+class CopyAction {
+public:
+  StringRef SplitDWO;
+  StringRef AddGnuDebugLink;
+  std::vector<StringRef> ToRemove;
+  std::vector<StringRef> Keep;
+  std::vector<StringRef> OnlyKeep;
+  std::vector<StringRef> AddSection;
+  bool StripAll;
+  bool StripAllGNU;
+  bool StripDebug;
+  bool StripSections;
+  bool StripNonAlloc;
+  bool StripDWO;
+  bool ExtractDWO;
+  bool LocalizeHidden;
+
+  virtual ~CopyAction();
+  virtual void Execute() const = 0;
+};
 
-static cl::opt<std::string>
-    OutputFormat("O", cl::desc("Set output format to one of the following:"
-                               "\n\tbinary"));
-static cl::list<std::string> ToRemove("remove-section",
-                                      cl::desc("Remove <section>"),
-                                      cl::value_desc("section"));
-static cl::alias ToRemoveA("R", cl::desc("Alias for remove-section"),
-                           cl::aliasopt(ToRemove));
-static cl::opt<bool> StripAll(
-    "strip-all",
-    cl::desc(
-        "Removes non-allocated sections other than .gnu.warning* sections"));
-static cl::opt<bool>
-    StripAllGNU("strip-all-gnu",
-                cl::desc("Removes symbol, relocation, and debug information"));
-static cl::list<std::string> Keep("keep", cl::desc("Keep <section>"),
-                                  cl::value_desc("section"));
-static cl::list<std::string> OnlyKeep("only-keep",
-                                      cl::desc("Remove all but <section>"),
-                                      cl::value_desc("section"));
-static cl::alias OnlyKeepA("j", cl::desc("Alias for only-keep"),
-                           cl::aliasopt(OnlyKeep));
-static cl::opt<bool> StripDebug("strip-debug",
-                                cl::desc("Removes all debug information"));
-static cl::opt<bool> StripSections("strip-sections",
-                                   cl::desc("Remove all section headers"));
-static cl::opt<bool>
-    StripNonAlloc("strip-non-alloc",
-                  cl::desc("Remove all non-allocated sections"));
-static cl::opt<bool>
-    StripDWO("strip-dwo", cl::desc("Remove all DWARF .dwo sections from file"));
-static cl::opt<bool> ExtractDWO(
-    "extract-dwo",
-    cl::desc("Remove all sections that are not DWARF .dwo sections from file"));
-static cl::opt<std::string>
-    SplitDWO("split-dwo",
-             cl::desc("Equivalent to extract-dwo on the input file to "
-                      "<dwo-file>, then strip-dwo on the input file"),
-             cl::value_desc("dwo-file"));
-static cl::list<std::string> AddSection(
-    "add-section",
-    cl::desc("Make a section named <section> with the contents of <file>."),
-    cl::value_desc("section=file"));
-static cl::opt<bool> LocalizeHidden(
-    "localize-hidden",
-    cl::desc(
-        "Mark all symbols that have hidden or internal visibility as local"));
-static cl::opt<std::string>
-    AddGnuDebugLink("add-gnu-debuglink",
-                    cl::desc("adds a .gnu_debuglink for <debug-file>"),
-                    cl::value_desc("debug-file"));
+CopyAction::~CopyAction() {}
+
+class ELFCopyAction : public CopyAction {
+private:
+  // TODO: Replace with Reader/Writer.
+  StringRef InputFilename;
+  StringRef OutputFilename;
+  // TODO: Don't handle this here. Handle this in construction of Builder.
+  StringRef OutputFormat;
+
+  void HandleArgs(Object &Obj, const Reader &Reader,
+                  ElfType OutputElfType) const;
+  std::unique_ptr<Writer> CreateWriter(Object &Obj, StringRef File,
+                                       ElfType OutputElfType) const;
+  std::unique_ptr<Reader> CreateReader(ElfType &OutputElfType) const;
+  void SplitDWOToFile(const Reader &Reader, StringRef File,
+                      ElfType OutputElfType) const;
+
+public:
+  ~ELFCopyAction() override;
+  void Execute() const override;
+
+  ELFCopyAction(StringRef InputFilename, StringRef OutputFilename,
+                StringRef OutputFormat)
+      : InputFilename(InputFilename), OutputFilename(OutputFilename),
+        OutputFormat(OutputFormat) {}
+};
 
 using SectionPred = std::function<bool(const SectionBase &Sec)>;
 
 bool IsDWOSection(const SectionBase &Sec) { return Sec.Name.endswith(".dwo"); }
 
 bool OnlyKeepDWOPred(const Object &Obj, const SectionBase &Sec) {
   // We can't remove the section header string table.
   if (&Sec == Obj.SectionNames)
     return false;
   // Short of keeping the string table we want to keep everything that is a DWO
   // section and remove everything else.
   return !IsDWOSection(Sec);
 }
 
-static ElfType OutputElfType;
-
-std::unique_ptr<Writer> CreateWriter(Object &Obj, StringRef File) {
+std::unique_ptr<Writer>
+ELFCopyAction::CreateWriter(Object &Obj, StringRef File,
+                            ElfType OutputElfType) const {
   if (OutputFormat == "binary") {
-    return llvm::make_unique<BinaryWriter>(OutputFilename, Obj);
+    return llvm::make_unique<BinaryWriter>(File, Obj);
   }
   // Depending on the initial ELFT and OutputFormat we need a different Writer.
   switch (OutputElfType) {
   case ELFT_ELF32LE:
     return llvm::make_unique<ELFWriter<ELF32LE>>(File, Obj, !StripSections);
   case ELFT_ELF64LE:
     return llvm::make_unique<ELFWriter<ELF64LE>>(File, Obj, !StripSections);
   case ELFT_ELF32BE:
     return llvm::make_unique<ELFWriter<ELF32BE>>(File, Obj, !StripSections);
   case ELFT_ELF64BE:
     return llvm::make_unique<ELFWriter<ELF64BE>>(File, Obj, !StripSections);
   }
   llvm_unreachable("Invalid output format");
 }
 
-void SplitDWOToFile(const Reader &Reader, StringRef File) {
+void ELFCopyAction::SplitDWOToFile(const Reader &Reader, StringRef File,
+                                   ElfType OutputElfType) const {
   auto DWOFile = Reader.create();
   DWOFile->removeSections(
       [&](const SectionBase &Sec) { return OnlyKeepDWOPred(*DWOFile, Sec); });
-  auto Writer = CreateWriter(*DWOFile, File);
+  auto Writer = CreateWriter(*DWOFile, File, OutputElfType);
   Writer->finalize();
   Writer->write();
 }
 
 // This function handles the high level operations of GNU objcopy including
 // handling command line options. It's important to outline certain properties
 // we expect to hold of the command line operations. Any operation that "keeps"
 // should keep regardless of a remove. Additionally any removal should respect
 // any previous removals. Lastly whether or not something is removed shouldn't
 // depend a) on the order the options occur in or b) on some opaque priority
 // system. The only priority is that keeps/copies overrule removes.
-void HandleArgs(Object &Obj, const Reader &Reader) {
+void ELFCopyAction::HandleArgs(Object &Obj, const Reader &Reader,
+                               ElfType OutputElfType) const {
 
   if (!SplitDWO.empty()) {
-    SplitDWOToFile(Reader, SplitDWO);
+    SplitDWOToFile(Reader, SplitDWO, OutputElfType);
   }
 
   // Localize:
 
   if (LocalizeHidden) {
     Obj.SymbolTable->localize([](const Symbol &Sym) {
       return Sym.Visibility == STV_HIDDEN || Sym.Visibility == STV_INTERNAL;
     });
   }
 
   SectionPred RemovePred = [](const SectionBase &) { return false; };
 
   // Removes:
 
   if (!ToRemove.empty()) {
-    RemovePred = [&](const SectionBase &Sec) {
+    RemovePred = [this](const SectionBase &Sec) {
       return std::find(std::begin(ToRemove), std::end(ToRemove), Sec.Name) !=
              std::end(ToRemove);
     };
   }
 
   if (StripDWO || !SplitDWO.empty())
     RemovePred = [RemovePred](const SectionBase &Sec) {
       return IsDWOSection(Sec) || RemovePred(Sec);
RemovePred = [RemovePred, &Obj](const SectionBase &Sec) {
       if (Sec.Name.startswith(".gnu.warning"))
         return false;
       return (Sec.Flags & SHF_ALLOC) == 0;
     };
 
   // Explicit copies:
 
   if (!OnlyKeep.empty()) {
-    RemovePred = [RemovePred, &Obj](const SectionBase &Sec) {
+    RemovePred = [this, RemovePred, &Obj](const SectionBase &Sec) {
       // Explicitly keep these sections regardless of previous removes.
       if (std::find(std::begin(OnlyKeep), std::end(OnlyKeep), Sec.Name) !=
           std::end(OnlyKeep))
         return false;
 
       // Allow all implicit removes.
       if (RemovePred(Sec))
         return true;
 
       // Keep special sections.
       if (Obj.SectionNames == &Sec)
         return false;
       if (Obj.SymbolTable == &Sec || Obj.SymbolTable->getStrTab() == &Sec)
         return false;
 
       // Remove everything else.
       return true;
     };
   }
 
   if (!Keep.empty()) {
-    RemovePred = [RemovePred](const SectionBase &Sec) {
+    RemovePred = [this, RemovePred](const SectionBase &Sec) {
       // Explicitly keep these sections regardless of previous removes.
       if (std::find(std::begin(Keep), std::end(Keep), Sec.Name) !=
           std::end(Keep))
         return false;
       // Otherwise defer to RemovePred.
       return RemovePred(Sec);
     };
   }
if (!AddSection.empty()) {
     }
   }
 
   if (!AddGnuDebugLink.empty()) {
     Obj.addSection<GnuDebugLinkSection>(StringRef(AddGnuDebugLink));
   }
 }
 
-std::unique_ptr<Reader> CreateReader() {
+std::unique_ptr<Reader>
+ELFCopyAction::CreateReader(ElfType &OutputElfType) const {
   // Right now we can only read ELF files so there's only one reader;
   auto Out = llvm::make_unique<ELFReader>(StringRef(InputFilename));
   // We need to set the default ElfType for output.
   OutputElfType = Out->getElfType();
   return std::move(Out);
 }
 
+ELFCopyAction::~ELFCopyAction() {}
+
+void ELFCopyAction::Execute() const {
+  ElfType OutputElfType;
+  auto Reader = CreateReader(OutputElfType);
+  auto Obj = Reader->create();
+  auto Writer = CreateWriter(*Obj, OutputFilename, OutputElfType);
+  HandleArgs(*Obj, *Reader, OutputElfType);
+  Writer->finalize();
+  Writer->write();
+}
+
+// ParseObjcopyOptions returns the config and sets the input arguments. If a
+// help flag is set then ParseObjcopyOptions will print the help messege and
+// exit.
+Config ParseObjcopyOptions(ArrayRef<const char *> ArgsArr,
+                           opt::InputArgList &InputArgs, ObjcopyOptTable &T) {
+  unsigned MissingArgumentIndex, MissingArgumentCount;
+  InputArgs = T.ParseArgs(ArgsArr, MissingArgumentIndex, MissingArgumentCount);
+
+  if (InputArgs.size() == 0 || InputArgs.hasArg(OBJCOPY_help)) {
+    T.PrintHelp(outs(), "llvm-objcopy <input> [ <output> ]", "objcopy tool");
+    exit(0);
+  }
+
+  SmallVector<const char *, 2> Positional;
+
+  for (auto Arg : InputArgs.filtered(OBJCOPY_UNKNOWN))
+    error("unknown argument '" + Arg->getAsString(InputArgs) + "'");
+
+  for (auto Arg : InputArgs.filtered(OBJCOPY_INPUT))
+    Positional.push_back(Arg->getValue());
+
+  if (Positional.size() == 0)
+    error("No input file specified");
+
+  if (Positional.size() > 2)
+    error("Too many positional arguments");
+
+  Config out;
+  out.InputFilename = Positional[0];
+  out.OutputFilename = Positional[Positional.size() == 1 ? 0 : 1];
+  out.InputFormat = InputArgs.getLastArgValue(OBJCOPY_input_target);
+  out.OutputFormat = InputArgs.getLastArgValue(OBJCOPY_output_target);
+  out.BinaryArch = InputArgs.getLastArgValue(OBJCOPY_binary_architecture);
+
+  return out;
+}
+
+// TODO: Don't rely on Config.
+std::unique_ptr<CopyAction>
+ELFObjcopyAction(Config Config, const opt::InputArgList &InputArgs) {
+  auto action = llvm::make_unique<ELFCopyAction>(
+      Config.InputFilename, Config.OutputFilename, Config.OutputFormat);
+
+  action->SplitDWO = InputArgs.getLastArgValue(OBJCOPY_split_dwo);
+  action->AddGnuDebugLink =
+      InputArgs.getLastArgValue(OBJCOPY_add_gnu_debuglink);
+  if (action->AddGnuDebugLink != "") {
+    errs() << "add-gnu-debuglink was set!\n";
+  } else {
+    errs() << "flag was not set!\n";
+  }
+  for (auto Arg : InputArgs.filtered(OBJCOPY_remove_section))
+    action->ToRemove.push_back(Arg->getValue());
+  for (auto Arg : InputArgs.filtered(OBJCOPY_keep))
+    action->Keep.push_back(Arg->getValue());
+  for (auto Arg : InputArgs.filtered(OBJCOPY_only_keep))
+    action->OnlyKeep.push_back(Arg->getValue());
+  for (auto Arg : InputArgs.filtered(OBJCOPY_add_section))
+    action->AddSection.push_back(Arg->getValue());
+  action->StripAll = InputArgs.hasArg(OBJCOPY_strip_all);
+  action->StripAllGNU = InputArgs.hasArg(OBJCOPY_strip_all_gnu);
+  action->StripDebug = InputArgs.hasArg(OBJCOPY_strip_debug);
+  action->StripDWO = InputArgs.hasArg(OBJCOPY_strip_dwo);
+  action->StripSections = InputArgs.hasArg(OBJCOPY_strip_sections);
+  action->StripNonAlloc = InputArgs.hasArg(OBJCOPY_strip_non_alloc);
+  action->ExtractDWO = InputArgs.hasArg(OBJCOPY_extract_dwo);
+  action->LocalizeHidden = InputArgs.hasArg(OBJCOPY_localize_hidden);
+
+  return action;
+}
+
 int main(int argc, char **argv) {
   // Print a stack trace if we signal out.
   sys::PrintStackTraceOnErrorSignal(argv[0]);
   PrettyStackTraceProgram X(argc, argv);
   llvm_shutdown_obj Y; // Call llvm_shutdown() on exit.
-  cl::ParseCommandLineOptions(argc, argv, "llvm objcopy utility\n");
   ToolName = argv[0];
-  if (InputFilename.empty()) {
-    cl::PrintHelpMessage();
-    return 2;
-  }
 
-  auto Reader = CreateReader();
-  auto Obj = Reader->create();
-  StringRef Output =
-      OutputFilename.getNumOccurrences() ? OutputFilename : InputFilename;
-  auto Writer = CreateWriter(*Obj, Output);
-  HandleArgs(*Obj, *Reader);
-  Writer->finalize();
-  Writer->write();
+  ObjcopyOptTable T;
+  opt::InputArgList InputArgs;
+  Config Config =
+      ParseObjcopyOptions(makeArrayRef(argv + 1, argc), InputArgs, T);
+  // TODO: Move handeling of InputFilename, and Outputfilename to here.
+  // these should be supplied with high level Reader/Writer objects *not*
+  // filenames. This will be crucial when multiple file formats are supported.
+  // The Config object alone should be sufficent to construct these high level
+  // objects and corresponding copy action.
+  auto CopyAction = ELFObjcopyAction(Config, InputArgs);
+  CopyAction->Execute();
+  return 0;
 }