[PATCH] D20951: [RFC][ARM][LLD] Initial support for ARM in lld

[Peter Smith via llvm-commits]

peter.smith marked an inline comment as done.
peter.smith added a comment.

I've put some answers to the comments inline. I'm working on an updated patch that I'll have ready later today.


================
Comment at: ELF/Driver.cpp:72-75
@@ -71,4 +71,6 @@
     return {ELF64LEKind, EM_X86_64};
   if (S == "aarch64linux")
     return {ELF64LEKind, EM_AARCH64};
+  if (S == "armelf_linux_eabi")
+    return {ELF32LEKind, EM_ARM};
   if (S == "i386pe" || S == "i386pep" || S == "thumb2pe")
----------------
ruiu wrote:
> I want to sort these `S==` lines alphabetically, but it should be done in a separate patch.
Ok, I'll leave as is for this patch.

================
Comment at: ELF/Target.cpp:1407
@@ +1406,3 @@
+  case R_ARM_JUMP24:
+    return (S.isPreemptible()) ? R_PLT_PC : R_PC;
+  case R_ARM_GOTOFF32:
----------------
ruiu wrote:
> Remove the outermost redundant ().
Ok, will do.

================
Comment at: ELF/Target.cpp:1446
@@ +1445,3 @@
+      0x04, 0xe0, 0x9f, 0xe5, //     ldr lr, L2
+      0x0e, 0xe0, 0x8f, 0xe0, // L1: add lr, pc, lr
+      0x08, 0xf0, 0xbe, 0xe5, //     ldr pc, [lr, #8]
----------------
ruiu wrote:
> You are not using `L1` label in the comment. What is it?
Thinking again, I should have a done a much better job with the comment. What I need to express is that the .word contains the the offset from L1 to the start of the .got.plt section, taking into account the ARM PC bias of +8. In a follow up patch I've come up with a better way of expressing this that is closer to the example given in ELF for the ARM architecture and the code for writePLT:
      str lr, [sp,#-4]!
      ldr lr, L2
L1: add lr, pc, lr
      ldr pc, [lr, #8]
L2: .word   &(.got.plt) - L1 - 8

Where &(.got.plt) = L1 + 8 + *L2 hence *L2 = &(.got.plt) - L1 - 8, and L1 = .plt + 8

================
Comment at: ELF/Target.cpp:1452
@@ +1451,3 @@
+  uint64_t GotPlt = Out<ELF32LE>::GotPlt->getVA();
+  uint64_t Dot = Out<ELF32LE>::Plt->getVA() + 16;
+  write32le(Buf + 16, GotPlt - Dot);
----------------
ruiu wrote:
> So `Dot` always points to PLT+16. Is this correct?
The 16 comes from combining the offset of 8 of L1 from the start of the .plt section and the 8 pc bias. I've made this is a bit clearer in the revised version. 

================
Comment at: ELF/Target.cpp:1477
@@ +1476,3 @@
+  case R_ARM_NONE:
+    // R_ARM_NONE has no effect on the place.
+    break;
----------------
ruiu wrote:
> I think you can remove this comment -- it is obvious from the relocation name.
Ok, comment removed.

================
Comment at: ELF/Target.cpp:1496-1498
@@ +1495,5 @@
+    checkInt<26>(Val, Type);
+    write32le(Loc,
+              (read32le(Loc) & ~0x00ffffff) |
+              ((Val >> 2) & 0x00ffffff));
+    break;
----------------
ruiu wrote:
> Is this what clang-format would format?
I've run clang format and will update the changes so that they conform.

================
Comment at: ELF/Target.cpp:1508
@@ +1507,3 @@
+              (((Val >> 16) & 0xf000) << 4) |
+              ( (Val >> 16) & 0xfff));
+    break;
----------------
ruiu wrote:
> clang-format
I've run clang format and will update the changes so that they conform.

================
Comment at: ELF/Writer.cpp:673
@@ +672,3 @@
+  if (!Config->Relocatable) {
+    if(Config->EMachine == EM_ARM)
+      // Existing ARM code requires an R_ARM_BASE_PREL relocation to
----------------
ruiu wrote:
> Format.
Will remove, see later comment.

================
Comment at: ELF/Writer.cpp:674-679
@@ +673,8 @@
+    if(Config->EMachine == EM_ARM)
+      // Existing ARM code requires an R_ARM_BASE_PREL relocation to
+      // _GLOBAL_OFFSET_TABLE_ to evaluate at the base of the
+      // .got section. The cleanest way to accomplish this is to define
+      // _GLOBAL_OFFSET_TABLE_ to be the base of the .got section.
+      addOptionalSynthetic(Symtab, "_GLOBAL_OFFSET_TABLE_", Out<ELFT>::Got,
+                           0);
+    else
----------------
ruiu wrote:
> This seems a bit tricky since we are giving a different meaning to _GLOBAL_OFFSET_TABLE_ only if ARM. I think I understand the issue you are trying to solve. Does one of your tests cover this case (I mean, is there any test that will fail if you remove this code?)
This is indeed tricky. I've re-read the relevant sections of ELF for the ARM Architecture which give enough freedom to a linker to special case the R_ARM_BASE_PREL relocation (see later). I've rewrote the entry in getRelExpr to use R_GOTONLY_PC rather than R_PC. This effectively ignores the value of the _GLOBAL_OFFSET_TABLE_ symbol. This is not ideal but it is ok for the initial target of a linux/bsd like system.

By the principle of least surprise I would prefer to define _GLOBAL_OFFSET_TABLE_ to the base of the .got, as is the case in ld.bfd, but I think that is best handled in a separate patch. I will try to find a real world program that depends on _GLOBAL_OFFSET_TABLE_ being defined to be the base of the .got outside the context of R_ARM_BASE_PREL.

- ELF for the ARM Architecture defines R_ARM_BASE_PREL to be: B(S) + A – P with the special case:
R_ARM_BASE_PREL with the NULL symbol (symbol 0) will give the offset of the GOT origin from the address
of the place.
B(S) means B(S) is the addressing origin of the output segment defining the symbol S. The origin is not required to be the base address of the segment. This value must always be word-aligned.

If I choose B(S) to be the base of the .got section then the relocation works regardless of the value of _GLOBAL_OFFSET_TABLE_.


http://reviews.llvm.org/D20951