[Lldb-commits] [PATCH] D126367: [lldb] Add gnu-debuglink support for Windows PE/COFF

[Alvin Wong via Phabricator via lldb-commits]

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