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

[Pavel Labath via Phabricator via lldb-commits]

labath 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);
----------------
Hui wrote:
> 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.
Ah, ok. Thanks for explaining.


================
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);
+
----------------
Hui wrote:
> 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?
> 
I don't think you can put that in the COFFObjectFile, as it lives in llvm, and the UUID class is an lldb concept. It might be possible to put some utility function into llvm to help with that, but it's not clear to me how exactly that would look (and that would need to be a separate patch with a separate review).

What would make kind of sense is to add another factory function to the `UUID` class in lldb (`UUID::fromCvRecord` ?), which both ObjectFilePECOFF and ProcessMinidump could call into. However, the problem with that is that the definition of the CvRecord is in llvm/Object, and it seems silly to have lldb/Utility depend on that just to pull the single struct. I think it would make sense to move this struct into llvm/BinaryFormat (which lldb/Utility already depends on) and then everything would be fine. If you want to try that out, then go ahead, but I don't think that's really necessary here. (swapping the bytes around should be just a couple of LOC).


================
Comment at: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:866
+
+  m_uuid = GetCoffUUID(GetFileSpec());
+  return m_uuid;
----------------
Hui wrote:
> 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. 
Yeah, I noticed that too, but I didn't want to throw too many things into this patch. However, if you feel like trying it out, then please go ahead.


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

https://reviews.llvm.org/D56229