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

[Alvin Wong via Phabricator via lldb-commits]

alvinhochun added inline comments.


================
Comment at: lldb/source/Plugins/SymbolVendor/PECOFF/SymbolVendorPECOFF.cpp:119
+  };
+  for (SectionType section_type : g_sections) {
+    if (SectionSP section_sp =
----------------
mstorsjo wrote:
> 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?
> So essentially, if GetDebugLink() would be a virtual method, both SymbolVendorELF and SymbolVendorPECOFF could theoretically be merged into one?

Yes, they may very well be merged if no other platform-specific logic is needed. There are some small differences now: SymbolVendorELF checks that `obj_file->GetUUID()` is valid, but this fails the test I added so I removed this check in SymbolVendorPECOFF. I also removed a few entries (those that doesn't show up in `ObjectFilePECOFF`) from the `g_sections` list, but this change is probably not necessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126367