alvinhochun added inline comments.
================ Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:883 + std::string gnu_debuglink_file = data.GetCStr(&gnu_debuglink_offset); + gnu_debuglink_offset = llvm::alignTo(gnu_debuglink_offset, 4); + data.GetU32(&gnu_debuglink_offset, &gnu_debuglink_crc, 1); ---------------- DavidSpickett wrote: > alvinhochun wrote: > > DavidSpickett wrote: > > > Is this aligning up or down, can you add a comment explaining? > > > > > > I think it is something like read a filename, return the offset of the > > > end of it. Align that up to a 4 byte boundary then read the crc from > > > there? > > I don't know really. I just copied this logic from > > https://github.com/llvm/llvm-project/blob/99a83b1286748501e0ccf199a582dc3ec5451ef5/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp#L1533, > > which was originally implemented 9 years ago with > > https://github.com/LLVM/llvm-project/commit/a7499c98301e847d2e525921801e1edcc44e34da. > Please confirm it and add a comment stating the logic. Ah yes, the linked spec states > zero to three bytes of padding, as needed to reach the next four-byte > boundary within the section before the crc checksum. ================ Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:884 + gnu_debuglink_offset = llvm::alignTo(gnu_debuglink_offset, 4); + data.GetU32(&gnu_debuglink_offset, &gnu_debuglink_crc, 1); + return FileSpec(gnu_debuglink_file); ---------------- DavidSpickett wrote: > alvinhochun wrote: > > DavidSpickett wrote: > > > You read the crc but it is unused. > > > > > > If you just want to make sure that the section actually has enough data > > > to have a crc in it, then check somehow that it was read or better, check > > > the size explicitly. > > > > > > As for calculating the crc I guess you'd have to read the whole linked > > > file so the most you could do here is stash the crc value somewhere for > > > later when you have opportunity to open it. Most of the time you won't > > > hit an issue but I guess it's here for a reason. Perhaps if you were to > > > debug a file that linked to something that had been rebuilt since? > > I didn't think much about it at first. I thought ObjectFileELF only used > > this CRC as one of the few ways of generating an UUID for the object file. > > But just now re-reading the code I realize that > > `Symbols::LocateExecutableSymbolFile` actually uses this UUID to check the > > validity of the debug file, so it now makes more sense. > > > > I think I may have to do the same for ObjectFilePECOFF. The tricky bit is > > that it already has code to calculate an UUID using the PDB info if it > > exists. What should happen if both PDB and gnu-debuglink exists for the > > COFF object file? > You'd have to work out if one can create such a file without "hacking" some > how like adding a debuglink with objcopy. And even if lldb could load both, > would they mostly be copies of each other. Seems like an odd scenario. > > If it's not something a default toolchain use is going to produce, I'd prefer > the more "normal" one (the PDB, just a guess) and if people get confused > because it chose the PDB then they've somewhat opted into paying that > investigation cost by choosing this unusual workflow. > > You can always log these sort of things too. I believe a lot of the Apple > code will emit things like ok I'm loading this file here, but actually > substituted it with this one etc. 99% of people never need to read them but > it's easy to say to someone who has an issue "enable this log and tell me > what you see". It is doable, Clang can be asked to produce both PDB and DWARF debug info by passing both `-gdwarf -gcodeview`, and link with `-Wl,-pdb=`. It turns out LLD always writes a synthetic "build id" for mingw even if no PDB is generated (https://github.com/llvm/llvm-project/blob/0498415f1d6ac0bfbda72486505c11a7b3d464c4/lld/COFF/Writer.cpp#L1926). Seems that this mechanism is already working to check that the executable and the debug file are from the same build at least for LLD-linked objects, because this "build id" does not get stripped. I have verified that lldb does generate the UUID for such objects. As a fallback for objects linked by the GNU linker, I can use the CRC to generate the UUID like how ObjectFileELF does it. I will add tests for these cases too. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126367/new/ https://reviews.llvm.org/D126367 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
