From 810865580a4d8ea3f48e56f3964602fba6087ddf Mon Sep 17 00:00:00 2001 From: Jacob Lalonde Date: Wed, 12 Jun 2024 13:57:10 -0700 Subject: [PATCH 01/16] Squash minidump-64-protoype. This is a change to reorder LLDB's minidumps to support 64b memory lists/minidumps while still keeping all directories under the 32b limit. Better O(N), not N^2 cleanup code --- .../Minidump/MinidumpFileBuilder.cpp | 520 ++++++++++++++---- .../ObjectFile/Minidump/MinidumpFileBuilder.h | 55 +- .../Minidump/ObjectFileMinidump.cpp | 38 +- llvm/include/llvm/BinaryFormat/Minidump.h | 11 + llvm/include/llvm/Object/Minidump.h | 7 + llvm/lib/ObjectYAML/MinidumpEmitter.cpp | 7 + 6 files changed, 482 insertions(+), 156 deletions(-) diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp index 7231433619ffb..c08b5206720ab 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp +++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp @@ -20,35 +20,106 @@ #include "lldb/Target/RegisterContext.h" #include "lldb/Target/StopInfo.h" #include "lldb/Target/ThreadList.h" +#include "lldb/Utility/DataBufferHeap.h" #include "lldb/Utility/DataExtractor.h" #include "lldb/Utility/LLDBLog.h" #include "lldb/Utility/Log.h" +#include "lldb/Utility/LLDBLog.h" #include "lldb/Utility/RegisterValue.h" #include "llvm/ADT/StringRef.h" #include "llvm/BinaryFormat/Minidump.h" #include "llvm/Support/ConvertUTF.h" +#include "llvm/Support/Endian.h" #include "llvm/Support/Error.h" #include "Plugins/Process/minidump/MinidumpTypes.h" +#include "lldb/lldb-enumerations.h" +#include "lldb/lldb-forward.h" +#include "lldb/lldb-types.h" #include +#include +#include +#include +#include +#include using namespace lldb; using namespace lldb_private; using namespace llvm::minidump; -void MinidumpFileBuilder::AddDirectory(StreamType type, size_t stream_size) { +Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories(const lldb_private::Target &target, const lldb::ProcessSP &process_sp) { + // First set the offset on the file, and on the bytes saved + //TODO: advance the core file. + m_saved_data_size += header_size; + // We know we will have at least Misc, SystemInfo, Modules, and ThreadList (corresponding memory list for stacks) + // And an additional memory list for non-stacks. + m_expected_directories = 6; + // Check if OS is linux + if (target.GetArchitecture().GetTriple().getOS() == + llvm::Triple::OSType::Linux) + m_expected_directories += 9; + + // Go through all of the threads and check for exceptions. + lldb_private::ThreadList thread_list = process_sp->GetThreadList(); + const uint32_t num_threads = thread_list.GetSize(); + for (uint32_t thread_idx = 0; thread_idx < num_threads; ++thread_idx) { + ThreadSP thread_sp(thread_list.GetThreadAtIndex(thread_idx)); + StopInfoSP stop_info_sp = thread_sp->GetStopInfo(); + if (stop_info_sp && stop_info_sp->GetStopReason() == StopReason::eStopReasonException) { + m_expected_directories++; + } + } + + // Now offset the file by the directores so we can write them in later. + offset_t directory_offset = m_expected_directories * directory_size; + //TODO: advance the core file. + m_saved_data_size += directory_offset; + // Replace this when we make a better way to do this. + Status error; + Header empty_header; + size_t bytes_written; + bytes_written = header_size; + error = m_core_file->Write(&empty_header, bytes_written); + if (error.Fail() || bytes_written != header_size) { + if (bytes_written != header_size) + error.SetErrorStringWithFormat( + "unable to write the header (written %zd/%zd)", bytes_written, + header_size); + return error; + } + + for (uint i = 0; i < m_expected_directories; i++) { + size_t bytes_written; + bytes_written = directory_size; + Directory empty_directory; + error = m_core_file->Write(&empty_directory, bytes_written); + if (error.Fail() || bytes_written != directory_size) { + if (bytes_written != directory_size) + error.SetErrorStringWithFormat( + "unable to write the directory (written %zd/%zd)", bytes_written, + directory_size); + return error; + } + } + + return error; +} + + +void MinidumpFileBuilder::AddDirectory(StreamType type, uint64_t stream_size) { LocationDescriptor loc; - loc.DataSize = static_cast(stream_size); + loc.DataSize = static_cast(stream_size); // Stream will begin at the current end of data section - loc.RVA = static_cast(GetCurrentDataEndOffset()); + loc.RVA = static_cast(GetCurrentDataEndOffset()); Directory dir; dir.Type = static_cast>(type); dir.Location = loc; m_directories.push_back(dir); + assert (m_expected_directories >= m_directories.size()); } Status MinidumpFileBuilder::AddSystemInfo(const llvm::Triple &target_triple) { @@ -114,7 +185,7 @@ Status MinidumpFileBuilder::AddSystemInfo(const llvm::Triple &target_triple) { sys_info.ProcessorArch = static_cast>(arch); // Global offset to beginning of a csd_string in a data section - sys_info.CSDVersionRVA = static_cast( + sys_info.CSDVersionRVA = static_cast( GetCurrentDataEndOffset() + sizeof(llvm::minidump::SystemInfo)); sys_info.PlatformId = platform_id; m_data.AppendData(&sys_info, sizeof(llvm::minidump::SystemInfo)); @@ -165,7 +236,6 @@ llvm::Expected getModuleFileSize(Target &target, } return SizeOfImage; } - SectionSP sect_sp = mod->GetObjectFile()->GetBaseAddress().GetSection(); if (!sect_sp) { @@ -238,12 +308,10 @@ Status MinidumpFileBuilder::AddModuleList(Target &target) { auto maybe_mod_size = getModuleFileSize(target, mod); if (!maybe_mod_size) { llvm::Error mod_size_err = maybe_mod_size.takeError(); - llvm::handleAllErrors(std::move(mod_size_err), - [&](const llvm::ErrorInfoBase &E) { - error.SetErrorStringWithFormat( - "Unable to get the size of module %s: %s.", - module_name.c_str(), E.message().c_str()); - }); + llvm::handleAllErrors(std::move(mod_size_err), [&](const llvm::ErrorInfoBase &E) { + error.SetErrorStringWithFormat("Unable to get the size of module %s: %s.", + module_name.c_str(), E.message().c_str()); + }); return error; } @@ -513,6 +581,32 @@ findStackHelper(const lldb::ProcessSP &process_sp, uint64_t rsp) { return std::make_pair(addr, size); } +Status MinidumpFileBuilder::FixThreads() { + Status error; + // If we have anything in the heap flush it. + DumpToFile(); + + m_core_file->SeekFromStart(thread_list_file_offset); + for (auto &pair : m_thread_by_range_start) { + // The thread objects will get a new memory descriptor added + // When we are emitting the memory list and then we write it here + llvm::minidump::Thread thread = pair.second; + size_t bytes_to_write = sizeof(llvm::minidump::Thread); + size_t bytes_written = bytes_to_write; + error = m_core_file->Write(&thread, bytes_written); + if (error.Fail() || bytes_to_write != bytes_written) { + error.SetErrorStringWithFormat( + "Wrote incorrect number of bytes to minidump file. (written %zd/%zd)", bytes_written, + bytes_to_write); + return error; + } + } + + // Seek the core file back to the end. + m_core_file->SeekFromEnd(0); + return error; +} + Status MinidumpFileBuilder::AddThreadList(const lldb::ProcessSP &process_sp) { constexpr size_t minidump_thread_size = sizeof(llvm::minidump::Thread); lldb_private::ThreadList thread_list = process_sp->GetThreadList(); @@ -533,7 +627,8 @@ Status MinidumpFileBuilder::AddThreadList(const lldb::ProcessSP &process_sp) { DataBufferHeap helper_data; const uint32_t num_threads = thread_list.GetSize(); - + thread_list_file_offset = GetCurrentDataEndOffset(); + Log *log = GetLog(LLDBLog::Object); for (uint32_t thread_idx = 0; thread_idx < num_threads; ++thread_idx) { ThreadSP thread_sp(thread_list.GetThreadAtIndex(thread_idx)); RegisterContextSP reg_ctx_sp(thread_sp->GetRegisterContext()); @@ -553,38 +648,18 @@ Status MinidumpFileBuilder::AddThreadList(const lldb::ProcessSP &process_sp) { arch.GetTriple().getArchName().str().c_str()); return error; } - uint64_t sp = reg_ctx->GetSP(); - auto expected_address_range = findStackHelper(process_sp, sp); - - if (!expected_address_range) { - consumeError(expected_address_range.takeError()); - error.SetErrorString("Unable to get the stack address."); - return error; - } - - std::pair range = std::move(*expected_address_range); - uint64_t addr = range.first; - uint64_t size = range.second; - auto data_up = std::make_unique(size, 0); - const size_t stack_bytes_read = - process_sp->ReadMemory(addr, data_up->GetBytes(), size, error); - - if (error.Fail()) - return error; - - LocationDescriptor stack_memory; - stack_memory.DataSize = - static_cast(stack_bytes_read); - stack_memory.RVA = static_cast( - size_before + thread_stream_size + helper_data.GetByteSize()); + uint64_t sp = reg_ctx->GetSP(); + MemoryRegionInfo sp_region; + process_sp->GetMemoryRegionInfo(sp, sp_region); + // Emit a blank descriptor MemoryDescriptor stack; - stack.StartOfMemoryRange = static_cast(addr); - stack.Memory = stack_memory; - - helper_data.AppendData(data_up->GetBytes(), stack_bytes_read); - + LocationDescriptor empty_label; + empty_label.DataSize = 0; + empty_label.RVA = 0; + stack.Memory = empty_label; + stack.StartOfMemoryRange = 0; LocationDescriptor thread_context_memory_locator; thread_context_memory_locator.DataSize = static_cast(thread_context.size()); @@ -593,6 +668,8 @@ Status MinidumpFileBuilder::AddThreadList(const lldb::ProcessSP &process_sp) { // Cache thie thread context memory so we can reuse for exceptions. m_tid_to_reg_ctx[thread_sp->GetID()] = thread_context_memory_locator; + LLDB_LOGF(log, "AddThreadList for thread %d: thread_context %zu bytes", + thread_idx, thread_context.size()); helper_data.AppendData(thread_context.data(), thread_context.size()); llvm::minidump::Thread t; @@ -604,9 +681,13 @@ Status MinidumpFileBuilder::AddThreadList(const lldb::ProcessSP &process_sp) { t.EnvironmentBlock = static_cast(0); t.Stack = stack, t.Context = thread_context_memory_locator; + // We save off the stack object so we can circle back and clean it up. + m_thread_by_range_start[sp_region.GetRange().GetRangeBase()] = t; m_data.AppendData(&t, sizeof(llvm::minidump::Thread)); } + LLDB_LOGF(log, "AddThreadList(): total helper_data %zu bytes", + helper_data.GetByteSize()); m_data.AppendData(helper_data.GetBytes(), helper_data.GetByteSize()); return Status(); } @@ -662,64 +743,6 @@ void MinidumpFileBuilder::AddExceptions(const lldb::ProcessSP &process_sp) { } } -lldb_private::Status -MinidumpFileBuilder::AddMemoryList(const lldb::ProcessSP &process_sp, - lldb::SaveCoreStyle core_style) { - Status error; - Process::CoreFileMemoryRanges core_ranges; - error = process_sp->CalculateCoreFileSaveRanges(core_style, core_ranges); - if (error.Fail()) { - error.SetErrorString("Process doesn't support getting memory region info."); - return error; - } - - DataBufferHeap helper_data; - std::vector mem_descriptors; - for (const auto &core_range : core_ranges) { - // Skip empty memory regions. - if (core_range.range.empty()) - continue; - const addr_t addr = core_range.range.start(); - const addr_t size = core_range.range.size(); - auto data_up = std::make_unique(size, 0); - const size_t bytes_read = - process_sp->ReadMemory(addr, data_up->GetBytes(), size, error); - if (error.Fail()) { - Log *log = GetLog(LLDBLog::Object); - LLDB_LOGF(log, "Failed to read memory region. Bytes read: %zu, error: %s", - bytes_read, error.AsCString()); - error.Clear(); - } - if (bytes_read == 0) - continue; - // We have a good memory region with valid bytes to store. - LocationDescriptor memory_dump; - memory_dump.DataSize = static_cast(bytes_read); - memory_dump.RVA = - static_cast(GetCurrentDataEndOffset()); - MemoryDescriptor memory_desc; - memory_desc.StartOfMemoryRange = - static_cast(addr); - memory_desc.Memory = memory_dump; - mem_descriptors.push_back(memory_desc); - m_data.AppendData(data_up->GetBytes(), bytes_read); - } - - AddDirectory(StreamType::MemoryList, - sizeof(llvm::support::ulittle32_t) + - mem_descriptors.size() * - sizeof(llvm::minidump::MemoryDescriptor)); - llvm::support::ulittle32_t memory_ranges_num(mem_descriptors.size()); - - m_data.AppendData(&memory_ranges_num, sizeof(llvm::support::ulittle32_t)); - for (auto memory_descriptor : mem_descriptors) { - m_data.AppendData(&memory_descriptor, - sizeof(llvm::minidump::MemoryDescriptor)); - } - - return error; -} - void MinidumpFileBuilder::AddMiscInfo(const lldb::ProcessSP &process_sp) { AddDirectory(StreamType::MiscInfo, sizeof(lldb_private::minidump::MinidumpMiscInfo)); @@ -782,6 +805,7 @@ void MinidumpFileBuilder::AddLinuxFileStreams( {StreamType::LinuxProcFD, "/proc/" + pid_str + "/fd"}); } + Status error; for (const auto &entry : files_with_stream_types) { StreamType stream = entry.first; std::string path = entry.second; @@ -797,10 +821,57 @@ void MinidumpFileBuilder::AddLinuxFileStreams( } } -Status MinidumpFileBuilder::Dump(lldb::FileUP &core_file) const { - constexpr size_t header_size = sizeof(llvm::minidump::Header); - constexpr size_t directory_size = sizeof(llvm::minidump::Directory); +Status MinidumpFileBuilder::AddMemory(const ProcessSP &process_sp, SaveCoreStyle core_style) { + Status error; + + Process::CoreFileMemoryRanges ranges; + error = process_sp->CalculateCoreFileSaveRanges(core_style, ranges); + if (error.Fail()) { + return error; + } + + Process::CoreFileMemoryRanges ranges_for_memory_list; + // We leave a little padding for dictionary and any other metadata we would want. + // Also so that we can put the header of the memory list 64 in 32b land, because the directory + // requires a 32b RVA. + uint64_t total_size = 256 + (ranges.size() * sizeof(llvm::minidump::MemoryDescriptor_64)); + // Enumerate all threads and save them off for the 32b range first + for (int i = ranges.size() - 1; i >= 0; i--) { + ranges_for_memory_list.push_back(ranges[i]); + total_size += ranges[i].range.size(); + total_size += sizeof(llvm::minidump::MemoryDescriptor); + ranges.erase(ranges.begin() + i); + } + + // Take all the memory that will fit in the 32b range. + for (int i = ranges.size() - 1; i >= 0; i--) { + addr_t size_to_add = ranges[i].range.size() + sizeof(llvm::minidump::MemoryDescriptor); + if (total_size + size_to_add < UINT32_MAX) { + ranges_for_memory_list.push_back(ranges[i]); + total_size += ranges[i].range.size(); + total_size += sizeof(llvm::minidump::MemoryDescriptor); + ranges.erase(ranges.begin() + i); + } + else { + break; + } + } + + error = AddMemoryList_32(process_sp, ranges_for_memory_list); + if (error.Fail()) + return error; + // Add the remaining memory as a 64b range. + if (ranges.size() > 0) { + error = AddMemoryList_64(process_sp, ranges); + if (error.Fail()) + return error; + } + + return FixThreads(); +} + +Status MinidumpFileBuilder::DumpHeader() const { // write header llvm::minidump::Header header; header.Signature = static_cast( @@ -808,9 +879,10 @@ Status MinidumpFileBuilder::Dump(lldb::FileUP &core_file) const { header.Version = static_cast( llvm::minidump::Header::MagicVersion); header.NumberOfStreams = - static_cast(GetDirectoriesNum()); + static_cast(m_directories.size()); + // We write the directories right after the header. header.StreamDirectoryRVA = - static_cast(GetCurrentDataEndOffset()); + static_cast(header_size); header.Checksum = static_cast( 0u), // not used in most of the writers header.TimeDateStamp = @@ -821,8 +893,9 @@ Status MinidumpFileBuilder::Dump(lldb::FileUP &core_file) const { Status error; size_t bytes_written; + m_core_file->SeekFromStart(0); bytes_written = header_size; - error = core_file->Write(&header, bytes_written); + error = m_core_file->Write(&header, bytes_written); if (error.Fail() || bytes_written != header_size) { if (bytes_written != header_size) error.SetErrorStringWithFormat( @@ -830,22 +903,20 @@ Status MinidumpFileBuilder::Dump(lldb::FileUP &core_file) const { header_size); return error; } + return error; +} - // write data - bytes_written = m_data.GetByteSize(); - error = core_file->Write(m_data.GetBytes(), bytes_written); - if (error.Fail() || bytes_written != m_data.GetByteSize()) { - if (bytes_written != m_data.GetByteSize()) - error.SetErrorStringWithFormat( - "unable to write the data (written %zd/%" PRIu64 ")", bytes_written, - m_data.GetByteSize()); - return error; - } +size_t MinidumpFileBuilder::GetCurrentDataEndOffset() const { + return m_data.GetByteSize() + m_saved_data_size; +} - // write directories +Status MinidumpFileBuilder::DumpDirectories() const { + Status error; + size_t bytes_written; + m_core_file->SeekFromStart(header_size); for (const Directory &dir : m_directories) { bytes_written = directory_size; - error = core_file->Write(&dir, bytes_written); + error = m_core_file->Write(&dir, bytes_written); if (error.Fail() || bytes_written != directory_size) { if (bytes_written != directory_size) error.SetErrorStringWithFormat( @@ -858,10 +929,217 @@ Status MinidumpFileBuilder::Dump(lldb::FileUP &core_file) const { return error; } -size_t MinidumpFileBuilder::GetDirectoriesNum() const { - return m_directories.size(); +Status MinidumpFileBuilder::AddMemoryList_32(const ProcessSP &process_sp, const Process::CoreFileMemoryRanges &ranges) { + std::vector descriptors; + Status error; + Log *log = GetLog(LLDBLog::Object); + int region_index = 0; + for (const auto &core_range : ranges) { + // Take the offset before we write. + const size_t offset_for_data = GetCurrentDataEndOffset(); + const addr_t addr = core_range.range.start(); + const addr_t size = core_range.range.size(); + std::cout << "Adding Memory Desciptor at " << std::hex << core_range.range.start() << std::dec << " with size " << core_range.range.size() << std::endl; + auto data_up = std::make_unique(size, 0); + + LLDB_LOGF(log, "AddMemoryList %d/%d reading memory for region (%zu bytes) [%zx, %zx)", + region_index, ranges.size(), size, addr, addr+ size); + ++region_index; + + const size_t bytes_read = + process_sp->ReadMemory(addr, data_up->GetBytes(), size, error); + if (error.Fail()) { + LLDB_LOGF(log, "Failed to read memory region. Bytes read: %zu, error: %s", + bytes_read, error.AsCString()); + // Add the cleanup offset as the difference of bytes not read + descriptors.at(region_index).Memory.DataSize = 0; + } + else if (bytes_read != size) { + LLDB_LOGF(log, "Memory region at: %zu failed to read %zu bytes", addr, size); + // Add the cleanup offset as the difference of bytes not read + descriptors.at(region_index).Memory.DataSize = bytes_read; + } + + MemoryDescriptor descriptor; + descriptor.StartOfMemoryRange = static_cast(addr); + descriptor.Memory.DataSize = static_cast(bytes_read); + descriptor.Memory.RVA = static_cast(offset_for_data); + descriptors.push_back(descriptor); + + // Add the data to the buffer, flush as needed. + error = AddData(data_up->GetBytes(), bytes_read); + if (error.Fail()) + return error; + } + + // Add a directory that references this list + // With a size of the number of ranges as a 32 bit num + // And then the size of all the ranges + AddDirectory(StreamType::MemoryList, + sizeof(llvm::support::ulittle32_t) + + ranges.size() * + sizeof(llvm::minidump::MemoryDescriptor)); + + llvm::support::ulittle32_t memory_ranges_num = static_cast(ranges.size()); + m_data.AppendData(&memory_ranges_num, sizeof(llvm::support::ulittle32_t)); + // For 32b we can get away with writing off the descriptors after the data. This means no cleanup loop needed. + m_data.AppendData(descriptors.data(), descriptors.size() * sizeof(MemoryDescriptor)); + + return error; } -size_t MinidumpFileBuilder::GetCurrentDataEndOffset() const { - return sizeof(llvm::minidump::Header) + m_data.GetByteSize(); +Status MinidumpFileBuilder::AddMemoryList_64(const ProcessSP &process_sp, const Process::CoreFileMemoryRanges &ranges) { + // Add a directory that references this list + // With a size of the number of ranges as a 32 bit num + // And then the size of all the ranges + // This is done first so that we can keep Directories are 32b. + AddDirectory(StreamType::Memory64List, + sizeof(llvm::support::ulittle32_t) + + ranges.size() * + sizeof(llvm::minidump::MemoryDescriptor_64)); + + llvm::support::ulittle32_t memory_ranges_num = static_cast(ranges.size()); + m_data.AppendData(&memory_ranges_num, sizeof(llvm::support::ulittle32_t)); + uint64_t current_offset = 0; + // Capture the starting offset, so we don't depend on the expansion of m_data. + uint64_t starting_offset = GetCurrentDataEndOffset(); + + std::map descriptors_to_clean_up_with_offset_fixes; + std::vector descriptors; + // Enumerate the ranges and create the memory descriptors so we can append them first + for (const auto core_range : ranges) { + // Add the space required to store the memory descriptor + current_offset += sizeof(MemoryDescriptor_64); + LocationDescriptor_64 memory_dump; + memory_dump.DataSize = static_cast(core_range.range.size()); + memory_dump.RVA = + static_cast(starting_offset + current_offset); + MemoryDescriptor_64 memory_desc; + memory_desc.StartOfMemoryRange = + static_cast(core_range.range.start()); + memory_desc.Memory = memory_dump; + descriptors.push_back(memory_desc); + // Now write this memory descriptor to the buffer. + m_data.AppendData(&memory_desc, sizeof(MemoryDescriptor_64)); + // Add the data size to the current offset + // so subsequent RVA's are accurate. + current_offset += core_range.range.size(); + } + + Status error; + Log *log = GetLog(LLDBLog::Object); + int region_index = 0; + for (const auto &core_range : ranges) { + const addr_t addr = core_range.range.start(); + const addr_t size = core_range.range.size(); + auto data_up = std::make_unique(size, 0); + + + LLDB_LOGF(log, "AddMemoryList %d/%d reading memory for region (%zu bytes) [%zx, %zx)", + region_index, ranges.size(), size, addr, addr+ size); + ++region_index; + + // TODO: Add a fixup functionality, when we fail to read the correct number of bytes or anything at all + // we need to go back and fix the memory descriptors and the subsequent RVA's. + const size_t bytes_read = + process_sp->ReadMemory(addr, data_up->GetBytes(), size, error); + if (error.Fail()) { + LLDB_LOGF(log, "Failed to read memory region. Bytes read: %zu, error: %s", + bytes_read, error.AsCString()); + descriptors_to_clean_up_with_offset_fixes[region_index] = size; + descriptors[region_index].Memory.DataSize = 0; + } + if (bytes_read != size) { + error.SetErrorStringWithFormat("Memory region at: %zu failed to read %zu bytes", addr, size); + descriptors_to_clean_up_with_offset_fixes[region_index] = size - bytes_read; + descriptors[region_index].Memory.DataSize = bytes_read; + } + + // Add the data to the buffer, flush as needed. + error = AddData(data_up->GetBytes(), bytes_read); + if (error.Fail()) + return error; + } + + // Early return if there is no cleanup needed. + if (descriptors.size() == 0) { + return error; + } + else { + // Flush to disk we can make the fixes in place. + DumpToFile(); + // Iterate through the list, moving up (subtracting) the RVA by the current fix up offset + // Then check if the current item is in the map + // If it is, add it's offset to the fix up delta. + // This is so if we fail to write a certain number of bytes, we fix the rest of the RVA's + // + addr_t current_shiftup_delta = 0; + for (uint i = 0; i < descriptors.size(); i++) { + MemoryDescriptor_64 &desc = descriptors[i]; + desc.Memory.RVA -= current_shiftup_delta; + // We update the delta afterwards because the RVA of the current + // descriptor doesn't depend on it's size. Only the subsequent Descriptors + // would depend on this delta. + if (descriptors_to_clean_up_with_offset_fixes.count(i) > 0) { + current_shiftup_delta += descriptors_to_clean_up_with_offset_fixes[i]; + } + } + + // Fixup the descriptors that were not read correctly. + m_core_file->SeekFromStart(starting_offset); + size_t bytes_written = sizeof(MemoryDescriptor) * descriptors.size(); + error = m_core_file->Write(descriptors.data(), bytes_written); + if (error.Fail() || bytes_written != sizeof(MemoryDescriptor) * descriptors.size()) { + error.SetErrorStringWithFormat( + "unable to write the memory descriptors (written %zd/%zd)", + bytes_written, sizeof(MemoryDescriptor) * descriptors.size()); + } + + return error; + } +} + +Status MinidumpFileBuilder::AddData(const void* data, size_t size) { + m_data.AppendData(data, size); + // TODO: Refactor to be a parameter. + if (m_data.GetByteSize() > 1024 * 1024 * 1024) { + return FlushToDisk(); + } + + return Status(); +} + +Status MinidumpFileBuilder::FlushToDisk() const { + Status error; + size_t bytes_to_write = m_data.GetByteSize(); + size_t bytes_written = bytes_to_write; + error = m_core_file->Write(m_data.GetBytes(), bytes_to_write); + if (error.Fail() || bytes_written != bytes_to_write) { + error.SetErrorStringWithFormat( + "Wrote incorrect number of bytes to minidump file. (written %zd/%zd)", bytes_written, + bytes_to_write); + return error; + } + + return error; +} + +Status MinidumpFileBuilder::DumpToFile() { + Status error; + // If anything is left unsaved, dump it. + error = FlushToDisk(); + if (error.Fail()) + return error; + + // Overwrite the header which we filled in earlier. + error = DumpHeader(); + if (error.Fail()) + return error; + + // Overwrite the space saved for directories + error = DumpDirectories(); + if (error.Fail()) + return error; + + return error; } diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h index b2e984191983f..5f34c5da10078 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h +++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h @@ -17,12 +17,18 @@ #define LLDB_SOURCE_PLUGINS_OBJECTFILE_MINIDUMP_MINIDUMPFILEBUILDER_H #include +#include #include +#include +#include "lldb/Target/Process.h" #include "lldb/Target/Target.h" #include "lldb/Utility/DataBufferHeap.h" #include "lldb/Utility/Status.h" +#include "lldb/lldb-forward.h" +#include "lldb/lldb-types.h" +#include "llvm/BinaryFormat/Minidump.h" #include "llvm/Object/Minidump.h" // Write std::string to minidump in the UTF16 format(with null termination char) @@ -40,7 +46,7 @@ lldb_private::Status WriteString(const std::string &to_write, /// the data on heap. class MinidumpFileBuilder { public: - MinidumpFileBuilder() = default; + MinidumpFileBuilder(lldb::FileUP&& core_file): m_core_file(std::move(core_file)) {}; MinidumpFileBuilder(const MinidumpFileBuilder &) = delete; MinidumpFileBuilder &operator=(const MinidumpFileBuilder &) = delete; @@ -50,6 +56,7 @@ class MinidumpFileBuilder { ~MinidumpFileBuilder() = default; + lldb_private::Status AddHeaderAndCalculateDirectories(const lldb_private::Target &target, const lldb::ProcessSP &process_sp); // Add SystemInfo stream, used for storing the most basic information // about the system, platform etc... lldb_private::Status AddSystemInfo(const llvm::Triple &target_triple); @@ -59,39 +66,57 @@ class MinidumpFileBuilder { // Add ThreadList stream, containing information about all threads running // at the moment of core saving. Contains information about thread // contexts. - lldb_private::Status AddThreadList(const lldb::ProcessSP &process_sp); - // Add Exception streams for any threads that stopped with exceptions. - void AddExceptions(const lldb::ProcessSP &process_sp); - // Add MemoryList stream, containing dumps of important memory segments - lldb_private::Status AddMemoryList(const lldb::ProcessSP &process_sp, - lldb::SaveCoreStyle core_style); // Add MiscInfo stream, mainly providing ProcessId void AddMiscInfo(const lldb::ProcessSP &process_sp); // Add informative files about a Linux process void AddLinuxFileStreams(const lldb::ProcessSP &process_sp); + // Add Exception streams for any threads that stopped with exceptions. + void AddExceptions(const lldb::ProcessSP &process_sp); // Dump the prepared data into file. In case of the failure data are // intact. - lldb_private::Status Dump(lldb::FileUP &core_file) const; - // Returns the current number of directories(streams) that have been so far - // created. This number of directories will be dumped when calling Dump() - size_t GetDirectoriesNum() const; + lldb_private::Status AddThreadList(const lldb::ProcessSP &process_sp); + + lldb_private::Status AddMemory(const lldb::ProcessSP &process_sp, + lldb::SaveCoreStyle core_style); + + lldb_private::Status DumpToFile(); private: + // Add data to the end of the buffer, if the buffer exceeds the flush level, trigger a flush. + lldb_private::Status AddData(const void *data, size_t size); + // Add MemoryList stream, containing dumps of important memory segments + lldb_private::Status AddMemoryList_64(const lldb::ProcessSP &process_sp, const lldb_private::Process::CoreFileMemoryRanges &ranges); + lldb_private::Status AddMemoryList_32(const lldb::ProcessSP &process_sp, const lldb_private::Process::CoreFileMemoryRanges &ranges); + lldb_private::Status FixThreads(); + lldb_private::Status FlushToDisk() const; + + lldb_private::Status DumpHeader() const; + lldb_private::Status DumpDirectories() const; + bool CheckIf_64Bit(const size_t size); // Add directory of StreamType pointing to the current end of the prepared // file with the specified size. - void AddDirectory(llvm::minidump::StreamType type, size_t stream_size); - size_t GetCurrentDataEndOffset() const; - - // Stores directories to later put them at the end of minidump file + void AddDirectory(llvm::minidump::StreamType type, uint64_t stream_size); + lldb::addr_t GetCurrentDataEndOffset() const; + // Stores directories to fill in later std::vector m_directories; + // When we write off the threads for the first time, we need to clean them up + // and give them the correct RVA once we write the stack memory list. + std::map m_thread_by_range_start; // Main data buffer consisting of data without the minidump header and // directories lldb_private::DataBufferHeap m_data; + uint m_expected_directories = 0; + uint64_t m_saved_data_size = 0; + size_t thread_list_file_offset = 0; + + static constexpr size_t header_size = sizeof(llvm::minidump::Header); + static constexpr size_t directory_size = sizeof(llvm::minidump::Directory); // More that one place can mention the register thread context locations, // so when we emit the thread contents, remember where it is so we don't have // to duplicate it in the exception data. std::map m_tid_to_reg_ctx; + lldb::FileUP m_core_file; }; #endif // LLDB_SOURCE_PLUGINS_OBJECTFILE_MINIDUMP_MINIDUMPFILEBUILDER_H diff --git a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp index 1af5d99f0b160..d64c1d7bfeef0 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp +++ b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp @@ -17,6 +17,7 @@ #include "lldb/Utility/LLDBLog.h" #include "llvm/Support/FileSystem.h" +#include using namespace lldb; using namespace lldb_private; @@ -65,37 +66,40 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp, if (!process_sp) return false; - MinidumpFileBuilder builder; + llvm::Expected maybe_core_file = FileSystem::Instance().Open( + outfile, File::eOpenOptionWriteOnly | File::eOpenOptionCanCreate); + if (!maybe_core_file) { + error = maybe_core_file.takeError(); + return false; + } + MinidumpFileBuilder builder(std::move(maybe_core_file.get())); + Target &target = process_sp->GetTarget(); + builder.AddHeaderAndCalculateDirectories(target, process_sp); Log *log = GetLog(LLDBLog::Object); error = builder.AddSystemInfo(target.GetArchitecture().GetTriple()); if (error.Fail()) { - LLDB_LOG(log, "AddSystemInfo failed: %s", error.AsCString()); - return false; - } - - error = builder.AddModuleList(target); - if (error.Fail()) { - LLDB_LOG(log, "AddModuleList failed: %s", error.AsCString()); + LLDB_LOGF(log, "AddSystemInfo failed: %s", error.AsCString()); return false; } + builder.AddModuleList(target); builder.AddMiscInfo(process_sp); error = builder.AddThreadList(process_sp); if (error.Fail()) { - LLDB_LOG(log, "AddThreadList failed: %s", error.AsCString()); + LLDB_LOGF(log, "AddThreadList failed: %s", error.AsCString()); return false; } // Add any exceptions but only if there are any in any threads. builder.AddExceptions(process_sp); - error = builder.AddMemoryList(process_sp, core_style); + error = builder.AddMemory(process_sp, core_style); if (error.Fail()) { - LLDB_LOG(log, "AddMemoryList failed: %s", error.AsCString()); + LLDB_LOGF(log, "AddMemoryList failed: %s", error.AsCString()); return false; } @@ -104,17 +108,11 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp, builder.AddLinuxFileStreams(process_sp); } - llvm::Expected maybe_core_file = FileSystem::Instance().Open( - outfile, File::eOpenOptionWriteOnly | File::eOpenOptionCanCreate); - if (!maybe_core_file) { - error = maybe_core_file.takeError(); + error = builder.DumpToFile(); + if (error.Fail()) { + LLDB_LOGF(log, "DumpToFile failed: %s", error.AsCString()); return false; } - lldb::FileUP core_file = std::move(maybe_core_file.get()); - - error = builder.Dump(core_file); - if (error.Fail()) - return false; return true; } diff --git a/llvm/include/llvm/BinaryFormat/Minidump.h b/llvm/include/llvm/BinaryFormat/Minidump.h index bc303929498d0..53cd0deb1b18b 100644 --- a/llvm/include/llvm/BinaryFormat/Minidump.h +++ b/llvm/include/llvm/BinaryFormat/Minidump.h @@ -62,6 +62,12 @@ struct LocationDescriptor { }; static_assert(sizeof(LocationDescriptor) == 8); +struct LocationDescriptor_64 { + support::ulittle64_t DataSize; + support::ulittle64_t RVA; +}; +static_assert(sizeof(LocationDescriptor_64) == 16); + /// Describes a single memory range (both its VM address and where to find it in /// the file) of the process from which this minidump file was generated. struct MemoryDescriptor { @@ -70,6 +76,11 @@ struct MemoryDescriptor { }; static_assert(sizeof(MemoryDescriptor) == 16); +struct MemoryDescriptor_64 { + support::ulittle64_t StartOfMemoryRange; + LocationDescriptor_64 Memory; +}; + struct MemoryInfoListHeader { support::ulittle32_t SizeOfHeader; support::ulittle32_t SizeOfEntry; diff --git a/llvm/include/llvm/Object/Minidump.h b/llvm/include/llvm/Object/Minidump.h index e45d4de0090de..a388fa3349772 100644 --- a/llvm/include/llvm/Object/Minidump.h +++ b/llvm/include/llvm/Object/Minidump.h @@ -52,6 +52,13 @@ class MinidumpFile : public Binary { return getDataSlice(getData(), Desc.RVA, Desc.DataSize); } + /// Returns the raw contents of an object given by the LocationDescriptor. An + /// error is returned if the descriptor points outside of the minidump file. + Expected> + getRawData(minidump::LocationDescriptor_64 Desc) const { + return getDataSlice(getData(), Desc.RVA, Desc.DataSize); + } + /// Returns the minidump string at the given offset. An error is returned if /// we fail to parse the string, or the string is invalid UTF16. Expected getString(size_t Offset) const; diff --git a/llvm/lib/ObjectYAML/MinidumpEmitter.cpp b/llvm/lib/ObjectYAML/MinidumpEmitter.cpp index 24b521a9925c7..a967e422bef67 100644 --- a/llvm/lib/ObjectYAML/MinidumpEmitter.cpp +++ b/llvm/lib/ObjectYAML/MinidumpEmitter.cpp @@ -6,9 +6,11 @@ // //===----------------------------------------------------------------------===// +#include "llvm/BinaryFormat/Minidump.h" #include "llvm/ObjectYAML/MinidumpYAML.h" #include "llvm/ObjectYAML/yaml2obj.h" #include "llvm/Support/ConvertUTF.h" +#include "llvm/Support/Endian.h" #include "llvm/Support/raw_ostream.h" #include @@ -119,6 +121,11 @@ static LocationDescriptor layout(BlobAllocator &File, yaml::BinaryRef Data) { support::ulittle32_t(File.allocateBytes(Data))}; } +static LocationDescriptor_64 layout_64(BlobAllocator &File, yaml::BinaryRef Data) { + return {support::ulittle64_t(Data.binary_size()), + support::ulittle64_t(File.allocateBytes(Data))}; +} + static size_t layout(BlobAllocator &File, MinidumpYAML::ExceptionStream &S) { File.allocateObject(S.MDExceptionStream); From 2885cb3ec6e672a5eacc7d8a962d9790cc29f9ea Mon Sep 17 00:00:00 2001 From: Jacob Lalonde Date: Wed, 12 Jun 2024 14:03:37 -0700 Subject: [PATCH 02/16] Remove debug changes to testprocesssavecore --- .../Minidump/MinidumpFileBuilder.cpp | 165 +++++++++--------- .../ObjectFile/Minidump/MinidumpFileBuilder.h | 9 +- .../Minidump/ObjectFileMinidump.cpp | 11 +- llvm/include/llvm/BinaryFormat/Minidump.h | 8 +- llvm/include/llvm/Object/Minidump.h | 7 - 5 files changed, 93 insertions(+), 107 deletions(-) diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp index c08b5206720ab..ecb16d2fbd388 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp +++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp @@ -38,10 +38,13 @@ #include "lldb/lldb-forward.h" #include "lldb/lldb-types.h" +#include #include #include +#include #include #include +#include #include #include @@ -584,9 +587,8 @@ findStackHelper(const lldb::ProcessSP &process_sp, uint64_t rsp) { Status MinidumpFileBuilder::FixThreads() { Status error; // If we have anything in the heap flush it. - DumpToFile(); - - m_core_file->SeekFromStart(thread_list_file_offset); + FlushToDisk(); + m_core_file->SeekFromStart(m_thread_list_start); for (auto &pair : m_thread_by_range_start) { // The thread objects will get a new memory descriptor added // When we are emitting the memory list and then we write it here @@ -602,8 +604,6 @@ Status MinidumpFileBuilder::FixThreads() { } } - // Seek the core file back to the end. - m_core_file->SeekFromEnd(0); return error; } @@ -624,10 +624,11 @@ Status MinidumpFileBuilder::AddThreadList(const lldb::ProcessSP &process_sp) { static_cast(thread_list.GetSize()); m_data.AppendData(&thread_count, sizeof(llvm::support::ulittle32_t)); + // Take the offset after the thread count. + m_thread_list_start = GetCurrentDataEndOffset(); DataBufferHeap helper_data; const uint32_t num_threads = thread_list.GetSize(); - thread_list_file_offset = GetCurrentDataEndOffset(); Log *log = GetLog(LLDBLog::Object); for (uint32_t thread_idx = 0; thread_idx < num_threads; ++thread_idx) { ThreadSP thread_sp(thread_list.GetThreadAtIndex(thread_idx)); @@ -824,29 +825,34 @@ void MinidumpFileBuilder::AddLinuxFileStreams( Status MinidumpFileBuilder::AddMemory(const ProcessSP &process_sp, SaveCoreStyle core_style) { Status error; - Process::CoreFileMemoryRanges ranges; - error = process_sp->CalculateCoreFileSaveRanges(core_style, ranges); + Process::CoreFileMemoryRanges ranges_for_memory_list; + error = process_sp->CalculateCoreFileSaveRanges(SaveCoreStyle::eSaveCoreStackOnly, ranges_for_memory_list); if (error.Fail()) { return error; } - Process::CoreFileMemoryRanges ranges_for_memory_list; + std::set stack_ranges; + for (const auto &core_range : ranges_for_memory_list) { + stack_ranges.insert(core_range.range.start()); + } // We leave a little padding for dictionary and any other metadata we would want. // Also so that we can put the header of the memory list 64 in 32b land, because the directory // requires a 32b RVA. - uint64_t total_size = 256 + (ranges.size() * sizeof(llvm::minidump::MemoryDescriptor_64)); - // Enumerate all threads and save them off for the 32b range first - for (int i = ranges.size() - 1; i >= 0; i--) { - ranges_for_memory_list.push_back(ranges[i]); - total_size += ranges[i].range.size(); - total_size += sizeof(llvm::minidump::MemoryDescriptor); - ranges.erase(ranges.begin() + i); + Process::CoreFileMemoryRanges ranges; + error = process_sp->CalculateCoreFileSaveRanges(core_style, ranges); + if (error.Fail()) { + return error; } + uint64_t total_size = 256 + (ranges.size() * sizeof(llvm::minidump::MemoryDescriptor_64)); // Take all the memory that will fit in the 32b range. for (int i = ranges.size() - 1; i >= 0; i--) { addr_t size_to_add = ranges[i].range.size() + sizeof(llvm::minidump::MemoryDescriptor); - if (total_size + size_to_add < UINT32_MAX) { + // filter out the stacks and check if it's below 32b max. + if (stack_ranges.count(ranges[i].range.start()) > 0) { + ranges.erase(ranges.begin() + i); + } + else if (total_size + size_to_add < UINT32_MAX) { ranges_for_memory_list.push_back(ranges[i]); total_size += ranges[i].range.size(); total_size += sizeof(llvm::minidump::MemoryDescriptor); @@ -948,16 +954,14 @@ Status MinidumpFileBuilder::AddMemoryList_32(const ProcessSP &process_sp, const const size_t bytes_read = process_sp->ReadMemory(addr, data_up->GetBytes(), size, error); - if (error.Fail()) { + if (error.Fail() || bytes_read == 0) { LLDB_LOGF(log, "Failed to read memory region. Bytes read: %zu, error: %s", bytes_read, error.AsCString()); - // Add the cleanup offset as the difference of bytes not read - descriptors.at(region_index).Memory.DataSize = 0; + // Just skip sections with errors or zero bytes in 32b mode + continue; } else if (bytes_read != size) { LLDB_LOGF(log, "Memory region at: %zu failed to read %zu bytes", addr, size); - // Add the cleanup offset as the difference of bytes not read - descriptors.at(region_index).Memory.DataSize = bytes_read; } MemoryDescriptor descriptor; @@ -965,6 +969,9 @@ Status MinidumpFileBuilder::AddMemoryList_32(const ProcessSP &process_sp, const descriptor.Memory.DataSize = static_cast(bytes_read); descriptor.Memory.RVA = static_cast(offset_for_data); descriptors.push_back(descriptor); + if (m_thread_by_range_start.count(addr) > 0) { + m_thread_by_range_start[addr].Stack = descriptor; + } // Add the data to the buffer, flush as needed. error = AddData(data_up->GetBytes(), bytes_read); @@ -977,10 +984,10 @@ Status MinidumpFileBuilder::AddMemoryList_32(const ProcessSP &process_sp, const // And then the size of all the ranges AddDirectory(StreamType::MemoryList, sizeof(llvm::support::ulittle32_t) + - ranges.size() * + descriptors.size() * sizeof(llvm::minidump::MemoryDescriptor)); - llvm::support::ulittle32_t memory_ranges_num = static_cast(ranges.size()); + llvm::support::ulittle32_t memory_ranges_num = static_cast(descriptors.size()); m_data.AppendData(&memory_ranges_num, sizeof(llvm::support::ulittle32_t)); // For 32b we can get away with writing off the descriptors after the data. This means no cleanup loop needed. m_data.AppendData(descriptors.data(), descriptors.size() * sizeof(MemoryDescriptor)); @@ -989,41 +996,30 @@ Status MinidumpFileBuilder::AddMemoryList_32(const ProcessSP &process_sp, const } Status MinidumpFileBuilder::AddMemoryList_64(const ProcessSP &process_sp, const Process::CoreFileMemoryRanges &ranges) { - // Add a directory that references this list - // With a size of the number of ranges as a 32 bit num - // And then the size of all the ranges - // This is done first so that we can keep Directories are 32b. AddDirectory(StreamType::Memory64List, - sizeof(llvm::support::ulittle32_t) + + (sizeof(llvm::support::ulittle64_t) * 2) + ranges.size() * sizeof(llvm::minidump::MemoryDescriptor_64)); - llvm::support::ulittle32_t memory_ranges_num = static_cast(ranges.size()); - m_data.AppendData(&memory_ranges_num, sizeof(llvm::support::ulittle32_t)); - uint64_t current_offset = 0; - // Capture the starting offset, so we don't depend on the expansion of m_data. + llvm::support::ulittle64_t memory_ranges_num = static_cast(ranges.size()); + m_data.AppendData(&memory_ranges_num, sizeof(llvm::support::ulittle64_t)); + llvm::support::ulittle64_t memory_ranges_base_rva = static_cast(GetCurrentDataEndOffset()); + m_data.AppendData(&memory_ranges_base_rva, sizeof(llvm::support::ulittle64_t)); + // Capture the starting offset, so we can do cleanup later if needed. uint64_t starting_offset = GetCurrentDataEndOffset(); - std::map descriptors_to_clean_up_with_offset_fixes; + bool cleanup_required = false; std::vector descriptors; // Enumerate the ranges and create the memory descriptors so we can append them first for (const auto core_range : ranges) { // Add the space required to store the memory descriptor - current_offset += sizeof(MemoryDescriptor_64); - LocationDescriptor_64 memory_dump; - memory_dump.DataSize = static_cast(core_range.range.size()); - memory_dump.RVA = - static_cast(starting_offset + current_offset); MemoryDescriptor_64 memory_desc; memory_desc.StartOfMemoryRange = static_cast(core_range.range.start()); - memory_desc.Memory = memory_dump; + memory_desc.DataSize = static_cast(core_range.range.size()); descriptors.push_back(memory_desc); // Now write this memory descriptor to the buffer. m_data.AppendData(&memory_desc, sizeof(MemoryDescriptor_64)); - // Add the data size to the current offset - // so subsequent RVA's are accurate. - current_offset += core_range.range.size(); } Status error; @@ -1034,25 +1030,23 @@ Status MinidumpFileBuilder::AddMemoryList_64(const ProcessSP &process_sp, const const addr_t size = core_range.range.size(); auto data_up = std::make_unique(size, 0); - - LLDB_LOGF(log, "AddMemoryList %d/%d reading memory for region (%zu bytes) [%zx, %zx)", + LLDB_LOGF(log, "AddMemoryList_64 %d/%d reading memory for region (%zu bytes) [%zx, %zx)", region_index, ranges.size(), size, addr, addr+ size); ++region_index; - // TODO: Add a fixup functionality, when we fail to read the correct number of bytes or anything at all - // we need to go back and fix the memory descriptors and the subsequent RVA's. const size_t bytes_read = process_sp->ReadMemory(addr, data_up->GetBytes(), size, error); if (error.Fail()) { LLDB_LOGF(log, "Failed to read memory region. Bytes read: %zu, error: %s", bytes_read, error.AsCString()); - descriptors_to_clean_up_with_offset_fixes[region_index] = size; - descriptors[region_index].Memory.DataSize = 0; + error.Clear(); + cleanup_required = true; + descriptors[region_index].DataSize = 0; } if (bytes_read != size) { - error.SetErrorStringWithFormat("Memory region at: %zu failed to read %zu bytes", addr, size); - descriptors_to_clean_up_with_offset_fixes[region_index] = size - bytes_read; - descriptors[region_index].Memory.DataSize = bytes_read; + LLDB_LOGF(log, "Memory region at: %zu failed to read %zu bytes", addr, size); + cleanup_required = true; + descriptors[region_index].DataSize = bytes_read; } // Add the data to the buffer, flush as needed. @@ -1062,65 +1056,64 @@ Status MinidumpFileBuilder::AddMemoryList_64(const ProcessSP &process_sp, const } // Early return if there is no cleanup needed. - if (descriptors.size() == 0) { + if (!cleanup_required) { return error; } else { // Flush to disk we can make the fixes in place. - DumpToFile(); - // Iterate through the list, moving up (subtracting) the RVA by the current fix up offset - // Then check if the current item is in the map - // If it is, add it's offset to the fix up delta. - // This is so if we fail to write a certain number of bytes, we fix the rest of the RVA's - // - addr_t current_shiftup_delta = 0; - for (uint i = 0; i < descriptors.size(); i++) { - MemoryDescriptor_64 &desc = descriptors[i]; - desc.Memory.RVA -= current_shiftup_delta; - // We update the delta afterwards because the RVA of the current - // descriptor doesn't depend on it's size. Only the subsequent Descriptors - // would depend on this delta. - if (descriptors_to_clean_up_with_offset_fixes.count(i) > 0) { - current_shiftup_delta += descriptors_to_clean_up_with_offset_fixes[i]; - } - } - + FlushToDisk(); // Fixup the descriptors that were not read correctly. m_core_file->SeekFromStart(starting_offset); - size_t bytes_written = sizeof(MemoryDescriptor) * descriptors.size(); + size_t bytes_written = sizeof(MemoryDescriptor_64) * descriptors.size(); error = m_core_file->Write(descriptors.data(), bytes_written); - if (error.Fail() || bytes_written != sizeof(MemoryDescriptor) * descriptors.size()) { + if (error.Fail() || bytes_written != sizeof(MemoryDescriptor_64) * descriptors.size()) { error.SetErrorStringWithFormat( "unable to write the memory descriptors (written %zd/%zd)", - bytes_written, sizeof(MemoryDescriptor) * descriptors.size()); + bytes_written, sizeof(MemoryDescriptor_64) * descriptors.size()); } return error; } } -Status MinidumpFileBuilder::AddData(const void* data, size_t size) { +Status MinidumpFileBuilder::AddData(const void* data, addr_t size) { + // This should also get chunked, because worst case we copy over a big + // object / memory range, say 5gb. In that case, we'd have to allocate 10gb + // 5 gb for the buffer we're copying from, and then 5gb for the buffer we're copying to. + // Which will be short lived and immedaitely go to disk, the goal here is to limit the number + // of bytes we need to host in memory at any given time. m_data.AppendData(data, size); - // TODO: Refactor to be a parameter. - if (m_data.GetByteSize() > 1024 * 1024 * 1024) { + if (m_data.GetByteSize() > m_write_chunk_max) { return FlushToDisk(); } return Status(); } -Status MinidumpFileBuilder::FlushToDisk() const { +Status MinidumpFileBuilder::FlushToDisk() { Status error; - size_t bytes_to_write = m_data.GetByteSize(); - size_t bytes_written = bytes_to_write; - error = m_core_file->Write(m_data.GetBytes(), bytes_to_write); - if (error.Fail() || bytes_written != bytes_to_write) { - error.SetErrorStringWithFormat( - "Wrote incorrect number of bytes to minidump file. (written %zd/%zd)", bytes_written, - bytes_to_write); - return error; + // Set the stream to it's end. + m_core_file->SeekFromEnd(0); + addr_t starting_size = m_data.GetByteSize(); + addr_t remaining_bytes = starting_size; + size_t offset = 0; + while (remaining_bytes > 0) { + size_t chunk_size = std::min(remaining_bytes, m_write_chunk_max); + size_t bytes_written = chunk_size; + error = m_core_file->Write(m_data.GetBytes() + offset, bytes_written); + if (error.Fail() || bytes_written != chunk_size) { + error.SetErrorStringWithFormat( + "Wrote incorrect number of bytes to minidump file. (written %zd/%zd)", bytes_written, + chunk_size); + return error; + } + + offset += bytes_written; + remaining_bytes -= bytes_written; } + m_saved_data_size += starting_size; + m_data.Clear(); return error; } diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h index 5f34c5da10078..c5fe9836f06a5 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h +++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h @@ -88,7 +88,7 @@ class MinidumpFileBuilder { lldb_private::Status AddMemoryList_64(const lldb::ProcessSP &process_sp, const lldb_private::Process::CoreFileMemoryRanges &ranges); lldb_private::Status AddMemoryList_32(const lldb::ProcessSP &process_sp, const lldb_private::Process::CoreFileMemoryRanges &ranges); lldb_private::Status FixThreads(); - lldb_private::Status FlushToDisk() const; + lldb_private::Status FlushToDisk(); lldb_private::Status DumpHeader() const; lldb_private::Status DumpDirectories() const; @@ -107,7 +107,12 @@ class MinidumpFileBuilder { lldb_private::DataBufferHeap m_data; uint m_expected_directories = 0; uint64_t m_saved_data_size = 0; - size_t thread_list_file_offset = 0; + size_t m_thread_list_start = 0; + // We set the max write amount to 128 mb + // Linux has a signed 32b - some buffer on writes + // and we have guarauntee a user memory region / 'object' could be over 2gb + // now that we support 64b memory dumps. + static constexpr size_t m_write_chunk_max = (1024 * 1024 * 128); static constexpr size_t header_size = sizeof(llvm::minidump::Header); static constexpr size_t directory_size = sizeof(llvm::minidump::Directory); diff --git a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp index d64c1d7bfeef0..3435d89accc49 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp +++ b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp @@ -93,21 +93,22 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp, LLDB_LOGF(log, "AddThreadList failed: %s", error.AsCString()); return false; } + if (target.GetArchitecture().GetTriple().getOS() == + llvm::Triple::OSType::Linux) { + builder.AddLinuxFileStreams(process_sp); + } // Add any exceptions but only if there are any in any threads. builder.AddExceptions(process_sp); + // Note: add memory HAS to be the last thing we do. It can overflow into 64b land + // and many RVA's only support 32b error = builder.AddMemory(process_sp, core_style); if (error.Fail()) { LLDB_LOGF(log, "AddMemoryList failed: %s", error.AsCString()); return false; } - if (target.GetArchitecture().GetTriple().getOS() == - llvm::Triple::OSType::Linux) { - builder.AddLinuxFileStreams(process_sp); - } - error = builder.DumpToFile(); if (error.Fail()) { LLDB_LOGF(log, "DumpToFile failed: %s", error.AsCString()); diff --git a/llvm/include/llvm/BinaryFormat/Minidump.h b/llvm/include/llvm/BinaryFormat/Minidump.h index 53cd0deb1b18b..9669303252615 100644 --- a/llvm/include/llvm/BinaryFormat/Minidump.h +++ b/llvm/include/llvm/BinaryFormat/Minidump.h @@ -62,12 +62,6 @@ struct LocationDescriptor { }; static_assert(sizeof(LocationDescriptor) == 8); -struct LocationDescriptor_64 { - support::ulittle64_t DataSize; - support::ulittle64_t RVA; -}; -static_assert(sizeof(LocationDescriptor_64) == 16); - /// Describes a single memory range (both its VM address and where to find it in /// the file) of the process from which this minidump file was generated. struct MemoryDescriptor { @@ -78,7 +72,7 @@ static_assert(sizeof(MemoryDescriptor) == 16); struct MemoryDescriptor_64 { support::ulittle64_t StartOfMemoryRange; - LocationDescriptor_64 Memory; + support::ulittle64_t DataSize; }; struct MemoryInfoListHeader { diff --git a/llvm/include/llvm/Object/Minidump.h b/llvm/include/llvm/Object/Minidump.h index a388fa3349772..e45d4de0090de 100644 --- a/llvm/include/llvm/Object/Minidump.h +++ b/llvm/include/llvm/Object/Minidump.h @@ -52,13 +52,6 @@ class MinidumpFile : public Binary { return getDataSlice(getData(), Desc.RVA, Desc.DataSize); } - /// Returns the raw contents of an object given by the LocationDescriptor. An - /// error is returned if the descriptor points outside of the minidump file. - Expected> - getRawData(minidump::LocationDescriptor_64 Desc) const { - return getDataSlice(getData(), Desc.RVA, Desc.DataSize); - } - /// Returns the minidump string at the given offset. An error is returned if /// we fail to parse the string, or the string is invalid UTF16. Expected getString(size_t Offset) const; From 762fb82eb05976444e154e508ad456c234dafa83 Mon Sep 17 00:00:00 2001 From: Jacob Lalonde Date: Wed, 12 Jun 2024 13:33:03 -0700 Subject: [PATCH 03/16] Add some comments and remove changes to ulittle64 --- .../ObjectFile/Minidump/MinidumpFileBuilder.cpp | 11 ++++++----- llvm/lib/ObjectYAML/MinidumpEmitter.cpp | 5 ----- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp index ecb16d2fbd388..a90b6ec05b838 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp +++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp @@ -54,7 +54,6 @@ using namespace llvm::minidump; Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories(const lldb_private::Target &target, const lldb::ProcessSP &process_sp) { // First set the offset on the file, and on the bytes saved - //TODO: advance the core file. m_saved_data_size += header_size; // We know we will have at least Misc, SystemInfo, Modules, and ThreadList (corresponding memory list for stacks) // And an additional memory list for non-stacks. @@ -77,7 +76,6 @@ Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories(const lldb_private: // Now offset the file by the directores so we can write them in later. offset_t directory_offset = m_expected_directories * directory_size; - //TODO: advance the core file. m_saved_data_size += directory_offset; // Replace this when we make a better way to do this. Status error; @@ -113,9 +111,9 @@ Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories(const lldb_private: void MinidumpFileBuilder::AddDirectory(StreamType type, uint64_t stream_size) { LocationDescriptor loc; - loc.DataSize = static_cast(stream_size); + loc.DataSize = static_cast(stream_size); // Stream will begin at the current end of data section - loc.RVA = static_cast(GetCurrentDataEndOffset()); + loc.RVA = static_cast(GetCurrentDataEndOffset()); Directory dir; dir.Type = static_cast>(type); @@ -188,7 +186,7 @@ Status MinidumpFileBuilder::AddSystemInfo(const llvm::Triple &target_triple) { sys_info.ProcessorArch = static_cast>(arch); // Global offset to beginning of a csd_string in a data section - sys_info.CSDVersionRVA = static_cast( + sys_info.CSDVersionRVA = static_cast( GetCurrentDataEndOffset() + sizeof(llvm::minidump::SystemInfo)); sys_info.PlatformId = platform_id; m_data.AppendData(&sys_info, sizeof(llvm::minidump::SystemInfo)); @@ -1097,6 +1095,9 @@ Status MinidumpFileBuilder::FlushToDisk() { addr_t starting_size = m_data.GetByteSize(); addr_t remaining_bytes = starting_size; size_t offset = 0; + + // Unix/Linux OS's return SSIZE for operations, this means a max of 2gb per IO operation + // so we chunk it here. We keep the file size small to try to minimize memory use. while (remaining_bytes > 0) { size_t chunk_size = std::min(remaining_bytes, m_write_chunk_max); size_t bytes_written = chunk_size; diff --git a/llvm/lib/ObjectYAML/MinidumpEmitter.cpp b/llvm/lib/ObjectYAML/MinidumpEmitter.cpp index a967e422bef67..6ca626f7842d2 100644 --- a/llvm/lib/ObjectYAML/MinidumpEmitter.cpp +++ b/llvm/lib/ObjectYAML/MinidumpEmitter.cpp @@ -121,11 +121,6 @@ static LocationDescriptor layout(BlobAllocator &File, yaml::BinaryRef Data) { support::ulittle32_t(File.allocateBytes(Data))}; } -static LocationDescriptor_64 layout_64(BlobAllocator &File, yaml::BinaryRef Data) { - return {support::ulittle64_t(Data.binary_size()), - support::ulittle64_t(File.allocateBytes(Data))}; -} - static size_t layout(BlobAllocator &File, MinidumpYAML::ExceptionStream &S) { File.allocateObject(S.MDExceptionStream); From 5bf2dbb9e082b315deff223bd83f34244a3de192 Mon Sep 17 00:00:00 2001 From: Jacob Lalonde Date: Wed, 12 Jun 2024 13:33:53 -0700 Subject: [PATCH 04/16] Run git clang format --- .../Minidump/MinidumpFileBuilder.cpp | 159 ++++++++++-------- .../ObjectFile/Minidump/MinidumpFileBuilder.h | 17 +- .../Minidump/ObjectFileMinidump.cpp | 7 +- 3 files changed, 107 insertions(+), 76 deletions(-) diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp index a90b6ec05b838..664d7100fa9ee 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp +++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp @@ -52,11 +52,13 @@ using namespace lldb; using namespace lldb_private; using namespace llvm::minidump; -Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories(const lldb_private::Target &target, const lldb::ProcessSP &process_sp) { +Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories( + const lldb_private::Target &target, const lldb::ProcessSP &process_sp) { // First set the offset on the file, and on the bytes saved m_saved_data_size += header_size; - // We know we will have at least Misc, SystemInfo, Modules, and ThreadList (corresponding memory list for stacks) - // And an additional memory list for non-stacks. + // We know we will have at least Misc, SystemInfo, Modules, and ThreadList + // (corresponding memory list for stacks) And an additional memory list for + // non-stacks. m_expected_directories = 6; // Check if OS is linux if (target.GetArchitecture().GetTriple().getOS() == @@ -69,7 +71,8 @@ Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories(const lldb_private: for (uint32_t thread_idx = 0; thread_idx < num_threads; ++thread_idx) { ThreadSP thread_sp(thread_list.GetThreadAtIndex(thread_idx)); StopInfoSP stop_info_sp = thread_sp->GetStopInfo(); - if (stop_info_sp && stop_info_sp->GetStopReason() == StopReason::eStopReasonException) { + if (stop_info_sp && + stop_info_sp->GetStopReason() == StopReason::eStopReasonException) { m_expected_directories++; } } @@ -108,7 +111,6 @@ Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories(const lldb_private: return error; } - void MinidumpFileBuilder::AddDirectory(StreamType type, uint64_t stream_size) { LocationDescriptor loc; loc.DataSize = static_cast(stream_size); @@ -120,7 +122,7 @@ void MinidumpFileBuilder::AddDirectory(StreamType type, uint64_t stream_size) { dir.Location = loc; m_directories.push_back(dir); - assert (m_expected_directories >= m_directories.size()); + assert(m_expected_directories >= m_directories.size()); } Status MinidumpFileBuilder::AddSystemInfo(const llvm::Triple &target_triple) { @@ -595,9 +597,9 @@ Status MinidumpFileBuilder::FixThreads() { size_t bytes_written = bytes_to_write; error = m_core_file->Write(&thread, bytes_written); if (error.Fail() || bytes_to_write != bytes_written) { - error.SetErrorStringWithFormat( - "Wrote incorrect number of bytes to minidump file. (written %zd/%zd)", bytes_written, - bytes_to_write); + error.SetErrorStringWithFormat( + "Wrote incorrect number of bytes to minidump file. (written %zd/%zd)", + bytes_written, bytes_to_write); return error; } } @@ -820,11 +822,13 @@ void MinidumpFileBuilder::AddLinuxFileStreams( } } -Status MinidumpFileBuilder::AddMemory(const ProcessSP &process_sp, SaveCoreStyle core_style) { +Status MinidumpFileBuilder::AddMemory(const ProcessSP &process_sp, + SaveCoreStyle core_style) { Status error; Process::CoreFileMemoryRanges ranges_for_memory_list; - error = process_sp->CalculateCoreFileSaveRanges(SaveCoreStyle::eSaveCoreStackOnly, ranges_for_memory_list); + error = process_sp->CalculateCoreFileSaveRanges( + SaveCoreStyle::eSaveCoreStackOnly, ranges_for_memory_list); if (error.Fail()) { return error; } @@ -833,42 +837,42 @@ Status MinidumpFileBuilder::AddMemory(const ProcessSP &process_sp, SaveCoreStyle for (const auto &core_range : ranges_for_memory_list) { stack_ranges.insert(core_range.range.start()); } - // We leave a little padding for dictionary and any other metadata we would want. - // Also so that we can put the header of the memory list 64 in 32b land, because the directory - // requires a 32b RVA. + // We leave a little padding for dictionary and any other metadata we would + // want. Also so that we can put the header of the memory list 64 in 32b land, + // because the directory requires a 32b RVA. Process::CoreFileMemoryRanges ranges; error = process_sp->CalculateCoreFileSaveRanges(core_style, ranges); if (error.Fail()) { return error; } - uint64_t total_size = 256 + (ranges.size() * sizeof(llvm::minidump::MemoryDescriptor_64)); + uint64_t total_size = + 256 + (ranges.size() * sizeof(llvm::minidump::MemoryDescriptor_64)); // Take all the memory that will fit in the 32b range. for (int i = ranges.size() - 1; i >= 0; i--) { - addr_t size_to_add = ranges[i].range.size() + sizeof(llvm::minidump::MemoryDescriptor); + addr_t size_to_add = + ranges[i].range.size() + sizeof(llvm::minidump::MemoryDescriptor); // filter out the stacks and check if it's below 32b max. if (stack_ranges.count(ranges[i].range.start()) > 0) { ranges.erase(ranges.begin() + i); - } - else if (total_size + size_to_add < UINT32_MAX) { + } else if (total_size + size_to_add < UINT32_MAX) { ranges_for_memory_list.push_back(ranges[i]); total_size += ranges[i].range.size(); total_size += sizeof(llvm::minidump::MemoryDescriptor); ranges.erase(ranges.begin() + i); - } - else { + } else { break; } } error = AddMemoryList_32(process_sp, ranges_for_memory_list); - if (error.Fail()) + if (error.Fail()) return error; // Add the remaining memory as a 64b range. if (ranges.size() > 0) { error = AddMemoryList_64(process_sp, ranges); - if (error.Fail()) + if (error.Fail()) return error; } @@ -933,7 +937,8 @@ Status MinidumpFileBuilder::DumpDirectories() const { return error; } -Status MinidumpFileBuilder::AddMemoryList_32(const ProcessSP &process_sp, const Process::CoreFileMemoryRanges &ranges) { +Status MinidumpFileBuilder::AddMemoryList_32( + const ProcessSP &process_sp, const Process::CoreFileMemoryRanges &ranges) { std::vector descriptors; Status error; Log *log = GetLog(LLDBLog::Object); @@ -943,11 +948,15 @@ Status MinidumpFileBuilder::AddMemoryList_32(const ProcessSP &process_sp, const const size_t offset_for_data = GetCurrentDataEndOffset(); const addr_t addr = core_range.range.start(); const addr_t size = core_range.range.size(); - std::cout << "Adding Memory Desciptor at " << std::hex << core_range.range.start() << std::dec << " with size " << core_range.range.size() << std::endl; + std::cout << "Adding Memory Desciptor at " << std::hex + << core_range.range.start() << std::dec << " with size " + << core_range.range.size() << std::endl; auto data_up = std::make_unique(size, 0); - LLDB_LOGF(log, "AddMemoryList %d/%d reading memory for region (%zu bytes) [%zx, %zx)", - region_index, ranges.size(), size, addr, addr+ size); + LLDB_LOGF( + log, + "AddMemoryList %d/%d reading memory for region (%zu bytes) [%zx, %zx)", + region_index, ranges.size(), size, addr, addr + size); ++region_index; const size_t bytes_read = @@ -956,16 +965,19 @@ Status MinidumpFileBuilder::AddMemoryList_32(const ProcessSP &process_sp, const LLDB_LOGF(log, "Failed to read memory region. Bytes read: %zu, error: %s", bytes_read, error.AsCString()); // Just skip sections with errors or zero bytes in 32b mode - continue; - } - else if (bytes_read != size) { - LLDB_LOGF(log, "Memory region at: %zu failed to read %zu bytes", addr, size); + continue; + } else if (bytes_read != size) { + LLDB_LOGF(log, "Memory region at: %zu failed to read %zu bytes", addr, + size); } MemoryDescriptor descriptor; - descriptor.StartOfMemoryRange = static_cast(addr); - descriptor.Memory.DataSize = static_cast(bytes_read); - descriptor.Memory.RVA = static_cast(offset_for_data); + descriptor.StartOfMemoryRange = + static_cast(addr); + descriptor.Memory.DataSize = + static_cast(bytes_read); + descriptor.Memory.RVA = + static_cast(offset_for_data); descriptors.push_back(descriptor); if (m_thread_by_range_start.count(addr) > 0) { m_thread_by_range_start[addr].Stack = descriptor; @@ -981,40 +993,48 @@ Status MinidumpFileBuilder::AddMemoryList_32(const ProcessSP &process_sp, const // With a size of the number of ranges as a 32 bit num // And then the size of all the ranges AddDirectory(StreamType::MemoryList, - sizeof(llvm::support::ulittle32_t) + - descriptors.size() * - sizeof(llvm::minidump::MemoryDescriptor)); + sizeof(llvm::support::ulittle32_t) + + descriptors.size() * + sizeof(llvm::minidump::MemoryDescriptor)); - llvm::support::ulittle32_t memory_ranges_num = static_cast(descriptors.size()); + llvm::support::ulittle32_t memory_ranges_num = + static_cast(descriptors.size()); m_data.AppendData(&memory_ranges_num, sizeof(llvm::support::ulittle32_t)); - // For 32b we can get away with writing off the descriptors after the data. This means no cleanup loop needed. - m_data.AppendData(descriptors.data(), descriptors.size() * sizeof(MemoryDescriptor)); + // For 32b we can get away with writing off the descriptors after the data. + // This means no cleanup loop needed. + m_data.AppendData(descriptors.data(), + descriptors.size() * sizeof(MemoryDescriptor)); return error; } -Status MinidumpFileBuilder::AddMemoryList_64(const ProcessSP &process_sp, const Process::CoreFileMemoryRanges &ranges) { +Status MinidumpFileBuilder::AddMemoryList_64( + const ProcessSP &process_sp, const Process::CoreFileMemoryRanges &ranges) { AddDirectory(StreamType::Memory64List, - (sizeof(llvm::support::ulittle64_t) * 2) + - ranges.size() * - sizeof(llvm::minidump::MemoryDescriptor_64)); + (sizeof(llvm::support::ulittle64_t) * 2) + + ranges.size() * sizeof(llvm::minidump::MemoryDescriptor_64)); - llvm::support::ulittle64_t memory_ranges_num = static_cast(ranges.size()); + llvm::support::ulittle64_t memory_ranges_num = + static_cast(ranges.size()); m_data.AppendData(&memory_ranges_num, sizeof(llvm::support::ulittle64_t)); - llvm::support::ulittle64_t memory_ranges_base_rva = static_cast(GetCurrentDataEndOffset()); - m_data.AppendData(&memory_ranges_base_rva, sizeof(llvm::support::ulittle64_t)); + llvm::support::ulittle64_t memory_ranges_base_rva = + static_cast(GetCurrentDataEndOffset()); + m_data.AppendData(&memory_ranges_base_rva, + sizeof(llvm::support::ulittle64_t)); // Capture the starting offset, so we can do cleanup later if needed. uint64_t starting_offset = GetCurrentDataEndOffset(); bool cleanup_required = false; std::vector descriptors; - // Enumerate the ranges and create the memory descriptors so we can append them first + // Enumerate the ranges and create the memory descriptors so we can append + // them first for (const auto core_range : ranges) { // Add the space required to store the memory descriptor MemoryDescriptor_64 memory_desc; memory_desc.StartOfMemoryRange = static_cast(core_range.range.start()); - memory_desc.DataSize = static_cast(core_range.range.size()); + memory_desc.DataSize = + static_cast(core_range.range.size()); descriptors.push_back(memory_desc); // Now write this memory descriptor to the buffer. m_data.AppendData(&memory_desc, sizeof(MemoryDescriptor_64)); @@ -1028,8 +1048,10 @@ Status MinidumpFileBuilder::AddMemoryList_64(const ProcessSP &process_sp, const const addr_t size = core_range.range.size(); auto data_up = std::make_unique(size, 0); - LLDB_LOGF(log, "AddMemoryList_64 %d/%d reading memory for region (%zu bytes) [%zx, %zx)", - region_index, ranges.size(), size, addr, addr+ size); + LLDB_LOGF(log, + "AddMemoryList_64 %d/%d reading memory for region (%zu bytes) " + "[%zx, %zx)", + region_index, ranges.size(), size, addr, addr + size); ++region_index; const size_t bytes_read = @@ -1042,7 +1064,8 @@ Status MinidumpFileBuilder::AddMemoryList_64(const ProcessSP &process_sp, const descriptors[region_index].DataSize = 0; } if (bytes_read != size) { - LLDB_LOGF(log, "Memory region at: %zu failed to read %zu bytes", addr, size); + LLDB_LOGF(log, "Memory region at: %zu failed to read %zu bytes", addr, + size); cleanup_required = true; descriptors[region_index].DataSize = bytes_read; } @@ -1056,30 +1079,31 @@ Status MinidumpFileBuilder::AddMemoryList_64(const ProcessSP &process_sp, const // Early return if there is no cleanup needed. if (!cleanup_required) { return error; - } - else { + } else { // Flush to disk we can make the fixes in place. FlushToDisk(); // Fixup the descriptors that were not read correctly. m_core_file->SeekFromStart(starting_offset); size_t bytes_written = sizeof(MemoryDescriptor_64) * descriptors.size(); error = m_core_file->Write(descriptors.data(), bytes_written); - if (error.Fail() || bytes_written != sizeof(MemoryDescriptor_64) * descriptors.size()) { - error.SetErrorStringWithFormat( - "unable to write the memory descriptors (written %zd/%zd)", - bytes_written, sizeof(MemoryDescriptor_64) * descriptors.size()); + if (error.Fail() || + bytes_written != sizeof(MemoryDescriptor_64) * descriptors.size()) { + error.SetErrorStringWithFormat( + "unable to write the memory descriptors (written %zd/%zd)", + bytes_written, sizeof(MemoryDescriptor_64) * descriptors.size()); } return error; } } -Status MinidumpFileBuilder::AddData(const void* data, addr_t size) { - // This should also get chunked, because worst case we copy over a big +Status MinidumpFileBuilder::AddData(const void *data, addr_t size) { + // This should also get chunked, because worst case we copy over a big // object / memory range, say 5gb. In that case, we'd have to allocate 10gb - // 5 gb for the buffer we're copying from, and then 5gb for the buffer we're copying to. - // Which will be short lived and immedaitely go to disk, the goal here is to limit the number - // of bytes we need to host in memory at any given time. + // 5 gb for the buffer we're copying from, and then 5gb for the buffer we're + // copying to. Which will be short lived and immedaitely go to disk, the goal + // here is to limit the number of bytes we need to host in memory at any given + // time. m_data.AppendData(data, size); if (m_data.GetByteSize() > m_write_chunk_max) { return FlushToDisk(); @@ -1096,16 +1120,17 @@ Status MinidumpFileBuilder::FlushToDisk() { addr_t remaining_bytes = starting_size; size_t offset = 0; - // Unix/Linux OS's return SSIZE for operations, this means a max of 2gb per IO operation - // so we chunk it here. We keep the file size small to try to minimize memory use. + // Unix/Linux OS's return SSIZE for operations, this means a max of 2gb per IO + // operation so we chunk it here. We keep the file size small to try to + // minimize memory use. while (remaining_bytes > 0) { size_t chunk_size = std::min(remaining_bytes, m_write_chunk_max); size_t bytes_written = chunk_size; error = m_core_file->Write(m_data.GetBytes() + offset, bytes_written); if (error.Fail() || bytes_written != chunk_size) { - error.SetErrorStringWithFormat( - "Wrote incorrect number of bytes to minidump file. (written %zd/%zd)", bytes_written, - chunk_size); + error.SetErrorStringWithFormat( + "Wrote incorrect number of bytes to minidump file. (written %zd/%zd)", + bytes_written, chunk_size); return error; } diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h index c5fe9836f06a5..178318a2d6d4b 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h +++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h @@ -56,7 +56,9 @@ class MinidumpFileBuilder { ~MinidumpFileBuilder() = default; - lldb_private::Status AddHeaderAndCalculateDirectories(const lldb_private::Target &target, const lldb::ProcessSP &process_sp); + lldb_private::Status + AddHeaderAndCalculateDirectories(const lldb_private::Target &target, + const lldb::ProcessSP &process_sp); // Add SystemInfo stream, used for storing the most basic information // about the system, platform etc... lldb_private::Status AddSystemInfo(const llvm::Triple &target_triple); @@ -77,16 +79,21 @@ class MinidumpFileBuilder { lldb_private::Status AddThreadList(const lldb::ProcessSP &process_sp); lldb_private::Status AddMemory(const lldb::ProcessSP &process_sp, - lldb::SaveCoreStyle core_style); + lldb::SaveCoreStyle core_style); lldb_private::Status DumpToFile(); private: - // Add data to the end of the buffer, if the buffer exceeds the flush level, trigger a flush. + // Add data to the end of the buffer, if the buffer exceeds the flush level, + // trigger a flush. lldb_private::Status AddData(const void *data, size_t size); // Add MemoryList stream, containing dumps of important memory segments - lldb_private::Status AddMemoryList_64(const lldb::ProcessSP &process_sp, const lldb_private::Process::CoreFileMemoryRanges &ranges); - lldb_private::Status AddMemoryList_32(const lldb::ProcessSP &process_sp, const lldb_private::Process::CoreFileMemoryRanges &ranges); + lldb_private::Status + AddMemoryList_64(const lldb::ProcessSP &process_sp, + const lldb_private::Process::CoreFileMemoryRanges &ranges); + lldb_private::Status + AddMemoryList_32(const lldb::ProcessSP &process_sp, + const lldb_private::Process::CoreFileMemoryRanges &ranges); lldb_private::Status FixThreads(); lldb_private::Status FlushToDisk(); diff --git a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp index 3435d89accc49..b490ff3e17ad3 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp +++ b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp @@ -74,9 +74,8 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp, } MinidumpFileBuilder builder(std::move(maybe_core_file.get())); - Target &target = process_sp->GetTarget(); - builder.AddHeaderAndCalculateDirectories(target, process_sp); + builder.AddHeaderAndCalculateDirectories(target, process_sp); Log *log = GetLog(LLDBLog::Object); error = builder.AddSystemInfo(target.GetArchitecture().GetTriple()); @@ -101,8 +100,8 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp, // Add any exceptions but only if there are any in any threads. builder.AddExceptions(process_sp); - // Note: add memory HAS to be the last thing we do. It can overflow into 64b land - // and many RVA's only support 32b + // Note: add memory HAS to be the last thing we do. It can overflow into 64b + // land and many RVA's only support 32b error = builder.AddMemory(process_sp, core_style); if (error.Fail()) { LLDB_LOGF(log, "AddMemoryList failed: %s", error.AsCString()); From c592936dedb0dc494b03144d741ce57ac0b27809 Mon Sep 17 00:00:00 2001 From: Jacob Lalonde Date: Wed, 12 Jun 2024 13:41:39 -0700 Subject: [PATCH 05/16] Change log to zu and region index to size_t --- .../ObjectFile/Minidump/MinidumpFileBuilder.cpp | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp index 664d7100fa9ee..3c1d88652cc92 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp +++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp @@ -942,20 +942,17 @@ Status MinidumpFileBuilder::AddMemoryList_32( std::vector descriptors; Status error; Log *log = GetLog(LLDBLog::Object); - int region_index = 0; + size_t region_index = 0; for (const auto &core_range : ranges) { // Take the offset before we write. const size_t offset_for_data = GetCurrentDataEndOffset(); const addr_t addr = core_range.range.start(); const addr_t size = core_range.range.size(); - std::cout << "Adding Memory Desciptor at " << std::hex - << core_range.range.start() << std::dec << " with size " - << core_range.range.size() << std::endl; auto data_up = std::make_unique(size, 0); LLDB_LOGF( log, - "AddMemoryList %d/%d reading memory for region (%zu bytes) [%zx, %zx)", + "AddMemoryList %zu/%zu reading memory for region (%zu bytes) [%zx, %zx)", region_index, ranges.size(), size, addr, addr + size); ++region_index; @@ -1042,14 +1039,14 @@ Status MinidumpFileBuilder::AddMemoryList_64( Status error; Log *log = GetLog(LLDBLog::Object); - int region_index = 0; + size_t region_index = 0; for (const auto &core_range : ranges) { const addr_t addr = core_range.range.start(); const addr_t size = core_range.range.size(); auto data_up = std::make_unique(size, 0); LLDB_LOGF(log, - "AddMemoryList_64 %d/%d reading memory for region (%zu bytes) " + "AddMemoryList_64 %zu/%zu reading memory for region (%zu bytes) " "[%zx, %zx)", region_index, ranges.size(), size, addr, addr + size); ++region_index; From 051ebab836a32eeb60be32e4284b65c5f04140d6 Mon Sep 17 00:00:00 2001 From: Jacob Lalonde Date: Thu, 13 Jun 2024 13:17:14 -0700 Subject: [PATCH 06/16] Code review feedback and an attempt to undo unintended changes --- .../Minidump/MinidumpFileBuilder.cpp | 174 ++++++++---------- .../ObjectFile/Minidump/MinidumpFileBuilder.h | 39 ++-- .../Minidump/ObjectFileMinidump.cpp | 19 +- llvm/lib/ObjectYAML/MinidumpEmitter.cpp | 2 - 4 files changed, 103 insertions(+), 131 deletions(-) diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp index 3c1d88652cc92..f379f4d1d7898 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp +++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp @@ -32,6 +32,7 @@ #include "llvm/Support/ConvertUTF.h" #include "llvm/Support/Endian.h" #include "llvm/Support/Error.h" +#include "llvm/TargetParser/Triple.h" #include "Plugins/Process/minidump/MinidumpTypes.h" #include "lldb/lldb-enumerations.h" @@ -52,13 +53,14 @@ using namespace lldb; using namespace lldb_private; using namespace llvm::minidump; -Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories( - const lldb_private::Target &target, const lldb::ProcessSP &process_sp) { +Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories() { // First set the offset on the file, and on the bytes saved m_saved_data_size += header_size; // We know we will have at least Misc, SystemInfo, Modules, and ThreadList // (corresponding memory list for stacks) And an additional memory list for // non-stacks. + + lldb_private::Target& target = m_process_sp->GetTarget(); m_expected_directories = 6; // Check if OS is linux if (target.GetArchitecture().GetTriple().getOS() == @@ -66,7 +68,7 @@ Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories( m_expected_directories += 9; // Go through all of the threads and check for exceptions. - lldb_private::ThreadList thread_list = process_sp->GetThreadList(); + lldb_private::ThreadList thread_list = m_process_sp->GetThreadList(); const uint32_t num_threads = thread_list.GetSize(); for (uint32_t thread_idx = 0; thread_idx < num_threads; ++thread_idx) { ThreadSP thread_sp(thread_list.GetThreadAtIndex(thread_idx)); @@ -125,8 +127,9 @@ void MinidumpFileBuilder::AddDirectory(StreamType type, uint64_t stream_size) { assert(m_expected_directories >= m_directories.size()); } -Status MinidumpFileBuilder::AddSystemInfo(const llvm::Triple &target_triple) { +Status MinidumpFileBuilder::AddSystemInfo() { Status error; + const llvm::Triple &target_triple = m_process_sp->GetTarget().GetArchitecture().GetTriple(); AddDirectory(StreamType::SystemInfo, sizeof(llvm::minidump::SystemInfo)); llvm::minidump::ProcessorArchitecture arch; @@ -276,10 +279,11 @@ llvm::Expected getModuleFileSize(Target &target, // single module. Additional data of variable length, such as module's names, // are stored just after the ModuleList stream. The llvm::minidump::Module // structures point to this helper data by global offset. -Status MinidumpFileBuilder::AddModuleList(Target &target) { +Status MinidumpFileBuilder::AddModuleList() { constexpr size_t minidump_module_size = sizeof(llvm::minidump::Module); Status error; + lldb_private::Target& target = m_process_sp->GetTarget(); const ModuleList &modules = target.GetImages(); llvm::support::ulittle32_t modules_count = static_cast(modules.GetSize()); @@ -551,39 +555,6 @@ class ArchThreadContexts { } }; -// Function returns start and size of the memory region that contains -// memory location pointed to by the current stack pointer. -llvm::Expected> -findStackHelper(const lldb::ProcessSP &process_sp, uint64_t rsp) { - MemoryRegionInfo range_info; - Status error = process_sp->GetMemoryRegionInfo(rsp, range_info); - // Skip failed memory region requests or any regions with no permissions. - if (error.Fail() || range_info.GetLLDBPermissions() == 0) - return llvm::createStringError( - std::errc::not_supported, - "unable to load stack segment of the process"); - - // This is a duplicate of the logic in - // Process::SaveOffRegionsWithStackPointers but ultimately, we need to only - // save up from the start of the stack down to the stack pointer - const addr_t range_end = range_info.GetRange().GetRangeEnd(); - const addr_t red_zone = process_sp->GetABI()->GetRedZoneSize(); - const addr_t stack_head = rsp - red_zone; - if (stack_head > range_info.GetRange().GetRangeEnd()) { - range_info.GetRange().SetRangeBase(stack_head); - range_info.GetRange().SetByteSize(range_end - stack_head); - } - - const addr_t addr = range_info.GetRange().GetRangeBase(); - const addr_t size = range_info.GetRange().GetByteSize(); - - if (size == 0) - return llvm::createStringError(std::errc::not_supported, - "stack segment of the process is empty"); - - return std::make_pair(addr, size); -} - Status MinidumpFileBuilder::FixThreads() { Status error; // If we have anything in the heap flush it. @@ -607,9 +578,9 @@ Status MinidumpFileBuilder::FixThreads() { return error; } -Status MinidumpFileBuilder::AddThreadList(const lldb::ProcessSP &process_sp) { +Status MinidumpFileBuilder::AddThreadList() { constexpr size_t minidump_thread_size = sizeof(llvm::minidump::Thread); - lldb_private::ThreadList thread_list = process_sp->GetThreadList(); + lldb_private::ThreadList thread_list = m_process_sp->GetThreadList(); // size of the entire thread stream consists of: // number of threads and threads array @@ -640,7 +611,7 @@ Status MinidumpFileBuilder::AddThreadList(const lldb::ProcessSP &process_sp) { return error; } RegisterContext *reg_ctx = reg_ctx_sp.get(); - Target &target = process_sp->GetTarget(); + Target &target = m_process_sp->GetTarget(); const ArchSpec &arch = target.GetArchitecture(); ArchThreadContexts thread_context(arch.GetMachine()); if (!thread_context.prepareRegisterContext(reg_ctx)) { @@ -652,7 +623,7 @@ Status MinidumpFileBuilder::AddThreadList(const lldb::ProcessSP &process_sp) { uint64_t sp = reg_ctx->GetSP(); MemoryRegionInfo sp_region; - process_sp->GetMemoryRegionInfo(sp, sp_region); + m_process_sp->GetMemoryRegionInfo(sp, sp_region); // Emit a blank descriptor MemoryDescriptor stack; @@ -693,8 +664,8 @@ Status MinidumpFileBuilder::AddThreadList(const lldb::ProcessSP &process_sp) { return Status(); } -void MinidumpFileBuilder::AddExceptions(const lldb::ProcessSP &process_sp) { - lldb_private::ThreadList thread_list = process_sp->GetThreadList(); +void MinidumpFileBuilder::AddExceptions() { + lldb_private::ThreadList thread_list = m_process_sp->GetThreadList(); const uint32_t num_threads = thread_list.GetSize(); for (uint32_t thread_idx = 0; thread_idx < num_threads; ++thread_idx) { @@ -744,7 +715,7 @@ void MinidumpFileBuilder::AddExceptions(const lldb::ProcessSP &process_sp) { } } -void MinidumpFileBuilder::AddMiscInfo(const lldb::ProcessSP &process_sp) { +void MinidumpFileBuilder::AddMiscInfo() { AddDirectory(StreamType::MiscInfo, sizeof(lldb_private::minidump::MinidumpMiscInfo)); @@ -756,7 +727,7 @@ void MinidumpFileBuilder::AddMiscInfo(const lldb::ProcessSP &process_sp) { misc_info.flags1 = static_cast(0); lldb_private::ProcessInstanceInfo process_info; - process_sp->GetProcessInfo(process_info); + m_process_sp->GetProcessInfo(process_info); if (process_info.ProcessIDIsValid()) { // Set flags1 to reflect that PID is filled in misc_info.flags1 = @@ -778,15 +749,14 @@ getFileStreamHelper(const std::string &path) { return std::move(maybe_stream.get()); } -void MinidumpFileBuilder::AddLinuxFileStreams( - const lldb::ProcessSP &process_sp) { +void MinidumpFileBuilder::AddLinuxFileStreams() { std::vector> files_with_stream_types = { {StreamType::LinuxCPUInfo, "/proc/cpuinfo"}, {StreamType::LinuxLSBRelease, "/etc/lsb-release"}, }; lldb_private::ProcessInstanceInfo process_info; - process_sp->GetProcessInfo(process_info); + m_process_sp->GetProcessInfo(process_info); if (process_info.ProcessIDIsValid()) { lldb::pid_t pid = process_info.GetProcessID(); std::string pid_str = std::to_string(pid); @@ -822,56 +792,66 @@ void MinidumpFileBuilder::AddLinuxFileStreams( } } -Status MinidumpFileBuilder::AddMemory(const ProcessSP &process_sp, - SaveCoreStyle core_style) { +Status MinidumpFileBuilder::AddMemory(SaveCoreStyle core_style) { Status error; - Process::CoreFileMemoryRanges ranges_for_memory_list; - error = process_sp->CalculateCoreFileSaveRanges( - SaveCoreStyle::eSaveCoreStackOnly, ranges_for_memory_list); - if (error.Fail()) { + Process::CoreFileMemoryRanges ranges_32; + Process::CoreFileMemoryRanges ranges_64; + error = m_process_sp->CalculateCoreFileSaveRanges( + SaveCoreStyle::eSaveCoreStackOnly, ranges_32); + if (error.Fail()) return error; - } - std::set stack_ranges; - for (const auto &core_range : ranges_for_memory_list) { - stack_ranges.insert(core_range.range.start()); - } - // We leave a little padding for dictionary and any other metadata we would - // want. Also so that we can put the header of the memory list 64 in 32b land, - // because the directory requires a 32b RVA. - Process::CoreFileMemoryRanges ranges; - error = process_sp->CalculateCoreFileSaveRanges(core_style, ranges); - if (error.Fail()) { + std::set stack_start_addresses; + for (const auto &core_range : ranges_32) + stack_start_addresses.insert(core_range.range.start()); + + + + + uint64_t total_size = ranges_32.size() * sizeof(llvm::minidump::MemoryDescriptor); + for (const auto &core_range : ranges_32) + total_size += core_range.range.size(); + + if (total_size >= UINT32_MAX) { + error.SetErrorStringWithFormat("Unable to write minidump. Stack memory exceeds 32b limit. (Num Stacks %zu)", ranges_32.size()); return error; } - uint64_t total_size = - 256 + (ranges.size() * sizeof(llvm::minidump::MemoryDescriptor_64)); - // Take all the memory that will fit in the 32b range. - for (int i = ranges.size() - 1; i >= 0; i--) { - addr_t size_to_add = - ranges[i].range.size() + sizeof(llvm::minidump::MemoryDescriptor); - // filter out the stacks and check if it's below 32b max. - if (stack_ranges.count(ranges[i].range.start()) > 0) { - ranges.erase(ranges.begin() + i); + Process::CoreFileMemoryRanges all_core_memory_ranges; + if (core_style != SaveCoreStyle::eSaveCoreStackOnly) { + error = m_process_sp->CalculateCoreFileSaveRanges(core_style, all_core_memory_ranges); + if (error.Fail()) + return error; + } + + // We need to calculate the MemoryDescriptor size in the worst case + // Where all memory descriptors are 64b. We also leave some additional padding + // So that we convert over to 64b with space to spare. This does not waste space in the dump + // But instead converts some memory from being in the memorylist_32 to the memorylist_64. + total_size += 256 + (ranges_64.size() - stack_start_addresses.size()) * sizeof(llvm::minidump::MemoryDescriptor_64); + + for (const auto &core_range : all_core_memory_ranges) { + addr_t size_to_add = core_range.range.size() + sizeof(llvm::minidump::MemoryDescriptor); + if (stack_start_addresses.count(core_range.range.start()) > 0) { + // Don't double save stacks. + continue; } else if (total_size + size_to_add < UINT32_MAX) { - ranges_for_memory_list.push_back(ranges[i]); - total_size += ranges[i].range.size(); + ranges_32.push_back(core_range); + total_size += core_range.range.size(); total_size += sizeof(llvm::minidump::MemoryDescriptor); - ranges.erase(ranges.begin() + i); } else { - break; + ranges_64.push_back(core_range); } } - error = AddMemoryList_32(process_sp, ranges_for_memory_list); + error = AddMemoryList_32(ranges_32); if (error.Fail()) return error; // Add the remaining memory as a 64b range. - if (ranges.size() > 0) { - error = AddMemoryList_64(process_sp, ranges); + if (ranges_64.size() > 0) { + error = AddMemoryList_64(ranges_64); if (error.Fail()) return error; } @@ -907,7 +887,7 @@ Status MinidumpFileBuilder::DumpHeader() const { if (error.Fail() || bytes_written != header_size) { if (bytes_written != header_size) error.SetErrorStringWithFormat( - "unable to write the header (written %zd/%zd)", bytes_written, + "Unable to write the minidump header (written %zd/%zd)", bytes_written, header_size); return error; } @@ -937,8 +917,7 @@ Status MinidumpFileBuilder::DumpDirectories() const { return error; } -Status MinidumpFileBuilder::AddMemoryList_32( - const ProcessSP &process_sp, const Process::CoreFileMemoryRanges &ranges) { +Status MinidumpFileBuilder::AddMemoryList_32(const Process::CoreFileMemoryRanges &ranges) { std::vector descriptors; Status error; Log *log = GetLog(LLDBLog::Object); @@ -952,12 +931,12 @@ Status MinidumpFileBuilder::AddMemoryList_32( LLDB_LOGF( log, - "AddMemoryList %zu/%zu reading memory for region (%zu bytes) [%zx, %zx)", + "/AddMemoryList/AddMemory/ %zu/%zu reading memory for region (%zu bytes) [%zx, %zx)", region_index, ranges.size(), size, addr, addr + size); ++region_index; const size_t bytes_read = - process_sp->ReadMemory(addr, data_up->GetBytes(), size, error); + m_process_sp->ReadMemory(addr, data_up->GetBytes(), size, error); if (error.Fail() || bytes_read == 0) { LLDB_LOGF(log, "Failed to read memory region. Bytes read: %zu, error: %s", bytes_read, error.AsCString()); @@ -1005,8 +984,7 @@ Status MinidumpFileBuilder::AddMemoryList_32( return error; } -Status MinidumpFileBuilder::AddMemoryList_64( - const ProcessSP &process_sp, const Process::CoreFileMemoryRanges &ranges) { +Status MinidumpFileBuilder::AddMemoryList_64(const Process::CoreFileMemoryRanges &ranges) { AddDirectory(StreamType::Memory64List, (sizeof(llvm::support::ulittle64_t) * 2) + ranges.size() * sizeof(llvm::minidump::MemoryDescriptor_64)); @@ -1046,13 +1024,13 @@ Status MinidumpFileBuilder::AddMemoryList_64( auto data_up = std::make_unique(size, 0); LLDB_LOGF(log, - "AddMemoryList_64 %zu/%zu reading memory for region (%zu bytes) " + "/AddMemoryList_64/AddMemory %zu/%zu reading memory for region (%zu bytes) " "[%zx, %zx)", region_index, ranges.size(), size, addr, addr + size); ++region_index; const size_t bytes_read = - process_sp->ReadMemory(addr, data_up->GetBytes(), size, error); + m_process_sp->ReadMemory(addr, data_up->GetBytes(), size, error); if (error.Fail()) { LLDB_LOGF(log, "Failed to read memory region. Bytes read: %zu, error: %s", bytes_read, error.AsCString()); @@ -1115,19 +1093,17 @@ Status MinidumpFileBuilder::FlushToDisk() { m_core_file->SeekFromEnd(0); addr_t starting_size = m_data.GetByteSize(); addr_t remaining_bytes = starting_size; - size_t offset = 0; + offset_t offset = 0; - // Unix/Linux OS's return SSIZE for operations, this means a max of 2gb per IO - // operation so we chunk it here. We keep the file size small to try to - // minimize memory use. while (remaining_bytes > 0) { - size_t chunk_size = std::min(remaining_bytes, m_write_chunk_max); - size_t bytes_written = chunk_size; + size_t bytes_written = 0; + // We don't care how many bytes we wrote unless we got an error + // so just decrement the remaining bytes. error = m_core_file->Write(m_data.GetBytes() + offset, bytes_written); - if (error.Fail() || bytes_written != chunk_size) { + if (error.Fail()) { error.SetErrorStringWithFormat( "Wrote incorrect number of bytes to minidump file. (written %zd/%zd)", - bytes_written, chunk_size); + starting_size - remaining_bytes, starting_size); return error; } diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h index 178318a2d6d4b..8f863956c21db 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h +++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h @@ -19,6 +19,7 @@ #include #include #include +#include #include #include "lldb/Target/Process.h" @@ -46,7 +47,8 @@ lldb_private::Status WriteString(const std::string &to_write, /// the data on heap. class MinidumpFileBuilder { public: - MinidumpFileBuilder(lldb::FileUP&& core_file): m_core_file(std::move(core_file)) {}; + MinidumpFileBuilder(lldb::FileUP&& core_file, const lldb::ProcessSP &process_sp) + : m_process_sp(std::move(process_sp)), m_core_file(std::move(core_file)) {}; MinidumpFileBuilder(const MinidumpFileBuilder &) = delete; MinidumpFileBuilder &operator=(const MinidumpFileBuilder &) = delete; @@ -56,31 +58,28 @@ class MinidumpFileBuilder { ~MinidumpFileBuilder() = default; + lldb_private::Status AddThreadList(); + // Add Exception streams for any threads that stopped with exceptions. + void AddExceptions(); lldb_private::Status - AddHeaderAndCalculateDirectories(const lldb_private::Target &target, - const lldb::ProcessSP &process_sp); + AddHeaderAndCalculateDirectories(); // Add SystemInfo stream, used for storing the most basic information // about the system, platform etc... - lldb_private::Status AddSystemInfo(const llvm::Triple &target_triple); + lldb_private::Status AddSystemInfo(); // Add ModuleList stream, containing information about all loaded modules // at the time of saving minidump. - lldb_private::Status AddModuleList(lldb_private::Target &target); + lldb_private::Status AddModuleList(); // Add ThreadList stream, containing information about all threads running // at the moment of core saving. Contains information about thread // contexts. // Add MiscInfo stream, mainly providing ProcessId - void AddMiscInfo(const lldb::ProcessSP &process_sp); + void AddMiscInfo(); // Add informative files about a Linux process - void AddLinuxFileStreams(const lldb::ProcessSP &process_sp); - // Add Exception streams for any threads that stopped with exceptions. - void AddExceptions(const lldb::ProcessSP &process_sp); - // Dump the prepared data into file. In case of the failure data are - // intact. - lldb_private::Status AddThreadList(const lldb::ProcessSP &process_sp); + void AddLinuxFileStreams(); - lldb_private::Status AddMemory(const lldb::ProcessSP &process_sp, - lldb::SaveCoreStyle core_style); + lldb_private::Status AddMemory(lldb::SaveCoreStyle core_style); + // Run cleanup and write all remaining bytes to file lldb_private::Status DumpToFile(); private: @@ -89,11 +88,9 @@ class MinidumpFileBuilder { lldb_private::Status AddData(const void *data, size_t size); // Add MemoryList stream, containing dumps of important memory segments lldb_private::Status - AddMemoryList_64(const lldb::ProcessSP &process_sp, - const lldb_private::Process::CoreFileMemoryRanges &ranges); + AddMemoryList_64(const lldb_private::Process::CoreFileMemoryRanges &ranges); lldb_private::Status - AddMemoryList_32(const lldb::ProcessSP &process_sp, - const lldb_private::Process::CoreFileMemoryRanges &ranges); + AddMemoryList_32(const lldb_private::Process::CoreFileMemoryRanges &ranges); lldb_private::Status FixThreads(); lldb_private::Status FlushToDisk(); @@ -103,7 +100,7 @@ class MinidumpFileBuilder { // Add directory of StreamType pointing to the current end of the prepared // file with the specified size. void AddDirectory(llvm::minidump::StreamType type, uint64_t stream_size); - lldb::addr_t GetCurrentDataEndOffset() const; + lldb::offset_t GetCurrentDataEndOffset() const; // Stores directories to fill in later std::vector m_directories; // When we write off the threads for the first time, we need to clean them up @@ -112,9 +109,11 @@ class MinidumpFileBuilder { // Main data buffer consisting of data without the minidump header and // directories lldb_private::DataBufferHeap m_data; + lldb::ProcessSP m_process_sp; + uint m_expected_directories = 0; uint64_t m_saved_data_size = 0; - size_t m_thread_list_start = 0; + lldb::offset_t m_thread_list_start = 0; // We set the max write amount to 128 mb // Linux has a signed 32b - some buffer on writes // and we have guarauntee a user memory region / 'object' could be over 2gb diff --git a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp index b490ff3e17ad3..c054077437613 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp +++ b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp @@ -72,37 +72,36 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp, error = maybe_core_file.takeError(); return false; } - MinidumpFileBuilder builder(std::move(maybe_core_file.get())); + MinidumpFileBuilder builder(std::move(maybe_core_file.get()), process_sp); Target &target = process_sp->GetTarget(); - builder.AddHeaderAndCalculateDirectories(target, process_sp); - + builder.AddHeaderAndCalculateDirectories(); Log *log = GetLog(LLDBLog::Object); - error = builder.AddSystemInfo(target.GetArchitecture().GetTriple()); + error = builder.AddSystemInfo(); if (error.Fail()) { LLDB_LOGF(log, "AddSystemInfo failed: %s", error.AsCString()); return false; } - builder.AddModuleList(target); - builder.AddMiscInfo(process_sp); + builder.AddModuleList(); + builder.AddMiscInfo(); - error = builder.AddThreadList(process_sp); + error = builder.AddThreadList(); if (error.Fail()) { LLDB_LOGF(log, "AddThreadList failed: %s", error.AsCString()); return false; } if (target.GetArchitecture().GetTriple().getOS() == llvm::Triple::OSType::Linux) { - builder.AddLinuxFileStreams(process_sp); + builder.AddLinuxFileStreams(); } // Add any exceptions but only if there are any in any threads. - builder.AddExceptions(process_sp); + builder.AddExceptions(); // Note: add memory HAS to be the last thing we do. It can overflow into 64b // land and many RVA's only support 32b - error = builder.AddMemory(process_sp, core_style); + error = builder.AddMemory(core_style); if (error.Fail()) { LLDB_LOGF(log, "AddMemoryList failed: %s", error.AsCString()); return false; diff --git a/llvm/lib/ObjectYAML/MinidumpEmitter.cpp b/llvm/lib/ObjectYAML/MinidumpEmitter.cpp index 6ca626f7842d2..24b521a9925c7 100644 --- a/llvm/lib/ObjectYAML/MinidumpEmitter.cpp +++ b/llvm/lib/ObjectYAML/MinidumpEmitter.cpp @@ -6,11 +6,9 @@ // //===----------------------------------------------------------------------===// -#include "llvm/BinaryFormat/Minidump.h" #include "llvm/ObjectYAML/MinidumpYAML.h" #include "llvm/ObjectYAML/yaml2obj.h" #include "llvm/Support/ConvertUTF.h" -#include "llvm/Support/Endian.h" #include "llvm/Support/raw_ostream.h" #include From b51cdbfddaed2de974746f89782c39c4b0addd01 Mon Sep 17 00:00:00 2001 From: Jacob Lalonde Date: Thu, 13 Jun 2024 13:49:03 -0700 Subject: [PATCH 07/16] Try to fix MinidumpFilerbuilder.h to minimize changes, and fix the flush to disk loop. --- .../ObjectFile/Minidump/MinidumpFileBuilder.cpp | 2 +- .../ObjectFile/Minidump/MinidumpFileBuilder.h | 14 +++++++------- .../ObjectFile/Minidump/ObjectFileMinidump.cpp | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp index f379f4d1d7898..0196ed191a01e 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp +++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp @@ -1096,7 +1096,7 @@ Status MinidumpFileBuilder::FlushToDisk() { offset_t offset = 0; while (remaining_bytes > 0) { - size_t bytes_written = 0; + size_t bytes_written = remaining_bytes; // We don't care how many bytes we wrote unless we got an error // so just decrement the remaining bytes. error = m_core_file->Write(m_data.GetBytes() + offset, bytes_written); diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h index 8f863956c21db..52f2af97be8b3 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h +++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h @@ -58,9 +58,6 @@ class MinidumpFileBuilder { ~MinidumpFileBuilder() = default; - lldb_private::Status AddThreadList(); - // Add Exception streams for any threads that stopped with exceptions. - void AddExceptions(); lldb_private::Status AddHeaderAndCalculateDirectories(); // Add SystemInfo stream, used for storing the most basic information @@ -72,6 +69,9 @@ class MinidumpFileBuilder { // Add ThreadList stream, containing information about all threads running // at the moment of core saving. Contains information about thread // contexts. + lldb_private::Status AddThreadList(); + // Add Exception streams for any threads that stopped with exceptions. + void AddExceptions(); // Add MiscInfo stream, mainly providing ProcessId void AddMiscInfo(); // Add informative files about a Linux process @@ -114,10 +114,10 @@ class MinidumpFileBuilder { uint m_expected_directories = 0; uint64_t m_saved_data_size = 0; lldb::offset_t m_thread_list_start = 0; - // We set the max write amount to 128 mb - // Linux has a signed 32b - some buffer on writes - // and we have guarauntee a user memory region / 'object' could be over 2gb - // now that we support 64b memory dumps. + // We set the max write amount to 128 mb, this is arbitrary + // but we want to try to keep the size of m_data small + // and we will only exceed a 128 mb buffer if we get a memory region + // that is larger than 128 mb. static constexpr size_t m_write_chunk_max = (1024 * 1024 * 128); static constexpr size_t header_size = sizeof(llvm::minidump::Header); diff --git a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp index c054077437613..62fad1b3c4160 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp +++ b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp @@ -114,4 +114,4 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp, } return true; -} +} From 53da9f58a9cccc7871af228d1f6b41ac42603600 Mon Sep 17 00:00:00 2001 From: Jacob Lalonde Date: Thu, 13 Jun 2024 13:49:27 -0700 Subject: [PATCH 08/16] Run git clang format --- .../Minidump/MinidumpFileBuilder.cpp | 63 +++++++++++-------- .../ObjectFile/Minidump/MinidumpFileBuilder.h | 9 +-- .../Minidump/ObjectFileMinidump.cpp | 2 +- 3 files changed, 42 insertions(+), 32 deletions(-) diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp index 0196ed191a01e..1360762c53a1e 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp +++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp @@ -24,7 +24,6 @@ #include "lldb/Utility/DataExtractor.h" #include "lldb/Utility/LLDBLog.h" #include "lldb/Utility/Log.h" -#include "lldb/Utility/LLDBLog.h" #include "lldb/Utility/RegisterValue.h" #include "llvm/ADT/StringRef.h" @@ -60,7 +59,7 @@ Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories() { // (corresponding memory list for stacks) And an additional memory list for // non-stacks. - lldb_private::Target& target = m_process_sp->GetTarget(); + lldb_private::Target &target = m_process_sp->GetTarget(); m_expected_directories = 6; // Check if OS is linux if (target.GetArchitecture().GetTriple().getOS() == @@ -129,7 +128,8 @@ void MinidumpFileBuilder::AddDirectory(StreamType type, uint64_t stream_size) { Status MinidumpFileBuilder::AddSystemInfo() { Status error; - const llvm::Triple &target_triple = m_process_sp->GetTarget().GetArchitecture().GetTriple(); + const llvm::Triple &target_triple = + m_process_sp->GetTarget().GetArchitecture().GetTriple(); AddDirectory(StreamType::SystemInfo, sizeof(llvm::minidump::SystemInfo)); llvm::minidump::ProcessorArchitecture arch; @@ -283,7 +283,7 @@ Status MinidumpFileBuilder::AddModuleList() { constexpr size_t minidump_module_size = sizeof(llvm::minidump::Module); Status error; - lldb_private::Target& target = m_process_sp->GetTarget(); + lldb_private::Target &target = m_process_sp->GetTarget(); const ModuleList &modules = target.GetImages(); llvm::support::ulittle32_t modules_count = static_cast(modules.GetSize()); @@ -315,10 +315,12 @@ Status MinidumpFileBuilder::AddModuleList() { auto maybe_mod_size = getModuleFileSize(target, mod); if (!maybe_mod_size) { llvm::Error mod_size_err = maybe_mod_size.takeError(); - llvm::handleAllErrors(std::move(mod_size_err), [&](const llvm::ErrorInfoBase &E) { - error.SetErrorStringWithFormat("Unable to get the size of module %s: %s.", - module_name.c_str(), E.message().c_str()); - }); + llvm::handleAllErrors(std::move(mod_size_err), + [&](const llvm::ErrorInfoBase &E) { + error.SetErrorStringWithFormat( + "Unable to get the size of module %s: %s.", + module_name.c_str(), E.message().c_str()); + }); return error; } @@ -806,33 +808,37 @@ Status MinidumpFileBuilder::AddMemory(SaveCoreStyle core_style) { for (const auto &core_range : ranges_32) stack_start_addresses.insert(core_range.range.start()); - - - - uint64_t total_size = ranges_32.size() * sizeof(llvm::minidump::MemoryDescriptor); + uint64_t total_size = + ranges_32.size() * sizeof(llvm::minidump::MemoryDescriptor); for (const auto &core_range : ranges_32) total_size += core_range.range.size(); if (total_size >= UINT32_MAX) { - error.SetErrorStringWithFormat("Unable to write minidump. Stack memory exceeds 32b limit. (Num Stacks %zu)", ranges_32.size()); + error.SetErrorStringWithFormat("Unable to write minidump. Stack memory " + "exceeds 32b limit. (Num Stacks %zu)", + ranges_32.size()); return error; } Process::CoreFileMemoryRanges all_core_memory_ranges; if (core_style != SaveCoreStyle::eSaveCoreStackOnly) { - error = m_process_sp->CalculateCoreFileSaveRanges(core_style, all_core_memory_ranges); + error = m_process_sp->CalculateCoreFileSaveRanges(core_style, + all_core_memory_ranges); if (error.Fail()) return error; } // We need to calculate the MemoryDescriptor size in the worst case // Where all memory descriptors are 64b. We also leave some additional padding - // So that we convert over to 64b with space to spare. This does not waste space in the dump - // But instead converts some memory from being in the memorylist_32 to the memorylist_64. - total_size += 256 + (ranges_64.size() - stack_start_addresses.size()) * sizeof(llvm::minidump::MemoryDescriptor_64); + // So that we convert over to 64b with space to spare. This does not waste + // space in the dump But instead converts some memory from being in the + // memorylist_32 to the memorylist_64. + total_size += 256 + (ranges_64.size() - stack_start_addresses.size()) * + sizeof(llvm::minidump::MemoryDescriptor_64); for (const auto &core_range : all_core_memory_ranges) { - addr_t size_to_add = core_range.range.size() + sizeof(llvm::minidump::MemoryDescriptor); + addr_t size_to_add = + core_range.range.size() + sizeof(llvm::minidump::MemoryDescriptor); if (stack_start_addresses.count(core_range.range.start()) > 0) { // Don't double save stacks. continue; @@ -887,8 +893,8 @@ Status MinidumpFileBuilder::DumpHeader() const { if (error.Fail() || bytes_written != header_size) { if (bytes_written != header_size) error.SetErrorStringWithFormat( - "Unable to write the minidump header (written %zd/%zd)", bytes_written, - header_size); + "Unable to write the minidump header (written %zd/%zd)", + bytes_written, header_size); return error; } return error; @@ -917,7 +923,8 @@ Status MinidumpFileBuilder::DumpDirectories() const { return error; } -Status MinidumpFileBuilder::AddMemoryList_32(const Process::CoreFileMemoryRanges &ranges) { +Status MinidumpFileBuilder::AddMemoryList_32( + const Process::CoreFileMemoryRanges &ranges) { std::vector descriptors; Status error; Log *log = GetLog(LLDBLog::Object); @@ -929,10 +936,10 @@ Status MinidumpFileBuilder::AddMemoryList_32(const Process::CoreFileMemoryRanges const addr_t size = core_range.range.size(); auto data_up = std::make_unique(size, 0); - LLDB_LOGF( - log, - "/AddMemoryList/AddMemory/ %zu/%zu reading memory for region (%zu bytes) [%zx, %zx)", - region_index, ranges.size(), size, addr, addr + size); + LLDB_LOGF(log, + "/AddMemoryList/AddMemory/ %zu/%zu reading memory for region " + "(%zu bytes) [%zx, %zx)", + region_index, ranges.size(), size, addr, addr + size); ++region_index; const size_t bytes_read = @@ -984,7 +991,8 @@ Status MinidumpFileBuilder::AddMemoryList_32(const Process::CoreFileMemoryRanges return error; } -Status MinidumpFileBuilder::AddMemoryList_64(const Process::CoreFileMemoryRanges &ranges) { +Status MinidumpFileBuilder::AddMemoryList_64( + const Process::CoreFileMemoryRanges &ranges) { AddDirectory(StreamType::Memory64List, (sizeof(llvm::support::ulittle64_t) * 2) + ranges.size() * sizeof(llvm::minidump::MemoryDescriptor_64)); @@ -1024,7 +1032,8 @@ Status MinidumpFileBuilder::AddMemoryList_64(const Process::CoreFileMemoryRanges auto data_up = std::make_unique(size, 0); LLDB_LOGF(log, - "/AddMemoryList_64/AddMemory %zu/%zu reading memory for region (%zu bytes) " + "/AddMemoryList_64/AddMemory %zu/%zu reading memory for region " + "(%zu bytes) " "[%zx, %zx)", region_index, ranges.size(), size, addr, addr + size); ++region_index; diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h index 52f2af97be8b3..d5d2782a9c6c2 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h +++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h @@ -47,8 +47,10 @@ lldb_private::Status WriteString(const std::string &to_write, /// the data on heap. class MinidumpFileBuilder { public: - MinidumpFileBuilder(lldb::FileUP&& core_file, const lldb::ProcessSP &process_sp) - : m_process_sp(std::move(process_sp)), m_core_file(std::move(core_file)) {}; + MinidumpFileBuilder(lldb::FileUP &&core_file, + const lldb::ProcessSP &process_sp) + : m_process_sp(std::move(process_sp)), + m_core_file(std::move(core_file)){}; MinidumpFileBuilder(const MinidumpFileBuilder &) = delete; MinidumpFileBuilder &operator=(const MinidumpFileBuilder &) = delete; @@ -58,8 +60,7 @@ class MinidumpFileBuilder { ~MinidumpFileBuilder() = default; - lldb_private::Status - AddHeaderAndCalculateDirectories(); + lldb_private::Status AddHeaderAndCalculateDirectories(); // Add SystemInfo stream, used for storing the most basic information // about the system, platform etc... lldb_private::Status AddSystemInfo(); diff --git a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp index 62fad1b3c4160..c054077437613 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp +++ b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp @@ -114,4 +114,4 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp, } return true; -} +} From e3841d81cdcfecb53630af40e5f77de15c846d3e Mon Sep 17 00:00:00 2001 From: Jacob Lalonde Date: Thu, 13 Jun 2024 15:45:46 -0700 Subject: [PATCH 09/16] Linting, and add some sorting so that we can determine the largest possible buffer ahead of time --- .../Minidump/MinidumpFileBuilder.cpp | 28 ++++++++++++++----- .../ObjectFile/Minidump/MinidumpFileBuilder.h | 11 ++++---- .../Minidump/ObjectFileMinidump.cpp | 2 +- 3 files changed, 27 insertions(+), 14 deletions(-) diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp index 1360762c53a1e..ed85ad72e096a 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp +++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp @@ -43,6 +43,7 @@ #include #include #include +#include #include #include #include @@ -828,6 +829,9 @@ Status MinidumpFileBuilder::AddMemory(SaveCoreStyle core_style) { return error; } + // Sort the ranges so we try to fit all the small ranges in 32b. + std::sort(all_core_memory_ranges.begin(), all_core_memory_ranges.end()); + // We need to calculate the MemoryDescriptor size in the worst case // Where all memory descriptors are 64b. We also leave some additional padding // So that we convert over to 64b with space to spare. This does not waste @@ -924,20 +928,24 @@ Status MinidumpFileBuilder::DumpDirectories() const { } Status MinidumpFileBuilder::AddMemoryList_32( - const Process::CoreFileMemoryRanges &ranges) { + Process::CoreFileMemoryRanges &ranges) { std::vector descriptors; Status error; + if (ranges.size() == 0) + return error; + + std::sort(ranges.begin(), ranges.end()); Log *log = GetLog(LLDBLog::Object); size_t region_index = 0; + auto data_up = std::make_unique(ranges.back().range.size(), 0); for (const auto &core_range : ranges) { // Take the offset before we write. const size_t offset_for_data = GetCurrentDataEndOffset(); const addr_t addr = core_range.range.start(); const addr_t size = core_range.range.size(); - auto data_up = std::make_unique(size, 0); LLDB_LOGF(log, - "/AddMemoryList/AddMemory/ %zu/%zu reading memory for region " + "AddMemoryList %zu/%zu reading memory for region " "(%zu bytes) [%zx, %zx)", region_index, ranges.size(), size, addr, addr + size); ++region_index; @@ -992,7 +1000,13 @@ Status MinidumpFileBuilder::AddMemoryList_32( } Status MinidumpFileBuilder::AddMemoryList_64( - const Process::CoreFileMemoryRanges &ranges) { + Process::CoreFileMemoryRanges &ranges) { + Status error; + if (ranges.empty()) + return error; + + // Sort the ranges so we can preallocate a buffer big enough for the largest item. + std::sort(ranges.begin(), ranges.end()); AddDirectory(StreamType::Memory64List, (sizeof(llvm::support::ulittle64_t) * 2) + ranges.size() * sizeof(llvm::minidump::MemoryDescriptor_64)); @@ -1023,16 +1037,16 @@ Status MinidumpFileBuilder::AddMemoryList_64( m_data.AppendData(&memory_desc, sizeof(MemoryDescriptor_64)); } - Status error; + Log *log = GetLog(LLDBLog::Object); size_t region_index = 0; + auto data_up = std::make_unique(ranges.back().range.size(), 0); for (const auto &core_range : ranges) { const addr_t addr = core_range.range.start(); const addr_t size = core_range.range.size(); - auto data_up = std::make_unique(size, 0); LLDB_LOGF(log, - "/AddMemoryList_64/AddMemory %zu/%zu reading memory for region " + "AddMemoryList_64 %zu/%zu reading memory for region " "(%zu bytes) " "[%zx, %zx)", region_index, ranges.size(), size, addr, addr + size); diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h index d5d2782a9c6c2..2ae966fb6bf38 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h +++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h @@ -49,8 +49,8 @@ class MinidumpFileBuilder { public: MinidumpFileBuilder(lldb::FileUP &&core_file, const lldb::ProcessSP &process_sp) - : m_process_sp(std::move(process_sp)), - m_core_file(std::move(core_file)){}; + : m_process_sp(process_sp), + m_core_file(std::move(core_file)) {}; MinidumpFileBuilder(const MinidumpFileBuilder &) = delete; MinidumpFileBuilder &operator=(const MinidumpFileBuilder &) = delete; @@ -73,7 +73,7 @@ class MinidumpFileBuilder { lldb_private::Status AddThreadList(); // Add Exception streams for any threads that stopped with exceptions. void AddExceptions(); - // Add MiscInfo stream, mainly providing ProcessId + // Add MiscInfo stream, mainly providing ProcessId 8 void AddMiscInfo(); // Add informative files about a Linux process void AddLinuxFileStreams(); @@ -89,15 +89,14 @@ class MinidumpFileBuilder { lldb_private::Status AddData(const void *data, size_t size); // Add MemoryList stream, containing dumps of important memory segments lldb_private::Status - AddMemoryList_64(const lldb_private::Process::CoreFileMemoryRanges &ranges); + AddMemoryList_64(lldb_private::Process::CoreFileMemoryRanges &ranges); lldb_private::Status - AddMemoryList_32(const lldb_private::Process::CoreFileMemoryRanges &ranges); + AddMemoryList_32(lldb_private::Process::CoreFileMemoryRanges &ranges); lldb_private::Status FixThreads(); lldb_private::Status FlushToDisk(); lldb_private::Status DumpHeader() const; lldb_private::Status DumpDirectories() const; - bool CheckIf_64Bit(const size_t size); // Add directory of StreamType pointing to the current end of the prepared // file with the specified size. void AddDirectory(llvm::minidump::StreamType type, uint64_t stream_size); diff --git a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp index c054077437613..66fdda5313229 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp +++ b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp @@ -103,7 +103,7 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp, // land and many RVA's only support 32b error = builder.AddMemory(core_style); if (error.Fail()) { - LLDB_LOGF(log, "AddMemoryList failed: %s", error.AsCString()); + LLDB_LOGF(log, "AddMemory failed: %s", error.AsCString()); return false; } From 5e375eaca4c31167edb531be7fd94e00c311923f Mon Sep 17 00:00:00 2001 From: Jacob Lalonde Date: Fri, 14 Jun 2024 17:03:44 -0700 Subject: [PATCH 10/16] Implement fut further feedback and safety checks. Fix bug with 64b being off by 8 + descirptor count * sizeof(descriptor_64) --- .../Minidump/MinidumpFileBuilder.cpp | 177 +++++++++++------- .../ObjectFile/Minidump/MinidumpFileBuilder.h | 23 ++- .../Minidump/ObjectFileMinidump.cpp | 42 +++-- 3 files changed, 151 insertions(+), 91 deletions(-) diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp index ed85ad72e096a..00e47b86eae59 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp +++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp @@ -24,6 +24,7 @@ #include "lldb/Utility/DataExtractor.h" #include "lldb/Utility/LLDBLog.h" #include "lldb/Utility/Log.h" +#include "lldb/Utility/RangeMap.h" #include "lldb/Utility/RegisterValue.h" #include "llvm/ADT/StringRef.h" @@ -59,10 +60,9 @@ Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories() { // We know we will have at least Misc, SystemInfo, Modules, and ThreadList // (corresponding memory list for stacks) And an additional memory list for // non-stacks. - lldb_private::Target &target = m_process_sp->GetTarget(); m_expected_directories = 6; - // Check if OS is linux + // Check if OS is linux and reserve directory space for all linux specific breakpad extension directories. if (target.GetArchitecture().GetTriple().getOS() == llvm::Triple::OSType::Linux) m_expected_directories += 9; @@ -82,38 +82,42 @@ Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories() { // Now offset the file by the directores so we can write them in later. offset_t directory_offset = m_expected_directories * directory_size; m_saved_data_size += directory_offset; - // Replace this when we make a better way to do this. Status error; - Header empty_header; - size_t bytes_written; - bytes_written = header_size; - error = m_core_file->Write(&empty_header, bytes_written); - if (error.Fail() || bytes_written != header_size) { - if (bytes_written != header_size) + size_t zeroes = 0; // 8 0's + size_t remaining_bytes = m_saved_data_size; + while (remaining_bytes > 0) { + // Keep filling in zero's until we preallocate enough space for the header + // and directory sections. + size_t bytes_written = std::min(remaining_bytes, sizeof(size_t)); + error = m_core_file->Write(&zeroes, bytes_written); + if (error.Fail()) { error.SetErrorStringWithFormat( - "unable to write the header (written %zd/%zd)", bytes_written, - header_size); - return error; - } - - for (uint i = 0; i < m_expected_directories; i++) { - size_t bytes_written; - bytes_written = directory_size; - Directory empty_directory; - error = m_core_file->Write(&empty_directory, bytes_written); - if (error.Fail() || bytes_written != directory_size) { - if (bytes_written != directory_size) - error.SetErrorStringWithFormat( - "unable to write the directory (written %zd/%zd)", bytes_written, - directory_size); - return error; + "Unable to write header and directory padding (written %zd/%zd)", + remaining_bytes - m_saved_data_size, m_saved_data_size); + break; } + + remaining_bytes -= bytes_written; } return error; } -void MinidumpFileBuilder::AddDirectory(StreamType type, uint64_t stream_size) { +Status MinidumpFileBuilder::AddDirectory(StreamType type, uint64_t stream_size) { + Status error; + if (GetCurrentDataEndOffset() > UINT32_MAX) { + error.SetErrorStringWithFormat( + "Unable to add directory for stream type %x, offset is greater then 32 bit limit.", type); + return error; + } + + if (m_directories.size() + 1 > m_expected_directories) { + error.SetErrorStringWithFormat( + "Unable to add directory for stream type %x, exceeded expected number of directories %d.", + type, m_expected_directories); + return error; + } + LocationDescriptor loc; loc.DataSize = static_cast(stream_size); // Stream will begin at the current end of data section @@ -124,14 +128,16 @@ void MinidumpFileBuilder::AddDirectory(StreamType type, uint64_t stream_size) { dir.Location = loc; m_directories.push_back(dir); - assert(m_expected_directories >= m_directories.size()); + return error; } Status MinidumpFileBuilder::AddSystemInfo() { Status error; const llvm::Triple &target_triple = m_process_sp->GetTarget().GetArchitecture().GetTriple(); - AddDirectory(StreamType::SystemInfo, sizeof(llvm::minidump::SystemInfo)); + error = AddDirectory(StreamType::SystemInfo, sizeof(llvm::minidump::SystemInfo)); + if (error.Fail()) + return error; llvm::minidump::ProcessorArchitecture arch; switch (target_triple.getArch()) { @@ -301,7 +307,9 @@ Status MinidumpFileBuilder::AddModuleList() { sizeof(llvm::support::ulittle32_t) + modules_count * minidump_module_size; // Adding directory describing this stream. - AddDirectory(StreamType::ModuleList, module_stream_size); + error = AddDirectory(StreamType::ModuleList, module_stream_size); + if (error.Fail()) + return error; m_data.AppendData(&modules_count, sizeof(llvm::support::ulittle32_t)); @@ -561,12 +569,12 @@ class ArchThreadContexts { Status MinidumpFileBuilder::FixThreads() { Status error; // If we have anything in the heap flush it. - FlushToDisk(); + FlushBufferToDisk(); m_core_file->SeekFromStart(m_thread_list_start); - for (auto &pair : m_thread_by_range_start) { + for (auto &pair : m_thread_by_range_end) { // The thread objects will get a new memory descriptor added // When we are emitting the memory list and then we write it here - llvm::minidump::Thread thread = pair.second; + const llvm::minidump::Thread &thread = pair.second; size_t bytes_to_write = sizeof(llvm::minidump::Thread); size_t bytes_written = bytes_to_write; error = m_core_file->Write(&thread, bytes_written); @@ -591,8 +599,10 @@ Status MinidumpFileBuilder::AddThreadList() { thread_list.GetSize() * minidump_thread_size; // save for the ability to set up RVA size_t size_before = GetCurrentDataEndOffset(); - - AddDirectory(StreamType::ThreadList, thread_stream_size); + Status error; + error = AddDirectory(StreamType::ThreadList, thread_stream_size); + if (error.Fail()) + return error; llvm::support::ulittle32_t thread_count = static_cast(thread_list.GetSize()); @@ -607,7 +617,6 @@ Status MinidumpFileBuilder::AddThreadList() { for (uint32_t thread_idx = 0; thread_idx < num_threads; ++thread_idx) { ThreadSP thread_sp(thread_list.GetThreadAtIndex(thread_idx)); RegisterContextSP reg_ctx_sp(thread_sp->GetRegisterContext()); - Status error; if (!reg_ctx_sp) { error.SetErrorString("Unable to get the register context."); @@ -657,7 +666,7 @@ Status MinidumpFileBuilder::AddThreadList() { t.Stack = stack, t.Context = thread_context_memory_locator; // We save off the stack object so we can circle back and clean it up. - m_thread_by_range_start[sp_region.GetRange().GetRangeBase()] = t; + m_thread_by_range_end[sp_region.GetRange().GetRangeEnd()] = t; m_data.AppendData(&t, sizeof(llvm::minidump::Thread)); } @@ -667,9 +676,9 @@ Status MinidumpFileBuilder::AddThreadList() { return Status(); } -void MinidumpFileBuilder::AddExceptions() { +Status MinidumpFileBuilder::AddExceptions() { lldb_private::ThreadList thread_list = m_process_sp->GetThreadList(); - + Status error; const uint32_t num_threads = thread_list.GetSize(); for (uint32_t thread_idx = 0; thread_idx < num_threads; ++thread_idx) { ThreadSP thread_sp(thread_list.GetThreadAtIndex(thread_idx)); @@ -688,7 +697,10 @@ void MinidumpFileBuilder::AddExceptions() { if (add_exception) { constexpr size_t minidump_exception_size = sizeof(llvm::minidump::ExceptionStream); - AddDirectory(StreamType::Exception, minidump_exception_size); + error = AddDirectory(StreamType::Exception, minidump_exception_size); + if (error.Fail()) + return error; + StopInfoSP stop_info_sp = thread_sp->GetStopInfo(); RegisterContextSP reg_ctx_sp(thread_sp->GetRegisterContext()); Exception exp_record = {}; @@ -716,11 +728,16 @@ void MinidumpFileBuilder::AddExceptions() { m_data.AppendData(&exp_stream, minidump_exception_size); } } + + return error; } -void MinidumpFileBuilder::AddMiscInfo() { - AddDirectory(StreamType::MiscInfo, +lldb_private::Status MinidumpFileBuilder::AddMiscInfo() { + Status error; + error = AddDirectory(StreamType::MiscInfo, sizeof(lldb_private::minidump::MinidumpMiscInfo)); + if (error.Fail()) + return error; lldb_private::minidump::MinidumpMiscInfo misc_info; misc_info.size = static_cast( @@ -742,6 +759,7 @@ void MinidumpFileBuilder::AddMiscInfo() { m_data.AppendData(&misc_info, sizeof(lldb_private::minidump::MinidumpMiscInfo)); + return error; } std::unique_ptr @@ -752,7 +770,12 @@ getFileStreamHelper(const std::string &path) { return std::move(maybe_stream.get()); } -void MinidumpFileBuilder::AddLinuxFileStreams() { +Status MinidumpFileBuilder::AddLinuxFileStreams() { + Status error; + // No-op if we are not on linux. + if (m_process_sp->GetTarget().GetArchitecture().GetTriple().getOS() != llvm::Triple::Linux) + return error; + std::vector> files_with_stream_types = { {StreamType::LinuxCPUInfo, "/proc/cpuinfo"}, {StreamType::LinuxLSBRelease, "/etc/lsb-release"}, @@ -779,7 +802,7 @@ void MinidumpFileBuilder::AddLinuxFileStreams() { {StreamType::LinuxProcFD, "/proc/" + pid_str + "/fd"}); } - Status error; + for (const auto &entry : files_with_stream_types) { StreamType stream = entry.first; std::string path = entry.second; @@ -789,13 +812,17 @@ void MinidumpFileBuilder::AddLinuxFileStreams() { size_t size = memory_buffer->getBufferSize(); if (size == 0) continue; - AddDirectory(stream, size); + error = AddDirectory(stream, size); + if (error.Fail()) + return error; m_data.AppendData(memory_buffer->getBufferStart(), size); } } + + return error; } -Status MinidumpFileBuilder::AddMemory(SaveCoreStyle core_style) { +Status MinidumpFileBuilder::AddMemoryList(SaveCoreStyle core_style) { Status error; Process::CoreFileMemoryRanges ranges_32; @@ -829,24 +856,23 @@ Status MinidumpFileBuilder::AddMemory(SaveCoreStyle core_style) { return error; } - // Sort the ranges so we try to fit all the small ranges in 32b. - std::sort(all_core_memory_ranges.begin(), all_core_memory_ranges.end()); // We need to calculate the MemoryDescriptor size in the worst case // Where all memory descriptors are 64b. We also leave some additional padding // So that we convert over to 64b with space to spare. This does not waste // space in the dump But instead converts some memory from being in the // memorylist_32 to the memorylist_64. - total_size += 256 + (ranges_64.size() - stack_start_addresses.size()) * + total_size += 256 + (all_core_memory_ranges.size() - stack_start_addresses.size()) * sizeof(llvm::minidump::MemoryDescriptor_64); for (const auto &core_range : all_core_memory_ranges) { addr_t size_to_add = core_range.range.size() + sizeof(llvm::minidump::MemoryDescriptor); - if (stack_start_addresses.count(core_range.range.start()) > 0) { + if (stack_start_addresses.count(core_range.range.start()) > 0) // Don't double save stacks. continue; - } else if (total_size + size_to_add < UINT32_MAX) { + + if (total_size + size_to_add < UINT32_MAX) { ranges_32.push_back(core_range); total_size += core_range.range.size(); total_size += sizeof(llvm::minidump::MemoryDescriptor); @@ -927,22 +953,30 @@ Status MinidumpFileBuilder::DumpDirectories() const { return error; } +static size_t GetLargestRange(const Process::CoreFileMemoryRanges &ranges) { + size_t max_size = 0; + for (const auto &core_range : ranges) { + max_size = std::max(max_size, core_range.range.size()); + } + return max_size; +} + Status MinidumpFileBuilder::AddMemoryList_32( - Process::CoreFileMemoryRanges &ranges) { + Process::CoreFileMemoryRanges &ranges) { std::vector descriptors; Status error; if (ranges.size() == 0) return error; - std::sort(ranges.begin(), ranges.end()); Log *log = GetLog(LLDBLog::Object); size_t region_index = 0; - auto data_up = std::make_unique(ranges.back().range.size(), 0); + auto data_up = std::make_unique(GetLargestRange(ranges), 0); for (const auto &core_range : ranges) { // Take the offset before we write. const size_t offset_for_data = GetCurrentDataEndOffset(); const addr_t addr = core_range.range.start(); const addr_t size = core_range.range.size(); + const addr_t end = core_range.range.end(); LLDB_LOGF(log, "AddMemoryList %zu/%zu reading memory for region " @@ -970,9 +1004,8 @@ Status MinidumpFileBuilder::AddMemoryList_32( descriptor.Memory.RVA = static_cast(offset_for_data); descriptors.push_back(descriptor); - if (m_thread_by_range_start.count(addr) > 0) { - m_thread_by_range_start[addr].Stack = descriptor; - } + if (m_thread_by_range_end.count(end) > 0) + m_thread_by_range_end[end].Stack = descriptor; // Add the data to the buffer, flush as needed. error = AddData(data_up->GetBytes(), bytes_read); @@ -983,10 +1016,12 @@ Status MinidumpFileBuilder::AddMemoryList_32( // Add a directory that references this list // With a size of the number of ranges as a 32 bit num // And then the size of all the ranges - AddDirectory(StreamType::MemoryList, + error = AddDirectory(StreamType::MemoryList, sizeof(llvm::support::ulittle32_t) + descriptors.size() * sizeof(llvm::minidump::MemoryDescriptor)); + if (error.Fail()) + return error; llvm::support::ulittle32_t memory_ranges_num = static_cast(descriptors.size()); @@ -1005,21 +1040,25 @@ Status MinidumpFileBuilder::AddMemoryList_64( if (ranges.empty()) return error; - // Sort the ranges so we can preallocate a buffer big enough for the largest item. - std::sort(ranges.begin(), ranges.end()); - AddDirectory(StreamType::Memory64List, + error = AddDirectory(StreamType::Memory64List, (sizeof(llvm::support::ulittle64_t) * 2) + ranges.size() * sizeof(llvm::minidump::MemoryDescriptor_64)); + if (error.Fail()) + return error; llvm::support::ulittle64_t memory_ranges_num = static_cast(ranges.size()); m_data.AppendData(&memory_ranges_num, sizeof(llvm::support::ulittle64_t)); + // Capture the starting offset for all the descriptors so we can clean them up + // if needed. + offset_t starting_offset = GetCurrentDataEndOffset() + sizeof(llvm::support::ulittle64_t); + // The base_rva needs to start after the directories, which is right after + // this 8 byte variable. + offset_t base_rva = starting_offset + (ranges.size() * sizeof(llvm::minidump::MemoryDescriptor_64)); llvm::support::ulittle64_t memory_ranges_base_rva = - static_cast(GetCurrentDataEndOffset()); + static_cast(base_rva); m_data.AppendData(&memory_ranges_base_rva, sizeof(llvm::support::ulittle64_t)); - // Capture the starting offset, so we can do cleanup later if needed. - uint64_t starting_offset = GetCurrentDataEndOffset(); bool cleanup_required = false; std::vector descriptors; @@ -1040,7 +1079,7 @@ Status MinidumpFileBuilder::AddMemoryList_64( Log *log = GetLog(LLDBLog::Object); size_t region_index = 0; - auto data_up = std::make_unique(ranges.back().range.size(), 0); + auto data_up = std::make_unique(GetLargestRange(ranges), 0); for (const auto &core_range : ranges) { const addr_t addr = core_range.range.start(); const addr_t size = core_range.range.size(); @@ -1079,7 +1118,7 @@ Status MinidumpFileBuilder::AddMemoryList_64( return error; } else { // Flush to disk we can make the fixes in place. - FlushToDisk(); + FlushBufferToDisk(); // Fixup the descriptors that were not read correctly. m_core_file->SeekFromStart(starting_offset); size_t bytes_written = sizeof(MemoryDescriptor_64) * descriptors.size(); @@ -1104,13 +1143,13 @@ Status MinidumpFileBuilder::AddData(const void *data, addr_t size) { // time. m_data.AppendData(data, size); if (m_data.GetByteSize() > m_write_chunk_max) { - return FlushToDisk(); + return FlushBufferToDisk(); } return Status(); } -Status MinidumpFileBuilder::FlushToDisk() { +Status MinidumpFileBuilder::FlushBufferToDisk() { Status error; // Set the stream to it's end. m_core_file->SeekFromEnd(0); @@ -1139,10 +1178,10 @@ Status MinidumpFileBuilder::FlushToDisk() { return error; } -Status MinidumpFileBuilder::DumpToFile() { +Status MinidumpFileBuilder::DumpFile() { Status error; // If anything is left unsaved, dump it. - error = FlushToDisk(); + error = FlushBufferToDisk(); if (error.Fail()) return error; diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h index 2ae966fb6bf38..31a587813b43e 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h +++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h @@ -19,6 +19,7 @@ #include #include #include +#include #include #include @@ -72,16 +73,16 @@ class MinidumpFileBuilder { // contexts. lldb_private::Status AddThreadList(); // Add Exception streams for any threads that stopped with exceptions. - void AddExceptions(); - // Add MiscInfo stream, mainly providing ProcessId 8 - void AddMiscInfo(); + lldb_private::Status AddExceptions(); + // Add MemoryList stream, containing dumps of important memory segments + lldb_private::Status AddMemoryList(lldb::SaveCoreStyle core_style); + // Add MiscInfo stream, mainly providing ProcessId + lldb_private::Status AddMiscInfo(); // Add informative files about a Linux process - void AddLinuxFileStreams(); - - lldb_private::Status AddMemory(lldb::SaveCoreStyle core_style); + lldb_private::Status AddLinuxFileStreams(); // Run cleanup and write all remaining bytes to file - lldb_private::Status DumpToFile(); + lldb_private::Status DumpFile(); private: // Add data to the end of the buffer, if the buffer exceeds the flush level, @@ -93,19 +94,21 @@ class MinidumpFileBuilder { lldb_private::Status AddMemoryList_32(lldb_private::Process::CoreFileMemoryRanges &ranges); lldb_private::Status FixThreads(); - lldb_private::Status FlushToDisk(); + lldb_private::Status FlushBufferToDisk(); lldb_private::Status DumpHeader() const; lldb_private::Status DumpDirectories() const; // Add directory of StreamType pointing to the current end of the prepared // file with the specified size. - void AddDirectory(llvm::minidump::StreamType type, uint64_t stream_size); + lldb_private::Status AddDirectory(llvm::minidump::StreamType type, uint64_t stream_size); lldb::offset_t GetCurrentDataEndOffset() const; // Stores directories to fill in later std::vector m_directories; // When we write off the threads for the first time, we need to clean them up // and give them the correct RVA once we write the stack memory list. - std::map m_thread_by_range_start; + // We save by the end because we only take from the stack pointer up + // So the saved off range base can differ from the memory region the stack pointer is in. + std::unordered_map m_thread_by_range_end; // Main data buffer consisting of data without the minidump header and // directories lldb_private::DataBufferHeap m_data; diff --git a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp index 66fdda5313229..5174d73e4f579 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp +++ b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp @@ -15,6 +15,7 @@ #include "lldb/Core/Section.h" #include "lldb/Target/Process.h" #include "lldb/Utility/LLDBLog.h" +#include "lldb/Utility/Log.h" #include "llvm/Support/FileSystem.h" #include @@ -74,42 +75,59 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp, } MinidumpFileBuilder builder(std::move(maybe_core_file.get()), process_sp); - Target &target = process_sp->GetTarget(); - builder.AddHeaderAndCalculateDirectories(); Log *log = GetLog(LLDBLog::Object); + error = builder.AddHeaderAndCalculateDirectories(); + if (error.Fail()) { + LLDB_LOGF(log, "AddHeaderAndCalculateDirectories failed: %s", error.AsCString()); + return false; + }; error = builder.AddSystemInfo(); if (error.Fail()) { LLDB_LOGF(log, "AddSystemInfo failed: %s", error.AsCString()); return false; } - builder.AddModuleList(); - builder.AddMiscInfo(); + error = builder.AddModuleList(); + if (error.Fail()) { + LLDB_LOGF(log, "AddModuleList failed: %s", error.AsCString()); + return false; + } + error = builder.AddMiscInfo(); + if (error.Fail()) { + LLDB_LOGF(log, "AddMiscInfo failed: %s", error.AsCString()); + return false; + } error = builder.AddThreadList(); if (error.Fail()) { LLDB_LOGF(log, "AddThreadList failed: %s", error.AsCString()); return false; } - if (target.GetArchitecture().GetTriple().getOS() == - llvm::Triple::OSType::Linux) { - builder.AddLinuxFileStreams(); + + error = builder.AddLinuxFileStreams(); + if (error.Fail()) { + LLDB_LOGF(log, "AddLinuxFileStreams failed: %s", error.AsCString()); + return false; } // Add any exceptions but only if there are any in any threads. - builder.AddExceptions(); + error = builder.AddExceptions(); + if (error.Fail()) { + LLDB_LOGF(log, "AddExceptions failed: %s", error.AsCString()); + return false; + } // Note: add memory HAS to be the last thing we do. It can overflow into 64b // land and many RVA's only support 32b - error = builder.AddMemory(core_style); + error = builder.AddMemoryList(core_style); if (error.Fail()) { - LLDB_LOGF(log, "AddMemory failed: %s", error.AsCString()); + LLDB_LOGF(log, "AddMemoryList failed: %s", error.AsCString()); return false; } - error = builder.DumpToFile(); + error = builder.DumpFile(); if (error.Fail()) { - LLDB_LOGF(log, "DumpToFile failed: %s", error.AsCString()); + LLDB_LOGF(log, "DumpFile failed: %s", error.AsCString()); return false; } From f5de3f1e30acd9f3229caf00d8a4bb0774895159 Mon Sep 17 00:00:00 2001 From: Jacob Lalonde Date: Mon, 17 Jun 2024 13:18:31 -0700 Subject: [PATCH 11/16] Implement seek logic instead of explicitly writing bytes. Fix bug and instead keep tracrack internally. Add comments explaining serialization formating and process as requested by Jeffrey. Run Git clang format. --- .../Minidump/MinidumpFileBuilder.cpp | 131 +++++++++--------- .../ObjectFile/Minidump/MinidumpFileBuilder.h | 57 ++++++-- .../Minidump/ObjectFileMinidump.cpp | 5 +- 3 files changed, 110 insertions(+), 83 deletions(-) diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp index 00e47b86eae59..9c7b23cc0926f 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp +++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp @@ -56,13 +56,14 @@ using namespace llvm::minidump; Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories() { // First set the offset on the file, and on the bytes saved - m_saved_data_size += header_size; + m_saved_data_size += HEADER_SIZE; // We know we will have at least Misc, SystemInfo, Modules, and ThreadList // (corresponding memory list for stacks) And an additional memory list for // non-stacks. lldb_private::Target &target = m_process_sp->GetTarget(); m_expected_directories = 6; - // Check if OS is linux and reserve directory space for all linux specific breakpad extension directories. + // Check if OS is linux and reserve directory space for all linux specific + // breakpad extension directories. if (target.GetArchitecture().GetTriple().getOS() == llvm::Triple::OSType::Linux) m_expected_directories += 9; @@ -79,41 +80,32 @@ Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories() { } } - // Now offset the file by the directores so we can write them in later. - offset_t directory_offset = m_expected_directories * directory_size; - m_saved_data_size += directory_offset; + m_saved_data_size += + m_expected_directories * sizeof(llvm::minidump::Directory); Status error; - size_t zeroes = 0; // 8 0's - size_t remaining_bytes = m_saved_data_size; - while (remaining_bytes > 0) { - // Keep filling in zero's until we preallocate enough space for the header - // and directory sections. - size_t bytes_written = std::min(remaining_bytes, sizeof(size_t)); - error = m_core_file->Write(&zeroes, bytes_written); - if (error.Fail()) { - error.SetErrorStringWithFormat( - "Unable to write header and directory padding (written %zd/%zd)", - remaining_bytes - m_saved_data_size, m_saved_data_size); - break; - } - - remaining_bytes -= bytes_written; - } + offset_t new_offset = m_core_file->SeekFromStart(m_saved_data_size); + if (new_offset != m_saved_data_size) + error.SetErrorStringWithFormat("Failed to fill in header and directory " + "sections. Written / Expected (%zu / %zu)", + new_offset, m_saved_data_size); return error; } -Status MinidumpFileBuilder::AddDirectory(StreamType type, uint64_t stream_size) { +Status MinidumpFileBuilder::AddDirectory(StreamType type, + uint64_t stream_size) { Status error; if (GetCurrentDataEndOffset() > UINT32_MAX) { - error.SetErrorStringWithFormat( - "Unable to add directory for stream type %x, offset is greater then 32 bit limit.", type); + error.SetErrorStringWithFormat("Unable to add directory for stream type " + "%x, offset is greater then 32 bit limit.", + type); return error; } if (m_directories.size() + 1 > m_expected_directories) { error.SetErrorStringWithFormat( - "Unable to add directory for stream type %x, exceeded expected number of directories %d.", + "Unable to add directory for stream type %x, exceeded expected number " + "of directories %d.", type, m_expected_directories); return error; } @@ -135,7 +127,8 @@ Status MinidumpFileBuilder::AddSystemInfo() { Status error; const llvm::Triple &target_triple = m_process_sp->GetTarget().GetArchitecture().GetTriple(); - error = AddDirectory(StreamType::SystemInfo, sizeof(llvm::minidump::SystemInfo)); + error = + AddDirectory(StreamType::SystemInfo, sizeof(llvm::minidump::SystemInfo)); if (error.Fail()) return error; @@ -566,7 +559,7 @@ class ArchThreadContexts { } }; -Status MinidumpFileBuilder::FixThreads() { +Status MinidumpFileBuilder::FixThreadStacks() { Status error; // If we have anything in the heap flush it. FlushBufferToDisk(); @@ -735,7 +728,7 @@ Status MinidumpFileBuilder::AddExceptions() { lldb_private::Status MinidumpFileBuilder::AddMiscInfo() { Status error; error = AddDirectory(StreamType::MiscInfo, - sizeof(lldb_private::minidump::MinidumpMiscInfo)); + sizeof(lldb_private::minidump::MinidumpMiscInfo)); if (error.Fail()) return error; @@ -773,7 +766,8 @@ getFileStreamHelper(const std::string &path) { Status MinidumpFileBuilder::AddLinuxFileStreams() { Status error; // No-op if we are not on linux. - if (m_process_sp->GetTarget().GetArchitecture().GetTriple().getOS() != llvm::Triple::Linux) + if (m_process_sp->GetTarget().GetArchitecture().GetTriple().getOS() != + llvm::Triple::Linux) return error; std::vector> files_with_stream_types = { @@ -802,7 +796,6 @@ Status MinidumpFileBuilder::AddLinuxFileStreams() { {StreamType::LinuxProcFD, "/proc/" + pid_str + "/fd"}); } - for (const auto &entry : files_with_stream_types) { StreamType stream = entry.first; std::string path = entry.second; @@ -832,14 +825,13 @@ Status MinidumpFileBuilder::AddMemoryList(SaveCoreStyle core_style) { if (error.Fail()) return error; - std::set stack_start_addresses; - for (const auto &core_range : ranges_32) - stack_start_addresses.insert(core_range.range.start()); - uint64_t total_size = ranges_32.size() * sizeof(llvm::minidump::MemoryDescriptor); - for (const auto &core_range : ranges_32) + std::unordered_set stack_start_addresses; + for (const auto &core_range : ranges_32) { + stack_start_addresses.insert(core_range.range.start()); total_size += core_range.range.size(); + } if (total_size >= UINT32_MAX) { error.SetErrorStringWithFormat("Unable to write minidump. Stack memory " @@ -856,26 +848,25 @@ Status MinidumpFileBuilder::AddMemoryList(SaveCoreStyle core_style) { return error; } - // We need to calculate the MemoryDescriptor size in the worst case // Where all memory descriptors are 64b. We also leave some additional padding // So that we convert over to 64b with space to spare. This does not waste // space in the dump But instead converts some memory from being in the // memorylist_32 to the memorylist_64. - total_size += 256 + (all_core_memory_ranges.size() - stack_start_addresses.size()) * - sizeof(llvm::minidump::MemoryDescriptor_64); + total_size += + 256 + (all_core_memory_ranges.size() - stack_start_addresses.size()) * + sizeof(llvm::minidump::MemoryDescriptor_64); for (const auto &core_range : all_core_memory_ranges) { addr_t size_to_add = core_range.range.size() + sizeof(llvm::minidump::MemoryDescriptor); - if (stack_start_addresses.count(core_range.range.start()) > 0) + if (stack_start_addresses.count(core_range.range.start()) > 0) // Don't double save stacks. continue; if (total_size + size_to_add < UINT32_MAX) { ranges_32.push_back(core_range); total_size += core_range.range.size(); - total_size += sizeof(llvm::minidump::MemoryDescriptor); } else { ranges_64.push_back(core_range); } @@ -886,13 +877,13 @@ Status MinidumpFileBuilder::AddMemoryList(SaveCoreStyle core_style) { return error; // Add the remaining memory as a 64b range. - if (ranges_64.size() > 0) { + if (!ranges_64.empty()) { error = AddMemoryList_64(ranges_64); if (error.Fail()) return error; } - return FixThreads(); + return FixThreadStacks(); } Status MinidumpFileBuilder::DumpHeader() const { @@ -906,7 +897,7 @@ Status MinidumpFileBuilder::DumpHeader() const { static_cast(m_directories.size()); // We write the directories right after the header. header.StreamDirectoryRVA = - static_cast(header_size); + static_cast(HEADER_SIZE); header.Checksum = static_cast( 0u), // not used in most of the writers header.TimeDateStamp = @@ -918,13 +909,13 @@ Status MinidumpFileBuilder::DumpHeader() const { size_t bytes_written; m_core_file->SeekFromStart(0); - bytes_written = header_size; + bytes_written = HEADER_SIZE; error = m_core_file->Write(&header, bytes_written); - if (error.Fail() || bytes_written != header_size) { - if (bytes_written != header_size) + if (error.Fail() || bytes_written != HEADER_SIZE) { + if (bytes_written != HEADER_SIZE) error.SetErrorStringWithFormat( "Unable to write the minidump header (written %zd/%zd)", - bytes_written, header_size); + bytes_written, HEADER_SIZE); return error; } return error; @@ -937,15 +928,15 @@ size_t MinidumpFileBuilder::GetCurrentDataEndOffset() const { Status MinidumpFileBuilder::DumpDirectories() const { Status error; size_t bytes_written; - m_core_file->SeekFromStart(header_size); + m_core_file->SeekFromStart(HEADER_SIZE); for (const Directory &dir : m_directories) { - bytes_written = directory_size; + bytes_written = DIRECTORY_SIZE; error = m_core_file->Write(&dir, bytes_written); - if (error.Fail() || bytes_written != directory_size) { - if (bytes_written != directory_size) + if (error.Fail() || bytes_written != DIRECTORY_SIZE) { + if (bytes_written != DIRECTORY_SIZE) error.SetErrorStringWithFormat( "unable to write the directory (written %zd/%zd)", bytes_written, - directory_size); + DIRECTORY_SIZE); return error; } } @@ -955,14 +946,13 @@ Status MinidumpFileBuilder::DumpDirectories() const { static size_t GetLargestRange(const Process::CoreFileMemoryRanges &ranges) { size_t max_size = 0; - for (const auto &core_range : ranges) { + for (const auto &core_range : ranges) max_size = std::max(max_size, core_range.range.size()); - } return max_size; } -Status MinidumpFileBuilder::AddMemoryList_32( - Process::CoreFileMemoryRanges &ranges) { +Status +MinidumpFileBuilder::AddMemoryList_32(Process::CoreFileMemoryRanges &ranges) { std::vector descriptors; Status error; if (ranges.size() == 0) @@ -1017,9 +1007,9 @@ Status MinidumpFileBuilder::AddMemoryList_32( // With a size of the number of ranges as a 32 bit num // And then the size of all the ranges error = AddDirectory(StreamType::MemoryList, - sizeof(llvm::support::ulittle32_t) + - descriptors.size() * - sizeof(llvm::minidump::MemoryDescriptor)); + sizeof(llvm::support::ulittle32_t) + + descriptors.size() * + sizeof(llvm::minidump::MemoryDescriptor)); if (error.Fail()) return error; @@ -1034,15 +1024,16 @@ Status MinidumpFileBuilder::AddMemoryList_32( return error; } -Status MinidumpFileBuilder::AddMemoryList_64( - Process::CoreFileMemoryRanges &ranges) { +Status +MinidumpFileBuilder::AddMemoryList_64(Process::CoreFileMemoryRanges &ranges) { Status error; - if (ranges.empty()) + if (ranges.empty()) return error; error = AddDirectory(StreamType::Memory64List, - (sizeof(llvm::support::ulittle64_t) * 2) + - ranges.size() * sizeof(llvm::minidump::MemoryDescriptor_64)); + (sizeof(llvm::support::ulittle64_t) * 2) + + ranges.size() * + sizeof(llvm::minidump::MemoryDescriptor_64)); if (error.Fail()) return error; @@ -1051,10 +1042,13 @@ Status MinidumpFileBuilder::AddMemoryList_64( m_data.AppendData(&memory_ranges_num, sizeof(llvm::support::ulittle64_t)); // Capture the starting offset for all the descriptors so we can clean them up // if needed. - offset_t starting_offset = GetCurrentDataEndOffset() + sizeof(llvm::support::ulittle64_t); + offset_t starting_offset = + GetCurrentDataEndOffset() + sizeof(llvm::support::ulittle64_t); // The base_rva needs to start after the directories, which is right after // this 8 byte variable. - offset_t base_rva = starting_offset + (ranges.size() * sizeof(llvm::minidump::MemoryDescriptor_64)); + offset_t base_rva = + starting_offset + + (ranges.size() * sizeof(llvm::minidump::MemoryDescriptor_64)); llvm::support::ulittle64_t memory_ranges_base_rva = static_cast(base_rva); m_data.AppendData(&memory_ranges_base_rva, @@ -1076,7 +1070,6 @@ Status MinidumpFileBuilder::AddMemoryList_64( m_data.AppendData(&memory_desc, sizeof(MemoryDescriptor_64)); } - Log *log = GetLog(LLDBLog::Object); size_t region_index = 0; auto data_up = std::make_unique(GetLargestRange(ranges), 0); @@ -1142,7 +1135,7 @@ Status MinidumpFileBuilder::AddData(const void *data, addr_t size) { // here is to limit the number of bytes we need to host in memory at any given // time. m_data.AppendData(data, size); - if (m_data.GetByteSize() > m_write_chunk_max) { + if (m_data.GetByteSize() > MAX_WRITE_CHUNK_SIZE) { return FlushBufferToDisk(); } @@ -1152,7 +1145,7 @@ Status MinidumpFileBuilder::AddData(const void *data, addr_t size) { Status MinidumpFileBuilder::FlushBufferToDisk() { Status error; // Set the stream to it's end. - m_core_file->SeekFromEnd(0); + m_core_file->SeekFromStart(m_saved_data_size); addr_t starting_size = m_data.GetByteSize(); addr_t remaining_bytes = starting_size; offset_t offset = 0; diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h index 31a587813b43e..f2fdfacf9045d 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h +++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h @@ -44,14 +44,39 @@ lldb_private::Status WriteString(const std::string &to_write, /// Minidump writer for Linux /// /// This class provides a Minidump writer that is able to -/// snapshot the current process state. For the whole time, it stores all -/// the data on heap. +/// snapshot the current process state. +/// +/// Minidumps are a Microsoft format for dumping process state. +/// This class constructs the minidump on disk starting with +/// Headers and Directories are written at the top of the file, +/// with the amount of bytes being precalculates before any writing takes place +/// Then the smaller data sections are written +/// SystemInfo, ModuleList, Misc Info. +/// Then Threads are emitted, threads are the first section that needs to be +/// 'fixed up' this happens when later we emit the memory stream, we identify if +/// that stream is the expected stack, and if so we update the stack with the +/// current RVA. Lastly the Memory lists are added. For Memory List, this will +/// contain everything that can fit within 4.2gb. MemoryList has it's +/// descriptors written at the end so it cannot be allowed to overflow. +/// +/// Memory64List is a special case where it has to be begin before 4.2gb but can +/// expand forever The difference in Memory64List is there are no RVA's and all +/// the addresses are figured out by starting at the base RVA, and adding the +/// antecedent memory sections. +/// +/// Because Memory64List can be arbitrarily large, this class has to write +/// chunks to disk this means we have to precalculate the descriptors and write +/// them first, and if we encounter any error, or are unable to read the same +/// number of bytes we have to go back and update them on disk. +/// +/// And as the last step, after all the directories have been added, we go back +/// to the top of the file to fill in the header and the redirectory sections +/// that we preallocated. class MinidumpFileBuilder { public: MinidumpFileBuilder(lldb::FileUP &&core_file, const lldb::ProcessSP &process_sp) - : m_process_sp(process_sp), - m_core_file(std::move(core_file)) {}; + : m_process_sp(process_sp), m_core_file(std::move(core_file)){}; MinidumpFileBuilder(const MinidumpFileBuilder &) = delete; MinidumpFileBuilder &operator=(const MinidumpFileBuilder &) = delete; @@ -61,6 +86,9 @@ class MinidumpFileBuilder { ~MinidumpFileBuilder() = default; + // This method only calculates the amount of bytes the header and directories + // will take up. It does not write the directories or headers. This function + // must be called with a followup to fill in the data. lldb_private::Status AddHeaderAndCalculateDirectories(); // Add SystemInfo stream, used for storing the most basic information // about the system, platform etc... @@ -93,22 +121,26 @@ class MinidumpFileBuilder { AddMemoryList_64(lldb_private::Process::CoreFileMemoryRanges &ranges); lldb_private::Status AddMemoryList_32(lldb_private::Process::CoreFileMemoryRanges &ranges); - lldb_private::Status FixThreads(); + // Update the thread list on disk with the newly emitted stack RVAs. + lldb_private::Status FixThreadStacks(); lldb_private::Status FlushBufferToDisk(); lldb_private::Status DumpHeader() const; lldb_private::Status DumpDirectories() const; // Add directory of StreamType pointing to the current end of the prepared // file with the specified size. - lldb_private::Status AddDirectory(llvm::minidump::StreamType type, uint64_t stream_size); + lldb_private::Status AddDirectory(llvm::minidump::StreamType type, + uint64_t stream_size); lldb::offset_t GetCurrentDataEndOffset() const; // Stores directories to fill in later std::vector m_directories; // When we write off the threads for the first time, we need to clean them up // and give them the correct RVA once we write the stack memory list. // We save by the end because we only take from the stack pointer up - // So the saved off range base can differ from the memory region the stack pointer is in. - std::unordered_map m_thread_by_range_end; + // So the saved off range base can differ from the memory region the stack + // pointer is in. + std::unordered_map + m_thread_by_range_end; // Main data buffer consisting of data without the minidump header and // directories lldb_private::DataBufferHeap m_data; @@ -121,15 +153,16 @@ class MinidumpFileBuilder { // but we want to try to keep the size of m_data small // and we will only exceed a 128 mb buffer if we get a memory region // that is larger than 128 mb. - static constexpr size_t m_write_chunk_max = (1024 * 1024 * 128); + static constexpr size_t MAX_WRITE_CHUNK_SIZE = (1024 * 1024 * 128); - static constexpr size_t header_size = sizeof(llvm::minidump::Header); - static constexpr size_t directory_size = sizeof(llvm::minidump::Directory); + static constexpr size_t HEADER_SIZE = sizeof(llvm::minidump::Header); + static constexpr size_t DIRECTORY_SIZE = sizeof(llvm::minidump::Directory); // More that one place can mention the register thread context locations, // so when we emit the thread contents, remember where it is so we don't have // to duplicate it in the exception data. - std::map m_tid_to_reg_ctx; + std::unordered_map + m_tid_to_reg_ctx; lldb::FileUP m_core_file; }; diff --git a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp index 5174d73e4f579..3668c37c5191d 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp +++ b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp @@ -78,7 +78,8 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp, Log *log = GetLog(LLDBLog::Object); error = builder.AddHeaderAndCalculateDirectories(); if (error.Fail()) { - LLDB_LOGF(log, "AddHeaderAndCalculateDirectories failed: %s", error.AsCString()); + LLDB_LOGF(log, "AddHeaderAndCalculateDirectories failed: %s", + error.AsCString()); return false; }; error = builder.AddSystemInfo(); @@ -103,7 +104,7 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp, LLDB_LOGF(log, "AddThreadList failed: %s", error.AsCString()); return false; } - + error = builder.AddLinuxFileStreams(); if (error.Fail()) { LLDB_LOGF(log, "AddLinuxFileStreams failed: %s", error.AsCString()); From 95a0a6c5f9652f6c536dd1d557813542079b462e Mon Sep 17 00:00:00 2001 From: Jacob Lalonde Date: Mon, 17 Jun 2024 18:05:16 -0700 Subject: [PATCH 12/16] Code review feedback on size_t, fix printf for uint64. Cast type to uint32_t to avoid warnings. Run git clang format. --- .../Minidump/MinidumpFileBuilder.cpp | 59 +++++++++++-------- 1 file changed, 33 insertions(+), 26 deletions(-) diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp index 9c7b23cc0926f..cd486f7947c3d 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp +++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp @@ -56,7 +56,7 @@ using namespace llvm::minidump; Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories() { // First set the offset on the file, and on the bytes saved - m_saved_data_size += HEADER_SIZE; + m_saved_data_size = HEADER_SIZE; // We know we will have at least Misc, SystemInfo, Modules, and ThreadList // (corresponding memory list for stacks) And an additional memory list for // non-stacks. @@ -86,7 +86,8 @@ Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories() { offset_t new_offset = m_core_file->SeekFromStart(m_saved_data_size); if (new_offset != m_saved_data_size) error.SetErrorStringWithFormat("Failed to fill in header and directory " - "sections. Written / Expected (%zu / %zu)", + "sections. Written / Expected (%" PRIx64 + " / %zu)", new_offset, m_saved_data_size); return error; @@ -94,11 +95,12 @@ Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories() { Status MinidumpFileBuilder::AddDirectory(StreamType type, uint64_t stream_size) { + // We explicitly cast type, an 32b enum, to uint32_t to avoid warnings. Status error; if (GetCurrentDataEndOffset() > UINT32_MAX) { error.SetErrorStringWithFormat("Unable to add directory for stream type " "%x, offset is greater then 32 bit limit.", - type); + (uint32_t)type); return error; } @@ -106,7 +108,7 @@ Status MinidumpFileBuilder::AddDirectory(StreamType type, error.SetErrorStringWithFormat( "Unable to add directory for stream type %x, exceeded expected number " "of directories %d.", - type, m_expected_directories); + (uint32_t)type, m_expected_directories); return error; } @@ -818,6 +820,10 @@ Status MinidumpFileBuilder::AddLinuxFileStreams() { Status MinidumpFileBuilder::AddMemoryList(SaveCoreStyle core_style) { Status error; + // We first save the thread stacks to ensure they fit in the first UINT32_MAX + // bytes of the core file. Thread structures in minidump files can only use + // 32 bit memory descriptiors, so we emit them first to ensure the memory is + // in accessible with a 32 bit offset. Process::CoreFileMemoryRanges ranges_32; Process::CoreFileMemoryRanges ranges_64; error = m_process_sp->CalculateCoreFileSaveRanges( @@ -848,23 +854,21 @@ Status MinidumpFileBuilder::AddMemoryList(SaveCoreStyle core_style) { return error; } - // We need to calculate the MemoryDescriptor size in the worst case - // Where all memory descriptors are 64b. We also leave some additional padding - // So that we convert over to 64b with space to spare. This does not waste - // space in the dump But instead converts some memory from being in the - // memorylist_32 to the memorylist_64. + // After saving the stacks, we start packing as much as we can into 32b. + // We apply a generous padding here so that the Directory, MemoryList and + // Memory64List sections all begin in 32b addressable space. + // Then anything overflow extends into 64b addressable space. total_size += 256 + (all_core_memory_ranges.size() - stack_start_addresses.size()) * sizeof(llvm::minidump::MemoryDescriptor_64); for (const auto &core_range : all_core_memory_ranges) { - addr_t size_to_add = - core_range.range.size() + sizeof(llvm::minidump::MemoryDescriptor); + addr_t range_size = core_range.range.size(); if (stack_start_addresses.count(core_range.range.start()) > 0) // Don't double save stacks. continue; - if (total_size + size_to_add < UINT32_MAX) { + if (total_size + range_size < UINT32_MAX) { ranges_32.push_back(core_range); total_size += core_range.range.size(); } else { @@ -944,8 +948,9 @@ Status MinidumpFileBuilder::DumpDirectories() const { return error; } -static size_t GetLargestRange(const Process::CoreFileMemoryRanges &ranges) { - size_t max_size = 0; +static uint64_t +GetLargestRangeSize(const Process::CoreFileMemoryRanges &ranges) { + uint64_t max_size = 0; for (const auto &core_range : ranges) max_size = std::max(max_size, core_range.range.size()); return max_size; @@ -960,17 +965,18 @@ MinidumpFileBuilder::AddMemoryList_32(Process::CoreFileMemoryRanges &ranges) { Log *log = GetLog(LLDBLog::Object); size_t region_index = 0; - auto data_up = std::make_unique(GetLargestRange(ranges), 0); + auto data_up = + std::make_unique(GetLargestRangeSize(ranges), 0); for (const auto &core_range : ranges) { // Take the offset before we write. - const size_t offset_for_data = GetCurrentDataEndOffset(); + const offset_t offset_for_data = GetCurrentDataEndOffset(); const addr_t addr = core_range.range.start(); const addr_t size = core_range.range.size(); const addr_t end = core_range.range.end(); LLDB_LOGF(log, "AddMemoryList %zu/%zu reading memory for region " - "(%zu bytes) [%zx, %zx)", + "(%" PRIx64 " bytes) [%" PRIx64 ", %" PRIx64 ")", region_index, ranges.size(), size, addr, addr + size); ++region_index; @@ -982,8 +988,9 @@ MinidumpFileBuilder::AddMemoryList_32(Process::CoreFileMemoryRanges &ranges) { // Just skip sections with errors or zero bytes in 32b mode continue; } else if (bytes_read != size) { - LLDB_LOGF(log, "Memory region at: %zu failed to read %zu bytes", addr, - size); + LLDB_LOGF( + log, "Memory region at: %" PRIx64 " failed to read %" PRIx64 " bytes", + addr, size); } MemoryDescriptor descriptor; @@ -1072,15 +1079,16 @@ MinidumpFileBuilder::AddMemoryList_64(Process::CoreFileMemoryRanges &ranges) { Log *log = GetLog(LLDBLog::Object); size_t region_index = 0; - auto data_up = std::make_unique(GetLargestRange(ranges), 0); + auto data_up = + std::make_unique(GetLargestRangeSize(ranges), 0); for (const auto &core_range : ranges) { const addr_t addr = core_range.range.start(); const addr_t size = core_range.range.size(); LLDB_LOGF(log, "AddMemoryList_64 %zu/%zu reading memory for region " - "(%zu bytes) " - "[%zx, %zx)", + "(%" PRIx64 "bytes) " + "[%" PRIx64 ", %" PRIx64 ")", region_index, ranges.size(), size, addr, addr + size); ++region_index; @@ -1094,8 +1102,8 @@ MinidumpFileBuilder::AddMemoryList_64(Process::CoreFileMemoryRanges &ranges) { descriptors[region_index].DataSize = 0; } if (bytes_read != size) { - LLDB_LOGF(log, "Memory region at: %zu failed to read %zu bytes", addr, - size); + LLDB_LOGF(log, "Memory region at: %" PRIx64 " failed to read %zu bytes", + addr, size); cleanup_required = true; descriptors[region_index].DataSize = bytes_read; } @@ -1135,9 +1143,8 @@ Status MinidumpFileBuilder::AddData(const void *data, addr_t size) { // here is to limit the number of bytes we need to host in memory at any given // time. m_data.AppendData(data, size); - if (m_data.GetByteSize() > MAX_WRITE_CHUNK_SIZE) { + if (m_data.GetByteSize() > MAX_WRITE_CHUNK_SIZE) return FlushBufferToDisk(); - } return Status(); } From 487c7fc5cd793af7afe0c1cc9bd44f0e5ada165b Mon Sep 17 00:00:00 2001 From: Jacob Lalonde Date: Tue, 18 Jun 2024 09:54:55 -0700 Subject: [PATCH 13/16] Extra cleanup that Greg pointed out, ensure full use of variable in loop and make it const. Fix m_total_siz_size formatting. Run git-clang-format --- .../ObjectFile/Minidump/MinidumpFileBuilder.cpp | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp index cd486f7947c3d..e6018bc08cfb4 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp +++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp @@ -87,7 +87,7 @@ Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories() { if (new_offset != m_saved_data_size) error.SetErrorStringWithFormat("Failed to fill in header and directory " "sections. Written / Expected (%" PRIx64 - " / %zu)", + " / %" PRIx64 ")", new_offset, m_saved_data_size); return error; @@ -858,19 +858,22 @@ Status MinidumpFileBuilder::AddMemoryList(SaveCoreStyle core_style) { // We apply a generous padding here so that the Directory, MemoryList and // Memory64List sections all begin in 32b addressable space. // Then anything overflow extends into 64b addressable space. - total_size += - 256 + (all_core_memory_ranges.size() - stack_start_addresses.size()) * - sizeof(llvm::minidump::MemoryDescriptor_64); + // All core memeroy ranges will either container nothing on stacks only + // or all the memory ranges including stacks + if (!all_core_memory_ranges.empty()) + total_size += + 256 + (all_core_memory_ranges.size() - stack_start_addresses.size()) * + sizeof(llvm::minidump::MemoryDescriptor_64); for (const auto &core_range : all_core_memory_ranges) { - addr_t range_size = core_range.range.size(); + const addr_t range_size = core_range.range.size(); if (stack_start_addresses.count(core_range.range.start()) > 0) // Don't double save stacks. continue; if (total_size + range_size < UINT32_MAX) { ranges_32.push_back(core_range); - total_size += core_range.range.size(); + total_size += range_size; } else { ranges_64.push_back(core_range); } From 86b0be17fbf04bf01201d5fb561bc77281ddef9c Mon Sep 17 00:00:00 2001 From: Jacob Lalonde Date: Thu, 20 Jun 2024 17:16:55 -0700 Subject: [PATCH 14/16] Start total_size at current offset, make exception directory entries for stop signals. Run git clang format --- .../ObjectFile/Minidump/MinidumpFileBuilder.cpp | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp index e6018bc08cfb4..39d428a1e5396 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp +++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp @@ -74,9 +74,11 @@ Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories() { for (uint32_t thread_idx = 0; thread_idx < num_threads; ++thread_idx) { ThreadSP thread_sp(thread_list.GetThreadAtIndex(thread_idx)); StopInfoSP stop_info_sp = thread_sp->GetStopInfo(); - if (stop_info_sp && - stop_info_sp->GetStopReason() == StopReason::eStopReasonException) { - m_expected_directories++; + if (stop_info_sp) { + const StopReason &stop_reason = stop_info_sp->GetStopReason(); + if (stop_reason == StopReason::eStopReasonException || + stop_reason == StopReason::eStopReasonSignal) + m_expected_directories++; } } @@ -831,8 +833,9 @@ Status MinidumpFileBuilder::AddMemoryList(SaveCoreStyle core_style) { if (error.Fail()) return error; - uint64_t total_size = - ranges_32.size() * sizeof(llvm::minidump::MemoryDescriptor); + // Calculate totalsize including the current offset. + uint64_t total_size = GetCurrentDataEndOffset(); + total_size += ranges_32.size() * sizeof(llvm::minidump::MemoryDescriptor); std::unordered_set stack_start_addresses; for (const auto &core_range : ranges_32) { stack_start_addresses.insert(core_range.range.start()); From 3354468824b15534d10557463569f56b611c0697 Mon Sep 17 00:00:00 2001 From: Jacob Lalonde Date: Thu, 20 Jun 2024 17:46:45 -0700 Subject: [PATCH 15/16] Resolve out of line warnings from MacOS + Warnings on addr_t --- .../Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp | 9 +++++---- .../Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp index 39d428a1e5396..48dd03766b3a7 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp +++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp @@ -667,7 +667,7 @@ Status MinidumpFileBuilder::AddThreadList() { m_data.AppendData(&t, sizeof(llvm::minidump::Thread)); } - LLDB_LOGF(log, "AddThreadList(): total helper_data %zu bytes", + LLDB_LOGF(log, "AddThreadList(): total helper_data %" PRIx64 " bytes", helper_data.GetByteSize()); m_data.AppendData(helper_data.GetBytes(), helper_data.GetByteSize()); return Status(); @@ -931,7 +931,7 @@ Status MinidumpFileBuilder::DumpHeader() const { return error; } -size_t MinidumpFileBuilder::GetCurrentDataEndOffset() const { +offset_t MinidumpFileBuilder::GetCurrentDataEndOffset() const { return m_data.GetByteSize() + m_saved_data_size; } @@ -1141,7 +1141,7 @@ MinidumpFileBuilder::AddMemoryList_64(Process::CoreFileMemoryRanges &ranges) { } } -Status MinidumpFileBuilder::AddData(const void *data, addr_t size) { +Status MinidumpFileBuilder::AddData(const void *data, uint64_t size) { // This should also get chunked, because worst case we copy over a big // object / memory range, say 5gb. In that case, we'd have to allocate 10gb // 5 gb for the buffer we're copying from, and then 5gb for the buffer we're @@ -1170,7 +1170,8 @@ Status MinidumpFileBuilder::FlushBufferToDisk() { error = m_core_file->Write(m_data.GetBytes() + offset, bytes_written); if (error.Fail()) { error.SetErrorStringWithFormat( - "Wrote incorrect number of bytes to minidump file. (written %zd/%zd)", + "Wrote incorrect number of bytes to minidump file. (written %" PRIx64 + "/%" PRIx64 ")", starting_size - remaining_bytes, starting_size); return error; } diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h index f2fdfacf9045d..b606f925f9912 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h +++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h @@ -115,7 +115,7 @@ class MinidumpFileBuilder { private: // Add data to the end of the buffer, if the buffer exceeds the flush level, // trigger a flush. - lldb_private::Status AddData(const void *data, size_t size); + lldb_private::Status AddData(const void *data, uint64_t size); // Add MemoryList stream, containing dumps of important memory segments lldb_private::Status AddMemoryList_64(lldb_private::Process::CoreFileMemoryRanges &ranges); From a590d3dcd76046085408ba6bd6bcb93921030cdb Mon Sep 17 00:00:00 2001 From: Jacob Lalonde Date: Fri, 21 Jun 2024 11:44:53 -0700 Subject: [PATCH 16/16] Last little formatting cleanup from MacOS warning --- .../Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp index 48dd03766b3a7..7a09c6104d08c 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp +++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp @@ -1108,8 +1108,9 @@ MinidumpFileBuilder::AddMemoryList_64(Process::CoreFileMemoryRanges &ranges) { descriptors[region_index].DataSize = 0; } if (bytes_read != size) { - LLDB_LOGF(log, "Memory region at: %" PRIx64 " failed to read %zu bytes", - addr, size); + LLDB_LOGF( + log, "Memory region at: %" PRIx64 " failed to read %" PRIx64 " bytes", + addr, size); cleanup_required = true; descriptors[region_index].DataSize = bytes_read; }