Write a hash of the binary as the PE Debug Directory Timestamp

[zturner]

Summary

Previously we were writing 0 for this field. This breaks symbol servers, and the value needs to be something, although it doesn't necessarily need to be a timestamp. I chose to use a hash of the binary here, calculated while the value of this field is still 0. Then, the 0 is replaced with the hash.

[ruiu]

LGTM

[smeenai]

lld/COFF/Writer.cpp

1045 ↗

(On Diff #136628)

built it? Did you mean timestamp? (Or build id, but that doesn't seem right here.)

[zturner]

It turns out I wasn't actually doing enough. The value that symbol servers *really* care about is the time stamp in the top-level coff file header. However, since link.exe sets this time stamp and the time stamp of al debug directories to matching values, we do the same here.

With my first version of the patch, symbol servers were still not working correctly, but after this version that writes the time stamp to the COFF file header, it is now working correctly.

[zturner]

Apparently this *still* isn't quite right. The scenario presented to me was this:

1. User does a clean build, hash X gets written into PDB, hash Y gets written into EXE.
2. User archives EXE and PDB on symbol store. EXE is in some directory X, PDB is in some directory Y.
3. User adds some blank lines and rebuilds. EXE doesn't change but PDB does.
4. User re-archives. EXE is unchanged so doesn't get updated on the symbol store, but PDB does. PDB now gets archived in directory Z.

So now there is only one EXE, Y, but there are two PDBs, X and Z. If you have the new source code, you will run the EXE, it will find it under directory Y in the sym store, map to the old PDB X, and the source won't match.

I think the solution to this is:

a) Hash the PDB first. Call it X, write this value into the PDB.
b) In the debug directories, write X for the time stamp.
c) Now hash the executable after you have written X in the debug directories, call this Y.
d) Write Y into the coff file header time stamp.

[smeenai]

"Hash the PDB"

I thought hashing the PDB itself was considered undesirable for speed reasons (since PDBs can get pretty large)? Or is xxHash64 fast enough that it doesn't matter?

[zturner]

In D43978#1024841, @smeenai wrote:

"Hash the PDB"

I thought hashing the PDB itself was considered undesirable for speed reasons (since PDBs can get pretty large)? Or is xxHash64 fast enough that it doesn't matter?

I haven't timed it (probably should). But, if we want reproducible builds, I dont' know of a better option.

[smeenai]

In D43978#1024843, @zturner wrote:

In D43978#1024841, @smeenai wrote:

"Hash the PDB"

I thought hashing the PDB itself was considered undesirable for speed reasons (since PDBs can get pretty large)? Or is xxHash64 fast enough that it doesn't matter?

I haven't timed it (probably should). But, if we want reproducible builds, I dont' know of a better option.

If it turns out that the hashing is really expensive, perhaps there could be an option to just use an actual timestamp or something, for people who care about link speed more than reproducibility?

Do you know how link.exe handles this? I know they're doing some sort of hashing for their timestamps, but I thought it was just based on the output binary, not its PDB.

[thakis]

In D43978#1024821, @zturner wrote:

Apparently this *still* isn't quite right. The scenario presented to me was this:

1. User does a clean build, hash X gets written into PDB, hash Y gets written into EXE.
2. User archives EXE and PDB on symbol store. EXE is in some directory X, PDB is in some directory Y.
3. User adds some blank lines and rebuilds. EXE doesn't change but PDB does.
4. User re-archives. EXE is unchanged so doesn't get updated on the symbol store, but PDB does. PDB now gets archived in directory Z.

So now there is only one EXE, Y, but there are two PDBs, X and Z. If you have the new source code, you will run the EXE, it will find it under directory Y in the sym store, map to the old PDB X, and the source won't match.

I don't understand this scenario. If the pdb changes, doesn't the RSDS header in the exe linking to the binary have to change as well? And then the hash will be updated, and all is good? https://blogs.msdn.microsoft.com/oldnewthing/20180103-00/?p=97705 sounds like link.exe really only hashes the binary.

[thakis]

In D43978#1025330, @thakis wrote:

In D43978#1024821, @zturner wrote:

Apparently this *still* isn't quite right. The scenario presented to me was this:

1. User does a clean build, hash X gets written into PDB, hash Y gets written into EXE.
2. User archives EXE and PDB on symbol store. EXE is in some directory X, PDB is in some directory Y.
3. User adds some blank lines and rebuilds. EXE doesn't change but PDB does.
4. User re-archives. EXE is unchanged so doesn't get updated on the symbol store, but PDB does. PDB now gets archived in directory Z.

So now there is only one EXE, Y, but there are two PDBs, X and Z. If you have the new source code, you will run the EXE, it will find it under directory Y in the sym store, map to the old PDB X, and the source won't match.

I don't understand this scenario. If the pdb changes, doesn't the RSDS header in the exe linking to the binary have to change as well?

Sorry, the RSDS header in the exe linking it to the _pdb_...

[zturner]

Attempt N+1 at making this right.

Now we hash the binary *after* setting the guid and age. This should address the problem of debug info changes with no binary changes, because when that happens we increment the age. By hashing *after* setting the guid and age, we ensure that even if only the debug info changes, the hash will also change.

[zturner]

Man, this is hard. There are so many weird edge cases.

Previous version of the patch would continue to write 0 for the timestamp if we weren't generating debug info. But, we really should be writing the time stamp in the coff file header even for non debug info builds. So this version does that.

I'm going to spend some time coming up with a set of tests for all of these edge cases, but throwing this up here in the meantime because I'm probably still doing something wrong.

[morehouse]

This patch is breaking a few COFF tests: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/14971/steps/check-lld%20msan/logs/stdio.

Please take a look.

[zturner]

Thanks, I'll revert until I can get a fix in.

COFF/Writer.cpp

 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/Support/BinaryStreamReader.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/Endian.h"
 #include "llvm/Support/FileOutputBuffer.h"
 #include "llvm/Support/Parallel.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/RandomNumberGenerator.h"
+#include "llvm/Support/xxhash.h"
 #include <algorithm>
 #include <cstdio>
 #include <map>
 #include <memory>
 #include <utility>
 
 using namespace llvm;
 using namespace llvm::COFF;
for (const Chunk *Record : Records) {
       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);
       ++D;
     }
   }
 
+  void setTimeDateStamp(uint32_t TimeDateStamp) {
+    for (support::ulittle32_t *TDS : TimeDateStamps)
+      *TDS = TimeDateStamp;
+  }
+
 private:
+  mutable std::vector<support::ulittle32_t *> TimeDateStamps;
   const std::vector<Chunk *> &Records;
 };
 
 class CVDebugRecordChunk : public Chunk {
 public:
   CVDebugRecordChunk() {
     PDBAbsPath = Config->PDBPath;
     if (!PDBAbsPath.empty())
private:
   std::vector<char> Strtab;
   std::vector<llvm::object::coff_symbol16> OutputSymtab;
   IdataContents Idata;
   DelayLoadContents DelayIdata;
   EdataContents Edata;
   RVATableChunk *GuardFidsTable = nullptr;
   RVATableChunk *SEHTable = nullptr;
 
-  Chunk *DebugDirectory = nullptr;
+  DebugDirectoryChunk *DebugDirectory = nullptr;
   std::vector<Chunk *> DebugRecords;
   CVDebugRecordChunk *BuildId = nullptr;
   Optional<codeview::DebugInfo> PreviousBuildId;
   ArrayRef<uint8_t> SectionTable;
 
   uint64_t FileSize;
   uint32_t PointerToSymbolTable = 0;
   uint64_t SizeOfImage;
for (OutputSection *Sec : OutputSections) {
     if (Sec->getPermissions() & IMAGE_SCN_CNT_CODE)
       memset(SecBuf, 0xCC, Sec->getRawSize());
     for_each(parallel::par, Sec->getChunks().begin(), Sec->getChunks().end(),
              [&](Chunk *C) { C->writeTo(SecBuf); });
   }
 }
 
 void Writer::writeBuildId() {
-  // If we're not writing a build id (e.g. because /debug is not specified),
-  // then just return;
-  if (!Config->Debug)
-    return;
-
+  // There are two important parts to the build ID.
+  // 1) If building with debug info, the COFF debug directory contains a
+  //    timestamp as well as a Guid and Age of the PDB.
+  // 2) In all cases, the PE COFF file header also contains a timestamp.
+  // For reproducibility, instead of a timestamp we want to use a hash of the
+  // binary, however when building with debug info the hash needs to take into
+  // account the debug info, since it's possible to add blank lines to a file
+  // which causes the debug info to change but not the generated code.
+  //
+  // To handle this, we first set the Guid and Age in the debug directory (but
+  // only if we're doing a debug build).  Then, we hash the binary (thus causing
+  // the hash to change if only the debug info changes, since the Age will be
+  // different).  Finally, we write that hash into the debug directory (if
+  // present) as well as the COFF file header (always).
+  if (Config->Debug) {
     assert(BuildId && "BuildId is not set!");
-
     if (PreviousBuildId.hasValue()) {
       *BuildId->BuildId = *PreviousBuildId;
       BuildId->BuildId->PDB70.Age = BuildId->BuildId->PDB70.Age + 1;
-    return;
-  }
-
+    } else {
       BuildId->BuildId->Signature.CVSignature = OMF::Signature::PDB70;
       BuildId->BuildId->PDB70.Age = 1;
       llvm::getRandomBytes(BuildId->BuildId->PDB70.Signature, 16);
     }
+  }
+
+  // At this point the only fields in the COFF file which remain unset are the
+  // "timestamp" in the COFF file header, and the ones in the coff debug
+  // directory.  Now we can hash the file and write that hash to the various
+  // timestamp fields in the file.
+  StringRef OutputFileData(
+      reinterpret_cast<const char *>(Buffer->getBufferStart()),
+      Buffer->getBufferSize());
+
+  uint32_t Hash = static_cast<uint32_t>(xxHash64(OutputFileData));
+
+  if (DebugDirectory)
+    DebugDirectory->setTimeDateStamp(Hash);
+
+  uint8_t *Buf = Buffer->getBufferStart();
+  Buf += DOSStubSize + sizeof(PEMagic);
+  object::coff_file_header *CoffHeader =
+      reinterpret_cast<coff_file_header *>(Buf);
+  CoffHeader->TimeDateStamp = Hash;
+}
 
 // Sort .pdata section contents according to PE/COFF spec 5.5.
 void Writer::sortExceptionTable() {
   OutputSection *Sec = findSection(".pdata");
   if (!Sec)
     return;
   // We assume .pdata contains function table entries only.
   uint8_t *Begin = Buffer->getBufferStart() + Sec->getFileOff();