[Lldb-commits] [PATCH] D65952: SymbolVendor: Have plugins return symbol files directly

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D65952#1641539 <https://reviews.llvm.org/D65952#1641539>, @clayborg wrote:

> In D65952#1624799 <https://reviews.llvm.org/D65952#1624799>, @labath wrote:
>
> > In D65952#1623297 <https://reviews.llvm.org/D65952#1623297>, @clayborg wrote:
> >
> > > So I am confused. Are we keeping SymbolVendor around for locating symbols files or are we getting rid of it entirely?
> >
> >
> > Well... right now my idea was to keep it around as an class with just some static methods for the purposes of locating symbol files, though I'm open to other ideas too. After this patch the SymbolVendor class will contain just one method (FindPlugin), but I think that in the future it could be used to host functionality that is common for different symbol vendors. Here, I'm mainly thinking of `Symbols/LocateSymbolFile.cpp` (formerly `Host/Symbols.cpp`), which contains a bunch of functionality for searching for symbol files (so it could fall under the symbol vendor mandate), and it is currently being called by other symbol vendors to do their job. However, it is also being called from placed other than symbol vendors, and so more refactoring would be needed for that to fit in neatly.
>
>
> IMHO: The functionality that locates symbol files should be moved into the Platform and we can get rid of SymbolVendor completely.


This sounded like an interesting idea at first, because Platforms already do some work with finding files and stuff. However, after thinking about it a bit, I am not so sure anymore :). The reason for that is twofold:

- Platform objects are tied to a Debugger (they don't reference it directly, but their instances are owned by the Debugger object), where as Modules are trying to be independent of the Debuggers. This would make it tricky for example to fetch a platform instance when searching for a symbol file.
- even if we work around that (say by making the search logic a static member of the platform class), then there still the issue that some symbol finding logic is not really tied to any particular platform. It is true that the two current symbol vendors could be assigned to platforms relatively cleanly (SymbolVendorELF -> PlatformPOSIX, SymbolVendorMacOSX -> PlatformDarwin), but then we'd have a problem with where to put the Breakpad finding logic (it isn't implemented yet, but I am hoping to get around to it eventually). Breakpad system for searching for symbol files involves directory structures like `$SYMBOL_NAME/$UUID/$SYMBOL_NAME.sym` (e.g. `ConsoleApplication1.pdb/897DD83EA8C8411897F3A925EE4BF7411/ConsoleApplication1.sym`). I think this system is inspired by some Microsoft scheme, but breakpad uses it everywhere, which means there isn't a single platform class that could house this functionality.

I also considered putting this functionality inside the SymbolFile classes. That would solve the `Debugger` problem, and it would definitely work for breakpad. However, it would mean munging ELF and MacOS symbol vendors into the DWARF symbol file, which has enough on its plate already (though maybe there is a case to be made for moving SymbolFileDWARFDebugMap into a separate plugin, in which case the symbol vendor could go there -- however that's pretty hard to do right now).

So, overall, I think its still best to keep symbol finding/vending as a separate piece of functionality, albeit without a a real (isntantiatable) class backing it. Theoretically, we could keep this as a separate entry point in the PluginManager, but get rid of the `Plugins/SymbolVendor` directories by spreading the implementation over various other plugin kinds (breakpad vendor -> SymbolFileBreakpad, "elf" vendor -> ObjectFileELF (?), macos vendor -> PlatformDarwin), but given that they these folders already exist, I don't see much of a reason for changing them.


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

https://reviews.llvm.org/D65952