[llvm-objcopy] Refactor llvm-objcopy to allow llvm-strip to differ

[jakehehrlich]

Summary

Being a drop in replacement for GNU objcopy has been a desire of the project as it has been devolved but we'd also like to replace GNU strip as well. Unfortunately they sport different interfaces and in one cases their interfaces contradict each other. This change adds a means of allowing for different behavior between objcopy and strip.

1. The options have been moved down further into the code and put into a namespace so that they're relatively hidden
2. InvokeMain now calls the appropriate function either Strip or Objcopy
3. The CopyConfig struct was added (and CopyBinary made a method of it) in order to drive CopyBinary instead of the options directly
4. The only difference added between llvm-objcopy and llvm-strip right now is that llvm-strip defaults to --strip-all

[jakehehrlich]

Added test for the "-S" option's behavior in both cases. Everything else should be common behavior.

[jhenderson]

It looks to me like you should split this change down into multiple parts, in no particular order:

1. Create a symlink for llvm-strip, and show that you can test it for a basic case.
2. Refactor the command-line options.
3. Add explicit -o option.
4. Add -S option.
5. Add support for explicit -O elf (by the way, you need a test for this, probably a simple copy, and binary diff against it not being specified).

What do you think?

tools/llvm-objcopy/llvm-objcopy.cpp

165

Why this change?

235

Again why this change?

259

Ditto

[jakehehrlich]

That order of changes sounds pretty reasonable to me. I'll work on breaking it up.

tools/llvm-objcopy/llvm-objcopy.cpp

165

ToRemove (and Keep and OnlyKeep) are members of this. This function became a method so that I could change all the variables in it via CopyConfig's members. Adding "this" to the capture list lets me use ToRemove in this function.

[jakehehrlich]

Split this diff up. This part of the split is most closely related to the original so I put it here.

[jhenderson]

test/tools/llvm-objcopy/basic-strip.test

1

I think you accidentally made this a diff against your previous version, although it's a new file!

tools/llvm-objcopy/llvm-objcopy.cpp

165

At least in this particular case, you are already capturing everything by reference anyway, so ToRemove seems to be available (at least in my MSVC build). The other two cases are fine. I'd also support here replacing the general & capture with an explicit this capture, since only ToRemove is used.

326

It feels to me like this should be a member of the CopyConfig class. Maybe even part of the constructor. Is there ever a case you can imagine when you would NOT want to call CommonConfig?

[jhenderson]

Oh, you also seem to have lost the changes that llvm-strip as an alias.

[jakehehrlich]

In D41100#953574, @jhenderson wrote:

Oh, you also seem to have lost the changes that llvm-strip as an alias.

That's in another change that I added you as a reviewer on. This diff (the one that mentions adding a symlink) is relative to that one.

[jakehehrlich]

tools/llvm-objcopy/llvm-objcopy.cpp

326

Nope, I think it will always be called. Also I'm currently working on splitting this up into two separate binaries instead of using this symlink hack. This is going to go in a common .o file that both will use. Good idea.

[jhenderson]

This change as-is looks fine to me, except for the previously highlighted issue of incompatibility in the positional arguments between the two. If you're working on splitting it up, I think it's best to wait to see what comes out before I review this further.

tools/llvm-objcopy/llvm-objcopy.cpp

334–335

This needs clang-formatting.

[jakehehrlich]

Alex did this (and this is a stupidly old change)

test/tools/llvm-objcopy/basic-strip.test

1

That was on purpose, there's another diff up for review that needs to be reviewed first.

test/tools/llvm-objcopy/basic-strip.test

 # RUN: yaml2obj %s > %t
 # RUN: llvm-strip %t %t2
 # RUN: llvm-readobj -file-headers -sections %t2 | FileCheck %s
 
 !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_REL
   Machine:         EM_X86_64
 Sections:
   - Name:            .bss
     Type:            SHT_NOBITS
     Flags:           [ SHF_ALLOC ]
   - Name:            .text
     Type:            SHT_PROGBITS
     Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
   - Name:            .blarg
     Type:            SHT_PROGBITS
     Flags:           [ ]
   - Name:            .gnu.warning.foo
     Type:            SHT_PROGBITS
 
-# CHECK: SectionHeaderCount: 8
+# CHECK: SectionHeaderCount: 5
 
 # CHECK: Name: .bss
 # CHECK: Name: .text
-# CHECK: Name: .blarg
 # CHECK: Name: .gnu.warning.foo
-# CHECK: Name: .symtab
-# CHECK: Name: .strtab
 # CHECK: Name: .shstrtab

tools/llvm-objcopy/llvm-objcopy.cpp

 #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 "llvm/Support/Path.h"
 #include "llvm/Support/PrettyStackTrace.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 #include <cassert>
 #include <cstdlib>
 #include <functional>
 #include <iterator>
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>"),
-                                    cl::init("-"));
-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"));
-
 using SectionPred = std::function<bool(const SectionBase &Sec)>;
 
 bool IsDWOSection(const SectionBase &Sec) {
   return Sec.Name.endswith(".dwo");
 }
 
 template <class ELFT>
 bool OnlyKeepDWOPred(const Object<ELFT> &Obj, const SectionBase &Sec) {
   // We can't remove the section header string table.
   if (&Sec == Obj.getSectionHeaderStrTab())
     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);
 }
 
 template <class ELFT>
 void WriteObjectFile(const Object<ELFT> &Obj, StringRef File) {
   std::unique_ptr<FileOutputBuffer> Buffer;
   Expected<std::unique_ptr<FileOutputBuffer>> BufferOrErr =
       FileOutputBuffer::create(File, Obj.totalSize(),
                                FileOutputBuffer::F_executable);
-  handleAllErrors(BufferOrErr.takeError(), [](const ErrorInfoBase &) {
-    error("failed to open " + OutputFilename);
+  handleAllErrors(BufferOrErr.takeError(), [=](const ErrorInfoBase &) {
+    error("failed to open " + File);
   });
   Buffer = std::move(*BufferOrErr);
 
   Obj.write(*Buffer);
   if (auto E = Buffer->commit())
     reportError(File, errorToErrorCode(std::move(E)));
 }
 
 template <class ELFT>
 void SplitDWOToFile(const ELFObjectFile<ELFT> &ObjFile, StringRef File) {
   // Construct a second output file for the DWO sections.
   ELFObject<ELFT> DWOFile(ObjFile);
 
   DWOFile.removeSections([&](const SectionBase &Sec) {
     return OnlyKeepDWOPred<ELFT>(DWOFile, Sec);
   });
   DWOFile.finalize();
   WriteObjectFile(DWOFile, File);
 }
 
+template <class ELFT>
+struct CopyConfig {
+  StringRef OutputFilename;
+  StringRef OutputFormat;
+  const ELFObjectFile<ELFT> &ObjFile;
+  std::vector<StringRef> ToRemove;
+  std::vector<StringRef> Keep;
+  std::vector<StringRef> OnlyKeep;
+  bool StripAll;
+  bool StripAllGNU;
+  bool StripSections;
+  bool StripDebug;
+  bool StripNonAlloc;
+  bool StripDWO;
+  bool ExtractDWO;
+  Optional<StringRef> SplitDWO;
+  CopyConfig(const ELFObjectFile<ELFT> &ObjFile) : ObjFile(ObjFile) {}
+  void copyBinary();
+};
+
 // 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.
 template <class ELFT>
-void CopyBinary(const ELFObjectFile<ELFT> &ObjFile) {
+void CopyConfig<ELFT>::copyBinary() {
   std::unique_ptr<Object<ELFT>> Obj;
 
-  if (!OutputFormat.empty() && OutputFormat != "binary")
+  if (OutputFormat != "elf" && OutputFormat != "binary")
     error("invalid output format '" + OutputFormat + "'");
-  if (!OutputFormat.empty() && OutputFormat == "binary")
+  if (OutputFormat == "binary")
     Obj = llvm::make_unique<BinaryObject<ELFT>>(ObjFile);
   else
     Obj = llvm::make_unique<ELFObject<ELFT>>(ObjFile);
 
-  if (!SplitDWO.empty())
-    SplitDWOToFile<ELFT>(ObjFile, SplitDWO.getValue());
+  if (SplitDWO)
+    SplitDWOToFile<ELFT>(ObjFile, *SplitDWO);
 
   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())
+  if (StripDWO || SplitDWO)
     RemovePred = [RemovePred](const SectionBase &Sec) {
       return IsDWOSection(Sec) || RemovePred(Sec);
     };
 
   if (ExtractDWO)
     RemovePred = [RemovePred, &Obj](const SectionBase &Sec) {
       return OnlyKeepDWOPred(*Obj, 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 = [RemovePred, &Obj, this](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->getSectionHeaderStrTab() == &Sec) {
         return false;
       }
       if (Obj->getSymTab() == &Sec || Obj->getSymTab()->getStrTab() == &Sec) {
         return false;
       }
       // Remove everything else.
       return true;
     };
   }
 
   if (!Keep.empty()) {
-    RemovePred = [RemovePred](const SectionBase &Sec) {
+    RemovePred = [RemovePred, this](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);
     };
   }
 
   Obj->removeSections(RemovePred);
   Obj->finalize();
-  WriteObjectFile(*Obj, OutputFilename.getValue());
+  WriteObjectFile(*Obj, OutputFilename);
+}
+
+// This namespace hides our flags so that only our driver functions see them.
+namespace flags {
+
+static cl::opt<std::string> InputFilename(cl::Positional, cl::desc("<input>"));
+static cl::opt<std::string> OutputFilename(cl::Positional, cl::desc("<output>"));
+static cl::opt<std::string>
+    OutputFormat("O", cl::desc("Set output format to one of the following:"
+                               "\n\tbinary\n\telf"), cl::init("elf"));
+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"));
+
+}
+
+// This nameless namespace is just to scope the useage of the flags namespace.
+namespace {
+
+using namespace flags;
+
+template <class ELFT>
+void CommonConfig(CopyConfig<ELFT>& Config) {
+  if (OutputFilename.empty())
+    Config.OutputFilename = InputFilename;
+  else
+    Config.OutputFilename = OutputFilename;
+  Config.OutputFormat = OutputFormat.getValue();
+  Config.ExtractDWO = ExtractDWO;
+  Config.Keep.insert(std::end(Config.Keep), std::begin(Keep), std::end(Keep));
+  Config.OnlyKeep.insert(std::end(Config.OnlyKeep), std::begin(OnlyKeep), std::end(OnlyKeep));
+  Config.ToRemove.insert(std::end(Config.ToRemove), std::begin(ToRemove), std::end(ToRemove));
+  if(!SplitDWO.empty())
+    Config.SplitDWO = SplitDWO;
+  Config.StripDWO = StripDWO;
+  Config.StripSections = StripSections;
+  Config.StripAllGNU = StripAllGNU;
+  Config.StripDebug = StripDebug;
+  Config.StripNonAlloc = StripNonAlloc;
+}
+
+template <class ELFT>
+void Strip(const ELFObjectFile<ELFT>& ObjFile) {
+  CopyConfig<ELFT> Config(ObjFile);
+  CommonConfig(Config);
+  Config.StripAll = StripAll ||
+                    !(StripAllGNU || StripDebug || StripDWO || StripNonAlloc ||
+                      StripSections);
+  Config.copyBinary();
+}
+
+template <class ELFT>
+void Objcopy(const ELFObjectFile<ELFT>& ObjFile) {
+  CopyConfig<ELFT> Config(ObjFile);
+  CommonConfig(Config);
+  Config.StripAll = StripAll;
+  Config.copyBinary();
+}
+
+}
+
+template <class ELFT>
+void InvokeMain(const ELFObjectFile<ELFT>& ObjFile) {
+  StringRef Stem = sys::path::stem(ToolName);
+  if (Stem.find("strip") != StringRef::npos) {
+    Strip(ObjFile);
+  } else {
+    Objcopy(ObjFile);
+  }
 }
 
 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;
   }
   Expected<OwningBinary<Binary>> BinaryOrErr = createBinary(InputFilename);
   if (!BinaryOrErr)
     reportError(InputFilename, BinaryOrErr.takeError());
   Binary &Binary = *BinaryOrErr.get().getBinary();
   if (auto *o = dyn_cast<ELFObjectFile<ELF64LE>>(&Binary)) {
-    CopyBinary(*o);
+    InvokeMain(*o);
     return 0;
   }
   if (auto *o = dyn_cast<ELFObjectFile<ELF32LE>>(&Binary)) {
-    CopyBinary(*o);
+    InvokeMain(*o);
     return 0;
   }
   if (auto *o = dyn_cast<ELFObjectFile<ELF64BE>>(&Binary)) {
-    CopyBinary(*o);
+    InvokeMain(*o);
     return 0;
   }
   if (auto *o = dyn_cast<ELFObjectFile<ELF32BE>>(&Binary)) {
-    CopyBinary(*o);
+    InvokeMain(*o);
     return 0;
   }
   reportError(InputFilename, object_error::invalid_file_type);
 }