COFF: Emit a symbol table if /debug is specified

[majnemer]

Summary

Providing a symbol table in the executable is quite useful when
debugging a fully-linked executable without having to reconstruct one
from DWARF.

[ruiu]

Can you add a test for the symbol table? I don't think long-section-name test covers the new feature.

COFF/InputFiles.cpp

149 ↗

(On Diff #29230)

Add a comment saying that we want to preserve DWARF debug sections only when /debug is on.

COFF/Writer.cpp

288 ↗

(On Diff #29230)

Return early by inverting the condition.

301 ↗

(On Diff #29230)

I prefer

auto *D = dyn_cast<...
if (!D || !D->isLive())
  continue;

to reduce indentation level.

317–320 ↗

(On Diff #29230)

Make this a separate member function.

328–329 ↗

(On Diff #29230)

If these actual values are 0, you don't need to set.

492 ↗

(On Diff #29230)

Return early.

504 ↗

(On Diff #29230)

Update comment ("then" no longer makes sense.)

COFF/Writer.h

64 ↗

(On Diff #29230)

Add a comment saying that this is 1-based section index.

65 ↗

(On Diff #29230)

Add a blank line before this line.

[majnemer]

• Address Rui's comments.

[majnemer]

COFF/Writer.cpp

317–320 ↗

(On Diff #29230)

I'm not sure what you mean here, the section that you highlighted isn't reusable elsewhere in the linker.

328–329 ↗

(On Diff #29230)

We must initialize it because otherwise the StorageClass and NumberOfAuxSymbols will be garbage and we would latter copy this garbage into the executable in writeHeader.

[ruiu]

LGTM

COFF/Writer.cpp

324–327 ↗

(On Diff #29236)

I meant this piece of code is repeated twice in this function. This is more like a personal perference, so if you don't want to make this change, I'm fine with that. I'll try to do this myself.

... = Strtab.size() + 4; // +4 for the size field
Strtab.insert(Strtab.end(), Name.begin(), Name.end());
Strtab.push_back('\0');

lld/trunk/COFF/Chunks.cpp

void SectionChunk::applyRelX64(uint8_t *Off, uint16_t Type, uint64_t S,
   case IMAGE_REL_AMD64_ADDR64:   add64(Off, S + Config->ImageBase); break;
   case IMAGE_REL_AMD64_ADDR32NB: add32(Off, S); break;
   case IMAGE_REL_AMD64_REL32:    add32(Off, S - P - 4); break;
   case IMAGE_REL_AMD64_REL32_1:  add32(Off, S - P - 5); break;
   case IMAGE_REL_AMD64_REL32_2:  add32(Off, S - P - 6); break;
   case IMAGE_REL_AMD64_REL32_3:  add32(Off, S - P - 7); break;
   case IMAGE_REL_AMD64_REL32_4:  add32(Off, S - P - 8); break;
   case IMAGE_REL_AMD64_REL32_5:  add32(Off, S - P - 9); break;
-  case IMAGE_REL_AMD64_SECTION:  add16(Off, Out->getSectionIndex() + 1); break;
+  case IMAGE_REL_AMD64_SECTION:  add16(Off, Out->SectionIndex); break;
   case IMAGE_REL_AMD64_SECREL:   add32(Off, S - Out->getRVA()); break;
   default:
     llvm::report_fatal_error("Unsupported relocation type");
   }
 }
 
 void SectionChunk::applyRelX86(uint8_t *Off, uint16_t Type, uint64_t S,
                                uint64_t P) {
   switch (Type) {
   case IMAGE_REL_I386_ABSOLUTE: break;
   case IMAGE_REL_I386_DIR32:    add32(Off, S + Config->ImageBase); break;
   case IMAGE_REL_I386_DIR32NB:  add32(Off, S); break;
   case IMAGE_REL_I386_REL32:    add32(Off, S - P - 4); break;
-  case IMAGE_REL_I386_SECTION:  add16(Off, Out->getSectionIndex() + 1); break;
+  case IMAGE_REL_I386_SECTION:  add16(Off, Out->SectionIndex); break;
   case IMAGE_REL_I386_SECREL:   add32(Off, S - Out->getRVA()); break;
   default:
     llvm::report_fatal_error("Unsupported relocation type");
   }
 }
 
 void SectionChunk::writeTo(uint8_t *Buf) {
   if (!hasData())

lld/trunk/COFF/InputFiles.cpp

if (auto EC = COFFObj->getSectionName(Sec, Name)) {
       return make_error_code(LLDError::BrokenFile);
     }
     if (Name == ".drectve") {
       ArrayRef<uint8_t> Data;
       COFFObj->getSectionContents(Sec, Data);
       Directives = std::string((const char *)Data.data(), Data.size());
       continue;
     }
-    if (Name.startswith(".debug"))
+    // We want to preserve DWARF debug sections only when /debug is on.
+    if (!Config->Debug && Name.startswith(".debug"))
       continue;
     if (Sec->Characteristics & llvm::COFF::IMAGE_SCN_LNK_REMOVE)
       continue;
     auto *C = new (Alloc) SectionChunk(this, Sec);
     Chunks.push_back(C);
     SparseChunks[I] = C;
   }
   return std::error_code();

lld/trunk/COFF/Writer.h

 
 // OutputSection represents a section in an output file. It's a
 // container of chunks. OutputSection and Chunk are 1:N relationship.
 // Chunks cannot belong to more than one OutputSections. The writer
 // creates multiple OutputSections and assign them unique,
 // non-overlapping file offsets and RVAs.
 class OutputSection {
 public:
-  OutputSection(StringRef N, uint32_t SI)
-      : Name(N), SectionIndex(SI), Header({}) {}
+  OutputSection(StringRef N) : Name(N), Header({}) {}
   void setRVA(uint64_t);
   void setFileOffset(uint64_t);
   void addChunk(Chunk *C);
   StringRef getName() { return Name; }
-  uint64_t getSectionIndex() { return SectionIndex; }
   std::vector<Chunk *> &getChunks() { return Chunks; }
   void addPermissions(uint32_t C);
   uint32_t getPermissions() { return Header.Characteristics & PermMask; }
   uint32_t getCharacteristics() { return Header.Characteristics; }
   uint64_t getRVA() { return Header.VirtualAddress; }
   uint64_t getFileOff() { return Header.PointerToRawData; }
   void writeHeaderTo(uint8_t *Buf);
 
   // Returns the size of this section in an executable memory image.
   // This may be smaller than the raw size (the raw size is multiple
   // of disk sector size, so there may be padding at end), or may be
   // larger (if that's the case, the loader reserves spaces after end
   // of raw data).
   uint64_t getVirtualSize() { return Header.VirtualSize; }
 
   // Returns the size of the section in the output file.
   uint64_t getRawSize() { return Header.SizeOfRawData; }
 
   // Set offset into the string table storing this section name.
   // Used only when the name is longer than 8 bytes.
   void setStringTableOff(uint32_t V) { StringTableOff = V; }
 
+  // N.B. The section index is one based.
+  uint32_t SectionIndex = 0;
+
 private:
   StringRef Name;
-  uint32_t SectionIndex;
   coff_section Header;
   uint32_t StringTableOff = 0;
   std::vector<Chunk *> Chunks;
 };
 
 // The writer writes a SymbolTable result to a file.
 class Writer {
 public:
   explicit Writer(SymbolTable *T);
   std::error_code write(StringRef Path);
 
 private:
   void markLive();
   void dedupCOMDATs();
   void createSections();
   void createMiscChunks();
   void createImportTables();
   void createExportTable();
   void assignAddresses();
   void removeEmptySections();
+  void createSymbolAndStringTable();
+  size_t addEntryToStringTable(StringRef Str);
   std::error_code openFile(StringRef OutputPath);
   template <typename PEHeaderTy> void writeHeader();
   void writeSections();
   void sortExceptionTable();
   void applyRelocations();
 
   OutputSection *findSection(StringRef Name);
   OutputSection *createSection(StringRef Name);
   void addBaserels(OutputSection *Dest);
   void addBaserelBlocks(OutputSection *Dest, std::vector<uint32_t> &V);
 
   uint32_t getSizeOfInitializedData();
   std::map<StringRef, std::vector<DefinedImportData *>> binImports();
 
   SymbolTable *Symtab;
   std::unique_ptr<llvm::FileOutputBuffer> Buffer;
   llvm::SpecificBumpPtrAllocator<OutputSection> CAlloc;
   llvm::SpecificBumpPtrAllocator<BaserelChunk> BAlloc;
   std::vector<OutputSection *> OutputSections;
+  std::vector<char> Strtab;
+  std::vector<llvm::object::coff_symbol16> OutputSymtab;
   IdataContents Idata;
   DelayLoadContents DelayIdata;
   EdataContents Edata;
 
   bool Is64;
   uint64_t FileSize;
+  uint32_t PointerToSymbolTable = 0;
   uint64_t SizeOfImage;
   uint64_t SizeOfHeaders;
 
   std::vector<std::unique_ptr<Chunk>> Chunks;
 };
 
 } // namespace coff
 } // namespace lld
 
 #endif

lld/trunk/COFF/Writer.cpp

std::error_code Writer::write(StringRef OutputPath) {
   createSections();
   createMiscChunks();
   createImportTables();
   createExportTable();
   if (Config->Relocatable)
     createSection(".reloc");
   assignAddresses();
   removeEmptySections();
+  createSymbolAndStringTable();
   if (auto EC = openFile(OutputPath))
     return EC;
   if (Is64) {
     writeHeader<pe32plus_header>();
   } else {
     writeHeader<pe32_header>();
   }
   writeSections();

 // Write the section header to a given buffer.
 void OutputSection::writeHeaderTo(uint8_t *Buf) {
   auto *Hdr = reinterpret_cast<coff_section *>(Buf);
   *Hdr = Header;
   if (StringTableOff) {
     // If name is too long, write offset into the string table as a name.
     sprintf(Hdr->Name, "/%d", StringTableOff);
   } else {
-    assert(Name.size() <= COFF::NameSize);
-    strncpy(Hdr->Name, Name.data(), Name.size());
+    assert(!Config->Debug || Name.size() <= COFF::NameSize);
+    strncpy(Hdr->Name, Name.data(),
+            std::min(Name.size(), (size_t)COFF::NameSize));
   }
 }
 
 Writer::Writer(SymbolTable *T)
     : Symtab(T), Is64(Config->MachineType == IMAGE_FILE_MACHINE_AMD64) {}
 
 // Set live bit on for each reachable chunk. Unmarked (unreachable)
 // COMDAT chunks will be ignored in the next step, so that they don't
void Writer::createSections() {
   // '$' and all following characters in input section names are
   // discarded when determining output section. So, .text$foo
   // contributes to .text, for example. See PE/COFF spec 3.2.
   std::map<StringRef, OutputSection *> Sections;
   for (auto Pair : Map) {
     StringRef Name = getOutputSection(Pair.first);
     OutputSection *&Sec = Sections[Name];
     if (!Sec) {
-      size_t SectIdx = OutputSections.size();
-      Sec = new (CAlloc.Allocate()) OutputSection(Name, SectIdx);
+      Sec = new (CAlloc.Allocate()) OutputSection(Name);
       OutputSections.push_back(Sec);
     }
     std::vector<Chunk *> &Chunks = Pair.second;
     for (Chunk *C : Chunks) {
       Sec->addChunk(C);
       Sec->addPermissions(C->getPermissions());
     }
   }

 
 // The Windows loader doesn't seem to like empty sections,
 // so we remove them if any.
 void Writer::removeEmptySections() {
   auto IsEmpty = [](OutputSection *S) { return S->getVirtualSize() == 0; };
   OutputSections.erase(
       std::remove_if(OutputSections.begin(), OutputSections.end(), IsEmpty),
       OutputSections.end());
+  uint32_t Idx = 1;
+  for (OutputSection *Sec : OutputSections)
+    Sec->SectionIndex = Idx++;
+}
+
+size_t Writer::addEntryToStringTable(StringRef Str) {
+  assert(Str.size() > COFF::NameSize);
+  size_t OffsetOfEntry = Strtab.size() + 4; // +4 for the size field
+  Strtab.insert(Strtab.end(), Str.begin(), Str.end());
+  Strtab.push_back('\0');
+  return OffsetOfEntry;
+}
+
+void Writer::createSymbolAndStringTable() {
+  if (!Config->Debug)
+    return;
+  // Name field in the section table is 8 byte long. Longer names need
+  // to be written to the string table. First, construct string table.
+  for (OutputSection *Sec : OutputSections) {
+    StringRef Name = Sec->getName();
+    if (Name.size() <= COFF::NameSize)
+      continue;
+    Sec->setStringTableOff(addEntryToStringTable(Name));
+  }
+  for (ObjectFile *File : Symtab->ObjectFiles) {
+    for (SymbolBody *B : File->getSymbols()) {
+      auto *D = dyn_cast<DefinedRegular>(B);
+      if (!D || !D->isLive())
+        continue;
+      uint64_t RVA = D->getRVA();
+      OutputSection *SymSec = nullptr;
+      for (OutputSection *Sec : OutputSections) {
+        if (Sec->getRVA() > RVA)
+          break;
+        SymSec = Sec;
+      }
+      uint64_t SectionRVA = SymSec->getRVA();
+      uint64_t SymbolValue = RVA - SectionRVA;
+
+      StringRef Name = D->getName();
+      coff_symbol16 Sym;
+      if (Name.size() > COFF::NameSize) {
+        Sym.Name.Offset.Zeroes = 0;
+        Sym.Name.Offset.Offset = addEntryToStringTable(Name);
+      } else {
+        memset(Sym.Name.ShortName, 0, COFF::NameSize);
+        memcpy(Sym.Name.ShortName, Name.data(), Name.size());
+      }
+
+      Sym.Value = SymbolValue;
+      Sym.SectionNumber = SymSec->SectionIndex;
+      Sym.StorageClass = IMAGE_SYM_CLASS_NULL;
+      Sym.NumberOfAuxSymbols = 0;
+      OutputSymtab.push_back(Sym);
+    }
+  }
+  OutputSection *LastSection = OutputSections.back();
+  // We position the symbol table to be adjacent to the end of the last section.
+  uint64_t FileOff =
+      LastSection->getFileOff() +
+      RoundUpToAlignment(LastSection->getRawSize(), FileAlignment);
+  if (!OutputSymtab.empty()) {
+    PointerToSymbolTable = FileOff;
+    FileOff += OutputSymtab.size() * sizeof(coff_symbol16);
+  }
+  if (!Strtab.empty())
+    FileOff += Strtab.size() + 4;
+  FileSize = SizeOfHeaders +
+             RoundUpToAlignment(FileOff - SizeOfHeaders, FileAlignment);
 }
 
 // Visits all sections to assign incremental, non-overlapping RVAs and
 // file offsets.
 void Writer::assignAddresses() {
   SizeOfHeaders = DOSStubSize + sizeof(PEMagic) + sizeof(coff_file_header) +
                   sizeof(data_directory) * NumberfOfDataDirectory +
                   sizeof(coff_section) * OutputSections.size();
template <typename PEHeaderTy> void Writer::writeHeader() {
   }
   if (Symbol *Sym = Symtab->find("_tls_used")) {
     if (Defined *B = dyn_cast<Defined>(Sym->Body)) {
       Dir[TLS_TABLE].RelativeVirtualAddress = B->getRVA();
       Dir[TLS_TABLE].Size = 40;
     }
   }
 
-  // Section table
-  // Name field in the section table is 8 byte long. Longer names need
-  // to be written to the string table. First, construct string table.
-  std::vector<char> Strtab;
-  for (OutputSection *Sec : OutputSections) {
-    StringRef Name = Sec->getName();
-    if (Name.size() <= COFF::NameSize)
-      continue;
-    Sec->setStringTableOff(Strtab.size() + 4); // +4 for the size field
-    Strtab.insert(Strtab.end(), Name.begin(), Name.end());
-    Strtab.push_back('\0');
-  }
-
   // Write section table
   for (OutputSection *Sec : OutputSections) {
     Sec->writeHeaderTo(Buf);
     Buf += sizeof(coff_section);
   }
 
-  // Write string table if we need to. The string table immediately
-  // follows the symbol table, so we create a dummy symbol table
-  // first. The symbol table contains one dummy symbol.
-  if (Strtab.empty())
+  if (OutputSymtab.empty())
     return;
-  COFF->PointerToSymbolTable = Buf - Buffer->getBufferStart();
-  COFF->NumberOfSymbols = 1;
-  auto *SymbolTable = reinterpret_cast<coff_symbol16 *>(Buf);
-  Buf += sizeof(*SymbolTable);
-  // (Set 4 to make the dummy symbol point to the first string table
-  // entry, so that tools to print out symbols don't read NUL bytes.)
-  SymbolTable->Name.Offset.Offset = 4;
-  // Then create the symbol table. The first 4 bytes is length
-  // including itself.
+
+  COFF->PointerToSymbolTable = PointerToSymbolTable;
+  uint32_t NumberOfSymbols = OutputSymtab.size();
+  COFF->NumberOfSymbols = NumberOfSymbols;
+  auto *SymbolTable = reinterpret_cast<coff_symbol16 *>(
+      Buffer->getBufferStart() + COFF->PointerToSymbolTable);
+  for (size_t I = 0; I != NumberOfSymbols; ++I)
+    SymbolTable[I] = OutputSymtab[I];
+  // Create the string table, it follows immediately after the symbol table.
+  // The first 4 bytes is length including itself.
+  Buf = reinterpret_cast<uint8_t *>(&SymbolTable[NumberOfSymbols]);
   write32le(Buf, Strtab.size() + 4);
   memcpy(Buf + 4, Strtab.data(), Strtab.size());
 }
 
 std::error_code Writer::openFile(StringRef Path) {
   if (auto EC = FileOutputBuffer::create(Path, FileSize, Buffer,
                                          FileOutputBuffer::F_executable)) {
     llvm::errs() << "failed to open " << Path << ": " << EC.message() << "\n";
uint32_t Perms = StringSwitch<uint32_t>(Name)
                        .Case(".edata", DATA | R)
                        .Case(".idata", DATA | R)
                        .Case(".rdata", DATA | R)
                        .Case(".reloc", DATA | DISCARDABLE | R)
                        .Case(".text", CODE | R | X)
                        .Default(0);
   if (!Perms)
     llvm_unreachable("unknown section name");
-  size_t SectIdx = OutputSections.size();
-  auto Sec = new (CAlloc.Allocate()) OutputSection(Name, SectIdx);
+  auto Sec = new (CAlloc.Allocate()) OutputSection(Name);
   Sec->addPermissions(Perms);
   OutputSections.push_back(Sec);
   return Sec;
 }
 
 // Dest is .reloc section. Add contents to that section.
 void Writer::addBaserels(OutputSection *Dest) {
   std::vector<uint32_t> V;

lld/trunk/test/COFF/long-section-name.test

 # RUN: yaml2obj < %s > %t.obj
-# RUN: lld -flavor link2 /out:%t.exe /entry:main %t.obj
+# RUN: lld -flavor link2 /debug /out:%t.exe /entry:main %t.obj
 # RUN: llvm-readobj -sections %t.exe | FileCheck %s
 
 # CHECK: Name: .data_long_section_name
 # CHECK: Name: .text_long_section_name
 
 ---
 header:
   Machine:         IMAGE_FILE_MACHINE_AMD64

lld/trunk/test/COFF/symtab.test

+# RUN: yaml2obj < %s > %t.obj
+# RUN: lld -flavor link2 /debug /out:%t.exe /entry:main %t.obj
+# RUN: llvm-readobj -symbols %t.exe | FileCheck %s
+
+# CHECK:      Symbols [
+# CHECK-NEXT:   Symbol {
+# CHECK-NEXT:     Name: .text
+# CHECK-NEXT:     Value: 0
+# CHECK-NEXT:     Section: .text
+# CHECK-NEXT:     BaseType: Null (0x0)
+# CHECK-NEXT:     ComplexType: Null (0x0)
+# CHECK-NEXT:     StorageClass: Null (0x0)
+# CHECK-NEXT:     AuxSymbolCount: 0
+# CHECK-NEXT:   }
+# CHECK-NEXT:   Symbol {
+# CHECK-NEXT:     Name: f
+# CHECK-NEXT:     Value: 2
+# CHECK-NEXT:     Section: .text
+# CHECK-NEXT:     BaseType: Null (0x0)
+# CHECK-NEXT:     ComplexType: Null (0x0)
+# CHECK-NEXT:     StorageClass: Null (0x0)
+# CHECK-NEXT:     AuxSymbolCount: 0
+# CHECK-NEXT:   }
+# CHECK-NEXT:   Symbol {
+# CHECK-NEXT:     Name: g
+# CHECK-NEXT:     Value: 4
+# CHECK-NEXT:     Section: .text
+# CHECK-NEXT:     BaseType: Null (0x0)
+# CHECK-NEXT:     ComplexType: Null (0x0)
+# CHECK-NEXT:     StorageClass: Null (0x0)
+# CHECK-NEXT:     AuxSymbolCount: 0
+# CHECK-NEXT:   }
+# CHECK-NEXT:   Symbol {
+# CHECK-NEXT:     Name: main
+# CHECK-NEXT:     Value: 0
+# CHECK-NEXT:     Section: .text
+# CHECK-NEXT:     BaseType: Null (0x0)
+# CHECK-NEXT:     ComplexType: Null (0x0)
+# CHECK-NEXT:     StorageClass: Null (0x0)
+# CHECK-NEXT:     AuxSymbolCount: 0
+# CHECK-NEXT:   }
+# CHECK-NEXT: ]
+
+---
+header:
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [  ]
+sections:
+  - Name:            .text
+    Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
+    Alignment:       4
+    SectionData:     B82A00
+symbols:
+  - Name:            .text
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+    SectionDefinition:
+      Length:          6
+      NumberOfRelocations: 0
+      NumberOfLinenumbers: 0
+      CheckSum:        0
+      Number:          0
+  - Name:            f
+    Value:           2
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+  - Name:            g
+    Value:           4
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+  - Name:            main
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+...