[PATCH] D18711: Change how we apply rellocations

[Rui Ueyama via llvm-commits]

ruiu added a comment.

First round of review comments.


================
Comment at: ELF/InputSection.cpp:265-268
@@ +264,6 @@
+  switch (Expr) {
+  case Relocation::R_ABS:
+  case Relocation::R_MIPS:
+  case Relocation::R_TLSLD:
+  case Relocation::R_TLSGD:
+  case Relocation::R_PPC_TOC:
----------------
Sort.

================
Comment at: ELF/InputSection.h:27
@@ +26,3 @@
+struct Relocation {
+  enum RelExpr {
+    R_ABS,
----------------
I'd define R_* names directly inside lld::ELF namespace because they don't conflict with other identifiers because of its R_ prefix.

================
Comment at: ELF/InputSection.h:38
@@ +37,3 @@
+    R_PLT,
+    R_PLT_OPD,
+    R_PLT_PC,
----------------
I'd name R_PPC_PLT_OPD since this is PPC-specific.

================
Comment at: ELF/InputSection.h:53
@@ +52,3 @@
+    R_TLSLD_PC
+  } Expr;
+  uint32_t Type;
----------------
I'd probably avoid naming this "Expr" because it is not an expression. It is more like the relocation type, right?

================
Comment at: ELF/Target.cpp:233
@@ -244,1 +232,3 @@
 
+Relocation::RelExpr TargetInfo::getRelExpr(uint32_t Type) const {
+  return Relocation::R_ABS;
----------------
You want to make this a pure virtual function.

================
Comment at: ELF/Target.cpp:531
@@ -542,1 +530,3 @@
+void X86TargetInfo::relocateOne(uint8_t *Loc, uint32_t Type,
+                                uint64_t SA) const {
   switch (Type) {
----------------
It is no longer SA (which implies S+A) right? You want to give a new name.

================
Comment at: ELF/Writer.cpp:611-618
@@ -477,11 +610,10 @@
     // load address.
-    if (!Config->Pic || Target->isRelRelative(Type) || Target->isSizeRel(Type))
-      continue;
+    bool IsSizeRel = Target->isSizeRel(Type);
+    if (!Config->Pic || Target->isRelRelative(Type) || IsSizeRel) {
+      if (IsSizeRel)
+        Expr = Relocation::R_SIZE;
+      C.Relocations.push_back({Expr, Type, RI.r_offset, Addend, &Body});
 
-    uintX_t Addend = getAddend<ELFT>(RI);
-    if (Config->EMachine == EM_PPC64 && RI.getType(false) == R_PPC64_TOC) {
-      Out<ELFT>::RelaDyn->addReloc({R_PPC64_RELATIVE, &C, RI.r_offset, false,
-                                    nullptr,
-                                    (uintX_t)getPPC64TocBase() + Addend});
       continue;
     }
+
----------------
You can separate IsSizeRel from other types.

  if (Target->isSizeRel(Type)) {
    C.Relocations.push_back({Relocation::R_SIZE, Type, RI.r_offset, Addend, &Body});
    continue;
  }

  if (!Config->Pic ||  ...) {
    C.Relocations.push_back({Expr, Type, RI.r_offset, Addend, &Body});
    continue;
  }


http://reviews.llvm.org/D18711