[PATCH] D14090: [ELF2] R_X86_64_COPY relocation implemented

[George Rimar via llvm-commits]

grimar 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) {
----------------
ruiu wrote:
> 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)) {
>   ...
> 
No, its not. It accessible from ELFSymbolBody, which is template and inherits SymbolBody. So anyways dyn_cast with using of concrete ELFT will be needed inside relocNeedsCopy. Did you mean that is correct way ?
I changed it in this way in updated patch.

================
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);
----------------
ruiu wrote:
> Shared is also derived from Body, so you don't have to pass to that function.
Ditto, changed.

================
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;
     }
 
----------------
ruiu wrote:
>   } else if (NeedsCopy) {
> 
Ok.

================
Comment at: ELF/OutputSections.cpp:207-208
@@ -197,2 +206,4 @@
     uintX_t Addend;
-    if (CanBePreempted)
+    if (NeedsCopy)
+      Addend = 0;
+    else if (CanBePreempted)
----------------
ruiu wrote:
> Can you return here?
No. Because below that ifs section there is a code which I need:


    if (IsRela)
      static_cast<Elf_Rela *>(P)->r_addend = Addend;

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

================
Comment at: ELF/OutputSections.cpp:1009
@@ +1008,3 @@
+        OutSec = Out<ELFT>::Bss;
+      }
+      break;
----------------
ruiu wrote:
> Remove {}. What's the point of setting Bss to OutSec? I just don't understand what that code is for.
Point is to have correct Ndx in .symtab. Below output of readelf with this code and without:
with:
    55: 00000000000121f8     8 OBJECT  GLOBAL DEFAULT   26 stdout
w/o:
    55: 00000000000121f8     8 OBJECT  GLOBAL DEFAULT  UND stdout

26 is bss section. 
I know you dont want to emulate ld behavior but thats just 3 lines of code that not only make behavor consistent with ld, which is good here I believe, but also make more clear symbol status for those who look into that table. Bss section index looks much better that UND. At least it gives surety that symbol was found and processed.


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

================
Comment at: ELF/Target.cpp:340-342
@@ -327,2 +339,5 @@
     break;
+  case R_X86_64_COPY:
+    write32le(Loc, SA - P);
+    break;
   default:
----------------
ruiu wrote:
> We only create copy relocations, no? Copy relocations are consumed only by the dynamic linker.
Ok, that code was called because of reassigning type of relocation in InputSection.cpp:


line 87:
      Type = Target->getCopyReloc();

That`s really not nessecary to do. Fixed.

================
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) {
----------------
ruiu wrote:
> What does this code do?
1) Founds if relocNeedsCopy and sets is to SharedSymbol->NeedsCopy  which is used in void SymbolTableSection<ELFT>::writeGlobalSymbols to write correct Ndx of body section.

2) GNU ld places symbols that needs copy relocations to .dynbss and its relocations to .rel[a].bss. Then "linker script puts the .dynbss section into the .bss section of the final image". I dont see the reasons to do the equal things if we can just allocate space in bss directly. But what we really need is to be sure that we dont create .rela.dyn relocations for one symbol twice. Thats why we have to use InRelaBss flag. It prevents to call Out<ELFT>::RelaDyn->addReloc({C, RI}); (which is at the end of this method) twice or more.
Its like Out<ELFT>::Got->addEntry(Body) sets got index and then if (Body->isInGot()) continue; called for the same.

================
Comment at: ELF/Writer.cpp:221
@@ -208,3 +220,3 @@
       }
-      NeedsGot = Target->relocNeedsGot(Type, *Body);
+      NeedsGot = !NeedsCopy && Target->relocNeedsGot(Type, *Body);
       if (NeedsGot) {
----------------
ruiu wrote:
> If Body needs a copy relocation, it never need a GOT, no? If so, relocNeedsGot should take care of that.
Probably you`re right. The same should be applied for relocNeedsPlt then.


http://reviews.llvm.org/D14090