[Lldb-commits] [PATCH] D108233: WIP: Add minidump save-core functionality to ELF object files

[Andrej Korman via Phabricator via lldb-commits]

Aj0SK marked 9 inline comments as done.
Aj0SK added a comment.

Thanks for the review! Some requested changes need to be clarified for me. I have a problem mainly with successfully registering the Plugin when it inherits only from PluginInterface.



================
Comment at: lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp:30-31
+      GetPluginNameStatic(), GetPluginDescriptionStatic(), CreateInstance,
+      /*create_memory_callback=*/nullptr,
+      /*get_module_specifications=*/nullptr, SaveCore);
+}
----------------
clayborg wrote:
> We have two options here:
> 1 - if we want to pass NULL for any callbacks, then we need to fix all callsites to that use PluginManager::GetObjectFileCreateMemoryCallbackAtIndex(...) and PluginManager::GetObjectFileGetModuleSpecificationsCallbackAtIndex(...) to have this done inside of new plug-in manager PluginManager::CreateMemoryCallbackAtIndex(...) and PluginManager::GetModuleSpecifications(...) where these new functions iterate over all plug-ins and skip any plug-ins that have NULL callbacks.
> 2 - add valid create_memory_callback and get_module_specifications callbacks that return invalid values
> 
> Otherwise this plug-in might stop the iteration early for existing callers of PluginManager::GetObjectFileCreateMemoryCallbackAtIndex(...) and PluginManager::GetObjectFileGetModuleSpecificationsCallbackAtIndex(...).
I would prefer modifying code outside of this functionality as little as possible so I would stick to the second option. However, maybe returning the invalid values is also not the best solution. Also, does this option works with ObjectFileMinidump inheriting from PluginInterface? I think that I am unable to use the same PluginManager::RegisterPlugin method, so not sure how this plugin gets to ObjectFilesInstances... Maybe I am missing out something. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108233