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

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

Thanks for moving over into object file. See inlined comments, we should be able to get this down to just the SaveCore and other static functions that just return nothing. Let me know if the inlined comments don't make sense.



================
Comment at: lldb/source/Commands/CommandObjectProcess.cpp:1235
     SaveCoreStyle m_requested_save_core_style;
+    std::string m_requested_plugin_name;
   };
----------------
Make this a ConstString


================
Comment at: lldb/source/Commands/CommandObjectProcess.cpp:1242
     if (process_sp) {
-      if (command.GetArgumentCount() == 1) {
+      ConstString plugin_name(m_options.m_requested_plugin_name.c_str());
+      if (command.GetArgumentCount() <= 2) {
----------------
Remove this if we switch m_requested_plugin_name to be a ConstString. Also a quick FYI: ConstString has a std::string constructor, so there is no need to add ".c_str()" here if this code was going to stay.


================
Comment at: lldb/source/Commands/CommandObjectProcess.cpp:1243
+      ConstString plugin_name(m_options.m_requested_plugin_name.c_str());
+      if (command.GetArgumentCount() <= 2) {
         FileSpec output_file(command.GetArgumentAtIndex(0));
----------------
We haven't changed the number of arguments here right? Options and their option values to no count towards the argument count. So all of these have just one argument:
```
(lldb) process save-core <file>
(lldb) process save-core -p <plugin> <file>
(lldb) proicess save-core -s <style> <file>
(lldb) process save-core -p <plugin> -s <style> <file>
```
The one argument is <file>


================
Comment at: lldb/source/Commands/CommandObjectProcess.cpp:1247
+        Status error = PluginManager::SaveCore(process_sp, output_file,
+                                               corefile_style, plugin_name);
         if (error.Success()) {
----------------



================
Comment at: lldb/source/Commands/Options.td:746
+  def process_save_core_plugin_name : Option<"plugin-name", "p">,
+    OptionalArg<"Plugin">, Desc<"Set plugin name that creates a dump file.">;
 }
----------------



================
Comment at: lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp:30-31
+      GetPluginNameStatic(), GetPluginDescriptionStatic(), CreateInstance,
+      /*create_memory_callback=*/nullptr,
+      /*get_module_specifications=*/nullptr, SaveCore);
+}
----------------
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(...).


================
Comment at: lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.h:15
+
+class ObjectFileMinidump : public lldb_private::ObjectFile {
+public:
----------------
Since minidump files are not object files, you can just inherit from lldb_private::PluginInterface. Then you don't need to override any pure virtual functions from lldb_private::ObjectFile. We should add a comment before this class stating something like:

```
///  ObjectFileMinidump is created only to be able to save minidump core files from existing processes with the ObjectFileMinidump::SaveCore function. Minidump files are not ObjectFile objects, but they are core files and currently LLDB's ObjectFile plug-ins handle emitting core files. If the core file saving ever moves into a new plug-in type within LLDB, this code should move as well, but for now this is where this belongs architecturally.
```



================
Comment at: lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.h:38-43
+  // LLVM RTTI support
+  static char ID;
+  bool isA(const void *ClassID) const override {
+    return ClassID == &ID || ObjectFile::isA(ClassID);
+  }
+  static bool classof(const ObjectFile *obj) { return obj->isA(&ID); }
----------------
If we don't inherit from lldb_private::ObjectFile, the we don't need the LLVM RTTI stuff either.


================
Comment at: lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.h:45-81
+  // ObjectFile Protocol.
+
+  bool ParseHeader() override;
+
+  lldb::ByteOrder GetByteOrder() const override {
+    return m_arch.GetByteOrder();
+  }
----------------
You can remove all "override" functions if you don't inherit from lldb_private::ObjectFile and inherit only from PluginInterface.



================
Comment at: lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.h:90-91
+private:
+  lldb_private::ArchSpec m_arch;
+  lldb_private::UUID m_uuid;
+
----------------
Remove


================
Comment at: lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.h:93-96
+  ObjectFileMinidump(const lldb::ModuleSP &module_sp,
+                     lldb::DataBufferSP &data_sp, lldb::offset_t data_offset,
+                     const lldb_private::FileSpec *file,
+                     lldb::offset_t file_offset, lldb::offset_t length);
----------------
Remove, since minidump files are not object files.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108233