[PATCH] D36758: [LLD COFF / PDB] Incrementally update the BuildId when writing a PDB.

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd added a comment.

Nice!  Im looking forward to this!  It was one of the bits that I hadn't gotten around to implementing just yet, so thanks for taking it on.



================
Comment at: lld/COFF/PDB.cpp:827
 
+  GUID Uuid{};
+  uint32_t Age = BuildId.PDB70.Age;
----------------
ruiu wrote:
> I believe you can remove `{}`.
Where is this used?


================
Comment at: lld/COFF/PDB.cpp:831
   auto &InfoBuilder = Builder.getInfoBuilder();
-  InfoBuilder.setAge(DI ? DI->PDB70.Age : 0);
+  InfoBuilder.setAge(Age);
 
----------------
ruiu wrote:
> If `Age` is assigned only once, I'd remove that variable and use `BuildId.PDB70.Age` directly instead.
+1 on the inline the variable use.


================
Comment at: lld/COFF/Writer.cpp:318
+    // allowing a debugger to match a PDB and an executable.  So we need it even
+    // if we're ultimately not going to write CodeView data to the PDB.
+    auto *CVChunk = make<CVDebugRecordChunk>();
----------------
I think that this is not particularly correct.  In the CV case, no PDB is supposed to be generated, and we should be creating a COFF group entry instead.  I realize that its unlikely that we will support the `/Z7` mode, but, the comment is slightly misleading.


================
Comment at: lld/COFF/Writer.cpp:792
+
+  auto ExpectedExe = llvm::object::createBinary(Path);
+  if (!ExpectedExe) {
----------------
Can you change this to `ExpectedOutput`?  It could be a `dll`.


================
Comment at: lld/COFF/Writer.cpp:799
+  auto Binary = std::move(*ExpectedExe);
+  if (!Binary.getBinary()->isCOFF())
+    return;
----------------
When would this be false?  I thought that `link` is never used as a resource compiler, and you use `resgen` for that.


================
Comment at: lld/COFF/Writer.cpp:809
+  // of the existing binary, don't try to re-use the build id.
+  if (File.is64() != Config->is64())
+    return;
----------------
Is the bitsex enough?  Shouldn't we check the machine and the bitsex?


================
Comment at: lld/COFF/Writer.cpp:820
+      return;
+    if (ExistingDI->Signature.CVSignature != OMF::Signature::PDB70)
+      return;
----------------
I think that this deserves a comment.  The only reason we need this is because we don't support the other versions rather than it is not the one that we want.  I think that we should be checking that the existing `CVSignature` matches the one that we are going to write out this time (which ATM, just happens to be this constant).


https://reviews.llvm.org/D36758