lld-link: Write an empty "repro" debug directory entry if /Brepro is passed

[thakis]

Summary

If the coff timestamp is set to a hash, like lld-link does if /Brepro is passed, the coff spec suggests that a IMAGE_DEBUG_TYPE_REPRO entry is in the debug directory. This lets lld-link write such a section. See bug for details.

link.exe creates a zero-length repro debug directory entry with /debug /Brepro/ Without debug, it puts a longer hash in the repro section. This here lets lld-link always write a zero-length repro debug directory entry.

If the repro entry is the only debug directory entry, it's now no longer guaranteed that DebugDirectoryStart + DataEntry->Size is within a section, so also tweak initDebugDirectoryPtr() to look like initBaseRelocPtr(). Also fix llvm-readobj to not error out when dumping the contents of a zero-length debug directory entry. Both these llvm changes are needed to get llvm-readobj working for the new lld test in this patch.

Fixes PR38429.

[rnk]

lld/COFF/Writer.cpp

114–115 ↗

(On Diff #163886)

format?

llvm/lib/Object/COFFObjectFile.cpp

640–641 ↗

(On Diff #163886)

This seems like it loses the error check that the debug directory is in bounds. Is there a better idiom for ensuring that a range is inside the object?

[pcc]

link.exe creates a zero-length repro debug directory entry with /debug /Brepro

Are you sure? That's not what I observed.

/mnt/c/src/tmp$ cl.exe /c empty.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 19.15.26726 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

empty.cpp
/mnt/c/src/tmp$ link.exe /Brepro empty.obj /noentry /dll
Microsoft (R) Incremental Linker Version 14.15.26726.0
Copyright (C) Microsoft Corporation.  All rights reserved.

/mnt/c/src/tmp$ dumpbin.exe /all empty.dll | grep repro
    30663452 repro         24 00001064      264    9C 15 A8 DA BA 36 1C DE 3B E0 CE 53 07 D4 02 0A E3 D6 16 41 E5 7E A7 34 E9 17 82 13 52 34 66 30
/mnt/c/src/tmp$ link.exe /Brepro empty.obj /noentry /dll /debug
Microsoft (R) Incremental Linker Version 14.15.26726.0
Copyright (C) Microsoft Corporation.  All rights reserved.

/mnt/c/src/tmp$ dumpbin.exe /all empty.dll | grep repro
    BFF6ACB7 repro         24 000011A8      3A8    A3 53 C2 0E CD F8 2F 36 62 4C 68 4C 7E CB CB E2 29 8D E0 58 3D 59 59 79 B3 F1 AD 30 B7 AC F6 BF

[thakis]

pcc: what if you do cl /Zi /Brepro foo.c?

[pcc]

Same result.

/mnt/c/src/tmp$ cl.exe /Zi /Brepro main.c
Microsoft (R) C/C++ Optimizing Compiler Version 19.15.26726 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

main.c
Microsoft (R) Incremental Linker Version 14.15.26726.0
Copyright (C) Microsoft Corporation.  All rights reserved.

/out:main.exe
/debug
/Brepro
main.obj
/mnt/c/src/tmp$ dumpbin.exe /all main.exe | grep repro
    CD094136 repro         24 000774DC    75CDC    01 77 A3 B7 4A AF D1 37 EB FA 80 9D CA A0 A0 85 13 D5 7D F8 47 FD 31 11 81 74 B5 6B 36 41 09 CD
/mnt/c/src/tmp$ cat main.c
int main() {}

Also with separate compilation and linking:

/mnt/c/src/tmp$ cl.exe /Zi /Brepro empty.cpp /c
Microsoft (R) C/C++ Optimizing Compiler Version 19.15.26726 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

empty.cpp
/mnt/c/src/tmp$ link.exe /Brepro empty.obj /noentry /dll
Microsoft (R) Incremental Linker Version 14.15.26726.0
Copyright (C) Microsoft Corporation.  All rights reserved.

/mnt/c/src/tmp$ dumpbin.exe /all empty.dll | grep repro
    30663452 repro         24 00001064      264    9C 15 A8 DA BA 36 1C DE 3B E0 CE 53 07 D4 02 0A E3 D6 16 41 E5 7E A7 34 E9 17 82 13 52 34 66 30
/mnt/c/src/tmp$ link.exe /Brepro empty.obj /noentry /dll /debug
Microsoft (R) Incremental Linker Version 14.15.26726.0
Copyright (C) Microsoft Corporation.  All rights reserved.

/mnt/c/src/tmp$ dumpbin.exe /all empty.dll | grep repro
    C5E2911E repro         24 000011A8      3A8    AD 6F 0D 48 73 0A 9F 93 1D F7 42 04 88 11 EC 84 88 C7 06 59 BE D2 37 5A 55 B9 DE 09 1E 91 E2 C5

/mnt/c/src/tmp$ touch empty.c
/mnt/c/src/tmp$ cl.exe /Zi /Brepro empty.c /c
Microsoft (R) C/C++ Optimizing Compiler Version 19.15.26726 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

empty.c
/mnt/c/src/tmp$ link.exe /Brepro empty.obj /noentry /dll
Microsoft (R) Incremental Linker Version 14.15.26726.0
Copyright (C) Microsoft Corporation.  All rights reserved.

/mnt/c/src/tmp$ dumpbin.exe /all empty.dll | grep repro
    1B739ED0 repro         24 00001064      264    B9 0B 63 16 2D E7 5B D7 F6 93 F0 91 BA A6 A6 C6 C2 FF B2 D3 1A D1 A6 AB 2C 2C 47 67 D0 9E 73 1B
/mnt/c/src/tmp$ link.exe /Brepro empty.obj /noentry /dll /debug
Microsoft (R) Incremental Linker Version 14.15.26726.0
Copyright (C) Microsoft Corporation.  All rights reserved.

/mnt/c/src/tmp$ dumpbin.exe /all empty.dll | grep repro
    B3F770F9 repro         24 000011A8      3A8    F7 04 47 23 67 3D ED E0 F7 D5 A8 9C 24 8C FD 50 DE BD FD 4C 29 65 B3 5B 81 38 CB 67 F9 70 F7 B3

[thakis]

MSVC on my laptop has the same behavior as on my desktop:

C:\src\chrome\src>type test.c
int main() {}

C:\src\chrome\src>cl /Brepro test.c /Zi
Microsoft (R) C/C++ Optimizing Compiler Version 19.14.26428.1 for x86
Copyright (C) Microsoft Corporation.  All rights reserved.

test.c
Microsoft (R) Incremental Linker Version 14.14.26428.1
Copyright (C) Microsoft Corporation.  All rights reserved.

/out:test.exe
/Brepro
/debug
test.obj

C:\src\chrome\src>dumpbin /headers test.exe | findstr repro
    8441EC31 repro          0 00000000        0

[pcc]

Interesting, maybe it was a bug in 14.14.26428.1 that was fixed in 14.15.26726.0?

I'll find out once I've installed the 14.14 toolset.

[pcc]

I can reproduce your result with the 14.14 toolset:

/mnt/c/src/tmp$ cl.exe /Zi /Brepro main.c
Microsoft (R) C/C++ Optimizing Compiler Version 19.14.26433.3 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

main.c
Microsoft (R) Incremental Linker Version 14.14.26433.3
Copyright (C) Microsoft Corporation.  All rights reserved.

/out:main.exe
/debug
/Brepro
main.obj
/mnt/c/src/tmp$ dumpbin.exe /all main.exe | grep repro
    BADAC48C repro          0 00000000        0

So it looks like it was just a bug that was fixed.

[thakis]

Quoting from the PE spec (see bug for more): "The raw data of this debug entry may be empty". So I don't think it was a bug. Maybe tools had problems with the zero-sized entries, but they didn't bother to update the spec. So I'd suggest to go with this and only add more code if needed (?). We could add a size and just add the 4-byte hash to TimeDateStamps to fill it in at the end, but while it's easy to do it feels a bit cargo-culty.

[pcc]

I wasn't imagining that we'd write 32 bits of hash to the repro directory but rather the 64-bit xxhash that we currently truncate to 32 bits to get it to fit into the timestamp field. Without that, isn't there a high probability of collisions after 65536 files (which could be small depending on the project)?

[pcc]

Admittedly though there isn't a regression and as you point out this output is compliant with the COFF spec so I'd be fine with this too.

[thakis]

In D51652#1224017, @pcc wrote:

I wasn't imagining that we'd write 32 bits of hash to the repro directory but rather the 64-bit xxhash that we currently truncate to 32 bits to get it to fit into the timestamp field. Without that, isn't there a high probability of collisions after 65536 files (which could be small depending on the project)?

I don't know what tools use the timestamp for. The two main uses I know about are:

1. I think it's used for dll binding

2. It's part of the key under which executables get uploaded to symbol servers (the key is executable name, timestamp, size: https://cs.chromium.org/chromium/src/tools/clang/scripts/package.py?type=cs&q=img_fingerprint&sq=package:chromium&g=0&l=119).

Neither of these use the repro header as far as I can tell, and for compatibility reasons it's probably difficult to change them to make them use it. Having a bit of metadata that says "the timestamp isn't actually a time" makes sense, and that's what this patch does. I can see the argument for putting more bytes there, but I don't know of any practical advantages of doing it. Maybe I can put a FIXME about doing that?

(One thing that _is_ weird is that if you do cl /Zi /Brepro foo.cc (where link writes a 0-sized timestamp in the older VS version) link writes both the zero-sized repro entry but also sets the timestamp to the current time.)

llvm/lib/Object/COFFObjectFile.cpp

640–641 ↗

(On Diff #163886)

This is a valid point, but the check that was here just checked that the first and the last object were both contained in some section. If the start was in one section and the end in another, the range would still be invalid and pass the check. What we really want here I think is to retrieve the section for the start and then check that it's large enough to also contain the end in the same section. (Same in initBaseRelocPtr.) That seems maybe out of scope for this patch? I added a FIXME.

[rnk]

lgtm

I think @pcc's concerns are addressed, too.

llvm/lib/Object/COFFObjectFile.cpp

640–641 ↗

(On Diff #163886)

Sounds good. Designing safe APIs is hard. =/

[pcc]

It's part of the key under which executables get uploaded to symbol servers (the key is executable name, timestamp, size: https://cs.chromium.org/chromium/src/tools/clang/scripts/package.py?type=cs&q=img_fingerprint&sq=package:chromium&g=0&l=119).

Okay. For some reason I thought that symbol servers would use the repro debug directory, but maybe not.

lld/trunk/COFF/Writer.cpp

 static_assert(DOSStubSize % 8 == 0, "DOSStub size must be multiple of 8");
 
 static const int NumberfOfDataDirectory = 16;
 
 namespace {
 
 class DebugDirectoryChunk : public Chunk {
 public:
-  DebugDirectoryChunk(const std::vector<Chunk *> &R) : Records(R) {}
+  DebugDirectoryChunk(const std::vector<Chunk *> &R, bool WriteRepro)
+      : Records(R), WriteRepro(WriteRepro) {}
 
   size_t getSize() const override {
-    return Records.size() * sizeof(debug_directory);
+    return (Records.size() + int(WriteRepro)) * sizeof(debug_directory);
   }
 
   void writeTo(uint8_t *B) const override {
     auto *D = reinterpret_cast<debug_directory *>(B + OutputSectionOff);
 
     for (const Chunk *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();
       OutputSection *OS = Record->getOutputSection();
       uint64_t Offs = OS->getFileOff() + (Record->getRVA() - OS->getRVA());
-      D->PointerToRawData = Offs;
-
-      TimeDateStamps.push_back(&D->TimeDateStamp);
+      fillEntry(D, COFF::IMAGE_DEBUG_TYPE_CODEVIEW, Record->getSize(),
+                Record->getRVA(), Offs);
       ++D;
     }
+
+    if (WriteRepro) {
+      // FIXME: The COFF spec allows either a 0-sized entry to just say
+      // "the timestamp field is really a hash", or a 4-byte size field
+      // followed by that many bytes containing a longer hash (with the
+      // lowest 4 bytes usually being the timestamp in little-endian order).
+      // Consider storing the full 8 bytes computed by xxHash64 here.
+      fillEntry(D, COFF::IMAGE_DEBUG_TYPE_REPRO, 0, 0, 0);
+    }
   }
 
   void setTimeDateStamp(uint32_t TimeDateStamp) {
     for (support::ulittle32_t *TDS : TimeDateStamps)
       *TDS = TimeDateStamp;
   }
 
 private:
+  void fillEntry(debug_directory *D, COFF::DebugType DebugType, size_t Size,
+                 uint64_t RVA, uint64_t Offs) const {
+    D->Characteristics = 0;
+    D->TimeDateStamp = 0;
+    D->MajorVersion = 0;
+    D->MinorVersion = 0;
+    D->Type = DebugType;
+    D->SizeOfData = Size;
+    D->AddressOfRawData = RVA;
+    D->PointerToRawData = Offs;
+
+    TimeDateStamps.push_back(&D->TimeDateStamp);
+  }
+
   mutable std::vector<support::ulittle32_t *> TimeDateStamps;
   const std::vector<Chunk *> &Records;
+  bool WriteRepro;
 };
 
 class CVDebugRecordChunk : public Chunk {
 public:
   size_t getSize() const override {
     return sizeof(codeview::DebugInfo) + Config->PDBAltPath.size() + 1;
   }
 
void Writer::createMiscChunks() {
 
   // Create thunks for locally-dllimported symbols.
   if (!Symtab->LocalImportChunks.empty()) {
     for (Chunk *C : Symtab->LocalImportChunks)
       RdataSec->addChunk(C);
   }
 
   // Create Debug Information Chunks
-  if (Config->Debug) {
-    DebugDirectory = make<DebugDirectoryChunk>(DebugRecords);
-
-    OutputSection *DebugInfoSec = Config->MinGW ? BuildidSec : RdataSec;
+  if (Config->Debug || Config->Repro) {
+    DebugDirectory = make<DebugDirectoryChunk>(DebugRecords, Config->Repro);
+    DebugInfoSec->addChunk(DebugDirectory);
+  }
 
+  if (Config->Debug) {
     // Make a CVDebugRecordChunk even when /DEBUG:CV is not specified.  We
     // output a PDB no matter what, and this chunk provides the only means of
     // allowing a debugger to match a PDB and an executable.  So we need it even
     // if we're ultimately not going to write CodeView data to the PDB.
     auto *CVChunk = make<CVDebugRecordChunk>();
     BuildId = CVChunk;
     DebugRecords.push_back(CVChunk);
 
-    DebugInfoSec->addChunk(DebugDirectory);
     for (Chunk *C : DebugRecords)
       DebugInfoSec->addChunk(C);
   }
 
   // Create SEH table. x86-only.
   if (Config->Machine == I386)
     createSEHTable();
 
template <typename PEHeaderTy> void Writer::writeHeader() {
   if (Symbol *Sym = Symtab->findUnderscore("_tls_used")) {
     if (Defined *B = dyn_cast<Defined>(Sym)) {
       Dir[TLS_TABLE].RelativeVirtualAddress = B->getRVA();
       Dir[TLS_TABLE].Size = Config->is64()
                                 ? sizeof(object::coff_tls_directory64)
                                 : sizeof(object::coff_tls_directory32);
     }
   }
-  if (Config->Debug) {
+  if (DebugDirectory) {
     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)) {
       SectionChunk *SC = B->getChunk();
       assert(B->getRVA() >= SC->getRVA());
       uint64_t OffsetInChunk = B->getRVA() - SC->getRVA();

lld/trunk/test/COFF/rsds.test

 # RUN: yaml2obj %s > %t.obj
 
 # RUN: rm -f %t.dll %t.pdb
 # RUN: lld-link /debug /pdbaltpath:test1.pdb /dll /out:%t.dll /entry:DllMain %t.obj
 # RUN: llvm-readobj -coff-debug-directory %t.dll > %t.1.txt
 # RUN: lld-link /debug /pdbaltpath:test2.pdb /dll /out:%t.dll /entry:DllMain %t.obj
 # RUN: llvm-readobj -coff-debug-directory %t.dll > %t.2.txt
 # RUN: cat %t.1.txt %t.2.txt | FileCheck %s
 
 # RUN: rm -f %t.dll %t.pdb
 # RUN: lld-link /debug /pdb:%t1.pdb /dll /out:%t.dll /entry:DllMain %t.obj
 # RUN: llvm-readobj -coff-debug-directory %t.dll > %t.3.txt
 # RUN: lld-link /debug /pdb:%t2.pdb /dll /out:%t.dll /entry:DllMain %t.obj
 # RUN: llvm-readobj -coff-debug-directory %t.dll > %t.4.txt
 # RUN: cat %t.3.txt %t.4.txt | FileCheck %s
 
+# RUN: rm -f %t.dll %t.pdb
+# RUN: lld-link /Brepro /dll /out:%t.dll /entry:DllMain %t.obj
+# RUN: llvm-readobj -coff-debug-directory %t.dll | FileCheck --check-prefix REPRO %s
+
+# RUN: rm -f %t.dll %t.pdb
+# RUN: lld-link /Brepro /debug /dll /out:%t.dll /entry:DllMain %t.obj
+# RUN: llvm-readobj -coff-debug-directory %t.dll | FileCheck --check-prefix REPRODEBUG %s
+
 # CHECK: File: [[FILE:.*]].dll
 # CHECK: DebugDirectory [
 # CHECK:   DebugEntry {
 # CHECK:     Characteristics: 0x0
 # CHECK:     TimeDateStamp: 
 # CHECK:     MajorVersion: 0x0
 # CHECK:     MinorVersion: 0x0
 # CHECK:     Type: CodeView (0x2)

 # CHECK:       PDBSignature: 0x53445352
 # CHECK:       PDBGUID: [[GUID]]
 # CHECK:       PDBAge: 2
 # CHECK:       PDBFileName: {{.*}}2.pdb
 # CHECK:     }
 # CHECK:   }
 # CHECK: ]
 
+# REPRO: File: {{.*}}.dll
+# REPRO: DebugDirectory [
+# REPRO:   DebugEntry {
+# REPRO:     Characteristics: 0x0
+# REPRO:     TimeDateStamp: 
+# REPRO:     MajorVersion: 0x0
+# REPRO:     MinorVersion: 0x0
+# REPRO:     Type: Repro (0x10)
+# REPRO:     SizeOfData: 0x0
+# REPRO:     AddressOfRawData: 0x0
+# REPRO:     PointerToRawData: 0x0
+# REPRO:   }
+# REPRO: ]
+
+# REPRODEBUG: File: {{.*}}.dll
+# REPRODEBUG: DebugDirectory [
+# REPRODEBUG:   DebugEntry {
+# REPRODEBUG:     Characteristics: 0x0
+# REPRODEBUG:     TimeDateStamp: 
+# REPRODEBUG:     MajorVersion: 0x0
+# REPRODEBUG:     MinorVersion: 0x0
+# REPRODEBUG:     Type: CodeView (0x2)
+# REPRODEBUG:     SizeOfData: 0x{{[^0]}}
+# REPRODEBUG:     AddressOfRawData: 0x{{[^0]}}
+# REPRODEBUG:     PointerToRawData: 0x{{[^0]}}
+# REPRODEBUG:     PDBInfo {
+# REPRODEBUG:       PDBSignature: 0x53445352
+# REPRODEBUG:       PDBGUID: 
+# REPRODEBUG:       PDBAge: 1
+# REPRODEBUG:       PDBFileName:
+# REPRODEBUG:     }
+# REPRODEBUG:   }
+# REPRODEBUG:   DebugEntry {
+# REPRODEBUG:     Characteristics: 0x0
+# REPRODEBUG:     TimeDateStamp: 
+# REPRODEBUG:     MajorVersion: 0x0
+# REPRODEBUG:     MinorVersion: 0x0
+# REPRODEBUG:     Type: Repro (0x10)
+# REPRODEBUG:     SizeOfData: 0x0
+# REPRODEBUG:     AddressOfRawData: 0x0
+# REPRODEBUG:     PointerToRawData: 0x0
+# REPRODEBUG:   }
+# REPRODEBUG: ]
 --- !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