[PATCH] D43913: Delay writing the PDB build id until just before file commit.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lld/COFF/PDB.cpp:1128-1136
+  // 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,
----------------
zturner wrote:
> ruiu wrote:
> > 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?
> 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.
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.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/InfoStreamBuilder.cpp:30
+      NamedStreams(NamedStreams) {
+  ::memset(Guid.Guid, 0, sizeof(Guid));
+}
----------------
zturner wrote:
> ruiu wrote:
> > This is suspicious -- is sizeof(Guid) == sizeof(Guid.Guid)?
> Yes, but I could add a `static_assert` to that effect.
But why don't you do

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

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


https://reviews.llvm.org/D43913