[tools] Introduce llvm-strip

[alexander-shaposhnikov]

Summary

To start the ball rolling this diff adds the initial bits for llvm-strip.
I think more options & tests can be added incrementally.

Test plan:
make check-all

[Eugene.Zelenko]

Will be good idea to add documentation and item in Release Notes.

[jakehehrlich]

LGTM but please wait on James

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

11

Can you add a diff here to ensure that there is binary equivalence?

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

11

diff here as well.

[jhenderson]

tools/llvm-objcopy/CMakeLists.txt

10

I'm not really familiar with cmake, but does this name have to be unique? If so, we might want to go back to the old name.

tools/llvm-objcopy/llvm-objcopy.cpp

487–490

I think the norm is to exit(1) if the input count is 0, unless help has been specified.

498–499

Test case?

519

I'm not a massive fan of using the tool's filename to determine the expected behaviour (example reason: if I change the executable from llvm-strip to llvm-strip-backup, when testing two versions of the tool side-by-side, I'd switch to objcopy style behaviour unexpectedly).

How feasible would it be to build two different executables, using the same source code, but a command-line specified define to control this?

[alexander-shaposhnikov]

tools/llvm-objcopy/CMakeLists.txt

10

if it matters - sure, i just wanted to have a consistent naming scheme Opts -> OptsTableGen, StripOpts -> StripOptsTableGen etc

tools/llvm-objcopy/llvm-objcopy.cpp

487–490

i was looking at the current behavior, would be better to have consistency here, we can update them both, though

519

@jhenderson
alexshap-mbp:tools alexshap$ grep -r -n add_llvm_tool_symlink ./*
./llvm-ar/CMakeLists.txt:17:add_llvm_tool_symlink(llvm-ranlib llvm-ar)
./llvm-ar/CMakeLists.txt:18:add_llvm_tool_symlink(llvm-lib llvm-ar)
./llvm-ar/CMakeLists.txt:19:add_llvm_tool_symlink(llvm-dlltool llvm-ar)
./llvm-ar/CMakeLists.txt:22: add_llvm_tool_symlink(ar llvm-ar)
./llvm-ar/CMakeLists.txt:23: add_llvm_tool_symlink(dlltool llvm-ar)
./llvm-ar/CMakeLists.txt:24: add_llvm_tool_symlink(ranlib llvm-ar)
./llvm-cxxfilt/CMakeLists.txt:11: add_llvm_tool_symlink(c++filt llvm-cxxfilt)
./llvm-dwp/CMakeLists.txt:20: add_llvm_tool_symlink(dwp llvm-dwp)
./llvm-nm/CMakeLists.txt:19: add_llvm_tool_symlink(nm llvm-nm)
./llvm-objcopy/CMakeLists.txt:21: add_llvm_tool_symlink(objcopy llvm-objcopy)
./llvm-objdump/CMakeLists.txt:30: add_llvm_tool_symlink(objdump llvm-objdump)
./llvm-readobj/CMakeLists.txt:26:add_llvm_tool_symlink(llvm-readelf llvm-readobj)
./llvm-readobj/CMakeLists.txt:29: add_llvm_tool_symlink(readelf llvm-readobj)
./llvm-size/CMakeLists.txt:11: add_llvm_tool_symlink(size llvm-size)
./llvm-strings/CMakeLists.txt:12: add_llvm_tool_symlink(strings llvm-strings)
./llvm-symbolizer/CMakeLists.txt:20: add_llvm_tool_symlink(addr2line llvm-symbolizer)

personally, I'm against building two different executables, it increases the build time, the size of the package, occupies the disk space.

[jhenderson]

tools/llvm-objcopy/CMakeLists.txt

10

If it doesn't need to be unique, I'm happy with it as is. If it does, I think we need a name that is more tool-specific (even if it's just the old name), to avoid clashing with some other tool.

tools/llvm-objcopy/llvm-objcopy.cpp

487–490

Yes, I think we should update them both. Happy for that to be a separate change though.

519

personally, I'm against building two different executables, it increases the build time, the size of the package, occupies the disk space.

I think this is just a philosophical difference. Happy to defer to the code owner's preference.

FWIW, most of those examples aren't fair comparisons, since they are just mapping the llvm tool name to the equivalent GNU name (which I have no issue with). In this case, we actually have different behaviour depending on the name, which I doubt is the case in any of those tools (although I haven't looked). That being said, LLD is also an example that does something similar (different behaviour depending on the file name).

[alexander-shaposhnikov]

tools/llvm-objcopy/llvm-objcopy.cpp

519

@jhenderson

which I doubt is the case in any of those tool

FWIW - that's not correct:

./llvm-ar/llvm-ar.cpp:957: Stem = sys::path::stem(ToolName);
./llvm-ar/llvm-ar.cpp-958- if (Stem.contains_lower("dlltool"))

./llvm-cov/llvm-cov.cpp:64: if (sys::path::stem(argv[0]).endswith_lower("gcov"))
./llvm-cov/llvm-cov.cpp-65- return gcovMain(argc, argv);

./llvm-readobj/llvm-readobj.cpp:564: if (sys::path::stem(argv[0]).find("readelf") != StringRef::npos)
./llvm-readobj/llvm-readobj.cpp-565- opts::Output = opts::GNU;

maybe more, i didn't look further

[alexander-shaposhnikov]

tools/llvm-objcopy/llvm-objcopy.cpp

487–490

ok, sure,
I will send a separate diff to update it once this gets committed.

498–499

yeah, will add

[alexander-shaposhnikov]

Address comments

[jakehehrlich]

tools/llvm-objcopy/CMakeLists.txt

10

Actully, can we rename Opts.td to ObjcopyOpts.td?

tools/llvm-objcopy/llvm-objcopy.cpp

519

I tried a while back, quite hard, to get cmake to build two tools using the same code. Unless we want to put the effort into making this a proper library (and e.g. making sure there is a sufficiently common interface to all object formats, making everything super nice, etc...) we can't make two separate binaries. Also teams that use llvm-objcopy care about toolchain size and this lets them add llvm-strip without worry. I thought a lot about how to make this a binary vs doing what every other tool has done and I decided I preferred the symlink option. Most of the point of moving to TableGen was to allow llvm-strip to exist. In a nutshell, I'd like to strongly advocate for this approach over what I see as the only alternative, making objcopy a library.

[alexander-shaposhnikov]

Opts.td -> ObjcopyOpts.td

[jhenderson]

As I had open comments, I'd have preferred this not been committed until after I'd given the thumbs up too...

Test comments are all that I've got though.

llvm/trunk/test/tools/llvm-objcopy/strip-debug.test

13 ↗

(On Diff #145517)

This should probably be a distinct test that doesn't use any command-line options (similar to the basic-copy style tests for objcopy).

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

494 ↗

(On Diff #145517)

Test case for this as well.

[alexander-shaposhnikov]

llvm/trunk/test/tools/llvm-objcopy/strip-debug.test

13 ↗

(On Diff #145517)

ok, i'll send a follow-up diff today

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

 # RUN: yaml2obj %s > %t
 # RUN: llvm-objcopy --strip-all %t %t2
 # RUN: llvm-readobj -file-headers -sections %t2 | FileCheck %s
 
+# We run yaml2obj again rather than copy %t to avoid interfering
+# with llvm-objcopy's test (which potentially could have corrupted/updated the binary).
+
+# RUN: yaml2obj %s > %t3
+# RUN: llvm-strip %t3 
+# RUN: llvm-readobj -file-headers -sections %t3 | FileCheck %s
+
 !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_REL
   Machine:         EM_X86_64
 Sections:
   - Name:            .bss

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

 # RUN: yaml2obj %s > %t
 # RUN: llvm-objcopy -strip-debug %t %t2
 # RUN: llvm-readobj -file-headers -sections -symbols %t2 | FileCheck %s
 
+# We run yaml2obj again rather than copy %t to avoid interfering 
+# with llvm-objcopy's test (which potentially could have corrupted/updated the binary).
+
+# RUN: yaml2obj %s > %t
+# RUN: llvm-strip -strip-debug %t3
+# RUN: llvm-readobj -file-headers -sections -symbols %t3 | FileCheck %s
+
 !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_REL
   Machine:         EM_X86_64
 Sections:
   - Name:            .debugfoo

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_public_tablegen_target(OptsTableGen)
+
+set(LLVM_TARGET_DEFINITIONS StripOpts.td)
+tablegen(LLVM StripOpts.inc -gen-opt-parser-defs)
+add_public_tablegen_target(StripOptsTableGen)
 
 add_llvm_tool(llvm-objcopy
   llvm-objcopy.cpp
   Object.cpp
   DEPENDS
-  ObjcopyTableGen
+  OptsTableGen
+  StripOptsTableGen
   )
 
+add_llvm_tool_symlink(llvm-strip llvm-objcopy)
+
 if(LLVM_INSTALL_BINUTILS_SYMLINKS)
   add_llvm_tool_symlink(objcopy llvm-objcopy)
+  add_llvm_tool_symlink(strip llvm-objcopy)
 endif()

tools/llvm-objcopy/StripOpts.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">;
+
+def strip_debug : Flag<["-", "--"], "strip-debug">,
+                  HelpText<"Remove debugging symbols only">;
+

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/InitLLVM.h"
+#include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 #include <cassert>
 #include <cstdlib>
 #include <functional>
 #include <iterator>
 #include <memory>
 #include <string>
 #include <system_error>
 #include <utility>
 
 using namespace llvm;
 using namespace object;
 using namespace ELF;
 
 namespace {
 
-enum ID {
+enum ObjcopyID {
   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
 };
 

 #undef OPTION
 };
 
 class ObjcopyOptTable : public opt::OptTable {
 public:
   ObjcopyOptTable() : OptTable(ObjcopyInfoTable, true) {}
 };
 
+enum StripID {
+  STRIP_INVALID = 0, // This is not an option ID.
+#define OPTION(PREFIX, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,  \
+               HELPTEXT, METAVAR, VALUES)                                      \
+  STRIP_##ID,
+#include "StripOpts.inc"
+#undef OPTION
+};
+
+static const opt::OptTable::Info StripInfoTable[] = {
+#define OPTION(PREFIX, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,  \
+               HELPTEXT, METAVAR, VALUES)                                      \
+  {PREFIX,          NAME,         HELPTEXT,                                    \
+   METAVAR,         STRIP_##ID, opt::Option::KIND##Class,                      \
+   PARAM,           FLAGS,        STRIP_##GROUP,                               \
+   STRIP_##ALIAS, ALIASARGS,    VALUES},
+#include "StripOpts.inc"
+#undef OPTION
+};
+
+class StripOptTable : public opt::OptTable {
+public:
+  StripOptTable() : OptTable(StripInfoTable, true) {}
+};
+
 } // namespace
 
 // The name this program was invoked as.
 static StringRef ToolName;
 
 namespace llvm {
 
 LLVM_ATTRIBUTE_NORETURN void error(Twine Message) {
struct CopyConfig {
   std::vector<StringRef> ToRemove;
   std::vector<StringRef> Keep;
   std::vector<StringRef> OnlyKeep;
   std::vector<StringRef> AddSection;
   std::vector<StringRef> SymbolsToLocalize;
   std::vector<StringRef> SymbolsToGlobalize;
   std::vector<StringRef> SymbolsToWeaken;
   StringMap<StringRef> SymbolsToRename;
-  bool StripAll;
-  bool StripAllGNU;
-  bool StripDebug;
-  bool StripSections;
-  bool StripNonAlloc;
-  bool StripDWO;
-  bool ExtractDWO;
-  bool LocalizeHidden;
-  bool Weaken;
-  bool DiscardAll;
+  bool StripAll = false;
+  bool StripAllGNU = false;
+  bool StripDebug = false;
+  bool StripSections = false;
+  bool StripNonAlloc = false;
+  bool StripDWO = false;
+  bool ExtractDWO = false;
+  bool LocalizeHidden = false;
+  bool Weaken = false;
+  bool DiscardAll = false;
 };
 
 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.
CopyConfig ParseObjcopyOptions(ArrayRef<const char *> ArgsArr) {
   for (auto Arg : InputArgs.filtered(OBJCOPY_globalize_symbol))
     Config.SymbolsToGlobalize.push_back(Arg->getValue());
   for (auto Arg : InputArgs.filtered(OBJCOPY_weaken_symbol))
     Config.SymbolsToWeaken.push_back(Arg->getValue());
 
   return Config;
 }
 
+// ParseStripOptions returns the config and sets the input arguments. If a
+// help flag is set then ParseStripOptions will print the help messege and
+// exit.
+CopyConfig ParseStripOptions(ArrayRef<const char *> ArgsArr) {
+  StripOptTable T;
+  unsigned MissingArgumentIndex, MissingArgumentCount;
+  llvm::opt::InputArgList InputArgs =
+      T.ParseArgs(ArgsArr, MissingArgumentIndex, MissingArgumentCount);
+
+  if (InputArgs.size() == 0 || InputArgs.hasArg(STRIP_help)) {
+    T.PrintHelp(outs(), "llvm-strip <input> [ <output> ]", "strip tool");
+    exit(0);
+  }
+
+  SmallVector<const char *, 2> Positional;
+  for (auto Arg : InputArgs.filtered(STRIP_UNKNOWN))
+    error("unknown argument '" + Arg->getAsString(InputArgs) + "'");
+  for (auto Arg : InputArgs.filtered(STRIP_INPUT))
+    Positional.push_back(Arg->getValue());
+
+  if (Positional.empty())
+    error("No input file specified");
+
+  if (Positional.size() > 2)
+    error("Support for multiple input files is not implemented yet");
+
+  CopyConfig Config;
+  Config.InputFilename = Positional[0];
+  Config.OutputFilename = Positional[0];
+
+  // Strip debug info only.
+  Config.StripDebug = InputArgs.hasArg(STRIP_strip_debug);
+  if (!Config.StripDebug)
+    Config.StripAll = true;
+  return Config;
+}
+
 int main(int argc, char **argv) {
   InitLLVM X(argc, argv);
   ToolName = argv[0];
-
-  CopyConfig Config = ParseObjcopyOptions(makeArrayRef(argv + 1, argc));
+  CopyConfig Config;
+  if (sys::path::stem(ToolName).endswith_lower("strip"))
+    Config = ParseStripOptions(makeArrayRef(argv + 1, argc));
+  else
+    Config = ParseObjcopyOptions(makeArrayRef(argv + 1, argc));
   ExecuteElfObjcopy(Config);
 }