This is an archive of the discontinued LLVM Phabricator instance.

SymbolVendor: Have plugins return symbol files directly
Needs ReviewPublic

Authored by labath on Aug 8 2019, 7:29 AM.

Details

Summary

This is the last patch in the SymbolVendor series. Now that the symbol
vendors don't do anything, we can avoid creating their instances
completely, and just return SymbolFiles directly, removing a lot of
boilerplate in the process.

SymbolVendors still remain responsible for finding symbol files, and I've
updated all the comments I could find to reflect that.

Event Timeline

labath created this revision.Aug 8 2019, 7:29 AM

So I am confused. Are we keeping SymbolVendor around for locating symbols files or are we getting rid of it entirely?

include/lldb/Core/Module.h
989–996

Maybe we _just_ have "m_symfiles" here and document that the last entry is always the current symbol file? No need for two ivars here?

include/lldb/Symbol/SymbolVendor.h
12

Just delete this entire file?

source/Core/Module.cpp
1049

rename to SymbolFile::FindPlugin?

source/Core/PluginManager.cpp
1743

Rename to SymbolFile?

1745

ditto

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.

Greg, any thoughts on my last comment? My biggest question is that if we remove the SymbolVendor completely, then where will the code that's currently in SymbolVendorELF and SymbolVendorMacOS go to...

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. So sounds like refactoring around that is a good idea. I am fine with this patch standing and addressing that refactor in a next patch?

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.

labath updated this revision to Diff 216782.Aug 23 2019, 1:57 AM

merge m_old_symfiles and m_symfiles_up. I am not entirely satisfied with how I implemented that, but then again the original implementation wasn't totally clean either.