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

[Peter Smith via llvm-commits]

peter.smith added a subscriber: peter.smith.
peter.smith added a comment.

I'm looking at this from an AARCH64 perspective. I've also checked to the best of my knowledge that the architecture independent parts of the descriptor model implemented here are compatible with other architectures such as X86.

This is my first time looking at LLD, so I hope any misunderstandings of the code-base don't show through too heavily.

General comments:

- it looks like you are only implementing the case where lazy specialization of TLS is permitted. Is this intentional?
- it looks like you are not defining _TLS_MODULE_BASE_ (points at beginning of TLS segment for module) which I think is used by the local dynamic model (local dynamic is not well supported for AARCH64 I belive)

I've left some comments above where I found it more difficult than usual to follow the code. Feel free to ignore.

Some more references that I found useful when looking at this and the (now abandoned http://reviews.llvm.org/D18839):

ARM doesn't have much public documentation on AARCH64 TLS besides the relocations defined in the ABI, the best alternative source of information I have seen is in https://llvm.org/bugs/show_bug.cgi?id=22408 clang no longer bootstraps itself on AArch64 linux.

Paper "Speeding Up Thread-Local Storage Access in Dynamic Libraries in the ARM platform": http://www.fsfla.org/~lxoliva/writeups/TLS/paper-lk2006.pdf

Relocations for AARCH64 are defined in ELF for the ARM 64-bit Architecture (AArch64) Table 4.6.10.5 Thread-local storage descriptors:
http://infocenter.arm.com/help/topic/com.arm.doc.ihi0056b/IHI0056B_aaelf64.pdf


================
Comment at: ELF/InputSection.cpp:294
@@ -293,2 +293,3 @@
     }
 
+    // TLS descriptor uses two words in the .got.plt which describes a
----------------
A descriptor can only have one parameter (2nd word in .got.plt) in the comment below would it be better to say "a function pointer and its argument", 

================
Comment at: ELF/OutputSections.cpp:92
@@ -78,2 +91,3 @@
   Target->writeGotPltHeader(Buf);
   Buf += Target->GotPltHeaderEntriesNum * sizeof(uintX_t);
+  // Since TLSDESC entries will contains nullptr there is no need to
----------------
Position of this comment is a bit confusing as the TLS Desc entries come after the Jump Slot entries. Perhaps move after the for loop?

I assume that nullptr will be written out as zero or some other padding value in the actual file.



================
Comment at: ELF/OutputSections.cpp:296
@@ -273,1 +295,3 @@
   }
+
+  // If module uses TLS descriptor relocations, it requires double .got.plt
----------------
I found this comment a bit confusing.

The double .got.plt entries are for the TLS descriptors, but we are talking about the PLT entry for the lazy resolver here. The writePltDescEntry() from what I can see of your aarch64 patch is really writing the contents of the _dl_tlsdesc_lazy_trampoline function, and nothing else. The DT_TLSDESC_PLT is created somewhere else.

Would it be worth simplifying the comment to what the code is doing locally?




================
Comment at: ELF/OutputSections.h:112
@@ -111,2 +111,3 @@
   void addEntry(SymbolBody &Sym);
   bool addDynTlsEntry(SymbolBody &Sym);
+  void addTlsDescEntry();
----------------
both Got and PltGot have an addTlsDescEntry member function that do different things.

In GotSection::addTlsDescEntry() we are reserving a single entry in the GOT that DT_TLSDESC_GOT will refer to. This member function will only add this entry once.

In GotPltSection::addTlsDescEntry(SymbolBody&) we are adding a TLS Descriptor to the .got.plt section for the symbol.

Would it be better to rename one of these member functions to capture the difference in meaning. Or perhaps add a comment. 

================
Comment at: ELF/OutputSections.h:167
@@ -155,2 +166,3 @@
 
 private:
+  std::vector<std::vector<const SymbolBody *>> Entries;
----------------
Is a vector of vectors really needed here? For example you could have two separate vectors: JumpSlotEntries and TlsDescEntries. I can't see too much advantage in the vector here.

https://sourceware.org/bugzilla/show_bug.cgi?id=13302 IRELATIVE relocation should come last shows that there is at least one more sub-section that will be needed, but I'm not aware of more than 3.

I've not been able to find a reference to why the Jump Slot entries and TlsDesc entries must be separated, although Gold does make the same separation. Do you happen to know? It might be worth adding a comment if there is a reason as in your AARCH64 review you explicitly test for the order.  


================
Comment at: ELF/OutputSections.h:256
@@ -242,2 +255,3 @@
 
 private:
+  size_t getRelocsSize() const;
----------------
As a first time look at the code base I had to check whether size meant number of relocs or size of relocs in bytes. Would getNumRelocs() work better? Apologies if this is an lld convention I've missed.

================
Comment at: ELF/OutputSections.h:427
@@ -412,2 +426,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.
----------------
Rather than add special which doesn't help you much when you see special in isolation, would it be better to introduce 2 separate kinds, one for each of DT_TLSDESC_PLT and DT_TLSDESC_GOT?

================
Comment at: ELF/Target.h:48
@@ -44,2 +47,3 @@
                         unsigned RelOff) const {}
 
+  virtual void writePltTlsDescEntry(uint8_t *Buf, uint64_t PltEntryAddr,
----------------
Is it worth a comment on what the PltDescEntry must do, seeing as it is a target specific function that we don't have an implementation for (in this patch anyway)?

================
Comment at: ELF/Writer.cpp:311
@@ -310,1 +310,3 @@
   }
+
+  // If module uses TLS descriptor relocations,
----------------
Would it be worth splitting the comment so that 1. appeared alongside Got->addTlsDescEntry() and 2. Appeared alongside GotPlt->addTlsDescEntry(Body).



http://reviews.llvm.org/D18960