[PATCH] D14090: [ELF2] R_X86_64_COPY relocation implemented

[George Rimar via llvm-commits]

grimar 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)
----------------
ruiu wrote:
> 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?
Probably it needs early return after 

uintX_t OrigAddend = 0; 

if IsRela false. But that is not for this patch I guess ?

================
Comment at: ELF/Writer.cpp:417
@@ +416,3 @@
+    Off = RoundUpToAlignment(Off, Sym.st_size);
+    C->OffsetInBSS = Off;
+    Off += Sym.st_size;
----------------
ruiu wrote:
> 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.
I found the next comment in gnu code (http://www.opensource.apple.com/source/gdb/gdb-213/src/bfd/elf64-x86-64.c):

  /* We need to figure out the alignment required for this symbol.  I
     have no idea how ELF linkers handle this.	16-bytes is the size
     of the largest type that requires hard alignment -- long double.  */
  /* FIXME: This is VERY ugly. Should be fixed for all architectures using
     this construct.  */

So it looks we can use 16.
Also I guess even if we take 32 bytes then alignment should be calculated as max(Sym.st_size, 32) ?


http://reviews.llvm.org/D14090