[llvm] [ARM] Use .reloc for dso_local weak symbols in PIC mode instead of GOT indirection (PR #209660)

[dong jianqiang via llvm-commits]

dongjianqiang2 wrote:

> Apologies for taking so long to reply.
> 
> Where I'm up to at the moment.
> 
> The original code, prior to any of these changes [3cb23f5](https://github.com/llvm/llvm-project/commit/3cb23f5a41328703f0cb9180503d63b29325eb48) had a isGVinGOT of
> 
> ```
> bool ARMSubtarget::isGVInGOT(const GlobalValue *GV) const {
>   return isTargetELF() && TM.isPositionIndependent() && !GV->isDSOLocal();
> }
> ```
> 
> `ARMFastIsel::ARMLowerPICELF` just used `!GV->isDSOLocal()` `ARMTargetLowering::LowerGlobalAddressELF` just used `GV->isDSOLocal()` to decide whether to use the GOT or not.
> 
> With this original code there was no test failure in the sanitizer run-time when building a shared object. I think a dso_local weak global value would not have gone through the GOT, but instead it would have been resolved entirely at assembly time, so the linker would not have seen the R_ARM_REL32 relocation to the preemptable symbol.
> 
> What I want to work out is why `isGVinGOT` has to be changed, and why do `ARMFastIsel::ARMLowerPICELF` and `ARMTargetLowering::LowerGlobalAddressELF` have to use it. It looks like it should be possible in `ARMAsmPrinter::emitMachineConstantPoolValue` to only use the R_ARM_REL32 sequence on non-default visibility weak symbols, perhaps by adding `&& !GV->hasDefaultVisibility()` instead of `!isGVinGOT()`
> 
> I still don't know why it is legal for dso_local to be used on a default visibility weak symbol. I'd like to work out how that is possible from C/C++. Yes I know we can hand-write the IR, but I'm not sure if that would be legal for C/C++. That may be a separate issue though.

Thanks for digging into this. I did try the `&& !GV->hasDefaultVisibility()` approach on the .reloc branch first, but it didn't quite work out - the `.reloc` branch isn't actually the only place `R_ARM_REL32` comes from. The normal constant-pool path (the `else` in `emitMachineConstantPoolValue` that emits `sym-(LPC+8)`) still produces one when the symbol's in a different section from the pool entry, say a weak global in `.data` referenced from a `.text` pool entry. With just the `.reloc` branch guarded, `@w = weak dso_local global  i32 42` still comes out as `.long w-(.LPC0_0+8)` and the .so link still fails. And for a weak function it'd fall back to eager assembly-time resolution, which I think is the #183916 problem again.

That's what pushed the GOT decision into the lowering (`isGVInGOT`/`LowerGlobalAddressELF`/`ARMLowerPICELF`) rather than just the `.reloc` path - it seemed like the only way to actually keep `R_ARM_REL32` off these symbols. Happy to revisit if you can see a simpler way though.

On whether `dso_local` on a default-visibility weak symbol is reachable from C/C++ - honestly I'm not sure it is, at least for shared libraries. From what I found, Clang's `shouldAssumeDSOLocal` only sets `dso_local` for functions that can benefit from a local alias, and `canBenefitFromLocalAlias()` is false for weak symbols. It does get set under PIE, but the link goes through there (lld accepts `R_ARM_REL32` for PIE), and as @efriedma-quic said gcc documents that `-fno-semantic-interposition` doesn't apply to weak symbols anyway. So for shared libraries it seems to really just be explicit `dso_local` in IR, which is pretty narrow. Happy to be corrected if I've got any of that wrong.

https://github.com/llvm/llvm-project/pull/209660