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

[paulsemel]

Summary

This option globalize the given symbol

[jhenderson]

test/tools/llvm-objcopy/globalize.test

19–24

You might as well get rid of this section, since you have no symbols in it.

tools/llvm-objcopy/llvm-objcopy.cpp

327–330

Two points regarding this and the SymbolsToLocalize bit above, which I only just noticed:

1. The .empty() query is unnecessary, since std::begin(<empty vector>) == std::end(<empty vector>), so find in this situation will always return as not found.
2. Could we factor out the std::find != std::end common code between this and the localize variation into something like a lambda or helper called contains/inVector or similar? (It might be worth checking that something like that doesn't already exists).

[paulsemel]

Applied James' suggestions :

• Refactor the std::find code into a contains function
• Removed useless section in test
• Removed useless check if (!Config.SymbolsToXX.empty())

[alexander-shaposhnikov]

tools/llvm-objcopy/llvm-objcopy.cpp

324

empty() was not completely useless here imo - this flag is probably not the most frequently used flag, thus when it's not specified the "early check" would save us at least 3 function calls per symbol (at least in the debug builds) (std::begin/end/find)

tools/llvm-objcopy/llvm-objcopy.h

38 ↗

(On Diff #144301)

if it's only used in llvm-objcopy.cpp - maybe move it there ? (and make it static to avoid putting it into the global namespace)

[alexander-shaposhnikov]

tools/llvm-objcopy/llvm-objcopy.h

39 ↗

(On Diff #144301)

it seems to me, that the name Vector is kind of misleading here - this function (as is) is more generic - maybe replace Vector with Sequence or Container ? + change Elt to smth like X (but that's just an opinion, not super important)

[paulsemel]

Added Alexander suggestions

tools/llvm-objcopy/llvm-objcopy.cpp

324

Yes, tbh I think you're right on this one, it might somehow save us time !

tools/llvm-objcopy/llvm-objcopy.h

39 ↗

(On Diff #144301)

Agree with you, I misnamed because I was probably thinking to the use I will make of this function !

[alexander-shaposhnikov]

added one more inline comment, otherwise, to me this looks good, but i would wait for Jake and James

tools/llvm-objcopy/llvm-objcopy.cpp

138

maybe i'm missing smth - but why not simply return bool ?

[paulsemel]

Sure, I will wait for them to review too ! Thanks for reviewing btw :)

tools/llvm-objcopy/llvm-objcopy.cpp

138

This line is here to ensure we are passing the function a container. Otherwise, it throws an error at CT.
This is a trick I've seen once, but maybe it's not a good practice ?

[alexander-shaposhnikov]

tools/llvm-objcopy/llvm-objcopy.cpp

139

well, actually, even better - let's delete this and make use of
./STLExtras.h:967 bool is_contained(R &&Range, const E &Element)

[paulsemel]

tools/llvm-objcopy/llvm-objcopy.cpp

139

Oups.. I'm sorry, I completely missed this function..
Changing to this one ! Thanks !

[paulsemel]

Removed contains implementation and used the is_contained.

[jakehehrlich]

LGTM

[paulsemel]

Committed on r331068

[jhenderson]

tools/llvm-objcopy/llvm-objcopy.cpp

324

I think this is a case of premature optimization. A standard library implementation of .empty() might easily just use the distance between the begin and end iterator, so might end up with just as many (potentially more) calls as saved. Similarly, an optimizer might be able to optimize the calls.

In other words: unless performance measurements show differently, I'd prefer to keep the code simpler.

test/tools/llvm-objcopy/globalize.test

+# RUN: yaml2obj %s > %t
+# RUN: llvm-objcopy --globalize-symbol Global --globalize-symbol Local --globalize-symbol Weak %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:     Local
+      Type:     STT_FUNC
+      Section:  .text
+      Value:    0x1000
+      Size:     8
+  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: Local
+#CHECK-NEXT:    Value: 0x1000
+#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: 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: Global
+#CHECK-NEXT:    Type: Function
+#CHECK-NEXT:    Other: 0
+#CHECK-NEXT:    Section: .text
+#CHECK-NEXT:  }
+#CHECK-NEXT:]

tools/llvm-objcopy/Opts.td

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>;
+defm globalize_symbol : Eq<"globalize-symbol">,
+                       MetaVarName<"symbol">,
+                       HelpText<"Mark <symbol> as global">;

tools/llvm-objcopy/llvm-objcopy.cpp

struct CopyConfig {
 
   StringRef SplitDWO;
   StringRef AddGnuDebugLink;
   std::vector<StringRef> ToRemove;
   std::vector<StringRef> Keep;
   std::vector<StringRef> OnlyKeep;
   std::vector<StringRef> AddSection;
   std::vector<StringRef> SymbolsToLocalize;
+  std::vector<StringRef> SymbolsToGlobalize;
   StringMap<StringRef> SymbolsToRename;
   bool StripAll;
   bool StripAllGNU;
   bool StripDebug;
   bool StripSections;
   bool StripNonAlloc;
   bool StripDWO;
   bool ExtractDWO;
   bool LocalizeHidden;
 };
 
 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 (&Sec == Obj.SectionNames)
     return false;
   // Short of keeping the string table we want to keep everything that is a DWO
   // section and remove everything else.
   return !IsDWOSection(Sec);
 }
 
 std::unique_ptr<Writer> CreateWriter(const CopyConfig &Config, Object &Obj,
void HandleArgs(const CopyConfig &Config, Object &Obj, const Reader &Reader,
   if (!Config.AddGnuDebugLink.empty())
     Obj.addSection<GnuDebugLinkSection>(Config.AddGnuDebugLink);
 
   if (Obj.SymbolTable) {
     Obj.SymbolTable->updateSymbols([&](Symbol &Sym) {
       if ((Config.LocalizeHidden &&
            (Sym.Visibility == STV_HIDDEN || Sym.Visibility == STV_INTERNAL)) ||
           (!Config.SymbolsToLocalize.empty() &&
-           std::find(std::begin(Config.SymbolsToLocalize),
-                     std::end(Config.SymbolsToLocalize),
-                     Sym.Name) != std::end(Config.SymbolsToLocalize)))
+           is_contained(Config.SymbolsToLocalize, Sym.Name)))
         Sym.Binding = STB_LOCAL;
 
+      if (!Config.SymbolsToGlobalize.empty() &&
+          is_contained(Config.SymbolsToGlobalize, Sym.Name))
+        Sym.Binding = STB_GLOBAL;
+
       const auto I = Config.SymbolsToRename.find(Sym.Name);
       if (I != Config.SymbolsToRename.end())
         Sym.Name = I->getValue();
     });
   }
 }
 
 std::unique_ptr<Reader> CreateReader(StringRef InputFilename,
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);
   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());
 
   return Config;
 }
 
 int main(int argc, char **argv) {
   InitLLVM X(argc, argv);
   ToolName = argv[0];
 
   CopyConfig Config = ParseObjcopyOptions(makeArrayRef(argv + 1, argc));
   ExecuteElfObjcopy(Config);
 }