[BOLT] Local hidden should be global syms

[rafauler]

Summary

Register all hidden local symbols as globals, since they are
still unique in the context of the input binary BOLT is
processing. This is used to fetch a single definition of
_GLOBAL_OFFSET_TABLE_.

[yota9]

bolt/lib/Rewrite/RewriteInstance.cpp

769

Maybe something like isSymbolDSOUnique? The global is a bit frustrating for me, e.g. only means SymbolRef::SF_Global.

770

BTW the WEAK symbols are visible outside the component, shoudn't we add check here too?

774

As I know the global hidden symbols are usually transformed to local default. If they are not transformed we would just return from the if global condition above. If the symbols stays local hidden for some reason (I think it is totally possible and does not contradicts anything, but I'm unable to generate it right now with my compilers and linkers) - is there any guaranties that the symbol was not generated by compiler like local (e.g. static symbol) and hidden due to fvisibility=hidden flag for example? I mean if the symbol is marked local I don't think we have guarantees that it is unique in DSO, are we?

[rafauler]

Thanks for the review!

bolt/lib/Rewrite/RewriteInstance.cpp

774

That's fair. Another option is to hardcode and promote only _GLOBAL_OFFSET_TABLE_ if it is local hidden, and leave other symbols alone.

[yota9]

bolt/lib/Rewrite/RewriteInstance.cpp

774

IMO It is better solution :) Please also consider to stay with this lambda and checking for WEAK symbols as I mentioned before, I think their handling might be missed before :)

[maksfb]

bolt/lib/Rewrite/RewriteInstance.cpp

774

If the change is just for the lookup of _GLOBAL_OFFSET_TABLE_, then it's better to encapsulate the lookup and maybe check for the global version first and then the local, or fix the naming just for that symbol as yota9 suggests.

Changing the internal naming may have an unintended consequence of missing/mismatching hidden functions in existing profiles.

bolt/lib/Rewrite/RewriteInstance.cpp

void RewriteInstance::discoverFileObjects() {
   StringRef FileSymbolName;
   bool SeenFileName = false;
   struct SymbolRefHash {
     size_t operator()(SymbolRef const &S) const {
       return std::hash<decltype(DataRefImpl::p)>{}(S.getRawDataRefImpl().p);
     }
   };
   std::unordered_map<SymbolRef, StringRef, SymbolRefHash> SymbolToFileName;
+  auto isSymbolGlobal = [](const ELFSymbolRef &Sym) {
+    if (cantFail(Sym.getFlags()) & SymbolRef::SF_Global)
+      return true;
+    // Hidden symbols are globals that were demoted to local at link-time.
+    // They should still be unique names at DSO-level.
+    if (cantFail(Sym.getFlags()) & SymbolRef::SF_Hidden)
+      return true;
+    return false;
+  };
   for (const ELFSymbolRef &Symbol : InputFile->symbols()) {
     Expected<StringRef> NameOrError = Symbol.getName();
     if (NameOrError && NameOrError->startswith("__asan_init")) {
       errs() << "BOLT-ERROR: input file was compiled or linked with sanitizer "
                 "support. Cannot optimize.\n";
       exit(1);
     }
     if (NameOrError && NameOrError->startswith("__llvm_coverage_mapping")) {
if (cantFail(Symbol.getType()) == SymbolRef::ST_File) {
       // Ignore Clang LTO artificial FILE symbol as it is not always generated,
       // and this uncertainty is causing havoc in function name matching.
       if (Name == "ld-temp.o")
         continue;
       FileSymbolName = Name;
       SeenFileName = true;
       continue;
     }
-    if (!FileSymbolName.empty() &&
-        !(cantFail(Symbol.getFlags()) & SymbolRef::SF_Global))
+    if (!FileSymbolName.empty() && !(isSymbolGlobal(Symbol)))
       SymbolToFileName[Symbol] = FileSymbolName;
   }
 
   // Sort symbols in the file by value. Ignore symbols from non-allocatable
   // sections.
   auto isSymbolInMemory = [this](const SymbolRef &Sym) {
     if (cantFail(Sym.getType()) == SymbolRef::ST_File)
       return false;
for (auto ISym = SortedFileSymbols.begin(); ISym != SortedSymbolsEnd;
     // always modify the local name.
     //
     // NOTE: the naming convention for local symbols should match
     //       the one we use for profile data.
     std::string UniqueName;
     std::string AlternativeName;
     if (Name.empty()) {
       UniqueName = "ANONYMOUS." + std::to_string(AnonymousId++);
-    } else if (cantFail(Symbol.getFlags()) & SymbolRef::SF_Global) {
+    } else if (isSymbolGlobal(Symbol)) {
       if (const BinaryData *BD = BC->getBinaryDataByName(Name)) {
         if (BD->getSize() == ELFSymbolRef(Symbol).getSize() &&
             BD->getAddress() == Address) {
           if (opts::Verbosity > 1)
             errs() << "BOLT-WARNING: ignoring duplicate global symbol " << Name
                    << "\n";
           // Ignore duplicate entry - possibly a bug in the linker
           continue;

bolt/test/AArch64/array_end.c

 // Test checks that bolt properly finds end section label.
 // Linker script contains gap after destructor array, so
 // __init_array_end address would not be owned by any section.
 
 // REQUIRES: system-linux
 // RUN: %clang %cflags -no-pie %s -o %t.exe -Wl,-q \
 // RUN:   -Wl,-T %S/Inputs/array_end.lld_script
 // RUN: llvm-bolt %t.exe -o %t.bolt --print-disasm \
 // RUN:  --print-only="callFini" | FileCheck %s
 
-// CHECK: adr [[REG:x[0-28]+]], "__fini_array_end/1"
+// CHECK: adr [[REG:x[0-28]+]], __fini_array_end
 
 __attribute__((destructor)) void destr() {}
 
 __attribute__((noinline)) void callFini() {
   extern void (*__fini_array_start[])();
   extern void (*__fini_array_end[])();
   unsigned long Count = __fini_array_end - __fini_array_start;
   for (unsigned long I = 0; I < Count; ++I)
     (*__fini_array_start[I])();
 }
 
 void _start() { callFini(); }