[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

29–36

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

38–49

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

29–36

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

38–49

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

38–49

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

38–49

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 :)

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

+# RUN: yaml2obj %s > %t
+# RUN: llvm-objcopy --strip-symbol foo --keep-file-symbols %t %t2
+# RUN: llvm-readobj -symbols %t2 | FileCheck %s
+
+!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
+
+#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: foo
+#CHECK-NEXT:    Value: 0x0
+#CHECK-NEXT:    Size: 0
+#CHECK-NEXT:    Binding: Local
+#CHECK-NEXT:    Type: File
+#CHECK-NEXT:    Other: 0
+#CHECK-NEXT:    Section: .text
+#CHECK-NEXT:  }
+#CHECK-NEXT:]

tools/llvm-objcopy/ObjcopyOpts.td

defm strip_symbol : Eq<"strip-symbol">,
                        HelpText<"Remove symbol <symbol>">;
 def N : JoinedOrSeparate<["-"], "N">,
         Alias<strip_symbol>;
 defm keep_symbol : Eq<"keep-symbol">,
                        MetaVarName<"symbol">,
                        HelpText<"Do not remove symbol <symbol>">;
 def K : JoinedOrSeparate<["-"], "K">,
         Alias<keep_symbol>;
+def keep_file_symbols : Flag<["-", "--"], "keep-file-symbols">,
+                      HelpText<"Do not remove file symbols">;

tools/llvm-objcopy/llvm-objcopy.cpp

struct CopyConfig {
   bool StripDebug = false;
   bool StripSections = false;
   bool StripNonAlloc = false;
   bool StripDWO = false;
   bool ExtractDWO = false;
   bool LocalizeHidden = false;
   bool Weaken = false;
   bool DiscardAll = 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.
Obj.SymbolTable->updateSymbols([&](Symbol &Sym) {
         Sym.Binding = STB_WEAK;
 
       const auto I = Config.SymbolsToRename.find(Sym.Name);
       if (I != Config.SymbolsToRename.end())
         Sym.Name = I->getValue();
     });
 
     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.SymbolsToRemove.empty() &&
CopyConfig ParseObjcopyOptions(ArrayRef<const char *> ArgsArr) {
   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.Weaken = InputArgs.hasArg(OBJCOPY_weaken);
   Config.DiscardAll = InputArgs.hasArg(OBJCOPY_discard_all);
+  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());