Delay writing the PDB build id until just before file commit.

[zturner]

Summary

The idea here is that we want to be able to create a reproducible hash of the PDB file before and use this as the timestamp field in the PDB and set the timestamp of the executable to match.

In order to do this, we need a way to defer emitting the signature until after everything else in the PDB file has been written. This way when we hash the PDB file, we hash it as if it had default values (e.g. all zeros) for the build id, and then update the build id field.

For now, this is NFC intended only to make the necessary refactoring to do this kind of deferred emission of the build id possible. No actual hashing of the PDB is done yet. We'll do that in a followup.

[ruiu]

lld/COFF/PDB.cpp

1124

Remove llvm::.

1128–1136

Does this unconditionally set time(nullptr) to the build id? If so, can't you pass it as an argument to Builder ctor or something?

llvm/lib/DebugInfo/PDB/Native/InfoStreamBuilder.cpp

30

This is suspicious -- is sizeof(Guid) == sizeof(Guid.Guid)?

[zturner]

lld/COFF/PDB.cpp

1128–1136

Yes, but for now I'm trying to keep this no functional change. This is exactly what the code did before, so I wanted to leave it the same. In followup patches when we actually use a hash, the call to time will just go away.

llvm/lib/DebugInfo/PDB/Native/InfoStreamBuilder.cpp

30

Yes, but I could add a static_assert to that effect.

[ruiu]

lld/COFF/PDB.cpp

1128–1136

I wonder if you could pass a lambda to compute a hash value instead. Lambda is a convenient way to pass a piece of code to other module. I'm wondering if it's applicable here.

llvm/lib/DebugInfo/PDB/Native/InfoStreamBuilder.cpp

30

But why don't you do

::memset(Guid.Guid, 0, sizeof(Guid.Guid));

which is always correct regardless of Guid and Guid.Guid size?

[zturner]

Updated based on suggestions from Rui.

[zturner]

lld/COFF/PDB.cpp

1128–1136

In practice this value is going to be a hash of the PDB. So instead of calling time(nullptr) I just call a function to compute the hash. Then somehow I pass this value back to lld::coff::Writer so it can write the same value to the Debug Directory in the PE. So there won't be a need to pass a lambda, because the PDB Linker will still be the one to compute it. So in a followup patch, I'll delete this, replace it with something like like Result.IdPtr->Signature = hashPdb(*Result.File); and then return the value of Result.IdPtr->Signature back to the caller so it can write the same value. So I don't think lambdas or anything fancy are needed.

[compnerd]

lld/COFF/PDB.cpp

1059

Can we sink this to the same site too? I think that the PDB writer indicating what it emitted would be nicer.

[zturner]

lld/COFF/PDB.cpp

1059

We could, but this value is useless for the PE writer and is definitely PDB specific. It's not part of the build id or anything and is really more of an implementation detail of the PDB, so that's why I left it here.

[ruiu]

lld/COFF/PDB.cpp

1128–1136

I'm little confused. Builder knows the pdb file it is creating, so it is not clear to me why only the hash computation needs to be done on the caller side. Why can't you do that inside Builder and make Builder return a Signature?

[zturner]

lld/COFF/PDB.cpp

1128–1136

It might work. I can try.

[zturner]

Re-write this to have PDBFileBuilder emit the signature. It's still done as the last step though, so that we can drop in a hash in a followup.

This was a little awkward, because in the case of something like yamlToPdb we actually want to use the exact value in the yaml and not recompute it. I made it work by having the InfoStreamBuilder allow you to optionally set the signature, and if it's not set, it will use the computed value.

[ruiu]

LGTM

lld/COFF/PDB.cpp

1058–1059

Uuid

lld/COFF/PDB.cpp

 
 class PDBLinker {
 public:
   PDBLinker(SymbolTable *Symtab)
       : Alloc(), Symtab(Symtab), Builder(Alloc), TypeTable(Alloc),
         IDTable(Alloc), GlobalTypeTable(Alloc), GlobalIDTable(Alloc) {}
 
   /// Emit the basic PDB structure: initial streams, headers, etc.
-  void initialize(const llvm::codeview::DebugInfo &BuildId);
+  void initialize(const codeview::DebugInfo &BuildId);
 
   /// Link CodeView from each object file in the symbol table into the PDB.
   void addObjectsToPDB();
 
   /// Link CodeView from a single object file into the PDB.
   void addObjFile(ObjFile *File);
 
   /// Produce a mapping from the type and item indices used in the object
public:
   /// Add the section map and section contributions to the PDB.
   void addSections(ArrayRef<OutputSection *> OutputSections,
                    ArrayRef<uint8_t> SectionTable);
 
   void addSectionContrib(pdb::DbiModuleDescriptorBuilder &LinkerModule,
                          OutputSection *OS, Chunk *C);
 
   /// Write the PDB to disk.
-  void commit();
+  void commit(const codeview::DebugInfo &BuildId);
 
 private:
   BumpPtrAllocator Alloc;
 
   SymbolTable *Symtab;
 
   pdb::PDBFileBuilder Builder;
 
static void addLinkerModuleSectionSymbol(pdb::DbiModuleDescriptorBuilder &Mod,
   Mod.addSymbol(codeview::SymbolSerializer::writeOneSymbol(
       Sym, Allocator, CodeViewContainer::Pdb));
 }
 
 // Creates a PDB file.
 void coff::createPDB(SymbolTable *Symtab,
                      ArrayRef<OutputSection *> OutputSections,
                      ArrayRef<uint8_t> SectionTable,
-                     const llvm::codeview::DebugInfo &BuildId) {
+                     const codeview::DebugInfo &BuildId) {
   ScopedTimer T1(TotalPdbLinkTimer);
   PDBLinker PDB(Symtab);
   PDB.initialize(BuildId);
   PDB.addObjectsToPDB();
   PDB.addSections(OutputSections, SectionTable);
 
   ScopedTimer T2(DiskCommitTimer);
-  PDB.commit();
+  PDB.commit(BuildId);
 }
 
-void PDBLinker::initialize(const llvm::codeview::DebugInfo &BuildId) {
+void PDBLinker::initialize(const codeview::DebugInfo &BuildId) {
   ExitOnErr(Builder.initialize(4096)); // 4096 is blocksize
 
   // Create streams in MSF for predefined streams, namely
   // PDB, TPI, DBI and IPI.
   for (int I = 0; I < (int)pdb::kSpecialStreamCount; ++I)
     ExitOnErr(Builder.getMsfBuilder().addStream(0));
 
   // Add an Info stream.
   auto &InfoBuilder = Builder.getInfoBuilder();
-  InfoBuilder.setAge(BuildId.PDB70.Age);
-
-  GUID uuid;
-  memcpy(&uuid, &BuildId.PDB70.Signature, sizeof(uuid));
-  InfoBuilder.setGuid(uuid);
-  InfoBuilder.setSignature(time(nullptr));
   InfoBuilder.setVersion(pdb::PdbRaw_ImplVer::PdbImplVC70);
 
   // Add an empty DBI stream.
   pdb::DbiStreamBuilder &DbiBuilder = Builder.getDbiBuilder();
   DbiBuilder.setAge(BuildId.PDB70.Age);
   DbiBuilder.setVersionHeader(pdb::PdbDbiV70);
   ExitOnErr(DbiBuilder.addDbgStream(pdb::DbgHeaderType::NewFPO, {}));
 }
 
void PDBLinker::addSections(ArrayRef<OutputSection *> OutputSections,
   SectionMap = pdb::DbiStreamBuilder::createSectionMap(Sections);
   DbiBuilder.setSectionMap(SectionMap);
 
   // Add COFF section header stream.
   ExitOnErr(
       DbiBuilder.addDbgStream(pdb::DbgHeaderType::SectionHdr, SectionTable));
 }
 
-void PDBLinker::commit() {
+void PDBLinker::commit(const codeview::DebugInfo &BuildId) {
   // Write to a file.
-  ExitOnErr(Builder.commit(Config->PDBPath));
+  pdb::FinalizedPdb Result = ExitOnErr(Builder.write(Config->PDBPath));
+
+  // Set the build id in the pdb file.
+  // FIXME: Set Signature here to the timestamp of the executable.  For
+  //        reproducible builds, we will probably want to go further and set
+  //        this field to a hash of the PDB file, then set the timestamp of
+  //        the executable to be the same hash.
+  Result.IdPtr->Age = BuildId.PDB70.Age;
+  ::memcpy(&Result.IdPtr->Guid, &BuildId.PDB70.Signature,
+           sizeof(codeview::GUID));
+  Result.IdPtr->Signature = time(nullptr);
+
+  ExitOnErr(Result.File->commit());
 }

llvm/include/llvm/DebugInfo/PDB/Native/PDBFileBuilder.h

 #include "llvm/ADT/Optional.h"
 #include "llvm/DebugInfo/PDB/Native/NamedStreamMap.h"
 #include "llvm/DebugInfo/PDB/Native/PDBFile.h"
 #include "llvm/DebugInfo/PDB/Native/PDBStringTableBuilder.h"
 #include "llvm/DebugInfo/PDB/Native/RawConstants.h"
 #include "llvm/Support/Allocator.h"
 #include "llvm/Support/Endian.h"
 #include "llvm/Support/Error.h"
+#include "llvm/Support/FileOutputBuffer.h"
 
 #include <memory>
 #include <vector>
 
 namespace llvm {
 namespace msf {
 class MSFBuilder;
 }
 namespace pdb {
+
+struct BuildId;
 class DbiStreamBuilder;
 class InfoStreamBuilder;
 class GSIStreamBuilder;
 class TpiStreamBuilder;
 
+struct FinalizedPdb {
+  std::unique_ptr<FileOutputBuffer> File;
+  BuildId *IdPtr;
+};
+
 class PDBFileBuilder {
 public:
   explicit PDBFileBuilder(BumpPtrAllocator &Allocator);
   ~PDBFileBuilder();
   PDBFileBuilder(const PDBFileBuilder &) = delete;
   PDBFileBuilder &operator=(const PDBFileBuilder &) = delete;
 
   Error initialize(uint32_t BlockSize);
 
   msf::MSFBuilder &getMsfBuilder();
   InfoStreamBuilder &getInfoBuilder();
   DbiStreamBuilder &getDbiBuilder();
   TpiStreamBuilder &getTpiBuilder();
   TpiStreamBuilder &getIpiBuilder();
   PDBStringTableBuilder &getStringTableBuilder();
   GSIStreamBuilder &getGsiBuilder();
 
-  Error commit(StringRef Filename);
+  Expected<FinalizedPdb> write(StringRef Filename);
+  Error writeAndCommit(StringRef Filename);
 
   Expected<uint32_t> getNamedStreamIndex(StringRef Name) const;
   Error addNamedStream(StringRef Name, uint32_t Size);
 
 private:
   Expected<msf::MSFLayout> finalizeMsfLayout();
 
   void commitFpm(WritableBinaryStream &MsfBuffer, const msf::MSFLayout &Layout);

llvm/include/llvm/DebugInfo/PDB/Native/RawTypes.h

struct TpiStreamHeader {
   EmbeddedBuf HashValueBuffer;
   EmbeddedBuf IndexOffsetBuffer;
   EmbeddedBuf HashAdjBuffer;
 };
 
 const uint32_t MinTpiHashBuckets = 0x1000;
 const uint32_t MaxTpiHashBuckets = 0x40000;
 
+struct BuildId {
+  support::ulittle32_t Signature; // Actually a timestamp or hash.
+  support::ulittle32_t Age;
+  codeview::GUID Guid;
+};
+
 /// The header preceeding the global PDB Stream (Stream 1)
 struct InfoStreamHeader {
   support::ulittle32_t Version;
-  support::ulittle32_t Signature;
-  support::ulittle32_t Age;
-  codeview::GUID Guid;
+  BuildId Id;
 };
 
 /// The header preceeding the /names stream.
 struct PDBStringTableHeader {
   support::ulittle32_t Signature;   // PDBStringTableSignature
   support::ulittle32_t HashVersion; // 1 or 2
   support::ulittle32_t ByteSize;    // Number of bytes of names buffer.
 };
 
 const uint32_t PDBStringTableSignature = 0xEFFEEFFE;
 
 } // namespace pdb
 } // namespace llvm
 
 #endif

llvm/include/llvm/Support/BinaryByteStream.h

virtual BinaryStreamFlags getFlags() const override {
     return BSF_Write | BSF_Append;
   }
 
   MutableArrayRef<uint8_t> data() { return Data; }
 };
 
 /// \brief An implementation of WritableBinaryStream backed by an llvm
 /// FileOutputBuffer.
-class FileBufferByteStream : public WritableBinaryStream {
-private:
-  class StreamImpl : public MutableBinaryByteStream {
-  public:
-    StreamImpl(std::unique_ptr<FileOutputBuffer> Buffer,
+class FileBufferByteStream : public MutableBinaryByteStream {
+  FileBufferByteStream(FileOutputBuffer &Buffer,
                        llvm::support::endianness Endian)
-        : MutableBinaryByteStream(
-              MutableArrayRef<uint8_t>(Buffer->getBufferStart(),
-                                       Buffer->getBufferEnd()),
-              Endian),
-          FileBuffer(std::move(Buffer)) {}
+            MutableArrayRef<uint8_t>(Buffer.getBufferStart(),
+                                     Buffer.getBufferEnd()),
+        FileBuffer(Buffer) {}
 
-    Error commit() override {
-      if (FileBuffer->commit())
-        return make_error<BinaryStreamError>(
-            stream_error_code::filesystem_error);
-      return Error::success();
-    }
+    if (FileBuffer.commit())
+      return make_error<BinaryStreamError>(stream_error_code::filesystem_error);
 
-  private:
-    std::unique_ptr<FileOutputBuffer> FileBuffer;
-  };
-
-public:
-  FileBufferByteStream(std::unique_ptr<FileOutputBuffer> Buffer,
-                       llvm::support::endianness Endian)
-      : Impl(std::move(Buffer), Endian) {}
-
-  llvm::support::endianness getEndian() const override {
-    return Impl.getEndian();
-  }
-
-  Error readBytes(uint32_t Offset, uint32_t Size,
-                  ArrayRef<uint8_t> &Buffer) override {
-    return Impl.readBytes(Offset, Size, Buffer);
-  }
-
-  Error readLongestContiguousChunk(uint32_t Offset,
-                                   ArrayRef<uint8_t> &Buffer) override {
-    return Impl.readLongestContiguousChunk(Offset, Buffer);
-  }
-
-  uint32_t getLength() override { return Impl.getLength(); }
-
-  Error writeBytes(uint32_t Offset, ArrayRef<uint8_t> Data) override {
-    return Impl.writeBytes(Offset, Data);
-  }
-
-  Error commit() override { return Impl.commit(); }
-
-private:
-  StreamImpl Impl;
+  FileOutputBuffer &FileBuffer;
 };
 
 } // end namespace llvm
 
 #endif // LLVM_SUPPORT_BYTESTREAM_H

llvm/lib/DebugInfo/PDB/Native/InfoStream.cpp

Error InfoStream::reload() {
   case PdbImplVC140:
     break;
   default:
     return make_error<RawError>(raw_error_code::corrupt_file,
                                 "Unsupported PDB stream version.");
   }
 
   Version = H->Version;
-  Signature = H->Signature;
-  Age = H->Age;
-  Guid = H->Guid;
+  Signature = H->Id.Signature;
+  Age = H->Id.Age;
+  Guid = H->Id.Guid;
 
   uint32_t Offset = Reader.getOffset();
   if (auto EC = NamedStreams.load(Reader))
     return EC;
   uint32_t NewOffset = Reader.getOffset();
   NamedStreamMapByteSize = NewOffset - Offset;
 
   Reader.setOffset(Offset);

llvm/lib/DebugInfo/PDB/Native/InfoStreamBuilder.cpp

 using namespace llvm;
 using namespace llvm::codeview;
 using namespace llvm::msf;
 using namespace llvm::pdb;
 
 InfoStreamBuilder::InfoStreamBuilder(msf::MSFBuilder &Msf,
                                      NamedStreamMap &NamedStreams)
     : Msf(Msf), Ver(PdbRaw_ImplVer::PdbImplVC70), Sig(-1), Age(0),
-      NamedStreams(NamedStreams) {}
+      NamedStreams(NamedStreams) {
+  ::memset(Guid.Guid, 0, sizeof(Guid.Guid));
+}
 
 void InfoStreamBuilder::setVersion(PdbRaw_ImplVer V) { Ver = V; }
 
 void InfoStreamBuilder::setSignature(uint32_t S) { Sig = S; }
 
 void InfoStreamBuilder::setAge(uint32_t A) { Age = A; }
 
 void InfoStreamBuilder::setGuid(GUID G) { Guid = G; }

 
 Error InfoStreamBuilder::commit(const msf::MSFLayout &Layout,
                                 WritableBinaryStreamRef Buffer) const {
   auto InfoS = WritableMappedBlockStream::createIndexedStream(
       Layout, Buffer, StreamPDB, Msf.getAllocator());
   BinaryStreamWriter Writer(*InfoS);
 
   InfoStreamHeader H;
-  H.Age = Age;
-  H.Signature = Sig;
   H.Version = Ver;
-  H.Guid = Guid;
+  H.Id.Age = Age;
+  H.Id.Signature = Sig;
+  H.Id.Guid = Guid;
   if (auto EC = Writer.writeObject(H))
     return EC;
 
   if (auto EC = NamedStreams.commit(Writer))
     return EC;
   if (auto EC = Writer.writeInteger(0))
     return EC;
   for (auto E : Features) {
     if (auto EC = Writer.writeEnum(E))
       return EC;
   }
   return Error::success();
 }

llvm/lib/DebugInfo/PDB/Native/PDBFileBuilder.cpp

for (uint32_t I = 0; I < 8; ++I) {
       ThisByte |= Mask;
       ++BI;
     }
     cantFail(FpmWriter.writeObject(ThisByte));
   }
   assert(FpmWriter.bytesRemaining() == 0);
 }
 
-Error PDBFileBuilder::commit(StringRef Filename) {
+Expected<FinalizedPdb> PDBFileBuilder::write(StringRef Filename) {
   assert(!Filename.empty());
   auto ExpectedLayout = finalizeMsfLayout();
   if (!ExpectedLayout)
     return ExpectedLayout.takeError();
   auto &Layout = *ExpectedLayout;
 
+  FinalizedPdb Result;
   uint64_t Filesize = Layout.SB->BlockSize * Layout.SB->NumBlocks;
   auto OutFileOrError = FileOutputBuffer::create(Filename, Filesize);
   if (auto E = OutFileOrError.takeError())
-    return E;
-  FileBufferByteStream Buffer(std::move(*OutFileOrError),
-                              llvm::support::little);
+    return std::move(E);
+  Result.File = std::move(*OutFileOrError);
+  FileBufferByteStream Buffer(*Result.File, llvm::support::little);
   BinaryStreamWriter Writer(Buffer);
 
   if (auto EC = Writer.writeObject(*Layout.SB))
-    return EC;
+    return std::move(EC);
 
   commitFpm(Buffer, Layout);
 
   uint32_t BlockMapOffset =
       msf::blockToOffset(Layout.SB->BlockMapAddr, Layout.SB->BlockSize);
   Writer.setOffset(BlockMapOffset);
   if (auto EC = Writer.writeArray(Layout.DirectoryBlocks))
-    return EC;
+    return std::move(EC);
 
   auto DirStream = WritableMappedBlockStream::createDirectoryStream(
       Layout, Buffer, Allocator);
   BinaryStreamWriter DW(*DirStream);
   if (auto EC = DW.writeInteger<uint32_t>(Layout.StreamSizes.size()))
-    return EC;
+    return std::move(EC);
 
   if (auto EC = DW.writeArray(Layout.StreamSizes))
-    return EC;
+    return std::move(EC);
 
   for (const auto &Blocks : Layout.StreamMap) {
     if (auto EC = DW.writeArray(Blocks))
-      return EC;
+      return std::move(EC);
   }
 
   auto ExpectedSN = getNamedStreamIndex("/names");
   if (!ExpectedSN)
     return ExpectedSN.takeError();
 
   auto NS = WritableMappedBlockStream::createIndexedStream(
       Layout, Buffer, *ExpectedSN, Allocator);
   BinaryStreamWriter NSWriter(*NS);
   if (auto EC = Strings.commit(NSWriter))
-    return EC;
+    return std::move(EC);
 
   if (Info) {
     if (auto EC = Info->commit(Layout, Buffer))
-      return EC;
+      return std::move(EC);
   }
 
   if (Dbi) {
     if (auto EC = Dbi->commit(Layout, Buffer))
-      return EC;
+      return std::move(EC);
   }
 
   if (Tpi) {
     if (auto EC = Tpi->commit(Layout, Buffer))
-      return EC;
+      return std::move(EC);
   }
 
   if (Ipi) {
     if (auto EC = Ipi->commit(Layout, Buffer))
-      return EC;
+      return std::move(EC);
   }
 
   if (Gsi) {
     if (auto EC = Gsi->commit(Layout, Buffer))
-      return EC;
-  }
+      return std::move(EC);
+  }
+
+  auto InfoStreamBlocks = Layout.StreamMap[StreamPDB];
+  assert(!InfoStreamBlocks.empty());
+  uint64_t InfoStreamFileOffset =
+      blockToOffset(InfoStreamBlocks.front(), Layout.SB->BlockSize);
+  InfoStreamHeader *H = reinterpret_cast<InfoStreamHeader *>(
+      Result.File->getBufferStart() + InfoStreamFileOffset);
+  Result.IdPtr = &H->Id;
+  return std::move(Result);
 
-  return Buffer.commit();
+Error PDBFileBuilder::writeAndCommit(StringRef Filename) {
+  auto Result = write(Filename);
+  if (!Result)
+    return Result.takeError();
+  return Result->File->commit();
 }

llvm/tools/llvm-pdbutil/llvm-pdbutil.cpp

static void yamlToPdb(StringRef Path) {
   IpiBuilder.setVersionHeader(Ipi.Version);
   for (const auto &R : Ipi.Records) {
     CVType Type = R.toCodeViewRecord(TS);
     IpiBuilder.addTypeRecord(Type.RecordData, None);
   }
 
   Builder.getStringTableBuilder().setStrings(*Strings.strings());
 
-  ExitOnErr(Builder.commit(opts::yaml2pdb::YamlPdbOutputFile));
+  ExitOnErr(Builder.writeAndCommit(opts::yaml2pdb::YamlPdbOutputFile));
 }
 
 static PDBFile &loadPDB(StringRef Path, std::unique_ptr<IPDBSession> &Session) {
   ExitOnErr(loadDataForPDB(PDB_ReaderType::Native, Path, Session));
 
   NativeSession *NS = static_cast<NativeSession *>(Session.get());
   return NS->getPDBFile();
 }
static void mergePdbs() {
   });
   Builder.getInfoBuilder().addFeature(PdbRaw_FeatureSig::VC140);
 
   SmallString<64> OutFile(opts::merge::PdbOutputFile);
   if (OutFile.empty()) {
     OutFile = opts::merge::InputFilenames[0];
     llvm::sys::path::replace_extension(OutFile, "merged.pdb");
   }
-  ExitOnErr(Builder.commit(OutFile));
+  ExitOnErr(Builder.writeAndCommit(OutFile));
 }
 
 static bool parseRange(StringRef Str,
                        Optional<opts::bytes::NumberRange> &Parsed) {
   if (Str.empty())
     return true;
 
   llvm::Regex R("^([^-]+)(-([^-]+))?$");