[llvm-objcopy] Add --keep-file-symbols option

[paulsemel]

Summary

This option prevent from removing file symbols while removing symbols.
This patch relies on this one (not merged yet) : https://reviews.llvm.org/D46819

[jhenderson]

Other than explicitly stripping the file symbol, is there any way of actually removing such symbols currently? It seems like adding a switch just to prevent you doing something you asked for is a bit superfluous.

Do you have a specific use-case at the moment?

test/tools/llvm-objcopy/keep-file-symbols-remove-section.test

1 ↗

(On Diff #146592)

I don't think you need this test, since the file symbol is by definition not in a section.

[paulsemel]

In D46830#1098085, @jhenderson wrote:

Other than explicitly stripping the file symbol, is there any way of actually removing such symbols currently? It seems like adding a switch just to prevent you doing something you asked for is a bit superfluous.

Do you have a specific use-case at the moment?

Well, --strip-unneeded can trigger the file symbols removal. Anyway, for the moment, I stopped implementing it because I have some problems with the error messages we are issuing for the symbol removal.

In all cases, I truly think that this way is the way to prevent from file symbol removal (particularly for the --strip-unneeded option). Maybe I can just remove the section removal test and land it like this, waiting for someone (maybe me) to implement --strip-unneeded and to add a better test case ?

What do you think about it ?

[paulsemel]

Applied James' suggestions

[jakehehrlich]

If it isn't too much trouble I'd like to wait until --strip-unneeded is implemented.

[paulsemel]

Thanks to @alexshap changes, I think this patch now makes sense !

[jhenderson]

Code change looks fine, but could you add a test to show that --keep-file-symbols trumps --strip-symbol, assuming that is the desired behaviour (double-check against GNU, I suggest here).

[paulsemel]

Added test for --strip-symbol and --keep-file-symbols.
This follows the same behavior as GNU objcopy

[jhenderson]

LGTM, with the suggested test change. The bug needs fixing separately.

test/tools/llvm-objcopy/keep-file-symbols.test

28–35 ↗

(On Diff #148598)

You only need two symbols - one file symbol and one non-file symbol.

37–48 ↗

(On Diff #148598)

Wait, shouldn't there be a null symbol here? Null symbols are required... That's probably a bug unrelated to your change, but should be fixed ASAP.

[paulsemel]

test/tools/llvm-objcopy/keep-file-symbols.test

28–35 ↗

(On Diff #148598)

You're totally right. I'll remove those :)

37–48 ↗

(On Diff #148598)

Well, yes, I added a TODO for this in my previous patch, about this dummy symbol. We shouldn't pass it to the removeSymbols function as @jakehehrlich stated :)
Do you want me to work on it ? :)

[jhenderson]

test/tools/llvm-objcopy/keep-file-symbols.test

37–48 ↗

(On Diff #148598)

If you don't mind. What we currently generate is actually illegal ELF. I was under the impression that the TODO was for coming up with a better way for handling the null symbol, and I assumed that it wasn't being stripped (unless the whole table was), hence why I didn't comment on it before.

[jhenderson]

Forgot to accept the revision.

[paulsemel]

Added James' suggestions.

test/tools/llvm-objcopy/keep-file-symbols.test

37–48 ↗

(On Diff #148598)

Well, you're actually right. The TODO was for this reason, definitely. But if we handle it the way we planned to handle it, this will also fix this issue :)

llvm/trunk/test/tools/llvm-objcopy/keep-file-symbols.test

+# RUN: yaml2obj %s > %t
+# RUN: llvm-objcopy --strip-all --keep-file-symbols %t %t2
+# RUN: llvm-readobj -symbols %t2 | FileCheck %s --check-prefix=STRIPALL
+# RUN: llvm-objcopy --keep-file-symbols --strip-symbol foo %t %t2
+# RUN: llvm-readobj -symbols %t2 | FileCheck %s --check-prefix=STRIP
+
+!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:     foo
+      Type:     STT_FILE
+      Section:  .text
+  Global:
+    - Name:     bar
+      Type:     STT_FUNC
+      Section:  .text
+
+#STRIPALL: Symbols [
+#STRIPALL-NEXT:  Symbol {
+#STRIPALL-NEXT:    Name: foo
+#STRIPALL-NEXT:    Value: 0x0
+#STRIPALL-NEXT:    Size: 0
+#STRIPALL-NEXT:    Binding: Local
+#STRIPALL-NEXT:    Type: File
+#STRIPALL-NEXT:    Other: 0
+#STRIPALL-NEXT:    Section: .text
+#STRIPALL-NEXT:  }
+#STRIPALL-NEXT:]
+
+#STRIP: Symbols [
+#STRIP-NEXT:  Symbol {
+#STRIP-NEXT:    Name:
+#STRIP-NEXT:    Value: 0x0
+#STRIP-NEXT:    Size: 0
+#STRIP-NEXT:    Binding: Local
+#STRIP-NEXT:    Type: None
+#STRIP-NEXT:    Other: 0
+#STRIP-NEXT:    Section: Undefined
+#STRIP-NEXT:  }
+#STRIP-NEXT:  Symbol {
+#STRIP-NEXT:    Name: foo
+#STRIP-NEXT:    Value: 0x0
+#STRIP-NEXT:    Size: 0
+#STRIP-NEXT:    Binding: Local
+#STRIP-NEXT:    Type: File
+#STRIP-NEXT:    Other: 0
+#STRIP-NEXT:    Section: .text
+#STRIP-NEXT:  }
+#STRIP-NEXT:  Symbol {
+#STRIP-NEXT:    Name: bar
+#STRIP-NEXT:    Value: 0x0
+#STRIP-NEXT:    Size: 0
+#STRIP-NEXT:    Binding: Global
+#STRIP-NEXT:    Type: Function
+#STRIP-NEXT:    Other: 0
+#STRIP-NEXT:    Section: .text
+#STRIP-NEXT:  }
+#STRIP-NEXT:]

llvm/trunk/tools/llvm-objcopy/ObjcopyOpts.td

defm keep_symbol : Eq<"keep-symbol">,
                        MetaVarName<"symbol">,
                        HelpText<"Do not remove symbol <symbol>">;
 def K : JoinedOrSeparate<["-"], "K">,
         Alias<keep_symbol>;
 def only_keep_debug : Flag<["-", "--"], "only-keep-debug">,
                           HelpText<"Currently ignored. Only for compaitability with GNU objcopy.">;
 def strip_unneeded : Flag<["-", "--"], "strip-unneeded">,
                       HelpText<"Remove all symbols not needed by relocations">;
+def keep_file_symbols : Flag<["-", "--"], "keep-file-symbols">,
+                      HelpText<"Do not remove file symbols">;

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

struct CopyConfig {
   bool StripNonAlloc = false;
   bool StripDWO = false;
   bool StripUnneeded = false;
   bool ExtractDWO = false;
   bool LocalizeHidden = false;
   bool Weaken = false;
   bool DiscardAll = false;
   bool OnlyKeepDebug = false;
+  bool KeepFileSymbols = 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.
if (Obj.SymbolTable) {
     // (like GroupSection or RelocationSection). This way, we know which
     // symbols are still 'needed' and wich are not.
     if (Config.StripUnneeded) {
       for (auto &Section : Obj.sections())
         Section.markSymbols();
     }
 
     Obj.removeSymbols([&](const Symbol &Sym) {
-      if (!Config.SymbolsToKeep.empty() &&
-          is_contained(Config.SymbolsToKeep, Sym.Name))
+      if ((!Config.SymbolsToKeep.empty() &&
+           is_contained(Config.SymbolsToKeep, Sym.Name)) ||
+          (Config.KeepFileSymbols && Sym.Type == STT_FILE))
         return false;
 
       if (Config.DiscardAll && Sym.Binding == STB_LOCAL &&
           Sym.getShndx() != SHN_UNDEF && Sym.Type != STT_FILE &&
           Sym.Type != STT_SECTION)
         return true;
 
       if (Config.StripAll || Config.StripAllGNU)
if (!Config.Keep.empty()) {
     };
   }
 
   // This has to be the last predicate assignment.
   // If the option --keep-symbol has been specified
   // and at least one of those symbols is present
   // (equivalently, the updated symbol table is not empty)
   // the symbol table and the string table should not be removed.
-  if (!Config.SymbolsToKeep.empty() && !Obj.SymbolTable->empty()) {
+  if ((!Config.SymbolsToKeep.empty() || Config.KeepFileSymbols) &&
+      !Obj.SymbolTable->empty()) {
     RemovePred = [&Obj, RemovePred](const SectionBase &Sec) {
       if (&Sec == Obj.SymbolTable || &Sec == Obj.SymbolTable->getStrTab())
         return false;
       return RemovePred(Sec);
     };
   }
 
   Obj.removeSections(RemovePred);
CopyConfig ParseObjcopyOptions(ArrayRef<const char *> ArgsArr) {
   Config.StripSections = InputArgs.hasArg(OBJCOPY_strip_sections);
   Config.StripNonAlloc = InputArgs.hasArg(OBJCOPY_strip_non_alloc);
   Config.StripUnneeded = InputArgs.hasArg(OBJCOPY_strip_unneeded);
   Config.ExtractDWO = InputArgs.hasArg(OBJCOPY_extract_dwo);
   Config.LocalizeHidden = InputArgs.hasArg(OBJCOPY_localize_hidden);
   Config.Weaken = InputArgs.hasArg(OBJCOPY_weaken);
   Config.DiscardAll = InputArgs.hasArg(OBJCOPY_discard_all);
   Config.OnlyKeepDebug = InputArgs.hasArg(OBJCOPY_only_keep_debug);
+  Config.KeepFileSymbols = InputArgs.hasArg(OBJCOPY_keep_file_symbols);
   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());
   for (auto Arg : InputArgs.filtered(OBJCOPY_strip_symbol))
     Config.SymbolsToRemove.push_back(Arg->getValue());