[PATCH] D56229: [PECOFF] Implementation of ObjectFilePECOFF:: GetUUID()

[Pavel Labath via Phabricator via llvm-commits]

labath edited reviewers, added: amccarth, labath; removed: zturner, llvm-commits.
labath added a subscriber: amccarth.
labath added a comment.

s/@zturner/@amccarth, as Zach probably won't have time to review this



================
Comment at: lit/Modules/PECOFF/export-dllfunc.yaml:11-12
+
+# timestamp and build id of debug info in the coff header varies. So does its UUID.
+# BASIC-CHECK-DAG: UUID: {{[0-9A-F]{7,}[0-9A-F]}}-{{.*}}
 
----------------
Since the U(G)UID needs to be stable and match the value computed from other sources, it would be good to have a test where we check that a file has some exact UUID.

Is there any way to use yaml2obj to generate such a file? For instance, if we drop the `lld-link` step and yamlify the resulting `dll` file instead. Would that work?


================
Comment at: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:60
+  llvm::StringRef pdb_file;
+  if (!COFFObj->getDebugPDBInfo(pdb_info, pdb_file) && pdb_info)
+    return UUID::fromOptionalData(pdb_info->PDB70.Signature);
----------------
Can `getDebugPDBInfo` succeed and still return a null pdb_info? If not, can we delete the second part?

Instead I believe you should check the CVSignature field of the returned struct to see that it indeed contains a PDB70 record.


================
Comment at: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:866
+
+  m_uuid = GetCoffUUID(GetFileSpec());
+  return m_uuid;
----------------
ObjectFilePECOFF already has a `llvm::object::Binary` created for the underlying file. I think it's super-wasteful (and potentially racy, etc.) to create a second one just to read out it's GUID. If you make a second version of this function, taking a `Binary` (and have the FileSpec version delegate to that), then you can avoid this.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56229/new/

https://reviews.llvm.org/D56229