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

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

COFF/Writer.cpp

288

Return early by inverting the condition.

301

I prefer

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

to reduce indentation level.

317–320

Make this a separate member function.

328–329

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

492

Return early.

504

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

COFF/Writer.h

64

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

65

Add a blank line before this line.

[majnemer]

• Address Rui's comments.

[majnemer]

COFF/Writer.cpp

317–320

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

328–329

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

317–320

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

for (const coff_relocation &Rel : Relocs) {
     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()); 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::addAssociative(SectionChunk *Child) {

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"))
+    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; }
 
+  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) : Symtab(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);
   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;
 
   uint64_t FileSize;
   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;
   writeHeader();
   writeSections();
   sortExceptionTable();
   return Buffer->commit();
 }
 

 // 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));
   }
 }
 
 // Set live bit on for each reachable chunk. Unmarked (unreachable)
 // COMDAT chunks will be ignored in the next step, so that they don't
 // come to the final output file.
 void Writer::markLive() {
   if (!Config->DoGC)

 
 // 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() {
+  OutputSection *LastSection = OutputSections.back();
+  uint64_t FileOff =
+      LastSection->getFileOff() +
+      RoundUpToAlignment(LastSection->getRawSize(), FileAlignment);
+  if (Config->Debug) {
+    // 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()) {
+        if (auto *D = dyn_cast<DefinedRegular>(B)) {
+          if (!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);
+        }
+      }
+    }
+    FileOff +=
+        (OutputSymtab.size() * sizeof(coff_symbol16)) + 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 = RoundUpToAlignment(
       DOSStubSize + sizeof(PEMagic) + sizeof(coff_file_header) +
       sizeof(pe32plus_header) +
       sizeof(data_directory) * NumberfOfDataDirectory +
       sizeof(coff_section) * OutputSections.size(), PageSize);
   uint64_t RVA = 0x1000; // The first page is kept unmapped.
   uint64_t FileOff = SizeOfHeaders;
   for (OutputSection *Sec : OutputSections) {
     if (Sec->getName() == ".reloc")
       addBaserels(Sec);
     Sec->setRVA(RVA);
     Sec->setFileOffset(FileOff);
     RVA += RoundUpToAlignment(Sec->getVirtualSize(), PageSize);
     FileOff += RoundUpToAlignment(Sec->getRawSize(), FileAlignment);
   }
   SizeOfImage = SizeOfHeaders + RoundUpToAlignment(RVA - 0x1000, PageSize);
-  FileSize = SizeOfHeaders +
-             RoundUpToAlignment(FileOff - SizeOfHeaders, FileAlignment);
 }
 
 static MachineTypes
 inferMachineType(const std::vector<ObjectFile *> &Files) {
   for (ObjectFile *F : Files) {
     // Try to infer machine type from the magic byte of the object file.
     auto MT = static_cast<MachineTypes>(F->getCOFFObj()->getMachine());
     if (MT != IMAGE_FILE_MACHINE_UNKNOWN)
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())
-    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
+  if (!OutputSymtab.empty()) {
+    OutputSection *LastSection = OutputSections.back();
+    COFF->PointerToSymbolTable =
+        LastSection->getFileOff() +
+        RoundUpToAlignment(LastSection->getRawSize(), FileAlignment);
+    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];
+    Buf = reinterpret_cast<uint8_t *>(&SymbolTable[NumberOfSymbols]);
+    // Then create the string table. The first 4 bytes is length
     // including itself.
     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();

test/COFF/long-section-name.test

 # RUN: yaml2obj < %s > %t.obj
-# RUN: lld -flavor link2 /out:%t.exe /entry:main %t.obj
-# RUN: llvm-readobj -sections %t.exe | FileCheck %s
+# RUN: lld -flavor link2 /debug /out:%t.exe /entry:main %t.obj
+# RUN: llvm-readobj -sections -symbols %t.exe | FileCheck %s
 
 # CHECK: Name: .data_long_section_name
 # CHECK: Name: .text_long_section_name
+# CHECK:      Symbols [
+# CHECK-NEXT:   Symbol {
+# CHECK-NEXT:     Name: .text_long_section_name
+# CHECK-NEXT:     Value: 0
+# CHECK-NEXT:     Section: .text_long_section_name (2)
+# 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: .data_long_section_name
+# CHECK-NEXT:     Value: 0
+# CHECK-NEXT:     Section: .data_long_section_name (1)
+# 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_long_section_name (2)
+# 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_long_section_name
     Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]