[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.

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