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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked 2 inline comments as done.
MaskRay added inline comments.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73228