[ELF] Make dot in .tbss correct

[MaskRay]

Summary

GNU ld doesn't support multiple SHF_TLS SHT_NOBITS output sections (it restores
the address after an SHF_TLS SHT_NOBITS section, so consecutive SHF_TLS
SHT_NOBITS sections will have conflicting address ranges).

That said, threadBssOffset implements limited support for consecutive SHF_TLS
SHT_NOBITS sections. (SHF_TLS SHT_PROGBITS following a SHF_TLS SHT_NOBITS can still be
incorrect.)

. in an output section description of an SHF_TLS SHT_NOBITS section is
incorrect. (https://lists.llvm.org/pipermail/llvm-dev/2021-July/151974.html)

This patch saves the end address of the previous tbss section in
ctx->tbssAddr, changes dot in the beginning of assignOffset so
that . evaluation will be correct.

[MaskRay]

.

[MaskRay]

accidentally lost LinkerScript.h change

[peter.smith]

Just got back from a vacation, will try and take a look as soon as I can, I've got this D107235, and D107234 on the to-do list.

[peter.smith]

I'm a little nervous about this change as is. I think that if we only support one SHT_NOBITS SHF_TLS OutputSection then we should give an error message if there is more than one. Perhaps detect this in the PT_TLS output code. My main concern is about the the internal default (no linker script) and orphan TLS sections that don't have the .tbss. prefix. IIUC these will be placed after .tbss and will be covered by the PT_TLS program header but we will silently give these the wrong address. I'm sure these will be rare, but without an error message we may silently break some programs. I'm not as concerned about the linker script case as the onus is on the author to get the patterns right so that there are no orphans.

As a thought, could we keep with a slight modification?

bool isTbss =
    (ctx->outSec->flags & SHF_TLS) && ctx->outSec->type == SHT_NOBITS;
uint64_t start = isTbss ? dot + ctx->threadBssOffset : dot;
start = alignTo(start, alignment);
uint64_t end = start + size;

if (isTbss)
  ctx->threadBssOffset = end - dot;

dot = end;
return end;

In this way we set dot, which should permit the linker script to define symbols within the same output section, but dot will then get reset by your new code

// NOBITS TLS sections reset the location as well. Having another TLS section
// below sec is unsupported and regarded as user error.

so the threadBssOffset should be as it was previously.

[MaskRay]

address comments

[MaskRay]

In D107208#2921796, @peter.smith wrote:

I'm a little nervous about this change as is. I think that if we only support one SHT_NOBITS SHF_TLS OutputSection then we should give an error message if there is more than one. Perhaps detect this in the PT_TLS output code. My main concern is about the the internal default (no linker script) and orphan TLS sections that don't have the .tbss. prefix. IIUC these will be placed after .tbss and will be covered by the PT_TLS program header but we will silently give these the wrong address. I'm sure these will be rare, but without an error message we may silently break some programs. I'm not as concerned about the linker script case as the onus is on the author to get the patterns right so that there are no orphans.

LG to keep the multiple tbss mechanism.

As a thought, could we keep with a slight modification?

bool isTbss =
    (ctx->outSec->flags & SHF_TLS) && ctx->outSec->type == SHT_NOBITS;
uint64_t start = isTbss ? dot + ctx->threadBssOffset : dot;
start = alignTo(start, alignment);
uint64_t end = start + size;

if (isTbss)
  ctx->threadBssOffset = end - dot;

dot = end;
return end;

In this way we set dot, which should permit the linker script to define symbols within the same output section, but dot will then get reset by your new code

// NOBITS TLS sections reset the location as well. Having another TLS section
// below sec is unsupported and regarded as user error.

so the threadBssOffset should be as it was previously.

LinkerScript::getSymbolValue does not read ctx->threadBssOffset. I tried repairing it (see below) but ended up with the current diff

 ExprValue LinkerScript::getSymbolValue(StringRef name, const Twine &loc) {
   if (name == ".") {
-    if (ctx)
-      return {ctx->outSec, false, dot - ctx->outSec->addr, loc};
-    error(loc + ": unable to get location counter value");
-    return 0;
+    if (!ctx) {
+      error(loc + ": unable to get location counter value");
+      return 0;
+    }
+    if ((ctx->outSec->flags & SHF_TLS) && ctx->outSec->type == SHT_NOBITS) {
+      uint64_t offset =
+          ctx->threadBssOffset -
+          (Out::tlsPhdr ? ctx->outSec->addr - Out::tlsPhdr->firstSec->addr : 0);
+      return {ctx->outSec, false, offset, loc};
+    }
+    return {ctx->outSec, false, dot - ctx->outSec->addr, loc};
   }

[peter.smith]

Thanks for the update, LGTM.

lld/ELF/LinkerScript.h

 };
 
 class LinkerScript final {
   // Temporary state used in processSectionCommands() and assignAddresses()
   // that must be reinitialized for each call to the above functions, and must
   // not be used outside of the scope of a call to the above functions.
   struct AddressState {
     AddressState();
-    uint64_t threadBssOffset = 0;
     OutputSection *outSec = nullptr;
     MemoryRegion *memRegion = nullptr;
     MemoryRegion *lmaRegion = nullptr;
     uint64_t lmaOffset = 0;
+    uint64_t tbssAddr = 0;
   };
 
   llvm::DenseMap<StringRef, OutputSection *> nameToOutputSection;
 
   void addSymbol(SymbolAssignment *cmd);
   void assignSymbol(SymbolAssignment *cmd, bool inSec);
   void setDot(Expr e, const Twine &loc, bool inSec);
   void expandOutputSection(uint64_t size);

lld/ELF/LinkerScript.cpp

for (const InputSectionBase *sec : orphanSections) {
     if (config->orphanHandling == OrphanHandlingPolicy::Error)
       error(toString(sec) + " is being placed in '" + name + "'");
     else
       warn(toString(sec) + " is being placed in '" + name + "'");
   }
 }
 
 uint64_t LinkerScript::advance(uint64_t size, unsigned alignment) {
-  bool isTbss =
-      (ctx->outSec->flags & SHF_TLS) && ctx->outSec->type == SHT_NOBITS;
-  uint64_t start = isTbss ? dot + ctx->threadBssOffset : dot;
-  start = alignTo(start, alignment);
-  uint64_t end = start + size;
-
-  if (isTbss)
-    ctx->threadBssOffset = end - dot;
-  else
-    dot = end;
-  return end;
+  dot = alignTo(dot, alignment) + size;
+  return dot;
 }
 
 void LinkerScript::output(InputSection *s) {
   assert(ctx->outSec == s->getParent());
   uint64_t before = advance(0, 1);
   uint64_t pos = advance(s->getSize(), s->alignment);
   s->outSecOff = pos - s->getSize() - ctx->outSec->addr;
 
for (OutputSection *sec : outputSections)
     if (sec->ptLoad == load)
       return sec;
   return nullptr;
 }
 
 // This function assigns offsets to input sections and an output section
 // for a single sections command (e.g. ".text { *(.text); }").
 void LinkerScript::assignOffsets(OutputSection *sec) {
+  const bool isTbss = (sec->flags & SHF_TLS) && sec->type == SHT_NOBITS;
   const bool sameMemRegion = ctx->memRegion == sec->memRegion;
   const bool prevLMARegionIsDefault = ctx->lmaRegion == nullptr;
   const uint64_t savedDot = dot;
   ctx->memRegion = sec->memRegion;
   ctx->lmaRegion = sec->lmaRegion;
 
-  if (sec->flags & SHF_ALLOC) {
+  if (!(sec->flags & SHF_ALLOC)) {
+    // Non-SHF_ALLOC sections have zero addresses.
+    dot = 0;
+  } else if (isTbss) {
+    // Allow consecutive SHF_TLS SHT_NOBITS output sections. The address range
+    // starts from the end address of the previous tbss section.
+    if (ctx->tbssAddr == 0)
+      ctx->tbssAddr = dot;
+    else
+      dot = ctx->tbssAddr;
+  } else {
     if (ctx->memRegion)
       dot = ctx->memRegion->curPos;
     if (sec->addrExpr)
       setDot(sec->addrExpr, sec->location, false);
 
     // If the address of the section has been moved forward by an explicit
     // expression so that it now starts past the current curPos of the enclosing
     // region, we need to expand the current region to account for the space
     // between the previous section, if any, and the start of this section.
     if (ctx->memRegion && ctx->memRegion->curPos < dot)
       expandMemoryRegion(ctx->memRegion, dot - ctx->memRegion->curPos,
                          ctx->memRegion->name, sec->name);
-  } else {
-    // Non-SHF_ALLOC sections have zero addresses.
-    dot = 0;
   }
 
   switchTo(sec);
 
   // ctx->lmaOffset is LMA minus VMA. If LMA is explicitly specified via AT() or
   // AT>, recompute ctx->lmaOffset; otherwise, if both previous/current LMA
   // region is the default, and the two sections are in the same memory region,
   // reuse previous lmaOffset; otherwise, reset lmaOffset to 0. This emulates
for (BaseCommand *base : sec->sectionCommands) {
     // It calculates and assigns the offsets for each section and also
     // updates the output section size.
     for (InputSection *sec : cast<InputSectionDescription>(base)->sections)
       output(sec);
   }
 
   // Non-SHF_ALLOC sections do not affect the addresses of other OutputSections
   // as they are not part of the process image.
-  if (!(sec->flags & SHF_ALLOC))
+  if (!(sec->flags & SHF_ALLOC)) {
     dot = savedDot;
+  } else if (isTbss) {
+    // NOBITS TLS sections are similar. Additionally save the end address.
+    ctx->tbssAddr = dot;
+    dot = savedDot;
+  }
 }
 
 static bool isDiscardable(OutputSection &sec) {
   if (sec.name == "/DISCARD/")
     return true;
 
   // We do not want to remove OutputSections with expressions that reference
   // symbols even if the OutputSection is empty. We want to ensure that the

lld/test/ELF/linkerscript/tbss.s

 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
-# RUN: echo "SECTIONS { \
+# RUN: echo 'SECTIONS { \
 # RUN:   . = SIZEOF_HEADERS; \
 # RUN:   .text : { *(.text) } \
-# RUN:   foo : { *(foo) } \
+# RUN:   .tbss : { __tbss_start = .; *(.tbss) __tbss_end = .; } \
+# RUN:   second_tbss : { second_tbss_start = .; *(second_tbss) second_tbss_end = .; } \
 # RUN:   bar : { *(bar) } \
-# RUN: }" > %t.script
-# RUN: ld.lld -T %t.script %t.o -o %t
-# RUN: llvm-readobj -S %t | FileCheck %s
-
-# test that a tbss section doesn't use address space.
-
-# CHECK:        Name: foo
-# CHECK-NEXT:   Type: SHT_NOBITS
-# CHECK-NEXT:   Flags [
-# CHECK-NEXT:     SHF_ALLOC
-# CHECK-NEXT:     SHF_TLS
-# CHECK-NEXT:     SHF_WRITE
-# CHECK-NEXT:   ]
-# CHECK-NEXT:   Address: 0x[[ADDR:.*]]
-# CHECK-NEXT:   Offset: 0x[[ADDR]]
-# CHECK-NEXT:   Size: 4
-# CHECK-NEXT:   Link: 0
-# CHECK-NEXT:   Info: 0
-# CHECK-NEXT:   AddressAlignment: 1
-# CHECK-NEXT:   EntrySize: 0
-# CHECK-NEXT: }
-# CHECK-NEXT: Section {
-# CHECK-NEXT:   Index:
-# CHECK-NEXT:   Name: bar
-# CHECK-NEXT:   Type: SHT_PROGBITS
-# CHECK-NEXT:   Flags [
-# CHECK-NEXT:     SHF_ALLOC
-# CHECK-NEXT:     SHF_WRITE
-# CHECK-NEXT:   ]
-# CHECK-NEXT:   Address: 0x[[ADDR]]
+# RUN: }' > %t.lds
+# RUN: ld.lld -T %t.lds %t.o -o %t1
+# RUN: llvm-readelf -S -s %t1 | FileCheck %s
 
-        .section foo,"awT",@nobits
-        .long   0
-        .section bar, "aw"
-        .long 0
+# RUN: echo 'PHDRS { text PT_LOAD; }' > %th.lds
+# RUN: cat %th.lds %t.lds > %t2.lds
+# RUN: ld.lld -T %t.lds %t.o -o %t2
+# RUN: llvm-readelf -S -s %t2 | FileCheck %s
+
+## Test that a tbss section doesn't affect the start address of the next section.
+
+# CHECK: Name        Type     Address              Off                Size   ES Flg
+# CHECK: .tbss       NOBITS   [[#%x,ADDR:]]        [[#%x,OFF:]]       000004 00 WAT
+# CHECK: second_tbss NOBITS   {{0+}}[[#%x,ADDR+4]] {{0+}}[[#%x,OFF]]  000001 00 WAT
+# CHECK: bar         PROGBITS {{0+}}[[#%x,ADDR]]   {{0+}}[[#%x,OFF]]  000004 00  WA
+
+## Test that . in a tbss section represents the current location, even if the
+## address will be reset.
+
+# CHECK: Value                {{.*}} Name
+# CHECK: {{0+}}[[#%x,ADDR]]   {{.*}} __tbss_start
+# CHECK: {{0+}}[[#%x,ADDR+4]] {{.*}} __tbss_end
+# CHECK: {{0+}}[[#%x,ADDR+4]] {{.*}} second_tbss_start
+# CHECK: {{0+}}[[#%x,ADDR+5]] {{.*}} second_tbss_end
+
+.globl _start
+_start:
+  nop
+
+.section .tbss,"awT",@nobits
+  .long 0
+.section second_tbss,"awT",@nobits
+  .byte 0