[PATCH] D43978: Write a hash of the binary as the PE Debug Directory Timestamp

[Zachary Turner via Phabricator via llvm-commits]

zturner updated this revision to Diff 136780.
zturner added a comment.

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.


https://reviews.llvm.org/D43978

Files:
  lld/COFF/Writer.cpp


Index: lld/COFF/Writer.cpp
===================================================================
--- lld/COFF/Writer.cpp
+++ lld/COFF/Writer.cpp
@@ -28,6 +28,7 @@
 #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>
@@ -71,11 +72,18 @@
       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;
 };
 
@@ -157,7 +165,7 @@
   RVATableChunk *GuardFidsTable = nullptr;
   RVATableChunk *SEHTable = nullptr;
 
-  Chunk *DebugDirectory = nullptr;
+  DebugDirectoryChunk *DebugDirectory = nullptr;
   std::vector<Chunk *> DebugRecords;
   CVDebugRecordChunk *BuildId = nullptr;
   Optional<codeview::DebugInfo> PreviousBuildId;
@@ -1032,16 +1040,46 @@
     return;
 
   assert(BuildId && "BuildId is not set!");
-
+  assert(DebugDirectory && "DebugDirectory is not set!");
+
+  // Set the time stamp to a hash of the executable file.  Make sure to
+  // do this *after* setting the GUID and age, this way if the debug
+  // info changes but the binary doesn't, the hash will also change.
+  // Otherwise you could have a situation where there is only one slot
+  // on a symbol server for a given binary, but two slots for the debug
+  // info, and you wouldn't be able to debug crash dumps for one of the
+  // versions.
+  //
+  // Note that there are several time stamps that need to be set.
+  // * The first is in the COFF file header which occurs immediately after
+  //   the DOS stub at the beginning of the file.
+  // * The second occur in the debug directories, one in each directory.  The
+  //   PE spec says this should be the time that the debug information was
+  //   created, but in practice I cannot find a link.exe generated binary where
+  //   the values aren't identical to the one in the COFF file header.
+  // It's not clear if they need to agree, but this is what the MS linker does
+  // so we match the behavior.
   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);
   }
 
-  BuildId->BuildId->Signature.CVSignature = OMF::Signature::PDB70;
-  BuildId->BuildId->PDB70.Age = 1;
-  llvm::getRandomBytes(BuildId->BuildId->PDB70.Signature, 16);
+  // Now that the Guid and Age are set, hashing the binary will incorporate
+  // those values as part of the hash.
+  StringRef OutputFileData(
+      reinterpret_cast<const char *>(Buffer->getBufferStart()),
+      Buffer->getBufferSize());
+  uint32_t Hash = static_cast<uint32_t>(xxHash64(OutputFileData));
+  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.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D43978.136780.patch
Type: text/x-patch
Size: 3535 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180302/b6ff085a/attachment.bin>