[PATCH] D73228: [AsmPrinter][ELF] Define local aliases (.Lfoo$local) for GlobalObjects

[Sean Fertile via Phabricator via llvm-commits]

sfertile accepted this revision.
sfertile added a comment.
This revision is now accepted and ready to land.

LGTM.



================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:456
+  if (TM.getTargetTriple().isOSBinFormatELF() &&
+      GlobalObject::isExternalLinkage(GV.getLinkage()) && GV.isDSOLocal() &&
+      !GV.isDeclaration() && isa<GlobalObject>(GV))
----------------
MaskRay wrote:
> sfertile wrote:
> > Ideally `GV.isDSOLocal() ` should fully determine if a symbol is replaceable at runtime or not. Not sure if its safer to use `TargetMachine::shouldAssumeDSOLocal` instead. I believe the only difference right now for ELF would be the copy relocation case.
> > 
> > That said, this conditional is only true if the symbol is not preemptable. IIUC the compiler shouldn't be accessing data GOT-indirect, and the linker won't be emitting PLTs for the functions.  Which means:
> > 
> > ```
> > costs brought by potential symbol interpositionloc
> > (relocations in object files, PLT/GOT in executables/shared objects).
> > ```
> > 
> > There shouldn't be any saving related to PLT/GOT. I'm not very familiar with linking on archs other then Power though so I could definitely be wrong here.
> It is not convenient to access `Module` in this function. We probably should improve `GlobalVariable::maybeSetDsoLocal` to take account of `TargetMachine::shouldAssumeDSOLocal`, so that isDSOLocal() and `TargetMachine::shouldAssumeDSOLocal` will return the same value.
> 
> We require a definition (`!GV.isDeclaration()`). So for copy relocation, e.g.
> 
> ```
> // x86 x86-64
> // or aarch64 -mpie-copy-relocations
> extern int var;
> int foo() { return var; }
> ```
> 
> We don't use a local alias (no issue here).
Good point, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73228