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

Mon Jun 6 08:04:25 PDT 2022

alvinhochun planned changes to this revision.
alvinhochun added a comment.

In D126367#3560233 <https://reviews.llvm.org/D126367#3560233>, @DavidSpickett wrote:

> Can you include some links to documentation of the debuglink feature in the commit message? I assume https://sourceware.org/gdb/onlinedocs/gdb/Separate-Debug-Files.html is what you'd want.

I suppose I can add that link.

================
Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:882
+      lldb::offset_t gnu_debuglink_offset = 0;
+      std::string gnu_debuglink_file = data.GetCStr(&gnu_debuglink_offset);
+      gnu_debuglink_offset = llvm::alignTo(gnu_debuglink_offset, 4);
----------------
DavidSpickett wrote:
> Is "leading directory components removed" significant here?
> ```
> A filename, with any leading directory components removed, followed by a zero byte,
> ```
> I think that means for `C:\a\b\c\foo` you get `foo`, and here we expect to just get the file name not a path?
I think that's the expected result. (Also see the reply below.)

================
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:
> 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.

================
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:
> 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?

================
Comment at: lldb/source/Plugins/SymbolVendor/PECOFF/SymbolVendorPECOFF.cpp:73
+  FileSpec fspec = module_sp->GetSymbolFileFileSpec();
+  // Otherwise, try gnu_debuglink, if one exists.
+  if (!fspec)
----------------
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`.)

================
Comment at: lldb/source/Symbol/LocateSymbolFile.cpp:368
+        if (num_specs == 2) {
+          // Special case to handle both i386 and i686 from ObjectFilePECOFF
+          ModuleSpec mspec2;
----------------
DavidSpickett wrote:
> What produces this special case?
`ObjectFilePECOFF::GetModuleSpecifications` returns two specs for `MachineX86` -- `"i386-pc-windows"` and `"i686-pc-windows"` (with `-msvc` after being normalized implicitly). I do not know why it does this and I am scared of touching it.

================
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
----------------
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`.)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126367