[PATCH] D13562: [ELF2] Add a base set of PPC64 relocations

[Rui Ueyama via llvm-commits]

ruiu added a comment.

Can you upload the new patch? Thanks!


================
Comment at: ELF/Target.cpp:232
@@ -207,1 +231,3 @@
 }
+uint64_t PPC64TargetInfo::getTocBase() const {
+  // The TOC consists of sections .got, .toc, .tocbss, .plt in that
----------------
Add a blank line before this line.

================
Comment at: ELF/Target.cpp:232
@@ -207,1 +231,3 @@
 }
+uint64_t PPC64TargetInfo::getTocBase() const {
+  // The TOC consists of sections .got, .toc, .tocbss, .plt in that
----------------
ruiu wrote:
> Add a blank line before this line.
This class does not use any members of the class, so let's make it a static non-member function.

================
Comment at: ELF/Target.cpp:248
@@ -208,1 +247,3 @@
+}
 void PPC64TargetInfo::writePltEntry(uint8_t *Buf, uint64_t GotEntryAddr,
+                                    uint64_t PltEntryAddr) const {
----------------
Blank line.

================
Comment at: ELF/Target.cpp:258
@@ +257,3 @@
+
+  write32be(Buf,      0xf8410000u);             // std %r2, 40(%r1)
+  write32be(Buf + 4,  0x3d620000u |
----------------
Drop 'u' suffix.

================
Comment at: ELF/Target.cpp:261-262
@@ +260,4 @@
+                      applyPPCHa(EntryOffset)); // addis %r11, %r2, X at ha
+  write32be(Buf + 8,  0xe98b0000u |
+                      applyPPCLo(EntryOffset)); // ld %r12, X at l(%r11)
+  write32be(Buf + 12, 0xe96c0000u);             // ld %r11,0(%r12)
----------------
Can this fit in one line if you use a shorter name for EntryOffset?

================
Comment at: ELF/Target.cpp:269
@@ -210,1 +268,3 @@
+}
 bool PPC64TargetInfo::relocNeedsGot(uint32_t Type, const SymbolBody &S) const {
+  if (relocNeedsPlt(Type, S))
----------------
Blank line.

================
Comment at: ELF/Target.cpp:284
@@ -212,2 +283,3 @@
 }
 bool PPC64TargetInfo::relocNeedsPlt(uint32_t Type, const SymbolBody &S) const {
+  if (Type != R_PPC64_REL24)
----------------
Blank line.

================
Comment at: ELF/Target.cpp:313-318
@@ +312,8 @@
+    switch (Type) {
+    case R_PPC64_TOC16:       Type = R_PPC64_ADDR16; break;
+    case R_PPC64_TOC16_DS:    Type = R_PPC64_ADDR16_DS; break;
+    case R_PPC64_TOC16_LO:    Type = R_PPC64_ADDR16_LO; break;
+    case R_PPC64_TOC16_LO_DS: Type = R_PPC64_ADDR16_LO_DS; break;
+    case R_PPC64_TOC16_HI:    Type = R_PPC64_ADDR16_HI; break;
+    case R_PPC64_TOC16_HA:    Type = R_PPC64_ADDR16_HA; break;
+    default: llvm_unreachable("Wrong relocation type.");
----------------
Write like this to remove the outer if.

  case R_PPC64_TOC16: Type = R_PPC64_ADDR16; A -= getTocBase(); break;
  case ...

================
Comment at: ELF/Target.cpp:334
@@ +333,3 @@
+      error("Relocation R_PPC64_ADDR16_DS overflow");
+    write16be(L, (read16be(L) & 3) | ((R) & ~3));
+    break;
----------------
(R) & ~3  ->  R & ~3

================
Comment at: ELF/Target.cpp:336
@@ +335,3 @@
+    break;
+  }
+  case R_PPC64_ADDR16_LO:
----------------
You don't need {} here. {} is used in switch-case to create a new scope for local variables, but you didn't define any in this {}.

================
Comment at: ELF/Target.cpp:343
@@ +342,3 @@
+    break;
+  }
+  case R_PPC64_ADDR16_HI:
----------------
Ditto

================
Comment at: ELF/Target.cpp:367
@@ +366,3 @@
+    // Preserve the AA/LK bits in the branch instruction
+    uint8_t AALK = *(L + 3);
+    write16be(L + 2, (AALK & 3) | (R & 0xfffc));
----------------
*(L + 3)  ->  L[3]


================
Comment at: ELF/Target.cpp:374
@@ +373,3 @@
+    break;
+  }
+  case R_PPC64_REL16_HI: {
----------------
Remove {}

================
Comment at: ELF/Target.cpp:378
@@ +377,3 @@
+    break;
+  }
+  case R_PPC64_REL16_HA: {
----------------
Ditto

================
Comment at: ELF/Target.cpp:382
@@ +381,3 @@
+    break;
+  }
+  case R_PPC64_ADDR32: {
----------------
Ditto

================
Comment at: ELF/Target.cpp:388
@@ +387,3 @@
+    break;
+  }
+  case R_PPC64_REL24: {
----------------
Ditto

================
Comment at: ELF/Target.cpp:401
@@ +400,3 @@
+    break;
+  }
+  case R_PPC64_REL64: {
----------------
Ditto

================
Comment at: ELF/Target.cpp:405
@@ +404,3 @@
+    break;
+  }
+  case R_PPC64_ADDR64:
----------------
Ditto


http://reviews.llvm.org/D13562