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

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

Thanks for the patch. I'm in favour of making the change as this does avoid an awkward corner case in the specification with no obvious drawbacks. A couple of suggestions for extra tests. I think there could be an alternative implementation that doesn't need an extra parameter although if it is possible it does have its own drawbacks.

I'll be out of office till next Tuesday, happy for others to approve in my absence.



================
Comment at: llvm/lib/Target/AArch64/MCTargetDesc/AArch64ELFObjectWriter.cpp:463
+                                                     unsigned) const {
+  return (Val.getRefKind() & AArch64MCExpr::VK_GOT) == AArch64MCExpr::VK_GOT;
+}
----------------
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.


================
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:
----------------
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.



================
Comment at: llvm/test/MC/AArch64/arm64-elf-relocs.s:319
 // CHECK-OBJ-LP64: R_AARCH64_GOT_LD_PREL19 sym
+
+// GOT relocations referencing local symbols are not converted to reference
----------------
Can we add a test case for :gotpage_lo15: as well.
```
        .text
        ldr x24, [x23, #:gotpage_lo15:sym]

        .bss
        .local sym
sym:
        .space 8
```
Will (without this change) use the section symbol + offset too:
```
Relocation section '.rela.text' at offset 0xc0 contains 1 entries:
    Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
0000000000000000  0000000300000139 R_AARCH64_LD64_GOTPAGE_LO15 0000000000000000 .bss + 0
```

There are a handful more got generating operators but these are not currently supported by LLVM only GCC
https://github.com/ARM-software/abi-aa/blob/main/sysvabi64/sysvabi64.rst#64assembler-language-addressing-mode-conventions (GOT operators)
 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158577