[llvm-objcopy] Add --discard-all (-x) option

[paulsemel]

Summary

This option removes non-global symbols from the binary.
I tried to follow GNU Binutils behavior as much as I could.

[alexander-shaposhnikov]

tools/llvm-objcopy/llvm-objcopy.cpp

343

const Symbol &Sym

344

return Config.DiscardAll && Sym.Binding == STB_LOCAL &&
           Sym.getShndx() != SHN_UNDEF && Sym.Type != STT_FILE &&
           Sym.Type != STT_SECTION

[jakehehrlich]

tools/llvm-objcopy/Object.cpp

196–197

Can you implement this in terms of removeSymbols?

212

Can you capture ToRemove by value? Capturing by reference just adds a layer of indirection. Also can you use const SymPtr& for the type?

216–217

Size = Symbols.size() * EntrySize

[jhenderson]

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

3

Could you extend this test to check that -x works the same as --discard-all, please.

26

Please define this symbol in a section.

tools/llvm-objcopy/Opts.td

76

I know this is what gnu objcopy says, but non-global means not STB_GLOBAL to me, but it doesn't affect some other symbol types either (i.e. STB_WEAK, STT_FILE, STT_SECTION). Could you change the help text to match what it actually does, please.

[paulsemel]

Added various fixes.

tools/llvm-objcopy/Opts.td

76

Yes, I know, this is a really bad help text. But I was out of ideas, as the real behavior of this option is really tricky... Any thoughts about it guys ?
I was thinking of "Remove all but file and section local symbols" , but even for me that sounds really bad to my ears, so..

tools/llvm-objcopy/llvm-objcopy.cpp

344

Actually, as there is other options that will use this removeSymbols function, this choice was made to make it clearer. I was thinking about something like :

if (Config.DiscardAll && Blabla...)
    return true;

if (Config.DiscardLocals && Blabla..)
    return true;

return false;

I personally find it easier to read, even if this is not the most optimized way for doing it. What do you think ?

[jhenderson]

tools/llvm-objcopy/Opts.td

76

How about "remove all local symbols except file and section symbols"? Similar to yours but feels a little less clunky.

[paulsemel]

Changed the option help text to make it more descriptive

tools/llvm-objcopy/Opts.td

76

I like this one :)

[jhenderson]

LGTM, but please wait for feedback from the other reviewers who have commented.

[jhenderson]

tools/llvm-objcopy/Opts.td

76

clang-format?

[paulsemel]

tools/llvm-objcopy/Opts.td

76

Hmm.. Just ran it again, and it doesn't seem I missed indent :)

[jhenderson]

tools/llvm-objcopy/Opts.td

76

Oh, wait, didn't realise this was Options.td! Sorry, never mind :)

[alexander-shaposhnikov]

tools/llvm-objcopy/llvm-objcopy.cpp

344

i think having

if (expression which evaluates to bool) 
    return true;
return false

is kinda strange because "if" and the second "return" can be eliminated (there is no actual branching here), thus i would prefer the less verbose form, clang-format (don't forget -style=llvm) will format the long expression in a nice way, so it's not a big issue. That's my preference, but i don't really insist.

[paulsemel]

tools/llvm-objcopy/llvm-objcopy.cpp

344

Sure, I understand that we can eliminate the condition, of course. My point was more that I find it "nicer", and, as I mentioned, there will be other conditions for other options, and I truly think that it might be ugly to have only one big return statement.
About the clang-format, I'm pretty sure I am doing it well, I tried again on this part of the code, but there was no changes :)

[paulsemel]

committed on r331400

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

+# RUN: yaml2obj %s > %t
+# RUN: llvm-objcopy --discard-all %t %t2
+# RUN: llvm-objcopy -x %t %t3
+# RUN: llvm-readobj -symbols %t2 | FileCheck %s
+# RUN: diff %t2 %t3
+
+!ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_X86_64
+Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    Address:         0x1000
+    AddressAlign:    0x0000000000000010
+    Size:            64
+Symbols:
+  Local:
+    - Name:     Local
+      Type:     STT_FUNC
+      Section:  .text
+      Value:    0x1000
+      Size:     8
+    - Name:     LocalSection
+      Type:     STT_SECTION
+      Section:  .text
+    - Name:     LocalFile
+      Type:     STT_FILE
+  Weak:
+    - Name:     Weak
+      Type:     STT_FUNC
+      Size:     8
+      Section:  .text
+      Value:    0x1008
+  Global:
+    - Name:     Global
+      Type:     STT_FUNC
+      Size:     8
+      Section:  .text
+      Value:    0x1010
+
+#CHECK: Symbols [
+#CHECK-NEXT:  Symbol {
+#CHECK-NEXT:    Name:
+#CHECK-NEXT:    Value: 0x0
+#CHECK-NEXT:    Size: 0
+#CHECK-NEXT:    Binding: Local
+#CHECK-NEXT:    Type: None
+#CHECK-NEXT:    Other: 0
+#CHECK-NEXT:    Section: Undefined
+#CHECK-NEXT:  }
+#CHECK-NEXT:  Symbol {
+#CHECK-NEXT:    Name: LocalSection
+#CHECK-NEXT:    Value: 0x0
+#CHECK-NEXT:    Size: 0
+#CHECK-NEXT:    Binding: Local
+#CHECK-NEXT:    Type: Section
+#CHECK-NEXT:    Other: 0
+#CHECK-NEXT:    Section: .text
+#CHECK-NEXT:  }
+#CHECK-NEXT:  Symbol {
+#CHECK-NEXT:    Name: LocalFile
+#CHECK-NEXT:    Value: 0x0
+#CHECK-NEXT:    Size: 0
+#CHECK-NEXT:    Binding: Local
+#CHECK-NEXT:    Type: File
+#CHECK-NEXT:    Other: 0
+#CHECK-NEXT:    Section: Undefined
+#CHECK-NEXT:  }
+#CHECK-NEXT:  Symbol {
+#CHECK-NEXT:    Name: Global
+#CHECK-NEXT:    Value: 0x1010
+#CHECK-NEXT:    Size: 8
+#CHECK-NEXT:    Binding: Global
+#CHECK-NEXT:    Type: Function
+#CHECK-NEXT:    Other: 0
+#CHECK-NEXT:    Section: .text
+#CHECK-NEXT:  }
+#CHECK-NEXT:  Symbol {
+#CHECK-NEXT:    Name: Weak
+#CHECK-NEXT:    Value: 0x1008
+#CHECK-NEXT:    Size: 8
+#CHECK-NEXT:    Binding: Weak
+#CHECK-NEXT:    Type: Function
+#CHECK-NEXT:    Other: 0
+#CHECK-NEXT:    Section: .text
+#CHECK-NEXT:  }
+#CHECK-NEXT:]

tools/llvm-objcopy/Object.h

 public:
   void addSymbol(StringRef Name, uint8_t Bind, uint8_t Type,
                  SectionBase *DefinedIn, uint64_t Value, uint8_t Visibility,
                  uint16_t Shndx, uint64_t Sz);
   void addSymbolNames();
   const SectionBase *getStrTab() const { return SymbolNames; }
   const Symbol *getSymbolByIndex(uint32_t Index) const;
   void updateSymbols(function_ref<void(Symbol &)> Callable);
+  void removeSymbols(function_ref<bool(Symbol &)> ToRemove);
 
   void removeSectionReferences(const SectionBase *Sec) override;
   void initialize(SectionTableRef SecTable) override;
   void finalize() override;
   void accept(SectionVisitor &Visitor) const override;
 
   static bool classof(const SectionBase *S) {
     return S->Type == ELF::SHT_SYMTAB;

tools/llvm-objcopy/Object.cpp

void SymbolTableSection::addSymbol(StringRef Name, uint8_t Bind, uint8_t Type,
   Size += this->EntrySize;
 }
 
 void SymbolTableSection::removeSectionReferences(const SectionBase *Sec) {
   if (SymbolNames == Sec) {
     error("String table " + SymbolNames->Name +
           " cannot be removed because it is referenced by the symbol table " +
           this->Name);
   }
-  auto Iter =
-      std::remove_if(std::begin(Symbols), std::end(Symbols),
-                     [=](const SymPtr &Sym) { return Sym->DefinedIn == Sec; });
-  Size -= (std::end(Symbols) - Iter) * this->EntrySize;
-  Symbols.erase(Iter, std::end(Symbols));
-  assignIndices();
+  removeSymbols([Sec](const Symbol &Sym) { return Sym.DefinedIn == Sec; });
 }
 
 void SymbolTableSection::updateSymbols(function_ref<void(Symbol &)> Callable) {
   for (auto &Sym : Symbols)
     Callable(*Sym);
   std::stable_partition(
       std::begin(Symbols), std::end(Symbols),
       [](const SymPtr &Sym) { return Sym->Binding == STB_LOCAL; });
   assignIndices();
 }
 
+void SymbolTableSection::removeSymbols(function_ref<bool(Symbol &)> ToRemove) {
+  Symbols.erase(
+      std::remove_if(std::begin(Symbols), std::end(Symbols),
+                     [ToRemove](const SymPtr &Sym) { return ToRemove(*Sym); }),
+      std::end(Symbols));
+  Size = Symbols.size() * EntrySize;
+  assignIndices();
+}
+
 void SymbolTableSection::initialize(SectionTableRef SecTable) {
   Size = 0;
   setStrTab(SecTable.getSectionOfType<StringTableSection>(
       Link,
       "Symbol table has link index of " + Twine(Link) +
           " which is not a valid index",
       "Symbol table has link index of " + Twine(Link) +
           " which is not a string table"));

tools/llvm-objcopy/Opts.td

 defm globalize_symbol : Eq<"globalize-symbol">,
                        MetaVarName<"symbol">,
                        HelpText<"Mark <symbol> as global">;
 defm weaken_symbol : Eq<"weaken-symbol">,
                        MetaVarName<"symbol">,
                        HelpText<"Mark <symbol> as weak">;
 def W : JoinedOrSeparate<["-"], "W">,
         Alias<weaken_symbol>;
+def discard_all : Flag<["-", "--"], "discard-all">,
+                      HelpText<"Remove all non-global symbols">;
+def x : Flag<["-"], "x">,
+        Alias<discard_all>;

tools/llvm-objcopy/llvm-objcopy.cpp

struct CopyConfig {
   bool StripAll;
   bool StripAllGNU;
   bool StripDebug;
   bool StripSections;
   bool StripNonAlloc;
   bool StripDWO;
   bool ExtractDWO;
   bool LocalizeHidden;
+  bool DiscardAll;
 };
 
 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.
Obj.SymbolTable->updateSymbols([&](Symbol &Sym) {
           is_contained(Config.SymbolsToWeaken, Sym.Name) &&
           Sym.Binding == STB_GLOBAL)
         Sym.Binding = STB_WEAK;
 
       const auto I = Config.SymbolsToRename.find(Sym.Name);
       if (I != Config.SymbolsToRename.end())
         Sym.Name = I->getValue();
     });
+
+    Obj.SymbolTable->removeSymbols([&](const Symbol &Sym) {
+      if (Config.DiscardAll && Sym.Binding == STB_LOCAL &&
+          Sym.getShndx() != SHN_UNDEF && Sym.Type != STT_FILE &&
+          Sym.Type != STT_SECTION)
+        return true;
+      return false;
+    });
   }
 }
 
 std::unique_ptr<Reader> CreateReader(StringRef InputFilename,
                                      ElfType &OutputElfType) {
   // Right now we can only read ELF files so there's only one reader;
   auto Out = llvm::make_unique<ELFReader>(InputFilename);
   // We need to set the default ElfType for output.
CopyConfig ParseObjcopyOptions(ArrayRef<const char *> ArgsArr) {
   Config.StripAll = InputArgs.hasArg(OBJCOPY_strip_all);
   Config.StripAllGNU = InputArgs.hasArg(OBJCOPY_strip_all_gnu);
   Config.StripDebug = InputArgs.hasArg(OBJCOPY_strip_debug);
   Config.StripDWO = InputArgs.hasArg(OBJCOPY_strip_dwo);
   Config.StripSections = InputArgs.hasArg(OBJCOPY_strip_sections);
   Config.StripNonAlloc = InputArgs.hasArg(OBJCOPY_strip_non_alloc);
   Config.ExtractDWO = InputArgs.hasArg(OBJCOPY_extract_dwo);
   Config.LocalizeHidden = InputArgs.hasArg(OBJCOPY_localize_hidden);
+  Config.DiscardAll = InputArgs.hasArg(OBJCOPY_discard_all);
   for (auto Arg : InputArgs.filtered(OBJCOPY_localize_symbol))
     Config.SymbolsToLocalize.push_back(Arg->getValue());
   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;