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

Mon Jun 6 08:57:10 PDT 2022

DavidSpickett 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);
----------------
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.

================
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);
----------------
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".

================
Comment at: lldb/source/Plugins/SymbolVendor/PECOFF/SymbolVendorPECOFF.cpp:73
+  FileSpec fspec = module_sp->GetSymbolFileFileSpec();
+  // Otherwise, try gnu_debuglink, if one exists.
+  if (!fspec)
----------------
alvinhochun wrote:
> DavidSpickett wrote:
> > Is this preference defined by some standard or are you assuming you won't see both?
> > 
> > FWIW it does make sense to me to use the filename in the usual place first, then the debuglink, just as you've got it here.
> I think that may actually be the option set by the user (`target symbols add` perhaps?) but I am not sure. (This was just copied from `SymbolVendorELF`.)
Ok so we support debuglink from ELF already? Cool.

================
Comment at: lldb/test/Shell/ObjectFile/PECOFF/dwarf-gnu-debuglink-i686.yaml:2
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objcopy --strip-all --add-gnu-debuglink=%t %t %t.stripped
+# RUN: lldb-test object-file %t.stripped | FileCheck %s
----------------
alvinhochun wrote:
> DavidSpickett wrote:
> > For these tests you are making a file, then giving it a debug link to itself then checking that lldb finds it? Seems like if the parsing of the debuglink failed then it could just find the original file and look like it passed.
> > 
> > Except you strip that original file of everything *but* the debug link. So if lldb tried to use the orignal file, it wouldn't have any debug info.
> > 
> > Assuming I got that right could you add comments to this and the other test stating that intent.
> No, I think the gnu-debuglink is only added to the output file `%t.stripped` and after it has already been stripped, the input file `%t` isn't touched at all. So no gnu-debuglink is being stripped from any files.
> 
> (By the way this test is a variation of `lldb/test/Shell/ObjectFile/ELF/gnu-debuglink.yaml`.)
Right, you load the stripped version which debug links to the unstripped version (that doesn't have a debug link).

The way it's done is fine, please document the logic though as it's not the most obvious thing on first look.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126367