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
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
