[PATCH] D14090: [ELF2] R_X86_64_COPY relocation implemented

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/InputSection.cpp:86
@@ +85,3 @@
+    if (ELFSymbolBody<ELFT> *E = dyn_cast<ELFSymbolBody<ELFT>>(&Body))
+      NeedsCopy = Target->relocNeedsCopy(Type, E->Sym.getType(), Body);
+    if (NeedsCopy) {
----------------
E->Sym.getType() is accessible from Body, so you can only pass Body to relocNeedsCopy. Then you can write.

  if (Target->relocNeedsCopy(Type, Body)) {
    Type = Target->getCopyReloc();
  } else if (Target->relocNeedsPlt(Type, Body)) {
  ...


================
Comment at: ELF/OutputSections.cpp:170
@@ +169,3 @@
+    bool NeedsCopy =
+        Shared && Target->relocNeedsCopy(Type, Shared->Sym.getType(), *Body);
+    bool NeedsGot = Body && !NeedsCopy && Target->relocNeedsGot(Type, *Body);
----------------
Shared is also derived from Body, so you don't have to pass to that function.

================
Comment at: ELF/OutputSections.cpp:195-200
@@ -188,5 +194,8 @@
         P->r_offset = Out<ELFT>::Got->getEntryAddr(*Body);
     } else {
-      P->r_offset = RI.r_offset + C.OutSec->getVA() + C.OutSecOff;
+      if (NeedsCopy)
+        P->r_offset = Out<ELFT>::Bss->getVA() + Shared->OffsetInBSS;
+      else
+        P->r_offset = RI.r_offset + C.OutSec->getVA() + C.OutSecOff;
     }
 
----------------
  } else if (NeedsCopy) {


================
Comment at: ELF/OutputSections.cpp:207-208
@@ -197,2 +206,4 @@
     uintX_t Addend;
-    if (CanBePreempted)
+    if (NeedsCopy)
+      Addend = 0;
+    else if (CanBePreempted)
----------------
Can you return here?

================
Comment at: ELF/OutputSections.cpp:632
@@ +631,3 @@
+    const auto &SS = cast<SharedSymbol<ELFT>>(S);
+    if (!SS.NeedsCopy)
+      return 0;
----------------
Revert the condition in `if`

  if (SS.NeedsCopy)
    return Out...;
  return 0;

================
Comment at: ELF/OutputSections.cpp:1009
@@ +1008,3 @@
+        OutSec = Out<ELFT>::Bss;
+      }
+      break;
----------------
Remove {}. What's the point of setting Bss to OutSec? I just don't understand what that code is for.

================
Comment at: ELF/Symbols.h:289-290
@@ -284,1 +288,4 @@
+  // Can have offset if requires copy relocation.
+  uintX_t OffsetInBSS;
+  bool NeedsCopy;
 };
----------------
  uintX_t OffsetInBss = 0;
  bool NeedsCopy = false;

================
Comment at: ELF/Target.cpp:340-342
@@ -327,2 +339,5 @@
     break;
+  case R_X86_64_COPY:
+    write32le(Loc, SA - P);
+    break;
   default:
----------------
We only create copy relocations, no? Copy relocations are consumed only by the dynamic linker.

================
Comment at: ELF/Writer.cpp:203-215
@@ -202,3 +202,15 @@
     if (Body) {
-      NeedsPlt = Target->relocNeedsPlt(Type, *Body);
+      bool NeedsCopy = false;
+      if (auto *E = dyn_cast<SharedSymbol<ELFT>>(Body)) {
+        NeedsCopy = Target->relocNeedsCopy(Type, E->Sym.getType(), *Body);
+        E->NeedsCopy = NeedsCopy;
+      }
+      if (NeedsCopy) {
+        if (Body->InRelaBss)
+          continue;
+        else
+          Body->InRelaBss = true;
+      }
+
+      NeedsPlt = !NeedsCopy && Target->relocNeedsPlt(Type, *Body);
       if (NeedsPlt) {
----------------
What does this code do?

================
Comment at: ELF/Writer.cpp:221
@@ -208,3 +220,3 @@
       }
-      NeedsGot = Target->relocNeedsGot(Type, *Body);
+      NeedsGot = !NeedsCopy && Target->relocNeedsGot(Type, *Body);
       if (NeedsGot) {
----------------
If Body needs a copy relocation, it never need a GOT, no? If so, relocNeedsGot should take care of that.


http://reviews.llvm.org/D14090