[llvm-objcopy] Add --localize-symbol option

[paulsemel]

Summary

This option permit to localize a symbol by given its name.

[jakehehrlich]

Looks mostly good to me, just need to merge the passes over the symbols.

tools/llvm-objcopy/llvm-objcopy.cpp

197–214

Accumulate the functions so that localize can happen in a single pass. See what I did above with sections. Also in https://reviews.llvm.org/D46029 I requested that localize be replaced with a slightly more generalized 'map over symbols'. You can go ahead and write this in terms of localize but depending on which patch goes though first one or the other of you will need to change some code. Just a heads up.

[jhenderson]

test/tools/llvm-objcopy/localize.test

2

Why do we need all the extra symbols in this test?

Could we also get tests for localizing a symbol that is already a local, and also localizing a weak symbol, please, and for localizing multiple symbols at the same time (maybe these could be all one test).

I think we've also tended to test both short and long options.

tools/llvm-objcopy/Opts.td

61

Let's use a slightly more meaningful help message, that doesn't just repeat the switch name, e.g. "Mark <symbol> as local" (see also --localize-hidden).

[paulsemel]

tools/llvm-objcopy/llvm-objcopy.cpp

197–214

Alright, I'll keep this in mind, thanks ! :)

[paulsemel]

Added Jake and James suggestions :

• improved help message
• removed multiple calls to localize function
• improved tests

[jhenderson]

Aside from the conflict with D46029, mentioned already, and a couple of minor test-related points, this looks fine from my point of view.

test/tools/llvm-objcopy/localize.test

31

Since the visibility isn't relevant to the test, you can get rid of this. I'd also rename all the symbols to be simpler named (i.e. something like "Local", "Weak" and "Global"). The default/hidden part was to do with the symbol visibility from the localize-hidden test (I assume?), to reflect which was which.

[paulsemel]

Added James' suggestions :

• renamed symbol names in tests
• removed unnecessary visibility field in tests

[jakehehrlich]

LGTM

[paulsemel]

In D46064#1079741, @jakehehrlich wrote:

LGTM

Should I wait for https://reviews.llvm.org/D46029 ? Or do you think I can commit ?

[jakehehrlich]

You can land it whenever suits you. Whoever lands it first makes the other person refactor their code a touch but it isn't a big deal. I'll alert the other person of the conflict when it happens.

[paulsemel]

In D46064#1079767, @jakehehrlich wrote:

You can land it whenever suits you. Whoever lands it first makes the other person refactor their code a touch but it isn't a big deal. I'll alert the other person of the conflict when it happens.

Ok, I am going to land it, and if Alexander needs help to rebase, I will help him (it might be really easy actually)

[paulsemel]

Commited on r330963

test/tools/llvm-objcopy/localize.test

+# RUN: yaml2obj %s > %t
+# RUN: llvm-objcopy --localize-symbol defaultGlobal -L hiddenLocal -L defaultWeak %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
+  - Name:            .data
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC ]
+    Address:         0x2000
+    AddressAlign:    0x0000000000000010
+    Content:         "0000000000000000"
+Symbols:
+  Local:
+    - Name:     hiddenLocal
+      Type:     STT_FUNC
+      Section:  .text
+      Value:    0x1000
+      Size:     8
+      Visibility: STV_HIDDEN
+  Weak:
+    - Name:     defaultWeak
+      Type:     STT_FUNC
+      Size:     8
+      Section:  .text
+      Value:    0x1008
+  Global:
+    - Name:     defaultGlobal
+      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: hiddenLocal
+#CHECK-NEXT:    Value: 0x1000
+#CHECK-NEXT:    Size: 8
+#CHECK-NEXT:    Binding: Local
+#CHECK-NEXT:    Type: Function
+#CHECK-NEXT:    Other [
+#CHECK-NEXT:      STV_HIDDEN
+#CHECK-NEXT:    ]
+#CHECK-NEXT:    Section: .text
+#CHECK-NEXT:  }
+#CHECK-NEXT:  Symbol {
+#CHECK-NEXT:    Name: defaultGlobal
+#CHECK-NEXT:    Value: 0x1010
+#CHECK-NEXT:    Size: 8
+#CHECK-NEXT:    Binding: Local
+#CHECK-NEXT:    Type: Function
+#CHECK-NEXT:    Other: 0
+#CHECK-NEXT:    Section: .text
+#CHECK-NEXT:  }
+#CHECK-NEXT:  Symbol {
+#CHECK-NEXT:    Name: defaultWeak
+#CHECK-NEXT:    Value: 0x1008
+#CHECK-NEXT:    Size: 8
+#CHECK-NEXT:    Binding: Local
+#CHECK-NEXT:    Type: Function
+#CHECK-NEXT:    Other: 0
+#CHECK-NEXT:    Section: .text
+#CHECK-NEXT:  }
+#CHECK-NEXT:]

tools/llvm-objcopy/Opts.td

 def strip_sections : Flag<["-", "--"], "strip-sections">,
                      HelpText<"Remove all section headers">;
 def strip_non_alloc : Flag<["-", "--"], "strip-non-alloc">,
                       HelpText<"Remove all non-allocated sections">;
 def extract_dwo : Flag<["-", "--"], "extract-dwo">,
                   HelpText<"Remove all sections that are not DWARF .dwo sections from file">;
 def localize_hidden : Flag<["-", "--"], "localize-hidden">,
                       HelpText<"Mark all symbols that have hidden or internal visibility as local">;
+defm localize_symbol : Eq<"localize-symbol">,
+                 MetaVarName<"symbol">,
+                 HelpText<"Mark <symbol> as local">;
+def L : JoinedOrSeparate<["-"], "L">,
+        Alias<localize_symbol>;

tools/llvm-objcopy/llvm-objcopy.cpp

struct CopyConfig {
   StringRef BinaryArch;
 
   StringRef SplitDWO;
   StringRef AddGnuDebugLink;
   std::vector<StringRef> ToRemove;
   std::vector<StringRef> Keep;
   std::vector<StringRef> OnlyKeep;
   std::vector<StringRef> AddSection;
+  std::vector<StringRef> LocalizeSymbol;
   bool StripAll;
   bool StripAllGNU;
   bool StripDebug;
   bool StripSections;
   bool StripNonAlloc;
   bool StripDWO;
   bool ExtractDWO;
   bool LocalizeHidden;
 };
 
 using SectionPred = std::function<bool(const SectionBase &Sec)>;
+using SymbolPred = std::function<bool(const Symbol &Sym)>;
 
 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 (&Sec == Obj.SectionNames)
     return false;
   // Short of keeping the string table we want to keep everything that is a DWO

 // system. The only priority is that keeps/copies overrule removes.
 void HandleArgs(const CopyConfig &Config, Object &Obj, const Reader &Reader,
                 ElfType OutputElfType) {
 
   if (!Config.SplitDWO.empty()) {
     SplitDWOToFile(Config, Reader, Config.SplitDWO, OutputElfType);
   }
 
+  SymbolPred LocalizePred = [](const Symbol &) { return false; };
+
   // Localize:
 
   if (Config.LocalizeHidden) {
-    Obj.SymbolTable->localize([](const Symbol &Sym) {
+    LocalizePred = [](const Symbol &Sym) {
       return Sym.Visibility == STV_HIDDEN || Sym.Visibility == STV_INTERNAL;
-    });
+    };
+  }
+
+  if (!Config.LocalizeSymbol.empty()) {
+    LocalizePred = [LocalizePred, &Config](const Symbol &Sym) {
+      return LocalizePred(Sym) ||
+             std::find(std::begin(Config.LocalizeSymbol),
+                       std::end(Config.LocalizeSymbol),
+                       Sym.Name) != std::end(Config.LocalizeSymbol);
+    };
   }
 
+  Obj.SymbolTable->localize(LocalizePred);
+
   SectionPred RemovePred = [](const SectionBase &) { return false; };
 
   // Removes:
 
   if (!Config.ToRemove.empty()) {
     RemovePred = [&Config](const SectionBase &Sec) {
       return std::find(std::begin(Config.ToRemove), std::end(Config.ToRemove),
                        Sec.Name) != std::end(Config.ToRemove);
     };
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);
+  for (auto Arg : InputArgs.filtered(OBJCOPY_localize_symbol))
+    Config.LocalizeSymbol.push_back(Arg->getValue());
 
   return Config;
 }
 
 int main(int argc, char **argv) {
   InitLLVM X(argc, argv);
   ToolName = argv[0];
 
   CopyConfig Config = ParseObjcopyOptions(makeArrayRef(argv + 1, argc));
   ExecuteElfObjcopy(Config);
 }