[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:

> Thanks for the update. Where I'm up to so far. Will need to get back to this over the weekend as I'm on holiday next week and I don't want to block this review if other's are happy.
> 
> I reproduce the R_ARM_REL32 relocation to `__libc_stack_end` in sanitizer_linux.cpp by compiling it for Arm on the commit that triggered the test failure.
> 
> `__libc_stack_end` was just an global default visibility weak reference.
> 
> I made a small patch that applies on top of #76b035c3b00e072334f6f516b0ccb31140f5975f which was the commit before the put all weak definitions through the GOT.
> 
> ```
> diff --git a/llvm/lib/Target/ARM/ARMAsmPrinter.cpp b/llvm/lib/Target/ARM/ARMAsmPrinter.cpp
> index 8b8d0bc9ada1..f888c4822ca5 100644
> --- a/llvm/lib/Target/ARM/ARMAsmPrinter.cpp
> +++ b/llvm/lib/Target/ARM/ARMAsmPrinter.cpp
> @@ -1006,7 +1006,7 @@ void ARMAsmPrinter::emitMachineConstantPoolValue(
>      MCSym = getMBBExceptionSym(MF->front());
>    } else if (ACPV->isBlockAddress()) {
>      const BlockAddress *BA =
> -      cast<ARMConstantPoolConstant>(ACPV)->getBlockAddress();
> +        cast<ARMConstantPoolConstant>(ACPV)->getBlockAddress();
>      MCSym = GetBlockAddressSymbol(BA);
>    } else if (ACPV->isGlobalValue()) {
>      const GlobalValue *GV = cast<ARMConstantPoolConstant>(ACPV)->getGV();
> @@ -1016,6 +1016,31 @@ void ARMAsmPrinter::emitMachineConstantPoolValue(
>      unsigned char TF =
>          TM.getTargetTriple().isOSBinFormatMachO() ? ARMII::MO_NONLAZY : 0;
>      MCSym = GetARMGVSymbol(GV, TF);
> +    if (GV->isWeakForLinker() &&
> +        (GV->isDSOLocal() || !GV->hasDefaultVisibility()) &&
> +        TM.getTargetTriple().isOSBinFormatELF() && TM.isPositionIndependent() &&
> +        ACPV->getPCAdjustment() != 0) {
> +      MCSymbol *CPILabel = OutContext.createTempSymbol();
> +      OutStreamer->emitLabel(CPILabel);
> +      // Emit local-only expression: CPILabel - (LPC+PCAdj)
> +      const MCExpr *LocalExpr = MCSymbolRefExpr::create(CPILabel, OutContext);
> +      MCSymbol *PCLabel =
> +          getPICLabel(DL.getInternalSymbolPrefix(), getFunctionNumber(),
> +                      ACPV->getLabelId(), OutContext);
> +      const MCExpr *PCRelExpr = MCSymbolRefExpr::create(PCLabel, OutContext);
> +      PCRelExpr = MCBinaryExpr::createAdd(
> +          PCRelExpr,
> +          MCConstantExpr::create(ACPV->getPCAdjustment(), OutContext),
> +          OutContext);
> +      LocalExpr = MCBinaryExpr::createSub(LocalExpr, PCRelExpr, OutContext);
> +      OutStreamer->emitValue(LocalExpr, Size);
> +      // Emit .reloc to force linker resolution of the weak symbol.
> +      const MCExpr *CPIExpr = MCSymbolRefExpr::create(CPILabel, OutContext);
> +      const MCExpr *SymExpr = MCSymbolRefExpr::create(MCSym, OutContext);
> +      OutStreamer->emitRelocDirective(*CPIExpr, "R_ARM_REL32", SymExpr,
> +                                      SMLoc());
> +      return;
> +    }
>    }
> ```
> 
> That is the patch from here, but instead of isGVinGOT it does `(GV->isDSOLocal() || !GV->hasDefaultVisibility())`
> 
> I haven't tested it beyond checking that it makes R_ARM_GOTPREL references to the symbols that had failed the test before.
> 
> I still need to test on your original use case, to make sure we don't get a R_ARM_GOTPREL or a resolved pc-relative expression for a hidden weak definition in the same section.
> 
> I also need to understand your comments about "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"

Thanks again for the reproduction and the patch - that was the nudge I needed to re-examine this.

On the constant-pool point you wanted to understand: the address of a weak symbol gets materialized through a PC-relative constant pool entry that lives in `.text`, and whether the assembler emits an `R_ARM_REL32` depends on which section the symbol itself is in. A weak global variable (in .data):
`  .long   w-(.LPC0_0+8)      @ pool in .text, w in .data -> cross-section -> R_ARM_REL32`
That comes out of the normal fixup path (the `else` in `emitMachineConstantPoolValue`), not the `.reloc` branch - so tightening the `.reloc` condition doesn't get rid of it for the variable case. A weak function (in `.text`) is the same expression but same-section, so in the original code it's resolved eagerly at assembly time and produces no relocation (the #183916 override problem).

On your other open item, the hidden weak definition in the same section: with the .reloc patch it gets `R_ARM_REL32` via the .reloc directive - not GOT_PREL, not eagerly resolved. I checked define weak dso_local hidden void @f with its address taken from a .text pool: it emits .reloc .Ltmp0, R_ARM_REL32, f and links cleanly on both lld and GNU ld. That's the #183916 linker-override behavior preserved.

That cross-section `R_ARM_REL32` for weak variables is what originally led me to add the lowering change routing default-visibility weak through the GOT - to keep `R_ARM_REL32` off them regardless of how the symbol became `dso_local`. But having dug into it more, the reported failure is `__libc_stack_end`, an extern_weak reference (undefined), and the `.reloc` `isDSOLocal()` gate already sends that through the GOT. The cross-section case only bites a defined default-visibility dso_local weak symbol, which Clang doesn't produce for shared libraries (it only marks weak definitions dso_local for PIE, where the link succeeds anyway) - and which the original code already handled the same way. So the lowering change isn't needed for the reported bug; the `.reloc` gate is enough. I've reverted the lowering change in a follow-up commit.

On the condition itself: mine is `isDSOLocal()`, yours is `isDSOLocal() || !hasDefaultVisibility()`. They're equivalent on every case reachable from C/C++ - extern_weak, defined hidden weak, defined default-vis weak all behave identically. The only place they differ is extern_weak hidden (an undefined hidden weak, a somewhat contradictory construct C/C++ can't produce): mine goes through the GOT, yours emits `R_ARM_REL32`. Both link; I left it at `isDSOLocal()` since the GOT is cleaner there, but it doesn't really matter.

One last thing, since I'd only tested with lld earlier: for a defined default-visibility weak symbol, lld rejects `R_ARM_REL32` but GNU ld accepts it (as a dynamic relocation in .text, i.e. a `TEXTREL`). Both reject it for an undefined one (the `__libc_stack_end`  case), and both accept it for hidden weak (resolved statically at link time, no `TEXTREL`). So under GNU ld there are no .so failures either way; the only lld rejection is the defined default-vis weak case, which isn't reachable from C/C++ in a shared library  anyway.

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