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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
