[PATCH] D28571: RuntimeDyldELF/AArch64: Implement basic support for PIC relocations

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

Hello Evgeny,

Just looking at this from a AArch64 perspective. If my understanding is correct it looks like there is at least one case that this would incorrectly resolve the LO12_NC relocation. I've put an example in the comment. I'm not familiar with RuntimeDyld so my comments may not be applicable to the use cases it would be expected to face, I think you'll need to get some comments from the RuntimeDyld code owners for that. I notice the title says basic support, if it is intentionally incomplete it will be worth putting a comment on what hasn't been done yet. The remainder of the patch looks like it is just moving things around so that all looks fine to me.

Peter



================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp:1098
+      // We expect several GOT relocations to be created for a single GOT entry
+      // Still we need to create only one entry for all of them.
+      uint64_t GOTOffset = allocateGOTEntries(SectionID, 1);
----------------
If there is more than one R_AARCH64_ADR_GOT_PAGE relocation to the same symbol S in the object won't this create more than one GOT entry for S? 


================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp:1111
+    } else if (RelType == ELF::R_AARCH64_LD64_GOT_LO12_NC) {
+      // Use GOT entry already created for us after processing
+      // R_AARCH64_ADR_GOT_PAGE. So we get current GOT offset and
----------------
Am I correct in thinking that this is assuming that all R_AARCH64_LD64_GOT_LO12_NC to symbol S relocations immediately follow a R_AARCH64_ADR_GOT_PAGE to symbol S?

There isn't anything in the AArch64 ABI that guarantees this and compilers can generate code that doesn't. For example when compiled with the Linaro gcc 5.3.1 20160113 the source file

```
struct S {
    int a;
    int b;
};
int array[1024];
struct S s;

int func(void)
{
    return s.a + s.b + array[0] + array[1023];
}
```
produces with aarch64-linux-gnu-gcc -c t.c -fpie -O3
`
Disassembly of section .text:

0000000000000000 <func>:
   0:	90000002 	adrp	x2, 8 <func+0x8>
			0: R_AARCH64_ADR_GOT_PAGE	s
   4:	90000001 	adrp	x1, 1000 <func+0x1000>
			4: R_AARCH64_ADR_GOT_PAGE	array
   8:	f9400042 	ldr	x2, [x2]
			8: R_AARCH64_LD64_GOT_LO12_NC	s
   c:	f9400021 	ldr	x1, [x1]
			c: R_AARCH64_LD64_GOT_LO12_NC	array
  10:	29400043 	ldp	w3, w0, [x2]
  14:	b9400022 	ldr	w2, [x1]
  18:	b94ffc21 	ldr	w1, [x1,#4092]
  1c:	0b000060 	add	w0, w3, w0
  20:	0b020000 	add	w0, w0, w2
  24:	0b010000 	add	w0, w0, w1
  28:	d65f03c0 	ret
  2c:	d503201f 	nop
`
The LO12_NC relocations do not immediately follow GOT_PAGE
 
I don't know enough about the use cases of dlydelf to know if this is a problem, for example it may only need to link code that is guaranteed to have L012_NC following a  GOT_PAGE.


================
Comment at: test/ExecutionEngine/RuntimeDyld/AArch64/ELF_ARM64_PIC_relocations.s:2
+# RUN: llvm-mc -triple=arm64-none-linux-gnu -filetype=obj -o %T/pic-reloc.o %s
+# RUN: llvm-rtdyld -triple=arm64-none-linux-gnu -verify  -check=%s %T/pic-reloc.o -map-section pic-reloc.o,.got=0x20000
+
----------------
I think it will be worth adding a few more tests. 

For example the adrp will only produce the R_AARCH64_ADR_GOT_PAGE to _start, you'll need ldr	x8, [x8, #:got_lo12:_start] to get R_AARCH64_LD64_GOT_LO12NC.



Repository:
  rL LLVM

https://reviews.llvm.org/D28571