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

[Hui Huang via Phabricator via lldb-commits]

Hui added inline comments.


================
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);
----------------
labath wrote:
> 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.
If the exe/dll is built without any debug info, the function succeeds and still returns null pdb_info.


================
Comment at: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:61
+  if (!COFFObj->getDebugPDBInfo(pdb_info, pdb_file) && pdb_info)
+    return UUID::fromOptionalData(pdb_info->PDB70.Signature);
+
----------------
labath wrote:
> Is there a specific reason you used this particular encoding of the UUID, or did you do that just because it was the easiest? I am asking because I have a reason to have this use a somewhat different encoding. :)
> 
> Let me elaborate: I think there are two things we can want from the UUID here: The first one is for it to match the UUID encoding we get from other sources (so that they can agree on whether they are talking about the same binary). The second one is for the uuid encoding to match the "native" UUID format of the given platform.
> 
> Right now, this implementation achieves neither. :) It fails the second criterion because the UUID strings comes out in different endianness than what the windows native tools produce (I'm using `dumpbin` as reference here.). And it also fails the first one, because e.g. minidump reading code parses the UUID differently <D60501>.
> 
> Now, for windows, these two criteria are slightly at odds with one another. In order to fully match the dumpbin format, we would need to have some kind of a special field for the "age" bit. But lldb has no such concept, and there doesn't seem to be much need to introduce it. However, including the "age" field in the "uuid" seems like the right thing to do, as two files with different "ages" should be considered different for debug info matching purposes (at least, that's what my limited understanding of pdbs tells me. if some of this is wrong, please let me know). So, in <D60501> I made a somewhat arbitrary decision to attach the age field to the UUID in big endian.
> That's the format that made most sense to me, though that can certainly be changed (the most important thing is for these things to stay in sync).
> 
> So, if you have a reason to use a different encoding, please let me know, so we can agree on a consistent implementation. Otherwise, could you change this to use the same UUID format as the minidump parser?
You are right. The encoding of MS struct GUID and the PDB70DebugInfo::Signature are different.  Can UUID format and the method to yield it from minidump parser be available in class COFFObjectFile?



================
Comment at: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:866
+
+  m_uuid = GetCoffUUID(GetFileSpec());
+  return m_uuid;
----------------
labath wrote:
> 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.
In addition, it is possible to simplify ObjectFilePECOFF ::GetModuleSpecifications API with such Binary.  In this sense, none of the arguments, like data_sp, data_offset will be used. 


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

https://reviews.llvm.org/D56229