This is an archive of the discontinued LLVM Phabricator instance.

WIP: Add minidump save-core functionality to ELF object files
ClosedPublic

Authored by Aj0SK on Aug 17 2021, 12:30 PM.

Details

Summary

This change adds save-core functionality into the ObjectFileELF that enables
saving minidump of a stopped process. This change is mainly targeting Linux
running on x86_64 machines. Minidump should contain basic information needed
to examine state of threads, local variables and stack traces. Full support
for other platforms is not so far implemented. API tests are using LLDB's
MinidumpParser.

Diff Detail

Event Timeline

Aj0SK created this revision.Aug 17 2021, 12:30 PM
Aj0SK requested review of this revision.Aug 17 2021, 12:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2021, 12:30 PM
clayborg requested changes to this revision.Aug 17 2021, 3:35 PM

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.

lldb/source/Plugins/ObjectFile/ELF/MinidumpFileBuilder.cpp
70 ↗(On Diff #366982)

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?

80 ↗(On Diff #366982)

We should be able to populate this correctly using the triple right? And error out if we don't support the OS?

lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
725–727 ↗(On Diff #366982)

remove braces for single statement if (LLVM coding guidelines)

730–732 ↗(On Diff #366982)

remove braces for single statement if (LLVM coding guidelines)

738–740 ↗(On Diff #366982)

remove braces for single statement if (LLVM coding guidelines)

743–745 ↗(On Diff #366982)

remove braces for single statement if (LLVM coding guidelines)

748–750 ↗(On Diff #366982)

remove braces for single statement if (LLVM coding guidelines)

762–764 ↗(On Diff #366982)

remove braces for single statement if (LLVM coding guidelines)

clayborg added inline comments.Aug 17 2021, 3:35 PM
lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
751 ↗(On Diff #366982)

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.

This revision now requires changes to proceed.Aug 17 2021, 3:35 PM
Aj0SK added a subscriber: werat.Aug 18 2021, 1:11 AM
Aj0SK updated this revision to Diff 368110.Aug 23 2021, 8:05 AM

Move minidump save-core functionality to separate plugin

clayborg requested changes to this revision.Aug 24 2021, 4:33 PM

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.

lldb/source/Commands/CommandObjectProcess.cpp
1235

Make this a ConstString

1242

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.

1243

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>

1247
lldb/source/Commands/Options.td
753
lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
31–32

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(...).

lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.h
16

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.
39–44

If we don't inherit from lldb_private::ObjectFile, the we don't need the LLVM RTTI stuff either.

46–82

You can remove all "override" functions if you don't inherit from lldb_private::ObjectFile and inherit only from PluginInterface.

91–92

Remove

94–97

Remove, since minidump files are not object files.

This revision now requires changes to proceed.Aug 24 2021, 4:33 PM
Aj0SK marked 9 inline comments as done.Aug 25 2021, 6:35 AM

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.

lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
31–32

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.

clayborg added inline comments.Aug 25 2021, 2:03 PM
lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
32

It should be fine to not inherit from ObjectFile and yes I agree, probably best to not modify other code.

All you need to provide is static function callbacks that have the correct static function signature, and that doesn't require your class to inherit from ObjectFile. You already have a CreateInstance function to see how to do this. The CreateInstance(...) function you have can do nothing and just return nullptr.

Then you need to make a create memory callback and a get module specifications:

ObjectFile *ObjectFileMinidump::CreateMemoryInstance(
    const lldb::ModuleSP &module_sp, DataBufferSP &data_sp,
    const ProcessSP &process_sp, lldb::addr_t header_addr) {
  return nullptr;
}

size_t ObjectFileMinidump::GetModuleSpecifications(
    const lldb_private::FileSpec &file, lldb::DataBufferSP &data_sp,
    lldb::offset_t data_offset, lldb::offset_t file_offset,
    lldb::offset_t length, lldb_private::ModuleSpecList &specs) {
  specs.Clear();
  return 0;
}

Then your registration call looks like:

PluginManager::RegisterPlugin(
  GetPluginNameStatic(), GetPluginDescriptionStatic(), CreateInstance,
      CreateMemoryInstance, GetModuleSpecifications, SaveCore);
Aj0SK marked 3 inline comments as done.Aug 25 2021, 3:46 PM
Aj0SK updated this revision to Diff 368765.Aug 25 2021, 3:47 PM

Change ObjectFileMinidump plugin to inherit from PluginInterface

Very nice! Structure is good now. I found a few other issue we should fix as well.

lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
226

I am not sure if the object file's full size is the right value here. I think the right value would be the last contiguous loaded address range for a given file. This information is going to be used to set the load addresses of any sections in the object file when it is loaded, and if we have a huge ELF file that has tons of debug info, this is going to make the loaded address range way too big.

So the algorithm I would suggest is:
1 - grab the section that contains base address:
2 - in a loop, grab the next section that starts at the end of this section, and as long as the addresses are contiguous, increase the SizeOfImage:

So something like:

lldb::SectionSP sect_sp = mod->GetObjectFile()->GetBaseAddress().GetSection();
uint64_t SizeOfImage = 0;
if (sect_sp) {
  lldb::addr_t sect_addr = sect_sp->GetLoadAddress(&target);
  // Use memory size since zero fill sections, like ".bss", will be smaller on disk.
  lldb::addr_t sect_size = sect_sp->MemorySize(); 
  // This will usually be zero, but make sure to calculate the BaseOfImage offset.
  const lldb::addr_t base_sect_offset = m.BaseOfImage - sect_addr;
  SizeOfImage = sect_size - base_sect_offset;
  lldb::addr_t next_sect_addr = sect_addr + sect_size;
  Address sect_so_addr;
  target.ResolveLoadAddress(next_sect_addr, sect_so_addr);
  lldb::SectionSP next_sect_sp = sect_so_addr.GetSections();
  while (next_sect_sp && next_sect_sp->GetLoadBaseAddress(&target) == next_sect_addr)) {
    sect_size = sect_sp->MemorySize(); 
    SizeOfImage += sect_size;
    next_sect_addr += sect_size;
    target.ResolveLoadAddress(next_sect_addr, sect_so_addr);
    next_sect_sp = sect_so_addr.GetSections();
  }
  m.SizeOfImage = static_cast<llvm::support::ulittle32_t>(SizeOfImage);
} else {
  m.SizeOfImage = static_cast<llvm::support::ulittle32_t>(sect_size);
}
520

This should take in the CoreStyle and only emit what was asked for.

If style is set to "eSaveCoreStackOnly", then grab only the memory regions for each thread stack pointer and save only those.

We can't tell from LLDB APIs if a page is dirty, so if we get "eSaveCoreDirtyOnly", then we will need to save all memory regions that have write permissions.

If it is either "eSaveCoreFull" or "eSaveCoreUnspecified" then save everything.

668

Do we need to make sure size is not zero?

lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
2–4

Fix this header comment line to be on one line.

Aj0SK added inline comments.Aug 26 2021, 7:01 AM
lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
226

Thank you for the suggestion. I will add it to the next revision.

520

I agree that this code should take care of a CoreStyle but it poses a significant problem to this implementation as it was mainly designed to save smaller Minidumps. This solution stores all of the information in memory before dumping them (this eases an implementation quite a bit because of the way how Minidump pointers (offsets) are working). This implementation, on my machine, exhausted memory before the minidump could be written in case of a full memory dump. At this time, we don't plan to reimplement this solution in the way it would allow to store all the data on disc at the time of minidump creation so there are two possible solutions that I see:

  1. If the type of the CoreStyle is full or dirty, this plugin will return false from the SaveCore function. Then maybe the default CoreStyle for this plugin should be changed to "eSaveCoreStackOnly".
  1. To the best of my knowledge, it should be theoretically possible to reimplement Dump() method to sort of take special care of a MemoryListStream, dumping also the memory information at the end of the minidump file as I think the memory dump is the most stressful for the memory and otherwise there is no problem with this.
668

I can add this. The idea was that even if the file is "empty", we at least give the user opening the minidump the information that we have been able to find this file and add it to the minidump instead of only not providing any information at all.

Aj0SK added inline comments.Aug 27 2021, 5:06 AM
lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
520

To state my preference, I would rather stick to the first option and landed a minimum viable product. The only reason for this is that I have this version way better tested and also I am not entirely sure about how minidump deals with the whole memory dumps as it can be a little problematic for a big memory chunks... Then, I would rather add the necessary changes in the next patch...

Aj0SK added a subscriber: jarin.Aug 27 2021, 6:41 AM
clayborg requested changes to this revision.Aug 27 2021, 10:52 AM

No worries on only saving out the minimal for now, but just make sure that you error out if CoreStyle is anything but "eSaveCoreUnspecified" or "eSaveCoreStackOnly".

lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
520

That is fine. I was just looking at the implementation below and I think I might know why this was taking up too much space. I will comment below, and maybe this will fix the issues of storing way too much info in the miniump

523

FYI: memory region for address zero often has no permissions, so there is no need to actually try and save this memory. For any memory region be sure the check if it has permissions using:

if (region_info.GetLLDBPermissions() != 0)

before trying to read or save this off. I will comment below

531

Should we have a std::set<lldb::addr_t> to store the base of any regions that have already been saved? Many PC values could be in the same regions (like when many threads are waiting in syscalls).

545–547

Why are we checking the region_info for address zero here? The first time through this loop, it will contain information for address zero which will be a region that has no permissions. I suggest a new main loop in the comment below which simplifies this entire logic and also makes sure we don't emit the same region more than once.

550

Shouldn't this be the main loop that would replace the "while (range_info.GetRange().GetRangeBase() != LLDB_INVALID_ADDRESS)" loop? I would think that the main loop wiould be something like:

std::set<addr_t> visited_region_base_addresses;
for (size_t interesting_address : interesting_addresses) {
  error = process_sp->GetMemoryRegionInfo(interesting_address, range_info);
  // Skip failed memory region requests or any regions with no permissions.
  if (error.Fail() || range_info.GetPermissions() == 0)
    continue;
  const addr_t addr = range_info.GetRange().GetRangeBase();
  // Skip any regions we have already saved out.
  if (visited_region_base_addresses.insert(addr).second == false)
    continue;
  const addr_t size = range_info.GetRange().GetByteSize();
  if (size == 0)
    continue;
  auto data_up = std::make_unique<DataBufferHeap>(size, 0);
  const size_t bytes_read = process_sp->ReadMemory(addr, data_up->GetBytes(), size, error);
  if (bytes_read == 0)
    continue;
  // We have a good memory region with valid bytes to store.
  LocationDescriptor memory_dump;
  memory_dump.DataSize = static_cast<llvm::support::ulittle32_t>(size);
  memory_dump.RVA =static_cast<llvm::support::ulittle32_t>(GetCurrentDataEndOffset());
  MemoryDescriptor memory_desc;
  memory_desc.StartOfMemoryRange = static_cast<llvm::support::ulittle64_t>(addr);
  memory_desc.Memory = memory_dump;
  mem_descriptors.push_back(memory_desc);
  m_data.AppendData(data_up->GetBytes(), bytes_read);
}
552

Make sure the memory region has any permissions. This returns a logic OR bitmask of lldb::Permissions bits:

enum Permissions {
  ePermissionsWritable = (1u << 0),
  ePermissionsReadable = (1u << 1),
  ePermissionsExecutable = (1u << 2)};
};

No permissions will return 0;

569

You should listen to the return value from this ReadMemory call and only write this many bytes to the memory region. See my above proposed full loop source code abiove, as it reads the memory first before creating the LocationDescriptor and MemoryDescriptor and it skips to the next address in the loop if things go bad during memory read. It also stores the number if bytes that were actually read in case the process memory read comes back with fewer or no bytes.

This revision now requires changes to proceed.Aug 27 2021, 10:52 AM
Aj0SK added inline comments.Aug 29 2021, 6:09 AM
lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
550

This kind of an approach was something I also thought about and in fact, I was mainly inspired by this approach. Also, the previous comment about the need of saving the loaded modules addresses in a set doesn't apply to my solution, am I correct? Just to clear it a little bit, I like your solution though and it's better when I think about it. Thanks for the suggestion!

Aj0SK marked 8 inline comments as done and an inline comment as not done.Aug 30 2021, 3:46 AM
Aj0SK added inline comments.
lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
520

In fact, we are mostly interested in the client-server setup where every spared MB counts. In case it would be also benefiting for LLDB, I can also come up with an implementation that doesn't save full 8MB per stack but also limits this... Most of the solutions, that were used for implementing this functionality(breakpad, crashpad) limit stack information etc. breakpad.

523

Thanks, good to know.

531

In the proposed approach, I believe so. In the old approach this was taken care of by the fact that we only went through the every segment only once.

550

One thing, that could be a little problematic with the new approach is, if the possibility to save full/dirty pages dump would be requested... In this case, we still need to go through all of the sections...

569

Also done this on other places where I use ReadMemory...

Aj0SK updated this revision to Diff 369410.Aug 30 2021, 3:50 AM
Aj0SK marked 2 inline comments as done and an inline comment as not done.

Add error for dump core style other than stack and change procedure getting the memory list stream.

clayborg accepted this revision.Aug 30 2021, 2:01 PM

Thanks for all of the changes! Looks great.

lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
550

Correct, we would just iterate over all memory regions and get the next region by asking for the region that starts at the end address of the current region.

This revision is now accepted and ready to land.Aug 30 2021, 2:01 PM
This revision was landed with ongoing or failed builds.Aug 31 2021, 4:05 AM
This revision was automatically updated to reflect the committed changes.
Aj0SK reopened this revision.Aug 31 2021, 6:02 AM
This revision is now accepted and ready to land.Aug 31 2021, 6:02 AM
Aj0SK updated this revision to Diff 369686.EditedAug 31 2021, 6:02 AM

Fixes arm and aarch64 build run fails. Adding aarch64 to the matching in SystemInfo stream and activating test only on x86_64 as this is the only platform where also thread info etc. is being saved.

Aj0SK requested review of this revision.Aug 31 2021, 8:20 AM
Aj0SK updated this revision to Diff 369722.Aug 31 2021, 9:48 AM

Fix not-correctly applied change from review regarding memory reading.

clayborg added inline comments.Aug 31 2021, 2:33 PM
lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
18

Do we only support x86_64 right now? If we are actually supporting other architectures and emitting a minidump for them, we should be testing them. Do we return an error for any architectures that we don't support in the ObjectFIleMinidump::SaveCore(...) function?

Aj0SK added inline comments.Aug 31 2021, 3:12 PM
lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
18

We only support "full-featured" minidump for the x86_64. For some platforms, listed in AddSystemInfo method we provide minidump with only basic information(SystemInfo, ModuleInfo and MiscInfo streams). For all other platforms, no minidump is created and it will error-out...

clayborg accepted this revision.Aug 31 2021, 3:19 PM
This revision is now accepted and ready to land.Aug 31 2021, 3:19 PM
This revision was landed with ongoing or failed builds.Sep 1 2021, 6:19 AM
This revision was automatically updated to reflect the committed changes.
MaskRay added inline comments.Sep 13 2021, 1:42 PM
lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
120
732

The format specifiers are wrong. size_t should use %zd and one uint64_t below should use PRIu64.

I have fixed them.