[PATCH] D43913: Delay writing the PDB build id until just before file commit.
Rui Ueyama via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Feb 28 15:16:03 PST 2018
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,
> ruiu wrote:
> > 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.
> 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.
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?
More information about the llvm-commits