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

144–145

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

COFF/Writer.cpp

295

Return early by inverting the condition.

308

I prefer

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

to reduce indentation level.

324–327

Make this a separate member function.

335–336

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

520

Return early.

532

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

COFF/Writer.h

64

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

67

Add a blank line before this line.

[majnemer]

• Address Rui's comments.

[majnemer]

COFF/Writer.cpp

324–327

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

335–336

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

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');

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())

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();

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, uint32_t SI) : 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();
   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

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

 
 // 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++;
+}
+
+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(Strtab.size() + 4); // +4 for the size field
+    Strtab.insert(Strtab.end(), Name.begin(), Name.end());
+    Strtab.push_back('\0');
+  }
+  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 = Strtab.size() + 4; // +4 for the size field
+        Strtab.insert(Strtab.end(), Name.begin(), Name.end());
+        Strtab.push_back('\0');
+      } 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";
     return EC;
   }
   return std::error_code();
 }
 
 // Write section contents to a mmap'ed file.
 void Writer::writeSections() {
   uint8_t *Buf = Buffer->getBufferStart();
   for (OutputSection *Sec : OutputSections) {
     // Fill gaps between functions in .text with INT3 instructions
     // instead of leaving as NUL bytes (which can be interpreted as
     // ADD instructions).
     if (Sec->getPermissions() & IMAGE_SCN_CNT_CODE)
       memset(Buf + Sec->getFileOff(), 0xCC, Sec->getRawSize());

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

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
+...