clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
I like the idea of this! I have a minidump creation tool I wrote in python for
making tests.
ELF files support core files in their native file format, so I think the
ObjectFileELF::SaveCore(...) should actually save an ELF core file if it saves
anything. So we should move the minidump creation to another location. I
mention ideas below.
When running "process save-core" we can add a new option like
"--plugin=<name>", and if this option is specified we will look for a plug-in
name that matches when iterating over the instances that support core file
exporting. Right now the save core calls the plugin manager:
Status PluginManager::SaveCore(const lldb::ProcessSP &process_sp,
const FileSpec &outfile,
lldb::SaveCoreStyle &core_style) {
Status error;
auto &instances = GetObjectFileInstances().GetInstances();
for (auto &instance : instances) {
if (instance.save_core &&
instance.save_core(process_sp, outfile, core_style, error))
return error;
}
error.SetErrorString(
"no ObjectFile plugins were able to save a core for this process");
return error;
}
Currently we are expecting only one ObjectFile class to return true for any
given process. But since ObjectFileELF::SaveCore(...) could eventually be added
to ObjectFileELF, we need a way to force a plug-in to be considered. If we add
an extra "ConstString plugin_name" to the arguments, we can check the name and
target a different plug-in. It should be possible to save a core file in
different formats.
So I propose:
- remove code from ELF ObjectFile plug-in
- modify PluginManager::SaveCore(...) to take an extra "ConstString
plugin_name" parameter.
- If this parameter is valid, skip any "instance" whose name doesn't match
- Modify the ObjectFileSaveCore callback definition to take a "bool force"
parameter which will make the plug-ins skip the triple detection stuff that
will skip saving the core file if each SaveCore(...) function didn't like the
triple it was passed. It is fine for a plug-in to return an error stating that
saving a core file with the current process doesn't make sense since each core
file has to have support for saving registers.
- make a new Minidump ObjectFile plug-in at
"lldb/source/Plugins/ObjectFile/Minidump"
- Create a lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp and
lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.h
- when you register the callbacks for this plug-in, only register the
ObjectFileMinidump::SaveCore callback (see example code below)
void ObjectFileMinidump::Initialize() {
PluginManager::RegisterPlugin(
GetPluginNameStatic(),
GetPluginDescriptionStatic(),
/*create_callback=*/nullptr,
/*create_memory_callback=*/nullptr,
/*get_module_specifications=*/nullptr,
SaveCore);
}
Then hopefully you can just make a very ObjectFileMinidump with a few plug-in
template functions and the SaveCore(...) function. Let me know if any of this
is not clear.
================
Comment at: lldb/source/Plugins/ObjectFile/ELF/MinidumpFileBuilder.cpp:70
+ default:
+ arch = ProcessorArchitecture::AMD64;
+ break;
----------------
Seems like we should return an error here if the architecture isn't supported
with a nice error string that gets displayed. We shouldn't default to AMD64
right?
================
Comment at: lldb/source/Plugins/ObjectFile/ELF/MinidumpFileBuilder.cpp:80
+ GetCurrentDataEndOffset() + sizeof(llvm::minidump::SystemInfo));
+ sys_info.PlatformId = OSPlatform::Linux;
+ m_data.AppendData(&sys_info, sizeof(llvm::minidump::SystemInfo));
----------------
We should be able to populate this correctly using the triple right? And error
out if we don't support the OS?
================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:725-727
+ if (error.Fail()) {
+ return false;
+ }
----------------
remove braces for single statement if (LLVM coding guidelines)
================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:730-732
+ if (error.Fail()) {
+ return false;
+ }
----------------
remove braces for single statement if (LLVM coding guidelines)
================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:738-740
+ if (error.Fail()) {
+ return false;
+ }
----------------
remove braces for single statement if (LLVM coding guidelines)
================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:743-745
+ if (error.Fail()) {
+ return false;
+ }
----------------
remove braces for single statement if (LLVM coding guidelines)
================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:748-750
+ if (error.Fail()) {
+ return false;
+ }
----------------
remove braces for single statement if (LLVM coding guidelines)
================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:751
+ }
+ }
+
----------------
The minidump parser in LLDB supports many linux specific streams (see
llvm/include/llvm/BinaryFormat/MinidumpConstants.def):
```
HANDLE_MDMP_STREAM_TYPE(0x47670003, LinuxCPUInfo) // /proc/cpuinfo
HANDLE_MDMP_STREAM_TYPE(0x47670004, LinuxProcStatus) // /proc/$x/status
HANDLE_MDMP_STREAM_TYPE(0x47670005, LinuxLSBRelease) // /etc/lsb-release
HANDLE_MDMP_STREAM_TYPE(0x47670006, LinuxCMDLine) // /proc/$x/cmdline
HANDLE_MDMP_STREAM_TYPE(0x47670007, LinuxEnviron) // /proc/$x/environ
HANDLE_MDMP_STREAM_TYPE(0x47670008, LinuxAuxv) // /proc/$x/auxv
HANDLE_MDMP_STREAM_TYPE(0x47670009, LinuxMaps) // /proc/$x/maps
HANDLE_MDMP_STREAM_TYPE(0x4767000A, LinuxDSODebug)
HANDLE_MDMP_STREAM_TYPE(0x4767000B, LinuxProcStat) // /proc/$x/stat
HANDLE_MDMP_STREAM_TYPE(0x4767000C, LinuxProcUptime) // uptime
HANDLE_MDMP_STREAM_TYPE(0x4767000D, LinuxProcFD) // /proc/$x/fd
```
Many of these could easily be added to the file as they are just a copy of
these files from disk.
================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:762-764
+ if (error.Fail()) {
+ return false;
+ }
----------------
remove braces for single statement if (LLVM coding guidelines)
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
