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

[Adhemerval Zanella via llvm-commits]

zatrazz added inline comments.

================
Comment at: ELF/InputSection.cpp:294-304
@@ -293,3 +293,13 @@
     }
 
+    // TLS descriptor uses two words in the .got.plt which describes a
+    // structure of a function pointer and its arguments. Both fields will
+    // be adjusted at runtime by the dynamic loader with a single
+    // relocation (usually R_<ARCH>_TLSDESC).
+    if (Target->isTlsDescRel(Type)) {
+      Target->relocateOne(BufLoc, BufEnd, Type, AddrLoc,
+                          Body.getGotPltTlsDescVA<ELFT>() + A);
+      continue;
+    }
+
     if (!RelTy::IsRela)
----------------
Ack.

================
Comment at: ELF/OutputSections.cpp:93
@@ -78,2 +92,3 @@
   Target->writeGotPltHeader(Buf);
   Buf += Target->GotPltHeaderEntriesNum * sizeof(uintX_t);
+  // Since TLSDESC entries will contains nullptr there is no need to
----------------
I will change this class to contain two internal vector representing both the jump slot and tlcdesc relocation.  And the the idea here is just to write down the Entries VA from actually symbols, the size will be computed 'finalize'.

================
Comment at: ELF/OutputSections.cpp:297
@@ -273,1 +296,3 @@
   }
+
+  // If module uses TLS descriptor relocations, it requires double .got.plt
----------------
What about something like:

```
// If module uses TLS description relocations writes down the architecture specific
// lazy relocation entrypoint.
```

================
Comment at: ELF/OutputSections.h:112-113
@@ -111,3 +111,4 @@
   void addEntry(SymbolBody &Sym);
   bool addDynTlsEntry(SymbolBody &Sym);
+  void addTlsDescEntry();
   bool addTlsIndex();
----------------
I think the idea is to follow the same one for addEntry, which will also for GotSection reserve a entry and for GotPltSection will add on the .got.plt. I do not see why changes their names.

================
Comment at: ELF/OutputSections.h:163
@@ -155,2 +162,3 @@
 
 private:
+  std::vector<std::vector<const SymbolBody *>> Entries;
----------------
Yes, from Rui comments I think just adding two vectors seems a better strategy and for IRELATIVE I think we can just add another vector if is the case.

I am trying to found out exactly why gold is doing this separation and based on IRELATIVE issue I am assuming it some dependency issue, so I assumed it would be wiser to follow up gold example. 

================
Comment at: ELF/OutputSections.h:252
@@ -242,2 +251,3 @@
 
 private:
+  size_t getRelocsSize() const;
----------------
I think it is, I will change it.

================
Comment at: ELF/OutputSections.h:423
@@ -412,2 +422,3 @@
   // The section consists of fixed size entries, which consist of
   // type and value fields. Value are one of plain integers, symbol
+  // addresses, or section addresses. Also value can be of special type.
----------------
Yes I think is would be better indeed, I will change it.

================
Comment at: ELF/Target.h:46-50
@@ -44,3 +45,7 @@
                         unsigned RelOff) const {}
 
+  virtual void writePltTlsDescEntry(uint8_t *Buf, uint64_t PltEntryAddr,
+                                    uint64_t GotTlsDescEntryAddr,
+                                    uint64_t GotPltVA) const {};
+
   // Returns true if a relocation is just a hint for linker to make for example
----------------
I will add it.

================
Comment at: ELF/Writer.cpp:311
@@ -310,1 +310,3 @@
   }
+
+  // If module uses TLS descriptor relocations,
----------------
Indeed, I will change it.


http://reviews.llvm.org/D18960