Don't omit dynamic relocations for the GOT.

[ed]

Summary

Where Clang's AArch64 backend seems to differ from the X86 backend is
that it tends to use the GOT more aggressively.

After getting CloudABI PIEs working on x86-64, I noticed that accessing
global variables would still crash on aarch64. Tracing it down, it turns
out that the GOT was filled with entries assuming the base address was
zero.

It turns out that we skip generating relocations for GOT entries in case
the relocation pointing towards the GOT is relative. This is probably
wrong. Whether the thing pointing to the GOT is absolute or relative
shouldn't make any difference; the GOT entry itself should contain the
absolute address, thus needs a relocation regardless.

Before turning this into a full patch containing tests, I'm first
sending this out for review, because again I don't really know what I'm
doing. Is my interpretation on this matter correct?

[ruiu]

This doesn't seem correct, but Rafael is updating this part of code. Rafael, can you take a look?

[• rafael]

The code does look wrong. It happens to work given the current implementations of isRelRelative. You should completely delete Dynrel, if we need a relocation for the got entry itself should only depend on the symbol and whether the output is relocatable (Config->Pic).

Do you have a testcase for which it fails it currently fails? If so, please add a testcase. If not, this is still a nice cleanup

[• rafael]

I accepted it, but phab is not sending emails.

[ruiu]

Did you accept it although you are saying that this looks wrong?

[ed]

Simplify, as suggested by Rafael. Also add a test.

[ed]

In D18739#391880, @rafael wrote:

The code does look wrong. It happens to work given the current implementations of isRelRelative. You should completely delete Dynrel, if we need a relocation for the got entry itself should only depend on the symbol and whether the output is relocatable (Config->Pic).

Done! Does this look all right?

Do you have a testcase for which it fails it currently fails? If so, please add a testcase. If not, this is still a nice cleanup

Yes! I've just written a test.

[• rafael]

LGTM. Do you have commit access?

[ed]

In D18739#392698, @rafael wrote:

LGTM. Do you have commit access?

Yes! Committed as r265453.

ELF/Writer.cpp

if (Target->needsGot(Type, Body)) {
       if (Config->EMachine == EM_MIPS)
         // MIPS ABI has special rules to process GOT entries
         // and doesn't require relocation entries for them.
         // See "Global Offset Table" in Chapter 5 in the following document
         // for detailed description:
         // ftp://www.linux-mips.org/pub/linux/mips/doc/ABI/mipsabi.pdf
         continue;
 
-      bool Dynrel = Config->Pic && !Target->isRelRelative(Type) &&
-                    !Target->isSizeRel(Type);
-      if (Preemptible || Dynrel) {
+      if (Preemptible || Config->Pic) {
         uint32_t DynType;
         if (Body.isTls())
           DynType = Target->TlsGotRel;
         else if (Preemptible)
           DynType = Target->GotRel;
         else
           DynType = Target->RelativeRel;
         Out<ELFT>::RelaDyn->addReloc(

test/ELF/aarch64-got-relocations.s

+# REQUIRES: aarch64
+# RUN: llvm-mc -filetype=obj -triple=aarch64-unknown-cloudabi %s -o %t.o
+# RUN: ld.lld -pie %t.o -o %t
+# RUN: llvm-readobj -r %t | FileCheck %s
+
+# If we're addressing a global relatively through the GOT, we still need to
+# emit a relocation for the entry in the GOT itself.
+# CHECK: Relocations [
+# CHECK:   Section (4) .rela.dyn {
+# CHECK:     0x{{[0-9A-F]+}} R_AARCH64_RELATIVE - 0x{{[0-9A-F]+}}
+# CHECK:   }
+# CHECK: ]
+
+  .globl  _start
+  .type _start,@function
+_start:
+  adrp  x8, :got:i
+  ldr  x8, [x8, :got_lo12:i]
+
+  .type i,@object
+  .comm i,4,4