[LLDB] Reapply SBSaveCore Add Memory List #107937
Merged
Conversation
This file contains 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
This reverts commit bb34346.
… the bug in process where we're passing range end as size
|
@llvm/pr-subscribers-lldb Author: Jacob Lalonde (Jlalond) ChangesRecently in #107731 this change was revereted due to excess memory size in Additionally, and requiring additional review, I added more unit tests and more verbose logic to the merging of save core memory regions. @jasonmolenda as an FYI. Patch is 48.83 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/107937.diff 25 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/SBSaveCoreOptions.h b/lldb/include/lldb/API/SBSaveCoreOptions.h
index ba48ba5eaea5a0..c076d3ce6f7575 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 SBError upon success, or an error if the region is
+ /// invalid.
+ /// \note Ranges that overlapped will 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..d90d08026016dc 100644
--- a/lldb/include/lldb/Symbol/SaveCoreOptions.h
+++ b/lldb/include/lldb/Symbol/SaveCoreOptions.h
@@ -10,13 +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 <set>
#include <string>
#include <unordered_set>
+using MemoryRanges = lldb_private::RangeVector<lldb::addr_t, lldb::addr_t>;
+
namespace lldb_private {
class SaveCoreOptions {
@@ -38,8 +40,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 +57,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/CoreFileMemoryRanges.h b/lldb/include/lldb/Target/CoreFileMemoryRanges.h
new file mode 100644
index 00000000000000..503ecd691e5948
--- /dev/null
+++ b/lldb/include/lldb/Target/CoreFileMemoryRanges.h
@@ -0,0 +1,50 @@
+//===-- CoreFileMemoryRanges.h ----------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "lldb/Utility/RangeMap.h"
+#include "lldb/Utility/Status.h"
+
+#include "llvm/ADT/AddressRanges.h"
+
+#ifndef LLDB_TARGET_COREFILEMEMORYRANGES_H
+#define LLDB_TARGET_COREFILEMEMORYRANGES_H
+
+namespace lldb_private {
+
+struct CoreFileMemoryRange {
+ llvm::AddressRange range; /// The address range to save into the core file.
+ uint32_t lldb_permissions; /// A bit set of lldb::Permissions bits.
+
+ bool operator==(const CoreFileMemoryRange &rhs) const {
+ return range == rhs.range && lldb_permissions == rhs.lldb_permissions;
+ }
+
+ bool operator!=(const CoreFileMemoryRange &rhs) const {
+ return !(*this == rhs);
+ }
+
+ bool operator<(const CoreFileMemoryRange &rhs) const {
+ if (range < rhs.range)
+ return true;
+ if (range == rhs.range)
+ return lldb_permissions < rhs.lldb_permissions;
+ return false;
+ }
+};
+
+class CoreFileMemoryRanges
+ : public lldb_private::RangeDataVector<lldb::addr_t, lldb::addr_t,
+ CoreFileMemoryRange> {
+public:
+ /// Finalize and merge all overlapping ranges in this collection. Ranges
+ /// will be seperated based on permissions.
+ Status FinalizeCoreFileSaveRanges();
+};
+} // namespace lldb_private
+
+#endif // LLDB_TARGET_COREFILEMEMORYRANGES_H
diff --git a/lldb/include/lldb/Target/Process.h b/lldb/include/lldb/Target/Process.h
index c66cfb2c245efd..b8c53a474ba6b9 100644
--- a/lldb/include/lldb/Target/Process.h
+++ b/lldb/include/lldb/Target/Process.h
@@ -35,6 +35,8 @@
#include "lldb/Host/ProcessLaunchInfo.h"
#include "lldb/Host/ProcessRunLock.h"
#include "lldb/Symbol/ObjectFile.h"
+#include "lldb/Symbol/SaveCoreOptions.h"
+#include "lldb/Target/CoreFileMemoryRanges.h"
#include "lldb/Target/ExecutionContextScope.h"
#include "lldb/Target/InstrumentationRuntime.h"
#include "lldb/Target/Memory.h"
@@ -710,29 +712,6 @@ class Process : public std::enable_shared_from_this<Process>,
/// is not supported by the plugin, error otherwise.
virtual llvm::Expected<bool> SaveCore(llvm::StringRef outfile);
- struct CoreFileMemoryRange {
- llvm::AddressRange range; /// The address range to save into the core file.
- uint32_t lldb_permissions; /// A bit set of lldb::Permissions bits.
-
- bool operator==(const CoreFileMemoryRange &rhs) const {
- return range == rhs.range && lldb_permissions == rhs.lldb_permissions;
- }
-
- bool operator!=(const CoreFileMemoryRange &rhs) const {
- return !(*this == rhs);
- }
-
- bool operator<(const CoreFileMemoryRange &rhs) const {
- if (range < rhs.range)
- return true;
- if (range == rhs.range)
- return lldb_permissions < rhs.lldb_permissions;
- return false;
- }
- };
-
- using CoreFileMemoryRanges = std::vector<CoreFileMemoryRange>;
-
/// Helper function for Process::SaveCore(...) that calculates the address
/// ranges that should be saved. This allows all core file plug-ins to save
/// consistent memory ranges given a \a core_style.
diff --git a/lldb/include/lldb/Utility/RangeMap.h b/lldb/include/lldb/Utility/RangeMap.h
index 8cc382bcc046ce..c636348129b647 100644
--- a/lldb/include/lldb/Utility/RangeMap.h
+++ b/lldb/include/lldb/Utility/RangeMap.h
@@ -450,6 +450,8 @@ class RangeDataVector {
void Append(const Entry &entry) { m_entries.emplace_back(entry); }
+ void Append(B &&b, S &&s, 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())
return false;
diff --git a/lldb/include/lldb/lldb-enumerations.h b/lldb/include/lldb/lldb-enumerations.h
index 7bfde8b9de1271..938f6e3abe8f2a 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,
+ eSaveCoreCustomOnly = 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 ef82b0253f1199..c79b57fa62c2be 100644
--- a/lldb/source/API/SBSaveCoreOptions.cpp
+++ b/lldb/source/API/SBSaveCoreOptions.cpp
@@ -7,6 +7,7 @@
//===----------------------------------------------------------------------===//
#include "lldb/API/SBSaveCoreOptions.h"
+#include "lldb/API/SBMemoryRegionInfo.h"
#include "lldb/Host/FileSystem.h"
#include "lldb/Symbol/SaveCoreOptions.h"
#include "lldb/Utility/Instrumentation.h"
@@ -89,6 +90,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 25eb633f1e6dad..5b0f4f66f248b6 100644
--- a/lldb/source/Commands/CommandObjectProcess.cpp
+++ b/lldb/source/Commands/CommandObjectProcess.cpp
@@ -25,6 +25,7 @@
#include "lldb/Interpreter/OptionArgParser.h"
#include "lldb/Interpreter/OptionGroupPythonClassWithDict.h"
#include "lldb/Interpreter/Options.h"
+#include "lldb/Symbol/SaveCoreOptions.h"
#include "lldb/Target/Platform.h"
#include "lldb/Target/Process.h"
#include "lldb/Target/StopInfo.h"
diff --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
index b28beab117cca4..06da83e26a26a5 100644
--- a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -6562,13 +6562,15 @@ bool ObjectFileMachO::SaveCore(const lldb::ProcessSP &process_sp,
}
if (make_core) {
- Process::CoreFileMemoryRanges core_ranges;
+ CoreFileMemoryRanges core_ranges;
error = process_sp->CalculateCoreFileSaveRanges(options, core_ranges);
if (error.Success()) {
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 5c9ba223ad143e..edc568a6b47e00 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
@@ -831,25 +831,32 @@ 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;
- Process::CoreFileMemoryRanges all_core_memory_ranges;
+ std::vector<CoreFileMemoryRange> ranges_32;
+ std::vector<CoreFileMemoryRange> ranges_64;
+ CoreFileMemoryRanges all_core_memory_ranges;
error = m_process_sp->CalculateCoreFileSaveRanges(m_save_core_options,
all_core_memory_ranges);
+
+ std::vector<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++;
}
@@ -869,11 +876,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.
@@ -958,15 +965,15 @@ Status MinidumpFileBuilder::DumpDirectories() const {
}
static uint64_t
-GetLargestRangeSize(const Process::CoreFileMemoryRanges &ranges) {
+GetLargestRangeSize(const std::vector<CoreFileMemoryRange> &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;
}
-Status
-MinidumpFileBuilder::AddMemoryList_32(Process::CoreFileMemoryRanges &ranges) {
+Status MinidumpFileBuilder::AddMemoryList_32(
+ std::vector<CoreFileMemoryRange> &ranges) {
std::vector<MemoryDescriptor> descriptors;
Status error;
if (ranges.size() == 0)
@@ -1042,8 +1049,8 @@ MinidumpFileBuilder::AddMemoryList_32(Process::CoreFileMemoryRanges &ranges) {
return error;
}
-Status
-MinidumpFileBuilder::AddMemoryList_64(Process::CoreFileMemoryRanges &ranges) {
+Status MinidumpFileBuilder::AddMemoryList_64(
+ std::vector<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..71001e26c00e91 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
@@ -23,6 +23,7 @@
#include <utility>
#include <variant>
+#include "lldb/Symbol/SaveCoreOptions.h"
#include "lldb/Target/Process.h"
#include "lldb/Target/Target.h"
#include "lldb/Utility/DataBufferHeap.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::CoreFileMemoryRange> &ranges);
lldb_private::Status
- AddMemoryList_32(lldb_private::Process::CoreFileMemoryRanges &ranges);
+ AddMemoryList_32(std::vector<lldb_private::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..8d9c919bc9b101 100644
--- a/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ b/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -17,6 +17,7 @@
#include "lldb/Interpreter/OptionValueDictionary.h"
#include "lldb/Interpreter/OptionValueProperties.h"
#include "lldb/Symbol/ObjectFile.h"
+#include "lldb/Symbol/SaveCoreOptions.h"
#include "lldb/Target/Process.h"
#include "lldb/Target/SectionLoadList.h"
#include "lldb/Target/Target.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 35943726f2e4ef..8d9aadece2152d 100644
--- a/lldb/source/Symbol/SaveCoreOptions.cpp
+++ b/lldb/source/Symbol/SaveCoreOptions.cpp
@@ -102,6 +102,19 @@ 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;
@@ -131,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/CMakeLists.txt b/lldb/source/Target/CMakeLists.txt
index a42c44b761dc56..a6d2eace975420 100644
--- a/lldb/source/Target/CMakeLists.txt
+++ b/lldb/source/Target/CMakeLists.txt
@@ -11,6 +11,7 @@ add_lldb_library(lldbTarget
ABI.cpp
AssertFrameRecognizer.cpp
DynamicRegisterInfo.cpp
+ CoreFileMemoryRanges.cpp
ExecutionContext.cpp
InstrumentationRuntime.cpp
InstrumentationRuntimeStopInfo.cpp
diff --git a/lldb/source/Target/CoreFileMemoryRanges.cpp b/lldb/source/Target/CoreFileMemoryRanges.cpp
new file mode 100644
index 00000000000000..f3436a56559385
--- /dev/null
+++ b/lldb/source/Target/CoreFileMemoryRanges.cpp...
[truncated]
|
Jlalond
commented
Sep 9, 2024
| using namespace lldb; | ||
| using namespace lldb_private; | ||
|
|
||
| using Entry = CoreFileMemoryRanges::Entry; |
Jlalond
commented
Sep 10, 2024
| @@ -450,6 +450,8 @@ class RangeDataVector { | |||
|
|
|||
| void Append(const Entry &entry) { m_entries.emplace_back(entry); } | |||
|
|
|||
| void Append(B &&b, S &&s, T &&t) { m_entries.emplace_back(Entry(b, s, t)); } | |||
There was a problem hiding this comment.
Nit to self, add code comments about Base Size And Data
There was a problem hiding this comment.
why not just name then base, size, data? Then the reader doesn't have to read the function contract. Also, would be curious what the guideline says about single letter naming for variables.
clayborg
approved these changes
Sep 10, 2024
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
mbucko
reviewed
Sep 11, 2024
| : public lldb_private::RangeDataVector<lldb::addr_t, lldb::addr_t, | ||
| CoreFileMemoryRange> { | ||
| public: | ||
| /// Finalize and merge all overlapping ranges in this collection. Ranges |
There was a problem hiding this comment.
I think you might need to explicitly inherit the ctors:
using lldb_private::RangeDataVector<lldb::addr_t, lldb::addr_t, CoreFileMemoryRange>::RangeDataVector;
| @@ -450,6 +450,8 @@ class RangeDataVector { | |||
|
|
|||
| void Append(const Entry &entry) { m_entries.emplace_back(entry); } | |||
|
|
|||
| void Append(B &&b, S &&s, T &&t) { m_entries.emplace_back(Entry(b, s, t)); } | |||
There was a problem hiding this comment.
why not just name then base, size, data? Then the reader doesn't have to read the function contract. Also, would be curious what the guideline says about single letter naming for variables.
| using namespace lldb; | ||
| using namespace lldb_private; | ||
|
|
||
| using Entry = CoreFileMemoryRanges::Entry; |
There was a problem hiding this comment.
maybe put these in an anonymous namespace
Comment on lines
+27
to
+28
| return IntersectHelper(region_one, region_two) || | ||
| IntersectHelper(region_two, region_one); |
There was a problem hiding this comment.
I believe IntersectHelper(region_one, region_two) is the same as IntersectHelper(region_two, region_one) no?
| region_two->GetRangeEnd() < region_one->GetRangeBase()); | ||
| } | ||
|
|
||
| static bool IntersectHelper(const Entry *region_one, const Entry *region_two) { |
There was a problem hiding this comment.
maybe a better name? for example Touching()
VitaNuo
pushed a commit
to VitaNuo/llvm-project
that referenced
this pull request
Sep 12, 2024
Recently in llvm#107731 this change was revereted due to excess memory size in `TestSkinnyCore`. This was due to a bug where a range's end was being passed as size. Creating massive memory ranges. Additionally, and requiring additional review, I added more unit tests and more verbose logic to the merging of save core memory regions. @jasonmolenda as an FYI.
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.
Recently in #107731 this change was revereted due to excess memory size in
TestSkinnyCore. This was due to a bug where a range's end was being passed as size. Creating massive memory ranges.Additionally, and requiring additional review, I added more unit tests and more verbose logic to the merging of save core memory regions.
@jasonmolenda as an FYI.