[PATCH] D13615: [lld][elf2] Local Dynamic TLS

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/OutputSections.cpp:58
@@ -55,1 +57,3 @@
+    if (!B)
+      continue;
     if (canBePreempted(B))
----------------
  continue;  // skip TLS empty second slot

================
Comment at: ELF/OutputSections.cpp:128
@@ -123,3 +127,3 @@
 
-    bool CanBePreempted = canBePreempted(Body);
+    bool CanBePreempted = canBePreempted(Body) || Type == R_X86_64_TLSLD;
     uintX_t Addend = 0;
----------------
I don't understand this. Is R_x86_64_TLSLD interpositioned? This function is really complicated and needs cleanup.

================
Comment at: ELF/OutputSections.h:111
@@ -110,3 +110,3 @@
   void writeTo(uint8_t *Buf) override;
-  void addEntry(SymbolBody *Sym);
+  void addEntry(SymbolBody *Sym, int Size = 1);
   bool empty() const { return Entries.empty(); }
----------------
It seems that we don't really have to pass the number to this function because the symbol should know that. If it's STT_TLS, we need two slots. Otherwise one. Can you remove the second parameter?

================
Comment at: ELF/Writer.cpp:143
@@ -141,2 +142,3 @@
   Out<ELFT>::Dynamic = &Dynamic;
+  Out<ELFT>::ThreadDataStart = 0;
 
----------------
Global uninitialized values should be initialized with zero, so no need to assign the same value again.

================
Comment at: ELF/Writer.cpp:220
@@ -217,3 +219,3 @@
     }
-    if (canBePreempted(Body)) {
+    if (canBePreempted(Body) || Type == R_X86_64_TLSLD) {
       Body->setUsedInDynamicReloc();
----------------
This is another use of canBePreempted(Body) || Type == R_X86_64_TLSLD

================
Comment at: ELF/Writer.cpp:292-327
@@ -289,11 +291,38 @@
 
-  uintX_t AFlags = A->getFlags();
-  uintX_t BFlags = B->getFlags();
-
-  // Allocatable sections go first to reduce the total PT_LOAD size and
-  // so debug info doesn't change addresses in actual code.
-  bool AIsAlloc = AFlags & SHF_ALLOC;
-  bool BIsAlloc = BFlags & SHF_ALLOC;
-  if (AIsAlloc != BIsAlloc)
-    return AIsAlloc;
+  auto CalcRank = [](OutputSectionBase<ELFT::Is64Bits> *Sec) {
+    uintX_t Flags = Sec->getFlags();
+    // The TLS initialization block goes at the start of the read/write PT_LOAD
+    // as it needs to first be loaded into memory so relocations can be applied.
+    // The rest of the flags are ignored as there can only be one TLS
+    // initialization block per binary.
+    if ((Flags & (SHF_ALLOC | SHF_TLS)) == (SHF_ALLOC | SHF_TLS))
+      return 2;
+
+    switch (Flags & (SHF_ALLOC | SHF_WRITE | SHF_EXECINSTR)) {
+    case SHF_ALLOC:
+      // We want the read only sections first so that they go in the PT_LOAD
+      // covering the program headers at the start of the file.
+      return 0;
+
+    // The relative order of the following 3 cases is arbitrary.
+
+    case SHF_ALLOC | SHF_EXECINSTR:
+      return 1;
+    case SHF_ALLOC | SHF_WRITE:
+      return 3;
+    case SHF_ALLOC | SHF_WRITE | SHF_EXECINSTR:
+      return 4;
+
+    case 0:
+    case SHF_WRITE | SHF_EXECINSTR:
+    case SHF_WRITE:
+    case SHF_EXECINSTR:
+      // Non allocatable sections go last to reduce the total PT_LOAD size and
+      // so debug info doesn't change addresses in actual code. There are also
+      // no special requirements for the relative order of non allocatable
+      // sections.
+      return 5;
+    }
+    llvm_unreachable("Covered switch.");
+  };
 
----------------
Can you move this function to top level? This doesn't have to be a lambda.

================
Comment at: ELF/Writer.cpp:298
@@ +297,3 @@
+    // initialization block per binary.
+    if ((Flags & (SHF_ALLOC | SHF_TLS)) == (SHF_ALLOC | SHF_TLS))
+      return 2;
----------------
  (Flags & SHF_ALLOC) && (Flags | SHF_TLS)

================
Comment at: ELF/Writer.cpp:563
@@ -542,1 +562,3 @@
       FileOff += Size;
+    if ((Sec->getFlags() & (SHF_ALLOC | SHF_TLS)) == (SHF_ALLOC | SHF_TLS)) {
+      if (Sec->getType() != SHT_NOBITS)
----------------
  (Sec->getFlags() & SHF_ALLOC) && (Sec->getFalgs() & SHF_TLS)

================
Comment at: ELF/Writer.cpp:563-586
@@ -542,2 +562,26 @@
       FileOff += Size;
+    if ((Sec->getFlags() & (SHF_ALLOC | SHF_TLS)) == (SHF_ALLOC | SHF_TLS)) {
+      if (Sec->getType() != SHT_NOBITS)
+        VA = RoundUpToAlignment(VA, Align);
+      uintX_t TVA = RoundUpToAlignment(VA + ThreadBSSOffset, Align);
+      Sec->setVA(TVA);
+      ThreadBSSOffset = TVA - VA;
+      if (Sec->getType() == SHT_NOBITS)
+        ThreadBSSOffset += Size;
+      else
+        VA += Size;
+      if (TlsPHDR.Header.p_offset == 0) {
+        TlsPHDR.Header.p_offset = Sec->getFileOff();
+        TlsPHDR.Header.p_vaddr = Sec->getVA();
+        TlsPHDR.Header.p_paddr = TlsPHDR.Header.p_vaddr;
+        Out<ELFT>::ThreadDataStart = TlsPHDR.Header.p_vaddr;
+      }
+      TlsPHDR.Header.p_filesz += Sec->getType() == SHT_NOBITS ? 0 : Size;
+      TlsPHDR.Header.p_memsz += Size;
+      TlsPHDR.Header.p_align = std::max<uintX_t>(TlsPHDR.Header.p_align, Align);
+    } else if (Sec->getFlags() & SHF_ALLOC) {
+      VA = RoundUpToAlignment(VA, Align);
+      Sec->setVA(VA);
+      VA += Size;
+    }
   }
----------------
ruiu wrote:
>   (Sec->getFlags() & SHF_ALLOC) && (Sec->getFalgs() & SHF_TLS)
Please try to reduce the amount of code to add this code, or completely split this function. This function is already too long.


http://reviews.llvm.org/D13615