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

[Rui Ueyama via llvm-commits]

ruiu added a comment.

This seems pretty straightforward code to support ARM. I'm happy to hear that you can link a "hello world" for ARM with only this amount of code. You want to have Rafael take a look at it as well -- particularly for relocation handling.


================
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")
----------------
I want to sort these `S==` lines alphabetically, but it should be done in a separate 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:
----------------
Remove the outermost redundant ().

================
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]
----------------
You are not using `L1` label in the comment. What is it?

================
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);
----------------
So `Dot` always points to PLT+16. Is this correct?

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

================
Comment at: ELF/Target.cpp:1496-1498
@@ +1495,5 @@
+    checkInt<26>(Val, Type);
+    write32le(Loc,
+              (read32le(Loc) & ~0x00ffffff) |
+              ((Val >> 2) & 0x00ffffff));
+    break;
----------------
Is this what clang-format would format?

================
Comment at: ELF/Target.cpp:1508
@@ +1507,3 @@
+              (((Val >> 16) & 0xf000) << 4) |
+              ( (Val >> 16) & 0xfff));
+    break;
----------------
clang-format

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

================
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
----------------
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?)


http://reviews.llvm.org/D20951