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

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added inline comments.


================
Comment at: lldb/source/Plugins/SymbolVendor/PECOFF/SymbolVendorPECOFF.cpp:119
+  };
+  for (SectionType section_type : g_sections) {
+    if (SectionSP section_sp =
----------------
alvinhochun wrote:
> mstorsjo wrote:
> > I'm curious - this adds new logic (copied from SymbolVendorELF afaik?) for iterating over sections and finding the ones that contain the dwarf debug info. Prior to this change, finding debug info within the executable itself has worked just fine.
> > 
> > What codepath and where has handled that? Has it fallen back on SymbolVendorELF so far?
> > 
> > (If that used to work, why is a specific plugin for PECOFF needed at this point for debuglink?)
> The SymbolVendorELF seems to only handle loading the external debug info specified by the `.gnu_debuglink` section. The dwarf debug info already embedded in the main executable should be handled somewhere else, which has worked fine for both ELF and PE/COFF. Though I don't know where specifically this was handled.
> 
> SymbolVendorELF tries to downcast the object file to `ObjectFileELF *`, so it could not have worked for PE/COFF.
Ok, so what I misunderstood is that this function doesn't seem to handle dwarf debug info in the main executable after all - this only tries to dig up dwarf sections from `GetSymbolFileFileSpec()` or `GetDebugLink()`. So the existing code that locates dwarf sections in the executables themselves still runs as before.

So then this seems reasonable.

So essentially, if `GetDebugLink()` would be a virtual method, both SymbolVendorELF and SymbolVendorPECOFF could theoretically be merged into one?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126367