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

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a reviewer: omjavaid.
DavidSpickett added a comment.

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.



================
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);
----------------
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?


================
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);
----------------
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?


================
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);
----------------
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?


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


================
Comment at: lldb/source/Symbol/LocateSymbolFile.cpp:368
+        if (num_specs == 2) {
+          // Special case to handle both i386 and i686 from ObjectFilePECOFF
+          ModuleSpec mspec2;
----------------
What produces this special case?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126367