[PATCH] D14090: [ELF2] R_X86_64_COPY relocation implemented

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/OutputSections.cpp:203-206
@@ -197,4 +202,6 @@
     uintX_t Addend;
-    if (CanBePreempted)
+    if (NeedsCopy)
+      Addend = 0;
+    else if (CanBePreempted)
       Addend = OrigAddend;
     else if (Body)
----------------
This makes me think whether this function as a whole is correct or not. If IsRela is false, we compute an addend and then discard that value? If that information is discardable, what's the point of computing that value here?

================
Comment at: ELF/OutputSections.cpp:1002
@@ +1001,3 @@
+    case SymbolBody::SharedKind: {
+      auto *Sym = cast<SharedSymbol<ELFT>>(Body->repl());
+      if (Sym->NeedsCopy)
----------------
I think you don't want to call repl(). Body->repl() may not be of type SharedSymbol if the same symbol is resolved to something else.

  if (cast<SharedSymbol<ELFT>>(Body)->NeedsCopy)
    Outsec = Out<ELFT>::Bss;
  break;

================
Comment at: ELF/Target.cpp:253
@@ +252,3 @@
+                                      const SymbolBody &S) const {
+
+  if (auto *E = dyn_cast<lld::elf2::ELFSymbolBody<ELF64LE>>(&S))
----------------
Remove this blank line.

================
Comment at: ELF/Target.cpp:254
@@ +253,3 @@
+
+  if (auto *E = dyn_cast<lld::elf2::ELFSymbolBody<ELF64LE>>(&S))
+    if (E->Sym.getType() != STT_OBJECT)
----------------
Do you need "lld::elf2::" namespace specifiers?

================
Comment at: ELF/Target.cpp:254-259
@@ +253,8 @@
+
+  if (auto *E = dyn_cast<lld::elf2::ELFSymbolBody<ELF64LE>>(&S))
+    if (E->Sym.getType() != STT_OBJECT)
+      return false;
+  if (!S.isShared())
+    return false;
+  return (Type == R_X86_64_32S || Type == R_X86_64_32 || Type == R_X86_64_PC32);
+}
----------------
ruiu wrote:
> Do you need "lld::elf2::" namespace specifiers?
You can make this a bit shorter.

  if (Type == R_X86_64_32S || Type == R_X86_64_32 || Type == R_X86_64_PC32)
    if (auto *SS = dyn_cast<SharedSymbol<ELFT>>(&S))
      return SS->Sym.getType() == STT_OBJECT;
  return false;

================
Comment at: ELF/Writer.cpp:417
@@ +416,3 @@
+    Off = RoundUpToAlignment(Off, Sym.st_size);
+    C->OffsetInBSS = Off;
+    Off += Sym.st_size;
----------------
st_size is too large to align. (If a copy relocation points to an array, st_size can be very large.) Maybe we should align to some reasonable boundary, say, 32-byte boundaries for now. I don't think we are going to have tons of copy relocations, so doing something simple should suffice.


http://reviews.llvm.org/D14090