[PATCH] D158577: [MC,AArch64] Suppress local symbol to STT_SECTION conversion for GOT relocations

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked an inline comment as done.
MaskRay added inline comments.


================
Comment at: llvm/lib/Target/AArch64/MCTargetDesc/AArch64ELFObjectWriter.cpp:463
+                                                     unsigned) const {
+  return (Val.getRefKind() & AArch64MCExpr::VK_GOT) == AArch64MCExpr::VK_GOT;
+}
----------------
peter.smith wrote:
> In theory this won't catch VK_GOTTPREL which would get used by something like:
> ```
>         adrp    x8, :gottprel:foo
>         ldr     x8, [x8, :gottprel_lo12:foo]
> ```
> Empirically it looks like TLS relocations to local symbols via GOT expressions does not use a section symbol
> ```
>         .text
>         .globl  f  
>         .p2align        2
>         .type   f, at function
> f:
>         adrp    x8, :gottprel:foo
>         ldr     x8, [x8, :gottprel_lo12:foo]
>         mrs     x9, TPIDR_EL0
>         ldr     w0, [x9, x8]
>         ret
> 
>         .local foo
>         .type   foo, at object
>         .section        .tbss,"awT", at nobits
>         .p2align        2, 0x0
> foo:
>         .word   0
>         .size   foo, 4
> ```
> generates:
> ```
> Relocation section '.rela.text' at offset 0x1c0 contains 2 entries:
>     Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
> 0000000000000000  000000040000021d R_AARCH64_TLSIE_ADR_GOTTPREL_PAGE21 0000000000000000 foo + 0
> 0000000000000004  000000040000021e R_AARCH64_TLSIE_LD64_GOTTPREL_LO12_NC 0000000000000000 foo + 0
> ```
> 
> Could be worth a test case to make sure it doesn't get broken in the future.
TLS symbols are handled by `if (Flags & ELF::SHF_TLS)` and this patch is unrelated to it.

I've tried implementing lld in the past for STT_SECTION but later decided that it's not a good idea. I'll need to study GNU behavior more and rewrite that gold centric comment.


================
Comment at: llvm/lib/Target/VE/MCTargetDesc/VEELFObjectWriter.cpp:148
   // as they're all marked as requiring a symbol anyways.
   case ELF::R_VE_GOT_HI32:
   case ELF::R_VE_GOT_LO32:
----------------
peter.smith wrote:
> Assuming AArch64 behaves the same way, an alternative implementation that wouldn't need the additional parameter could use the AArch64 equivalent relocation types.
> 
> For example 
> ```
> case ELF::R_AARCH64_LD64_GOT_LO12_NC:
> ...
> return true;
> ```
> 
> This would be a bit tedious to construct (ILP32 as well) wouldn't be as future proof for further GOT expressions, and we couldn't easily test it in full due to not being able to generate the relocations.
> 
Yes. VK_GOT is associated with many relocation types in `AArch64ELFObjectWriter.cpp`. Those and ILP32 make me think that we do need an extra parameter `const MCValue &Val`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158577