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 ↗

(On Diff #136403)

Remove llvm::.

1128–1136 ↗

(On Diff #136403)

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 ↗

(On Diff #136403)

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

[zturner]

lld/COFF/PDB.cpp

1128–1136 ↗

(On Diff #136403)

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 ↗

(On Diff #136403)

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

[ruiu]

lld/COFF/PDB.cpp

1128–1136 ↗

(On Diff #136403)

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 ↗

(On Diff #136403)

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 ↗

(On Diff #136403)

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 ↗

(On Diff #136403)

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 ↗

(On Diff #136403)

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 ↗

(On Diff #136403)

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 ↗

(On Diff #136403)

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

1059 ↗

(On Diff #136429)

Uuid

lld/trunk/COFF/PDB.cpp

void PDBLinker::initialize(const llvm::codeview::DebugInfo &BuildId) {
 
   // 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.setAge(BuildId.PDB70.Age);
   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, {}));
 }

llvm/trunk/include/llvm/DebugInfo/PDB/Native/InfoStreamBuilder.h

public:
   InfoStreamBuilder &operator=(const InfoStreamBuilder &) = delete;
 
   void setVersion(PdbRaw_ImplVer V);
   void setSignature(uint32_t S);
   void setAge(uint32_t A);
   void setGuid(codeview::GUID G);
   void addFeature(PdbRaw_FeatureSig Sig);
 
+  uint32_t getAge() const { return Age; }
+  codeview::GUID getGuid() const { return Guid; }
+  Optional<uint32_t> getSignature() const { return Signature; }
+
   uint32_t finalize();
 
   Error finalizeMsfLayout();
 
   Error commit(const msf::MSFLayout &Layout,
                WritableBinaryStreamRef Buffer) const;
 
 private:
   msf::MSFBuilder &Msf;
 
   std::vector<PdbRaw_FeatureSig> Features;
   PdbRaw_ImplVer Ver;
-  uint32_t Sig;
   uint32_t Age;
+  Optional<uint32_t> Signature;
   codeview::GUID Guid;
 
   NamedStreamMap &NamedStreams;
 };
 }
 }
 
 #endif

llvm/trunk/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) {}
+    : Msf(Msf), Ver(PdbRaw_ImplVer::PdbImplVC70), Age(0),
+      NamedStreams(NamedStreams) {
+  ::memset(&Guid, 0, sizeof(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::setSignature(uint32_t S) { Signature = S; }
+
 void InfoStreamBuilder::setGuid(GUID G) { Guid = G; }
 
 void InfoStreamBuilder::addFeature(PdbRaw_FeatureSig Sig) {
   Features.push_back(Sig);
 }
 
 Error InfoStreamBuilder::finalizeMsfLayout() {
   uint32_t Length = sizeof(InfoStreamHeader) +
                     NamedStreams.calculateSerializedLength() +
                     (Features.size() + 1) * sizeof(uint32_t);
   if (auto EC = Msf.setStreamSize(StreamPDB, Length))
     return EC;
   return Error::success();
 }
 
 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;
+  // Leave the build id fields 0 so they can be set as the last step before
+  // committing the file to disk.
+  ::memset(&H, 0, sizeof(H));
   H.Version = Ver;
-  H.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/trunk/lib/DebugInfo/PDB/Native/PDBFileBuilder.cpp

Error PDBFileBuilder::commit(StringRef Filename) {
   if (!ExpectedLayout)
     return ExpectedLayout.takeError();
   auto &Layout = *ExpectedLayout;
 
   uint64_t Filesize = Layout.SB->BlockSize * Layout.SB->NumBlocks;
   auto OutFileOrError = FileOutputBuffer::create(Filename, Filesize);
   if (auto E = OutFileOrError.takeError())
     return E;
+  FileOutputBuffer *FOB = OutFileOrError->get();
+
   FileBufferByteStream Buffer(std::move(*OutFileOrError),
                               llvm::support::little);
   BinaryStreamWriter Writer(Buffer);
 
   if (auto EC = Writer.writeObject(*Layout.SB))
     return EC;
 
   commitFpm(Buffer, Layout);
if (auto EC = Ipi->commit(Layout, Buffer))
       return EC;
   }
 
   if (Gsi) {
     if (auto EC = Gsi->commit(Layout, Buffer))
       return EC;
   }
 
+  auto InfoStreamBlocks = Layout.StreamMap[StreamPDB];
+  assert(!InfoStreamBlocks.empty());
+  uint64_t InfoStreamFileOffset =
+      blockToOffset(InfoStreamBlocks.front(), Layout.SB->BlockSize);
+  InfoStreamHeader *H = reinterpret_cast<InfoStreamHeader *>(
+      FOB->getBufferStart() + InfoStreamFileOffset);
+
+  // Set the build id at the very end, after every other byte of the PDB
+  // has been written.
+  // FIXME: Use a hash of the PDB rather than time(nullptr) for the signature.
+  H->Age = Info->getAge();
+  H->Guid = Info->getGuid();
+  Optional<uint32_t> Sig = Info->getSignature();
+  H->Signature = Sig.hasValue() ? *Sig : time(nullptr);
+
   return Buffer.commit();
 }