[PATCH] D18960: [lld] Initial implementation of TLSDESC relocation handling

[Adhemerval Zanella via llvm-commits]

zatrazz added a comment.

In http://reviews.llvm.org/D18960#397780, @ruiu wrote:

> This seems to conflict with http://reviews.llvm.org/D18711, so you may want to wait for that.


Right, I will rebase using this patch as base.


================
Comment at: ELF/InputSection.cpp:301
@@ +300,3 @@
+      Target->relocateOne(BufLoc, BufEnd, Type, AddrLoc,
+                          Body.getGotPltTlsDescVA<ELFT>() + A);
+      continue;
----------------
ruiu wrote:
> I'd name this getTlsDescVA rather than getGotPltTlsDescVA if there's no reason to include "GotPlt".
Ack.

================
Comment at: ELF/OutputSections.cpp:64
@@ -62,2 +63,3 @@
 
 template <class ELFT> void GotPltSection<ELFT>::addEntry(SymbolBody &Sym) {
+  Sym.GotPltIndex = Target->GotPltHeaderEntriesNum +
----------------
grimar wrote:
> Method does not seem to be clang formatted.
Indeed, I will fix it and other parts as well (I forgot to use clang-format-diff in this patch).

================
Comment at: ELF/OutputSections.cpp:76
@@ -66,1 +75,3 @@
+  Entries[DynReloc_TlsDesc].push_back(nullptr);
+  Entries[DynReloc_TlsDesc].push_back(nullptr);
 }
----------------
grimar wrote:
> I would write:
> 
> ```
> Entries[DynReloc_TlsDesc].insert(Entries[DynReloc_TlsDesc].end(), { nullptr, nullptr });
> ```
> As in fact this is single entry
I think this is slight worse regarding readability, but I will change it.

================
Comment at: ELF/OutputSections.cpp:84
@@ -72,1 +83,3 @@
 template <class ELFT> void GotPltSection<ELFT>::finalize() {
+  size_t entriesSize = Entries[DynReloc_JumpSlot].size() +
+                       Entries[DynReloc_TlsDesc].size();
----------------
grimar wrote:
> ```
> EntriesSize 
> ```
Ack.

================
Comment at: ELF/OutputSections.cpp:351
@@ -302,11 +350,3 @@
 template <class ELFT> void RelocationSection<ELFT>::writeTo(uint8_t *Buf) {
-  for (const DynamicReloc<ELFT> &Rel : Relocs) {
-    auto *P = reinterpret_cast<Elf_Rela *>(Buf);
-    Buf += Config->Rela ? sizeof(Elf_Rela) : sizeof(Elf_Rel);
-    SymbolBody *Sym = Rel.Sym;
-
-    if (Config->Rela)
-      P->r_addend = Rel.UseSymVA ? Sym->getVA<ELFT>(Rel.Addend) : Rel.Addend;
-    P->r_offset = Rel.OffsetInSec + Rel.OffsetSec->getVA();
-    uint32_t SymIdx = (!Rel.UseSymVA && Sym) ? Sym->DynsymIndex : 0;
-    P->setSymbolAndType(SymIdx, Rel.Type, Config->Mips64EL);
+  for (auto RelocVec : Relocs) {
+    for (const DynamicReloc<ELFT> &Rel : RelocVec) {
----------------
grimar wrote:
> I don't think we use auto in such cases as type is not obvious here. So I would use
> 
> ```
>   for (std::vector<DynamicReloc<ELFT>> &Vec : Relocs) {
>     for (const DynamicReloc<ELFT> &Rel : Vec) {
> ```
Ack.

================
Comment at: ELF/OutputSections.h:153
@@ +152,3 @@
+  DynReloc_NumKind = DynReloc_TlsDesc + 1
+};
+
----------------
grimar wrote:
> Do you really need direct assignmens here ? Why don't write like:
> 
> ```
>   DynReloc_JumpSlot,
>   DynReloc_TlsDesc,
>   DynReloc_NumKind
> ```
It is just to make it explicit, since C++11 does state first enum is 0 and next one will have the value incremented by 1. I will change it.

================
Comment at: ELF/OutputSections.h:164
@@ -156,3 +163,3 @@
 private:
-  std::vector<const SymbolBody *> Entries;
+  std::vector<std::vector<const SymbolBody *>> Entries;
 };
----------------
emaste wrote:
> ruiu wrote:
> > This code is hard to understand. If I understand it correctly, it is a vector of vectors binned according to their DynReloc_* types. Is this correct?
> > 
> > There seems to be no reason to aggregate multiple vectors into one vector (of vectors) here. Why don't you define multiple vectors?
> I think this comes out of a discussion in D18330: http://reviews.llvm.org/D18330#inline-153615 @grimar or @zatrazz?
It is correct and I thought about adding default vector for each type of relocation, I used this approach mainly to no tie relocation types to a specific vector. Although I think now using different vectors seems a better alternative, I will change it.

================
Comment at: ELF/Symbols.cpp:171
@@ +170,3 @@
+template <class ELFT>
+typename ELFT::uint SymbolBody::getGotPltTlsDescVA() const {
+  return Out<ELFT>::GotPlt->getVA() +
----------------
grimar wrote:
> This one not formatted either. Please use clang-format for this patch.
Ack.

================
Comment at: ELF/Target.cpp:177
@@ -176,2 +176,3 @@
                    uint64_t SA) const override;
+
   size_t relaxTlsGdToLe(uint8_t *Loc, uint8_t *BufEnd, uint32_t Type,
----------------
grimar wrote:
> Remove the empty line please.
Ack.


http://reviews.llvm.org/D18960