[PATCH] D158910: [BOLT] Local hidden should be global syms

Tue Aug 29 13:31:58 PDT 2023

maksfb added inline comments.

================
Comment at: bolt/lib/Rewrite/RewriteInstance.cpp:774
+    // They should still be unique names at DSO-level.
+    if (cantFail(Sym.getFlags()) & SymbolRef::SF_Hidden)
+      return true;
----------------
yota9 wrote:
> rafauler wrote:
> > yota9 wrote:
> > > 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?
> > That's fair. Another option is to hardcode and promote only _GLOBAL_OFFSET_TABLE_ if it is local hidden, and leave other symbols alone.
> 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 :)
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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158910/new/

https://reviews.llvm.org/D158910