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

[Sean Fertile via Phabricator via llvm-commits]

sfertile added a comment.

Patch makes sense to me.



================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:454
+MCSymbol *AsmPrinter::getSymbolPreferLocal(const GlobalValue &GV) const {
+  // On ELF, use .Lfoo$local if GV is a non-interposable GlobalObject definion.
+  if (TM.getTargetTriple().isOSBinFormatELF() &&
----------------
I think you explained why we only are doing this for `ExternalLinkage` well in the patch description, do you think the reasoning should be here as a comment? 



================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:456
+  if (TM.getTargetTriple().isOSBinFormatELF() &&
+      GlobalObject::isExternalLinkage(GV.getLinkage()) && GV.isDSOLocal() &&
+      !GV.isDeclaration() && isa<GlobalObject>(GV))
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73228