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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
