COFF: add beginnings of debug directory creation

[compnerd]

Summary

The IMAGE_FILE_HEADER structure contains a (RVA, size) to an array of
COFF_DEBUG_DIRECTORY records. Each one of these records contains an RVA to a
OMF Debug Directory. These OMF debug directories are derived into newer types
such as PDB70, PDB20, etc. This constructs a PDB70 structure which will allow
us to associate a GUID with a build to actually tie debug information.

[compnerd]

This is obviously a very preliminary patch. I wanted to get some consensus on how to handle the GUID associated with the binary and an overall structure (whether to merge the content into a different file or keep it separate).

[ruiu]

COFF/Debug.cpp

41 ↗

(On Diff #67936)

Move.

79 ↗

(On Diff #67936)

chunks -> Chunks

COFF/Debug.h

21 ↗

(On Diff #67936)

This class is a container of chunks, but only one element is currently added to this vector, so we don't actually need this class (you could add a member of type std::unique_ptr<CVDebugRecordChunk> to the Writer instead of this class.) Are you planning to do more complex thing with this class? If not, I'd remove this class.

21–22 ↗

(On Diff #67936)

Move this after all public members for consistency with other class declarations.

COFF/Writer.cpp

291

I'd name this Rdata.

[compnerd]

COFF/Debug.h

21 ↗

(On Diff #67936)

Well, it should add a coffgrp record by default unless CV is present. Furthermore, we should at some point emit the features record as well. Ideally, at some point, this would also integrate into the PDB handling to retrieve the GUID and Age so that rather than re-generating it on every build, we could just increment the age.

I don't think that all of this will happen immediately, but is something that we will need, so I was tempted to create the class. I don't think that Im really tied to that idea and i you think going with the simpler route for now is better, I can merge this into the writer.

[ruiu]

Then I'd merge them to the write to keep it small.

[compnerd]

Refactor the addition based on @ruiu's comments. This is still missing a test, but should be functional for stubbing out the section otherwise. We should still figure out the best way to generate the GUID for the RSDS record.

[compnerd]

This adds a test as well. Right now the only thing which is missing is the GUID generation. Its unclear whether we want to use the random generation on each build (bad for reproducible builds) or hash the contents. Im not sure how to best do the latter. But, I think beyond that point, this is ready.

[ruiu]

LGTM with a few nits.

COFF/Writer.cpp

58

auto -> std::unique_ptr<Chunk>

74

Why don't you use ArrayRef?

[compnerd]

SVN r280012 with the NIT addressed.

COFF/Config.h

   bool Debug = false;
   bool WriteSymtab = true;
   unsigned DebugTypes = static_cast<unsigned>(DebugType::None);
+  StringRef PDBPath;
 
   // Symbols in this set are considered as live by the garbage collector.
   std::set<Undefined *> GCRoot;

COFF/Driver.cpp

             : getDefaultDebugType(Args);
   }
 
+  // Create a dummy PDB file to satisfy build sytem rules.
+  if (auto *Arg = Args.getLastArg(OPT_pdb)) {
+    Config->PDBPath = Arg->getValue();
+    createPDB(Config->PDBPath);
+  }
+
   // Handle /noentry
   if (Args.hasArg(OPT_noentry)) {
     if (!Args.hasArg(OPT_dll))

   if (Config->Manifest == Configuration::SideBySide)
     createSideBySideManifest();
 
-  // Create a dummy PDB file to satisfy build sytem rules.
-  if (auto *Arg = Args.getLastArg(OPT_pdb))
-    createPDB(Arg->getValue());
-
   // Identify unreferenced COMDAT sections.
   if (Config->DoGC)
     markLive(Symtab.getChunks());

COFF/Writer.cpp

 static const int NumberfOfDataDirectory = 16;
 
 namespace {
+
+class DebugDirectoryChunk : public Chunk {
+public:
+  DebugDirectoryChunk(const std::vector<std::unique_ptr<Chunk>> &R)
+      : Records(R) {}
+
+  size_t getSize() const override {
+    return Records.size() * sizeof(debug_directory);
+  }
+
+  void writeTo(uint8_t *B) const override {
+    auto *D = reinterpret_cast<debug_directory *>(B + OutputSectionOff);
+
+    for (const auto &Record : Records) {
+      D->Characteristics = 0;
+      D->TimeDateStamp = 0;
+      D->MajorVersion = 0;
+      D->MinorVersion = 0;
+      D->Type = COFF::IMAGE_DEBUG_TYPE_CODEVIEW;
+      D->SizeOfData = Record->getSize();
+      D->AddressOfRawData = Record->getRVA();
+      // TODO(compnerd) get the file offset
+      D->PointerToRawData = 0;
+
+      ++D;
+    }
+  }
+
+private:
+  const std::vector<std::unique_ptr<Chunk>> &Records;
+};
+
+class CVDebugRecordChunk : public Chunk {
+  size_t getSize() const override {
+    return sizeof(codeview::DebugInfo) + Config->PDBPath.size() + 1;
+  }
+
+  void writeTo(uint8_t *B) const override {
+    auto *R = reinterpret_cast<codeview::DebugInfo *>(B + OutputSectionOff);
+
+    R->Signature.CVSignature = OMF::Signature::PDB70;
+    // TODO(compnerd) fill in a GUID by hashing the contents of the binary to
+    // get a reproducible build
+    memset(R->PDB70.Signature, 0, sizeof(R->PDB70.Signature));
+    // TODO(compnerd) track the Age
+    R->PDB70.Age = 1;
+
+    // variable sized field (PDB Path)
+    auto *P = reinterpret_cast<char *>(B + OutputSectionOff + sizeof(*R));
+    memcpy(P, Config->PDBPath.data(), Config->PDBPath.size());
+    P[Config->PDBPath.size()] = '\0';
+  }
+};
+
 // The writer writes a SymbolTable result to a file.
 class Writer {
 public:

   EdataContents Edata;
   std::unique_ptr<SEHTableChunk> SEHTable;
 
+  std::unique_ptr<Chunk> DebugDirectory;
+  std::vector<std::unique_ptr<Chunk>> DebugRecords;
+
   uint64_t FileSize;
   uint32_t PointerToSymbolTable = 0;
   uint64_t SizeOfImage;

       RData->addChunk(C);
   }
 
+  // Create Debug Information Chunks
+  if (Config->Debug) {
+    DebugDirectory = make_unique<DebugDirectoryChunk>(DebugRecords);
+
+    // TODO(compnerd) create a coffgrp entry if DebugType::CV is not enabled
+    if (Config->DebugTypes & static_cast<unsigned>(coff::DebugType::CV))
+      DebugRecords.push_back(make_unique<CVDebugRecordChunk>());
+
+    RData->addChunk(DebugDirectory.get());
+    for (const std::unique_ptr<Chunk> &C : DebugRecords)
+      RData->addChunk(C.get());
+  }
+
   // Create SEH table. x86-only.
   if (Config->Machine != I386)
     return;

                                 : sizeof(object::coff_tls_directory32);
     }
   }
+  if (Config->Debug) {
+    Dir[DEBUG_DIRECTORY].RelativeVirtualAddress = DebugDirectory->getRVA();
+    Dir[DEBUG_DIRECTORY].Size = DebugDirectory->getSize();
+  }
   if (Symbol *Sym = Symtab->findUnderscore("_load_config_used")) {
     if (auto *B = dyn_cast<DefinedRegular>(Sym->Body)) {
       SectionChunk *SC = B->getChunk();

test/COFF/delayimports32.test

 IMPORT-NEXT:   Attributes: 0x1
 IMPORT-NEXT:   ModuleHandle: 0x1018
 IMPORT-NEXT:   ImportAddressTable: 0x1020
-IMPORT-NEXT:   ImportNameTable: 0x3040
+IMPORT-NEXT:   ImportNameTable: 0x4040
 IMPORT-NEXT:   BoundDelayImportTable: 0x0
 IMPORT-NEXT:   UnloadDelayImportTable: 0x0
 IMPORT-NEXT:   Import {

 BASEREL-NEXT: ]
 
 DISASM:      202b:      68 20 10 40 00  pushl   $4198432
-DISASM-NEXT: 2030:      68 00 30 40 00  pushl   $4206592
+DISASM-NEXT: 2030:      68 00 40 40 00  pushl   $4210688
 DISASM-NEXT: 2035:      e8 c6 ff ff ff  calll   -58 <_main@0>
 DISASM-NEXT: 203a:      5a      popl    %edx
 DISASM-NEXT: 203b:      59      popl    %ecx

 DISASM-NEXT: 203e:      51      pushl   %ecx
 DISASM-NEXT: 203f:      52      pushl   %edx
 DISASM-NEXT: 2040:      68 24 10 40 00  pushl   $4198436
-DISASM-NEXT: 2045:      68 00 30 40 00  pushl   $4206592
+DISASM-NEXT: 2045:      68 00 40 40 00  pushl   $4210688
 DISASM-NEXT: 204a:      e8 b1 ff ff ff  calll   -79 <_main@0>
 DISASM-NEXT: 204f:      5a      popl    %edx
 DISASM-NEXT: 2050:      59      popl    %ecx

test/COFF/rsds.test

+# RUN: yaml2obj %s > %t.obj
+
+# RUN: lld-link /debug /dll /out:%t.dll /entry:DllMain %t.obj
+# RUN: llvm-readobj -coff-debug-directory %t.dll | FileCheck %s
+
+# RUN: lld-link /debug /pdb:%t.pdb /dll /out:%t.dll /entry:DllMain %t.obj
+# RUN: llvm-readobj -coff-debug-directory %t.dll | FileCheck %s -check-prefix CHECK-PDB
+
+# CHECK: DebugDirectory [
+# CHECK:   DebugEntry {
+# CHECK:     Characteristics: 0x0
+# CHECK:     TimeDateStamp: 1970-01-01 00:00:00 (0x0)
+# CHECK:     MajorVersion: 0x0
+# CHECK:     MinorVersion: 0x0
+# CHECK:     Type: CodeView (0x2)
+# CHECK:     SizeOfData: 0x19
+# CHECK:     AddressOfRawData:
+# CHECK:     PointerToRawData:
+# CHECK:     PDBInfo {
+# CHECK:       PDBSignature: 0x53445352
+# CHECK:       PDBGUID: (00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00)
+# CHECK:       PDBAge: 1
+# CHECK:       PDBFileName: {{$}}
+# CHECK:     }
+# CHECK:   }
+# CHECK: ]
+
+# CHECK-PDB: DebugDirectory [
+# CHECK-PDB:   DebugEntry {
+# CHECK-PDB:     Characteristics: 0x0
+# CHECK-PDB:     TimeDateStamp: 1970-01-01 00:00:00 (0x0)
+# CHECK-PDB:     MajorVersion: 0x0
+# CHECK-PDB:     MinorVersion: 0x0
+# CHECK-PDB:     Type: CodeView (0x2)
+# CHECK-PDB:     SizeOfData:
+# CHECK-PDB:     AddressOfRawData:
+# CHECK-PDB:     PointerToRawData:
+# CHECK-PDB:     PDBInfo {
+# CHECK-PDB:       PDBSignature: 0x53445352
+# CHECK-PDB:       PDBGUID: (00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00)
+# CHECK-PDB:       PDBAge: 1
+# CHECK-PDB:       PDBFileName: {{.*}}.pdb
+# CHECK-PDB:     }
+# CHECK-PDB:   }
+# CHECK-PDB: ]
+
+--- !COFF
+header:
+  Machine:         IMAGE_FILE_MACHINE_I386
+  Characteristics: [  ]
+sections:
+  - Name:            .text
+    Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
+    Alignment:       4
+    SectionData:     31C0C3
+  - Name:            .data
+    Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
+    Alignment:       4
+    SectionData:     ''
+  - Name:            .bss
+    Characteristics: [ IMAGE_SCN_CNT_UNINITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
+    Alignment:       4
+    SectionData:     ''
+symbols:
+  - Name:            .text
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+    SectionDefinition:
+      Length:          3
+      NumberOfRelocations: 0
+      NumberOfLinenumbers: 0
+      CheckSum:        3963538403
+      Number:          1
+  - Name:            .data
+    Value:           0
+    SectionNumber:   2
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+    SectionDefinition:
+      Length:          0
+      NumberOfRelocations: 0
+      NumberOfLinenumbers: 0
+      CheckSum:        0
+      Number:          2
+  - Name:            .bss
+    Value:           0
+    SectionNumber:   3
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+    SectionDefinition:
+      Length:          0
+      NumberOfRelocations: 0
+      NumberOfLinenumbers: 0
+      CheckSum:        0
+      Number:          3
+  - Name:            '@feat.00'
+    Value:           1
+    SectionNumber:   -1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+  - Name:            _DllMain
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_FUNCTION
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+...

test/COFF/symtab.test

 # CHECK-NEXT:   Symbol {
 # CHECK-NEXT:     Name: __imp_ExitProcess
 # CHECK-NEXT:     Value: 64
-# CHECK-NEXT:     Section: .idata (4)
+# CHECK-NEXT:     Section: .idata (5)
 # CHECK-NEXT:     BaseType: Null (0x0)
 # CHECK-NEXT:     ComplexType: Null (0x0)
 # CHECK-NEXT:     StorageClass: External (0x2)

 # CHECK-NEXT:   Symbol {
 # CHECK-NEXT:     Name: __imp_MessageBoxA
 # CHECK-NEXT:     Value: 72
-# CHECK-NEXT:     Section: .idata (4)
+# CHECK-NEXT:     Section: .idata (5)
 # CHECK-NEXT:     BaseType: Null (0x0)
 # CHECK-NEXT:     ComplexType: Null (0x0)
 # CHECK-NEXT:     StorageClass: External (0x2)