[LLDB][SBSaveCore] Add selectable memory regions to SBSaveCore #105442
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…nge the interal api to void
…ive to save core by default. Create a custom save core enum to allow a 'user-only' core dump. Add a check on the save core options for threads so we can skip thread minification if it's custom and there is only memory ranges. Needed to extend RangeVector for my use case, and fix where we have SaveCoreOptions in the lldb private headers
|
@llvm/pr-subscribers-lldb Author: Jacob Lalonde (Jlalond) ChangesThis patch adds the option to specify specific memory ranges to be included in a given core file. The current implementation lets user specified ranges either be in addition to a certain save style, or independent of them via the newly added custom enum. To achieve being inclusive of save style, I've moved from a std::vector of ranges to a RangeDataVector, and to join overlapping ranges to prevent duplication of memory ranges in the core file. As a non function bonus, when SBSavecore was initially created, the header was included in the lldb-private interfaces, and I've fixed that and moved it the forward declare as an oversight. CC @bulbazord in case we need to include that into swift. Patch is 26.09 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/105442.diff 21 Files Affected:
diff --git a/lldb/include/lldb/API/SBMemoryRegionInfo.h b/lldb/include/lldb/API/SBMemoryRegionInfo.h
index be55de4ead1fa8..f9a5dc993d7cb6 100644
--- a/lldb/include/lldb/API/SBMemoryRegionInfo.h
+++ b/lldb/include/lldb/API/SBMemoryRegionInfo.h
@@ -120,7 +120,7 @@ class LLDB_API SBMemoryRegionInfo {
private:
friend class SBProcess;
friend class SBMemoryRegionInfoList;
-
+ friend class SBSaveCoreOptions;
friend class lldb_private::ScriptInterpreter;
lldb_private::MemoryRegionInfo &ref();
diff --git a/lldb/include/lldb/API/SBMemoryRegionInfoList.h b/lldb/include/lldb/API/SBMemoryRegionInfoList.h
index 1d939dff55faa3..7207171bf6d647 100644
--- a/lldb/include/lldb/API/SBMemoryRegionInfoList.h
+++ b/lldb/include/lldb/API/SBMemoryRegionInfoList.h
@@ -45,7 +45,6 @@ class LLDB_API SBMemoryRegionInfoList {
private:
friend class SBProcess;
-
lldb_private::MemoryRegionInfos &ref();
const lldb_private::MemoryRegionInfos &ref() const;
diff --git a/lldb/include/lldb/API/SBSaveCoreOptions.h b/lldb/include/lldb/API/SBSaveCoreOptions.h
index ba48ba5eaea5a0..d14ffe023188a2 100644
--- a/lldb/include/lldb/API/SBSaveCoreOptions.h
+++ b/lldb/include/lldb/API/SBSaveCoreOptions.h
@@ -80,6 +80,17 @@ class LLDB_API SBSaveCoreOptions {
/// \return True if the thread was removed, false if it was not in the list.
bool RemoveThread(lldb::SBThread thread);
+ /// Add a memory region to save in the core file.
+ ///
+ /// \param region The memory region to save.
+ /// \returns An empty SBStatus upon success, or an error if the region is
+ /// invalid.
+ /// \note Ranges that overlapped with be unioned into a single region this
+ /// also supercedes stack minification. Specifying full regions and a
+ /// non-custom core style will include the specified regions and union them
+ /// with all style specific regions.
+ SBError AddMemoryRegionToSave(const SBMemoryRegionInfo ®ion);
+
/// Reset all options.
void Clear();
diff --git a/lldb/include/lldb/Symbol/SaveCoreOptions.h b/lldb/include/lldb/Symbol/SaveCoreOptions.h
index f4fed4676fa4ae..a95686ada96a79 100644
--- a/lldb/include/lldb/Symbol/SaveCoreOptions.h
+++ b/lldb/include/lldb/Symbol/SaveCoreOptions.h
@@ -10,12 +10,15 @@
#define LLDB_SOURCE_PLUGINS_OBJECTFILE_SaveCoreOPTIONS_H
#include "lldb/Utility/FileSpec.h"
-#include "lldb/lldb-forward.h"
-#include "lldb/lldb-types.h"
+#include "lldb/Utility/RangeMap.h"
+
#include <optional>
#include <string>
#include <unordered_set>
+#include <set>
+
+using MemoryRanges = lldb_private::RangeVector<lldb::addr_t, lldb::addr_t>;
namespace lldb_private {
@@ -38,8 +41,12 @@ class SaveCoreOptions {
Status AddThread(lldb::ThreadSP thread_sp);
bool RemoveThread(lldb::ThreadSP thread_sp);
bool ShouldThreadBeSaved(lldb::tid_t tid) const;
+ bool HasSpecifiedThreads() const;
Status EnsureValidConfiguration(lldb::ProcessSP process_sp) const;
+ const MemoryRanges& GetCoreFileMemoryRanges() const;
+
+ void AddMemoryRegionToSave(const lldb_private::MemoryRegionInfo ®ion);
void Clear();
@@ -51,6 +58,7 @@ class SaveCoreOptions {
std::optional<lldb::SaveCoreStyle> m_style;
lldb::ProcessSP m_process_sp;
std::unordered_set<lldb::tid_t> m_threads_to_save;
+ MemoryRanges m_regions_to_save;
};
} // namespace lldb_private
diff --git a/lldb/include/lldb/Target/Process.h b/lldb/include/lldb/Target/Process.h
index 9cf6f50558570b..73228f13dc6715 100644
--- a/lldb/include/lldb/Target/Process.h
+++ b/lldb/include/lldb/Target/Process.h
@@ -43,6 +43,7 @@
#include "lldb/Target/ThreadList.h"
#include "lldb/Target/ThreadPlanStack.h"
#include "lldb/Target/Trace.h"
+#include "lldb/Symbol/SaveCoreOptions.h"
#include "lldb/Utility/AddressableBits.h"
#include "lldb/Utility/ArchSpec.h"
#include "lldb/Utility/Broadcaster.h"
@@ -733,7 +734,7 @@ class Process : public std::enable_shared_from_this<Process>,
}
};
- using CoreFileMemoryRanges = std::vector<CoreFileMemoryRange>;
+ using CoreFileMemoryRanges = lldb_private::RangeDataVector<lldb::addr_t, lldb::addr_t, CoreFileMemoryRange>;
/// Helper function for Process::SaveCore(...) that calculates the address
/// ranges that should be saved. This allows all core file plug-ins to save
diff --git a/lldb/include/lldb/Utility/RangeMap.h b/lldb/include/lldb/Utility/RangeMap.h
index 8cc382bcc046ce..dacc638c193a10 100644
--- a/lldb/include/lldb/Utility/RangeMap.h
+++ b/lldb/include/lldb/Utility/RangeMap.h
@@ -449,6 +449,10 @@ class RangeDataVector {
~RangeDataVector() = default;
void Append(const Entry &entry) { m_entries.emplace_back(entry); }
+
+ void Append(const B&& b, const S&& s, const T&& t) {
+ m_entries.emplace_back(Entry(b, s, t));
+ }
bool Erase(uint32_t start, uint32_t end) {
if (start >= end || end > m_entries.size())
diff --git a/lldb/include/lldb/lldb-enumerations.h b/lldb/include/lldb/lldb-enumerations.h
index 7bfde8b9de1271..768045c72280a0 100644
--- a/lldb/include/lldb/lldb-enumerations.h
+++ b/lldb/include/lldb/lldb-enumerations.h
@@ -1222,6 +1222,7 @@ enum SaveCoreStyle {
eSaveCoreFull = 1,
eSaveCoreDirtyOnly = 2,
eSaveCoreStackOnly = 3,
+ eSaveCoreCustom = 4,
};
/// Events that might happen during a trace session.
diff --git a/lldb/include/lldb/lldb-forward.h b/lldb/include/lldb/lldb-forward.h
index 337eff696fcf3f..5fb288ad43af48 100644
--- a/lldb/include/lldb/lldb-forward.h
+++ b/lldb/include/lldb/lldb-forward.h
@@ -207,6 +207,7 @@ class StackFrameRecognizer;
class StackFrameRecognizerManager;
class StackID;
class Status;
+class SaveCoreOptions;
class StopInfo;
class Stoppoint;
class StoppointCallbackContext;
diff --git a/lldb/include/lldb/lldb-private-interfaces.h b/lldb/include/lldb/lldb-private-interfaces.h
index b3c8cda899b95e..5bac5cd3e86b59 100644
--- a/lldb/include/lldb/lldb-private-interfaces.h
+++ b/lldb/include/lldb/lldb-private-interfaces.h
@@ -9,7 +9,6 @@
#ifndef LLDB_LLDB_PRIVATE_INTERFACES_H
#define LLDB_LLDB_PRIVATE_INTERFACES_H
-#include "lldb/Symbol/SaveCoreOptions.h"
#include "lldb/lldb-enumerations.h"
#include "lldb/lldb-forward.h"
#include "lldb/lldb-private-enumerations.h"
diff --git a/lldb/source/API/SBSaveCoreOptions.cpp b/lldb/source/API/SBSaveCoreOptions.cpp
index 2cd431611ef558..03d237d3499727 100644
--- a/lldb/source/API/SBSaveCoreOptions.cpp
+++ b/lldb/source/API/SBSaveCoreOptions.cpp
@@ -7,6 +7,9 @@
//===----------------------------------------------------------------------===//
#include "lldb/API/SBSaveCoreOptions.h"
+#include "lldb/API/SBError.h"
+#include "lldb/API/SBFileSpec.h"
+#include "lldb/API/SBMemoryRegionInfo.h"
#include "lldb/Host/FileSystem.h"
#include "lldb/Symbol/SaveCoreOptions.h"
#include "lldb/Utility/Instrumentation.h"
@@ -90,6 +93,16 @@ bool SBSaveCoreOptions::RemoveThread(lldb::SBThread thread) {
return m_opaque_up->RemoveThread(thread.GetSP());
}
+
+lldb::SBError SBSaveCoreOptions::AddMemoryRegionToSave(const SBMemoryRegionInfo ®ion) {
+ LLDB_INSTRUMENT_VA(this, region);
+ // Currently add memory region can't fail, so we always return a success
+ // SBerror, but because these API's live forever, this is the most future
+ // proof thing to do.
+ m_opaque_up->AddMemoryRegionToSave(region.ref());
+ return SBError();
+}
+
void SBSaveCoreOptions::Clear() {
LLDB_INSTRUMENT_VA(this);
m_opaque_up->Clear();
diff --git a/lldb/source/Commands/CommandObjectProcess.cpp b/lldb/source/Commands/CommandObjectProcess.cpp
index 28413b7a8591f6..398eb574f7d7c9 100644
--- a/lldb/source/Commands/CommandObjectProcess.cpp
+++ b/lldb/source/Commands/CommandObjectProcess.cpp
@@ -31,6 +31,7 @@
#include "lldb/Target/Target.h"
#include "lldb/Target/Thread.h"
#include "lldb/Target/UnixSignals.h"
+#include "lldb/Symbol/SaveCoreOptions.h"
#include "lldb/Utility/Args.h"
#include "lldb/Utility/ScriptedMetadata.h"
#include "lldb/Utility/State.h"
diff --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
index 22ece4f4dacf79..f9e1fa1b3b5fda 100644
--- a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -6567,7 +6567,9 @@ bool ObjectFileMachO::SaveCore(const lldb::ProcessSP &process_sp,
const uint32_t addr_byte_size = target_arch.GetAddressByteSize();
const ByteOrder byte_order = target_arch.GetByteOrder();
std::vector<llvm::MachO::segment_command_64> segment_load_commands;
- for (const auto &core_range : core_ranges) {
+ for (const auto &core_range_info : core_ranges) {
+ // TODO: Refactor RangeDataVector to have a data iterator.
+ const auto &core_range = core_range_info.data;
uint32_t cmd_type = LC_SEGMENT_64;
uint32_t segment_size = sizeof(llvm::MachO::segment_command_64);
if (addr_byte_size == 4) {
diff --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
index 27bc237aaac48d..be87112df7d898 100644
--- a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
+++ b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
@@ -12,6 +12,7 @@
#include "lldb/Core/Address.h"
#include "lldb/Host/SafeMachO.h"
#include "lldb/Symbol/ObjectFile.h"
+#include "lldb/Symbol/SaveCoreOptions.h"
#include "lldb/Utility/FileSpec.h"
#include "lldb/Utility/FileSpecList.h"
#include "lldb/Utility/RangeMap.h"
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
index c0cc3af638a777..fb323b78735499 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
@@ -823,25 +823,31 @@ Status MinidumpFileBuilder::AddMemoryList() {
// 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;
+ std::vector<Process::CoreFileMemoryRange> ranges_32;
+ std::vector<Process::CoreFileMemoryRange> ranges_64;
Process::CoreFileMemoryRanges all_core_memory_ranges;
error = m_process_sp->CalculateCoreFileSaveRanges(m_save_core_options,
all_core_memory_ranges);
+
+ std::vector<Process::CoreFileMemoryRange> all_core_memory_vec;
+ // Extract all the data into just a vector of data. So we can mutate this in place.
+ for (const auto &core_range : all_core_memory_ranges)
+ all_core_memory_vec.push_back(core_range.data);
+
if (error.Fail())
return error;
// Start by saving all of the stacks and ensuring they fit under the 32b
// limit.
uint64_t total_size = GetCurrentDataEndOffset();
- auto iterator = all_core_memory_ranges.begin();
- while (iterator != all_core_memory_ranges.end()) {
+ auto iterator = all_core_memory_vec.begin();
+ while (iterator != all_core_memory_vec.end()) {
if (m_saved_stack_ranges.count(iterator->range.start()) > 0) {
// We don't save stacks twice.
ranges_32.push_back(*iterator);
total_size +=
iterator->range.size() + sizeof(llvm::minidump::MemoryDescriptor);
- iterator = all_core_memory_ranges.erase(iterator);
+ iterator = all_core_memory_vec.erase(iterator);
} else {
iterator++;
}
@@ -860,11 +866,11 @@ Status MinidumpFileBuilder::AddMemoryList() {
// Then anything overflow extends into 64b addressable space.
// 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() *
+ if (!all_core_memory_vec.empty())
+ total_size += 256 + (all_core_memory_vec.size() *
sizeof(llvm::minidump::MemoryDescriptor_64));
- for (const auto &core_range : all_core_memory_ranges) {
+ for (const auto &core_range : all_core_memory_vec) {
const addr_t range_size = core_range.range.size();
// We don't need to check for stacks here because we already removed them
// from all_core_memory_ranges.
@@ -949,7 +955,7 @@ Status MinidumpFileBuilder::DumpDirectories() const {
}
static uint64_t
-GetLargestRangeSize(const Process::CoreFileMemoryRanges &ranges) {
+GetLargestRangeSize(const std::vector<Process::CoreFileMemoryRange> &ranges) {
uint64_t max_size = 0;
for (const auto &core_range : ranges)
max_size = std::max(max_size, core_range.range.size());
@@ -957,7 +963,7 @@ GetLargestRangeSize(const Process::CoreFileMemoryRanges &ranges) {
}
Status
-MinidumpFileBuilder::AddMemoryList_32(Process::CoreFileMemoryRanges &ranges) {
+MinidumpFileBuilder::AddMemoryList_32(std::vector<Process::CoreFileMemoryRange> &ranges) {
std::vector<MemoryDescriptor> descriptors;
Status error;
if (ranges.size() == 0)
@@ -1032,7 +1038,7 @@ MinidumpFileBuilder::AddMemoryList_32(Process::CoreFileMemoryRanges &ranges) {
}
Status
-MinidumpFileBuilder::AddMemoryList_64(Process::CoreFileMemoryRanges &ranges) {
+MinidumpFileBuilder::AddMemoryList_64(std::vector<Process::CoreFileMemoryRange> &ranges) {
Status error;
if (ranges.empty())
return error;
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
index 762de83db5a39c..195650de317178 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
@@ -25,6 +25,7 @@
#include "lldb/Target/Process.h"
#include "lldb/Target/Target.h"
+#include "lldb/Symbol/SaveCoreOptions.h"
#include "lldb/Utility/DataBufferHeap.h"
#include "lldb/Utility/Status.h"
#include "lldb/lldb-forward.h"
@@ -120,9 +121,9 @@ class MinidumpFileBuilder {
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);
+ AddMemoryList_64(std::vector<lldb_private::Process::CoreFileMemoryRange> &ranges);
lldb_private::Status
- AddMemoryList_32(lldb_private::Process::CoreFileMemoryRanges &ranges);
+ AddMemoryList_32(std::vector<lldb_private::Process::CoreFileMemoryRange> &ranges);
// Update the thread list on disk with the newly emitted stack RVAs.
lldb_private::Status FixThreadStacks();
lldb_private::Status FlushBufferToDisk();
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.h b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.h
index b76fcd0052a8a8..2f45f01558e667 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.h
+++ b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.h
@@ -21,6 +21,7 @@
#define LLDB_SOURCE_PLUGINS_OBJECTFILE_MINIDUMP_OBJECTFILEMINIDUMP_H
#include "lldb/Symbol/ObjectFile.h"
+#include "lldb/Symbol/SaveCoreOptions.h"
#include "lldb/Utility/ArchSpec.h"
class ObjectFileMinidump : public lldb_private::PluginInterface {
diff --git a/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp b/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
index 9d01089745dfc9..8b0e0c5250106b 100644
--- a/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ b/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -20,6 +20,7 @@
#include "lldb/Target/Process.h"
#include "lldb/Target/SectionLoadList.h"
#include "lldb/Target/Target.h"
+#include "lldb/Symbol/SaveCoreOptions.h"
#include "lldb/Utility/ArchSpec.h"
#include "lldb/Utility/DataBufferHeap.h"
#include "lldb/Utility/FileSpec.h"
diff --git a/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h b/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
index 8bccf3be3e5f63..4f4dedf773c5ba 100644
--- a/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
+++ b/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
@@ -13,6 +13,7 @@
#include <vector>
#include "lldb/Symbol/ObjectFile.h"
+#include "lldb/Symbol/SaveCoreOptions.h"
#include "llvm/Object/COFF.h"
class ObjectFilePECOFF : public lldb_private::ObjectFile {
diff --git a/lldb/source/Symbol/SaveCoreOptions.cpp b/lldb/source/Symbol/SaveCoreOptions.cpp
index 3c4ca2d852b76a..e6a9c483c8ff69 100644
--- a/lldb/source/Symbol/SaveCoreOptions.cpp
+++ b/lldb/source/Symbol/SaveCoreOptions.cpp
@@ -31,7 +31,9 @@ Status SaveCoreOptions::SetPluginName(const char *name) {
return error;
}
-void SaveCoreOptions::SetStyle(lldb::SaveCoreStyle style) { m_style = style; }
+void SaveCoreOptions::SetStyle(lldb::SaveCoreStyle style) {
+ m_style = style;
+}
void SaveCoreOptions::SetOutputFile(FileSpec file) { m_file = file; }
@@ -101,6 +103,18 @@ bool SaveCoreOptions::ShouldThreadBeSaved(lldb::tid_t tid) const {
return m_threads_to_save.count(tid) > 0;
}
+bool SaveCoreOptions::HasSpecifiedThreads() const {
+ return !m_threads_to_save.empty();
+}
+
+void SaveCoreOptions::AddMemoryRegionToSave(const lldb_private::MemoryRegionInfo ®ion) {
+ m_regions_to_save.Insert(region.GetRange(), /*combine=*/true);
+}
+
+const MemoryRanges &SaveCoreOptions::GetCoreFileMemoryRanges() const {
+ return m_regions_to_save;
+}
+
Status SaveCoreOptions::EnsureValidConfiguration(
lldb::ProcessSP process_sp) const {
Status error;
@@ -130,4 +144,5 @@ void SaveCoreOptions::Clear() {
m_style = std::nullopt;
m_threads_to_save.clear();
m_process_sp.reset();
+ m_regions_to_save.Clear();
}
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index 3c9247fdbbbc96..e0b4d0313d4ec1 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -6516,14 +6516,14 @@ static bool AddDirtyPages(const MemoryRegionInfo ®ion,
} else {
// Add previous contiguous range and init the new range with the
// current dirty page.
- ranges.push_back({range, lldb_permissions});
+ ranges.Append(range.start(), range.end(), {range, lldb_permissions});
range = llvm::AddressRange(page_addr, page_addr + page_size);
}
}
}
// The last range
if (!range.empty())
- ranges.push_back({range, lldb_permissions});
+ ranges.Append(range.start(), range.end(), {range, lldb_permissions});
return true;
}
@@ -6534,7 +6534,8 @@ static bool AddDirtyPages(const MemoryRegionInfo ®ion,
// given region. If the region has dirty page information, only dirty pages
// will be added to \a ranges, else the entire range will be added to \a
// ranges.
-static void AddRegion(const MemoryRegionInfo ®ion, bool try_dirty_pages,
+static void AddRegion(const MemoryRegionInfo ®ion,
+ bool try_dirty_pages,
Process::CoreFileMemoryRanges &ranges) {
// Don't add empty ranges.
if (region.GetRange().GetByteSize() == 0)
@@ -6544,7 +6545,8 @@ static void AddRegion(const MemoryRegionInfo ®ion, bool try_dirty_pages,
return;
if (try_dirty_pages && AddDirtyPages(region, ranges))
return;
- ranges.push_back(CreateCoreFileMemoryRange(region));
+
+ ranges.Append(region.GetRange().GetRangeBase(), region.GetRange().GetByteSize(), CreateCoreFileMemoryRange(region));
}
static void SaveOffRegionsWithStackPointers(
@@ -6593,7 +6595,7 @@ static void GetCoreFileSaveRangesFull(Process &process,
Process::CoreFileMemoryRanges &ranges,
std::set<addr_t> &stack_ends) {
- // Don't add only dirty pages, add full regions.
+// Don't add only dirty pages, add full regions.
const bool try_dirty_pages = false;
for (const auto ®ion : regions)
if (stack_ends.count(region.GetRange().GetRangeEnd()) == 0)
@@ -6650,6 +6652,22 @@ static void GetCoreFileSaveRangesStackOnly(
}
}
+static void GetUserSpecifiedCoreFileSaveRanges(Process &process,
+ const MemoryRegionInfos ®ions,
+ const SaveCoreOptions &options,
+ Process::CoreFileMemoryRanges &ranges) {
+ const auto &option_ranges = options.GetCoreFileMemoryRanges();
+ if (option_ranges.IsEmpty())...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
✅ With the latest revision this PR passed the Python code formatter. |
mbucko
reviewed
Aug 21, 2024
bulbazord
reviewed
Aug 21, 2024
| /// \param region The memory region to save. | ||
| /// \returns An empty SBError upon success, or an error if the region is | ||
| /// invalid. | ||
| /// \note Ranges that overlapped with be unioned into a single region this |
There was a problem hiding this comment.
"Ranges that overlap will be unioned into a single region, this also supercedes stack minification."
Some other things we could do with this patch:
- if a region crosses a permission boundary, then break it up so we can safe off memory with the right permissions. So if we have [0x1000-0x2000) with "rw" permissions and [0x2000-0x3000) with "r" permissions, and the user asks for [0x1000-0x3000) to be saved, we should probably break it up and save them separately.
- Allow the user to ask for a huge range and break it up into any regions that fit, like ask for
[0x100000000-0xFFFFFFFFFFFFFFFF)and we would save all regions in that range, separated out by permission boundaries
| @@ -7,6 +7,9 @@ | |||
| //===----------------------------------------------------------------------===// | |||
|
|
|||
| #include "lldb/API/SBSaveCoreOptions.h" | |||
| #include "lldb/API/SBError.h" | |||
| #include "lldb/API/SBFileSpec.h" | |||
There was a problem hiding this comment.
SBFileSpec is unused right? Shouldn't need this include.
There was a problem hiding this comment.
Good call, I'll try to trim all the unneeded usings now that the header isn't in private interfaces
| LLDB_INSTRUMENT_VA(this, region); | ||
| // Currently add memory region can't fail, so we always return a success | ||
| // SBerror, but because these API's live forever, this is the most future | ||
| // proof thing to do. |
There was a problem hiding this comment.
Thanks for being forward-looking. 😄
clayborg
reviewed
Aug 22, 2024
| @@ -45,7 +45,6 @@ class LLDB_API SBMemoryRegionInfoList { | |||
|
|
|||
| private: | |||
| friend class SBProcess; | |||
|
|
|||
There was a problem hiding this comment.
revert whitespace only change
| /// \param region The memory region to save. | ||
| /// \returns An empty SBError upon success, or an error if the region is | ||
| /// invalid. | ||
| /// \note Ranges that overlapped with be unioned into a single region this |
There was a problem hiding this comment.
"Ranges that overlap will be unioned into a single region, this also supercedes stack minification."
Some other things we could do with this patch:
- if a region crosses a permission boundary, then break it up so we can safe off memory with the right permissions. So if we have [0x1000-0x2000) with "rw" permissions and [0x2000-0x3000) with "r" permissions, and the user asks for [0x1000-0x3000) to be saved, we should probably break it up and save them separately.
- Allow the user to ask for a huge range and break it up into any regions that fit, like ask for
[0x100000000-0xFFFFFFFFFFFFFFFF)and we would save all regions in that range, separated out by permission boundaries
| @@ -1222,6 +1222,7 @@ enum SaveCoreStyle { | |||
| eSaveCoreFull = 1, | |||
| eSaveCoreDirtyOnly = 2, | |||
| eSaveCoreStackOnly = 3, | |||
| eSaveCoreCustom = 4, | |||
There was a problem hiding this comment.
Should we name this eSaveCoreCustomOnly? We should add some header doc for each of these above ones as well explaining what will be done and how they interact with any custom ranges that are specified.
/// Save all memory ranges that are readable to the core file. If
/// custom memory ranges are specified, they will be ingored.
eSaveCoreFull = 1,
/// If the platform supports detecting dirty pages within read/write
/// regions, save only the modified pages. If the platform doesn't
/// support detecting dirty pages, then save all regions with read +
/// write permissions. If custom memory ranges are specified, they
/// will be merged into the regions that get saved.
eSaveCoreDirtyOnly = 2,
/// Save the stacks for all threads. Minimal stacks will be saved
/// where only the SP to the top of the stack will be saved taking
/// into account the red zone. If custom memory ranges are specified,
/// they will be merged into the regions that get saved.
eSaveCoreStackOnly = 3,
/// Save only the custom regions that are specified.
eSaveCoreCustom = 4,
There was a problem hiding this comment.
I think CustomOnly sounds a bit clunky, but it does convey only custom fields will be saved and fits with the current naming standard.
lldb/source/Target/Process.cpp
Outdated
| @@ -6544,7 +6545,8 @@ static void AddRegion(const MemoryRegionInfo ®ion, bool try_dirty_pages, | |||
| return; | |||
| if (try_dirty_pages && AddDirtyPages(region, ranges)) | |||
| return; | |||
| ranges.push_back(CreateCoreFileMemoryRange(region)); | |||
|
|
|||
| ranges.Append(region.GetRange().GetRangeBase(), region.GetRange().GetByteSize(), CreateCoreFileMemoryRange(region)); | |||
There was a problem hiding this comment.
Currently I'm having issues with my git-clang format. I'll sort before we go to commit.
lldb/source/Target/Process.cpp
Outdated
| return Status("no valid address ranges found for core style"); | ||
|
|
||
| // Sort the range data vector to dedupe ranges before returning. | ||
| ranges.Sort(); |
There was a problem hiding this comment.
This is where we should go over the ranges and break them up by permissions. Also if we wanted to support the user specifying large ranges that contain many regions, then we would break them up here as well.
…re we merge user regions and save style regions
…mory regions but can't due to proc pid maps, and fix some spelling and grammar mistakes
clayborg
approved these changes
Aug 27, 2024
|
It looks like this broke lldb macOS bots: https://green.lab.llvm.org/job/llvm.org/view/LLDB/job/as-lldb-cmake/10450/ @Jlalond can you take a look ? Thanks. |
|
@medismailben Ack |
Jlalond
added a commit
to Jlalond/llvm-project
that referenced
this pull request
Aug 27, 2024
llvm#105442)" This reverts commit d517b22.
adrian-prantl
pushed a commit
to adrian-prantl/llvm-project
that referenced
this pull request
Oct 11, 2024
…105442) This patch adds the option to specify specific memory ranges to be included in a given core file. The current implementation lets user specified ranges either be in addition to a certain save style, or independent of them via the newly added custom enum. To achieve being inclusive of save style, I've moved from a std::vector of ranges to a RangeDataVector, and to join overlapping ranges to prevent duplication of memory ranges in the core file. As a non function bonus, when SBSavecore was initially created, the header was included in the lldb-private interfaces, and I've fixed that and moved it the forward declare as an oversight. CC @bulbazord in case we need to include that into swift. (cherry picked from commit d517b22)
jasonmolenda
pushed a commit
to jasonmolenda/llvm-project
that referenced
this pull request
Jan 23, 2025
llvm#106293) Reverts llvm#105442. Due to `TestSkinnyCoreFailing` and root causing of the failure will likely take longer than EOD. (cherry picked from commit b959532)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch adds the option to specify specific memory ranges to be included in a given core file. The current implementation lets user specified ranges either be in addition to a certain save style, or independent of them via the newly added custom enum.
To achieve being inclusive of save style, I've moved from a std::vector of ranges to a RangeDataVector, and to join overlapping ranges to prevent duplication of memory ranges in the core file.
As a non function bonus, when SBSavecore was initially created, the header was included in the lldb-private interfaces, and I've fixed that and moved it the forward declare as an oversight. CC @bulbazord in case we need to include that into swift.