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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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) |
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. |
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 | ||
746 | ||
lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp | ||
30–31 | We have two options here: 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 | ||
15 | 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. | |
38–43 | If we don't inherit from lldb_private::ObjectFile, the we don't need the LLVM RTTI stuff either. | |
45–81 | You can remove all "override" functions if you don't inherit from lldb_private::ObjectFile and inherit only from PluginInterface. | |
90–91 | Remove | |
93–96 | Remove, since minidump files are not object files. |
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 | ||
---|---|---|
30–31 | 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. |
lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp | ||
---|---|---|
31 | 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); |
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: 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. |
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:
| |
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. |
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... |
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. |
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! |
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... |
Add error for dump core style other than stack and change procedure getting the memory list stream.
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. |
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? |
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... |
lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp | ||
---|---|---|
121 | https://llvm.org/docs/CodingStandards.html#error-and-warning-messages Don't capitalize messages and don't append . | |
733 | The format specifiers are wrong. size_t should use %zd and one uint64_t below should use PRIu64. I have fixed them. |
clang-tidy: error: 'lldb/Core/Architecture.h' file not found [clang-diagnostic-error]
not useful